feat: add http routes and handlers to get/patch the memstore config #293

samlaf · 2025-02-15T05:12:51Z

This is needed to create API-driven tests on the kurtosis devnet in our op fork.

See the new memstore README for details of the REST API.

Fixes Issue

Fixes #

Changes proposed

Screenshots (Optional)

Note to reviewers

ethenotethan · 2025-02-15T09:47:59Z

store/generated_key/memstore/memconfig/config.go

+// Patches are reads as ConfigUpdates instead to handle omitted fields.
+func (c Config) MarshalJSON() ([]byte, error) {
+	return json.Marshal(struct {
+		MaxBlobSizeBytes        uint64 `json:"MaxBlobSizeBytes"`


Why not declare these tags in the above struct vs having a shadow declaration here?

Originally I was using tags to change the name (using snake case), but I opted to just keep the names the same as the struct names. This struct is different from the struct above so would need its own tags, but you are right that they arent even needed (because default is to use same names), so I removed them: c2bc549

ethenotethan · 2025-02-15T09:53:47Z

store/generated_key/memstore/memconfig/http_handlers.go

+// It adds routes to the proxy's main router (to be served on same port as the main proxy routes):
+// - GET /memstore/config: returns the current memstore configuration
+// - PATCH /memstore/config: updates the memstore configuration
+type HandlerHTTP struct {


Can we please put the handler outside of this package? Feels more appropriate to keep all the handlers bundled together

I disagree, in my opinion every package should define its config/flags/metrics/routes, and then these can all be enabled/disabled, and the handlers can be added to the main router conditionally. This is also the way that op does it (look at rpc/api.go which defines the admin api for the batcher, op-node has a similar one, etc).

I could move them under memconfig/handlers/handlers.go if you prefer, but I felt this would just be unnecessary complexity for such a small package.

Also originally I had this memstore api behind a conditional flag, but I decided to just always enable it when memstore is enabled. Are you fine with that decision?

my concern is we have a handlers.go defined in /server and now a handler construction in a file set that was initially just storage backend implementations - causing some complexity. imo we could either:

move the existing handlers from /server to eigenda backend. This wouldn't really work with V2 though since these method implementations are cross compatible between versions.

create a server/handlers package where we store all handler implementations - this would look smth like (handlers/proxy, handlers/memconfig). I'm very biased towards this approach since its how I've scaffolded go servers for the past few years haha

tangentially the abstract structuring of proxy could definitely use a reskin. E.g we use a load_store.go file for all dependency injection when we could have cleaner representations. In retrospect I wish this service wasn't pushed as a one week deliverable where we copy pasta'd OP Plasma's example server.

For 1. you prob meant move existing handlers to /store and not eigenda? The handlers in /server are the main handlers for the entire service, not only for eigenda backend.
I personally like this approach better, because I think the server should have a single responsibility of handling http traffic, and just being fed a router and using it dumbly. Every backend store or service understands its structure, so it should be the one creating handlers. Then the server only has to decide which router to bind these handlers to.

This structure makes a lot more sense to me. But what did you mean by "these method implementations are cross compatible between versions."?

But honestly I don't really anticipate having a ton more handlers defined going forward I think...? So prob would be fine to go with your approach.

ethenotethan · 2025-02-15T09:58:23Z

verify/verifier.go

@@ -61,10 +61,13 @@ func NewVerifier(cfg *Config, l logging.Logger) (*Verifier, error) {
 	var err error

 	if cfg.VerifyCerts {
+		log.Info("Certificate verification against Ethereum state enabled")


Would prefer if we consolidated this message into a single Boolean field in the below service startup log

What do you mean by this? What is "below service startup log"? Originally this log was in log_store.go but it felt more natural to me to move it here. Where would you prefer to have it?

Moved the log around a bit (though still not sure this is what you wanted) and added a log line for kzg verifier: cf912ea

Now startup logs looks like:

oh my bad - was referring to here: https://github.com/Layr-Labs/eigenda-proxy/blob/main/server/load_store.go#L138-L142

hmm but that thing only shows "eigenda" and doesnt differentiate whether its memstore or actual eigenda

honestly I still think we should remove the option to disable cert verification when running eigenda. and for memstore perhaps we can disable the entire thing always (see your other msg). Perhaps we can do this in another PR though, since its a big change that deserves its own discussion potentially.

…ing logged on startup)

Not sure why the ci error started appearing just now...

…ter the refactor)

store/generated_key/memstore/README.md

ethenotethan · 2025-02-16T10:23:22Z

store/generated_key/memstore/README.md

+
+See [memconfig/config.go](./memconfig/config.go) for the configuration options.
+These can all be set via their respective flags or environment variables.


can also run:

./bin/eigenda-proxy --help | grep memstore

ethenotethan · 2025-02-16T10:24:03Z

store/generated_key/memstore/README.md

+The PATCH request allows to patch the configuration. This allows only sending a subset of the configuration options. The other fields will be left intact.
+
+```bash
+$ curl -X PATCH http://localhost:3100/memstore/config -d '{"PutReturnsFailoverError": true}'
+{"MaxBlobSizeBytes":16777216,"BlobExpiration":"25m0s","PutLatency":"0s","GetLatency":"0s","PutReturnsFailoverError":true}
+```
+
+One can of course still build a jq pipe to produce the same result (although still using PATCH instead of PUT since that is the only method available):
+```bash
+$ curl http://localhost:3100/memstore/config | \
+  jq '.PutLatency = "5s" | .GetLatency = "2s"' | \
+  curl -X PATCH http://localhost:3100/memstore/config -d @-
+```


should we add a todo to codify a client for this?

ethenotethan · 2025-02-16T10:25:20Z

store/generated_key/memstore/memconfig/config.go

+func (sc *SafeConfig) LatencyPUTRoute() time.Duration {
+	sc.mu.RLock()
+	defer sc.mu.RUnlock()
+	return sc.config.PutLatency
+}
+func (sc *SafeConfig) SetLatencyPUTRoute(latency time.Duration) {
+	sc.mu.Lock()
+	defer sc.mu.Unlock()
+	sc.config.PutLatency = latency
+}
+
+func (sc *SafeConfig) LatencyGETRoute() time.Duration {
+	sc.mu.RLock()
+	defer sc.mu.RUnlock()
+	return sc.config.GetLatency
+}
+func (sc *SafeConfig) SetLatencyGETRoute(latency time.Duration) {
+	sc.mu.Lock()
+	defer sc.mu.Unlock()
+	sc.config.GetLatency = latency
+}


why does latency have put/get accessor but other variables don't?

put/get accessor?
They are all structured the same way: name of the variable is the getter, and SetXXX is the setter.

ethenotethan · 2025-02-16T10:26:44Z

store/generated_key/memstore/memstore.go

+	// We only use the verifier for kzgCommitment verification.
+	// MemStore generates random certs which can't be verified.
+	// TODO: we should probably refactor the Verifier to be able to only take in a BlobVerifier here.
+	verifier *verify.Verifier


do we even need this?

idk lol... wdyt? Wait is it even being used? The cert can't be fully random otherwise the kzgCommitment verification would fail right?

Co-authored-by: ethenotethan <[email protected]>

ethenotethan

LGTM

feat: add http routes and handlers to get/patch the memstore config

055b938

samlaf requested review from ethenotethan and litt3 February 15, 2025 05:12

test(memstore_handlers): fix GET tests

5ce2dbc

ethenotethan reviewed Feb 15, 2025

View reviewed changes

samlaf added 4 commits February 15, 2025 11:22

style: make format

75cf365

style(memconfig): remove unneeded tags from struct in MarshalJSON

c2bc549

chore: add marshalJSON function to memstore safeconfig (it wasnt gett…

2101c7a

…ing logged on startup)

style(verifier): better startup logs

cf912ea

samlaf requested a review from ethenotethan February 15, 2025 17:01

samlaf added 3 commits February 15, 2025 12:05

chore: fix golangci.yml errors/warnings

129babd

Not sure why the ci error started appearing just now...

chore(makefile): clean up irrelevant run-memstore-server flags

8b8a114

test: fix fuzz test (CreateTestSuite was not registering the route af…

4216b01

…ter the refactor)

samlaf force-pushed the feat--memstore-config-http-api branch from cb47d2b to 4216b01 Compare February 15, 2025 17:15

ethenotethan reviewed Feb 16, 2025

View reviewed changes

samlaf and others added 2 commits February 16, 2025 12:35

Update store/generated_key/memstore/README.md

0a9e2ad

Co-authored-by: ethenotethan <[email protected]>

docs(memstore): add command to print memstore flags

90112cb

samlaf mentioned this pull request Feb 16, 2025

Create a golang client for the memconfig routes #294

Open

samlaf requested a review from ethenotethan February 17, 2025 16:27

ethenotethan approved these changes Feb 18, 2025

View reviewed changes

samlaf added 2 commits February 18, 2025 14:15

Merge branch 'main' into feat--memstore-config-http-api

45be4b8

Merge branch 'main' into feat--memstore-config-http-api

796fe54

samlaf merged commit daf39e2 into main Feb 18, 2025
10 checks passed

samlaf deleted the feat--memstore-config-http-api branch February 18, 2025 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add http routes and handlers to get/patch the memstore config #293

feat: add http routes and handlers to get/patch the memstore config #293

samlaf commented Feb 15, 2025

ethenotethan Feb 15, 2025

samlaf Feb 15, 2025

ethenotethan Feb 15, 2025

samlaf Feb 15, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan Feb 15, 2025

samlaf Feb 15, 2025

samlaf Feb 15, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan Feb 16, 2025

samlaf Feb 16, 2025

ethenotethan left a comment


		See [memconfig/config.go](./memconfig/config.go) for the configuration options.
		These can all be set via their respective flags or environment variables.

feat: add http routes and handlers to get/patch the memstore config #293

feat: add http routes and handlers to get/patch the memstore config #293

Conversation

samlaf commented Feb 15, 2025

Fixes Issue

Changes proposed

Screenshots (Optional)

Note to reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethenotethan left a comment

Choose a reason for hiding this comment