Read only mode #126

hopeyen · 2024-09-17T10:15:36Z

Added flag EIGENDA_PROXY_READ_ONLY
Generate random private key if not provided
Add readonly config to EigenDA router that simply return bad request when calling PUT methods

Makefile

bxue-l2

lgtm

server/config.go

samlaf · 2024-09-17T17:49:21Z

server/config.go

+		&cli.BoolFlag{
+			Name:    ReadOnlyFlagName,
+			Usage:   "Whether the proxy should operate in read-only mode.",
+			Value:   false,
+			EnvVars: prefixEnvVars("READ_ONLY"),
+		},


Why are we adding this? Who is this feature useful for? Can we add some description here or somewhere else as to the intent of this feature.

This is a feature included in the v1.5.0 document. I think this was requested by celo. The use case is they only need retrieval when running proxy, and it made sense to develop a read mode that does not require a key.

I've modified the usage string to say Whether to run the proxy in read-only mode; set to true to only allow retrieval from EigenDA. Please let me know if this is sufficient

I think the problem now is that if some op-nodes just want to run the derivation pipeline, there is no way to start the proxy without putting in some fake privateKey.

samlaf · 2024-09-17T17:50:14Z

store/router.go

+	if r.readonly {
+		return nil, errors.New("read-only router does not support Put")
+	}


What will this return to the client? Will the Put handler return a 500 with this error?

One other idea is to just disable the put route/handler entirely when in read mode. Have you considered that?

Yes, this will return 500 with this error.

I've considered this approach and tried implementing this approach prior to the current PR. There's restriction from IRouter interface requiring the [put](https://github.com/Layr-Labs/eigenda-proxy/blob/d124ef8bd80eb97f4feadd6901cc3947d6350874/store/router.go#L18) handler, but I didn't think it made sense to remove put from the interface?

Also thought about making a separate router type that contains exactly this super simple put handler, and create Router type based on the readonly config, but there was a lot of code repetition for all the other functions, and I didn't find a way to easily share functions between two child struct types (the regular Router and a ReadOnly 🤔 And thought about a wrapper type, but didn't think it was worth it at the end and a bit counter-intuitive .

Please suggest and I'm happy to learn and redo:)

Gotcha! This works I think. We should be returning a 400 instead though. I noticed we return 500s in a bunch of places where we shouldn't (see #127 for eg)

bxue-l2 · 2024-09-17T21:04:54Z

server/config.go

+			DisableTLS:               ctx.Bool(DisableTLSFlagName),
+			ResponseTimeout:          ctx.Duration(ResponseTimeoutFlagName),
+			CustomQuorumIDs:          ctx.UintSlice(CustomQuorumIDsFlagName),
+			SignerPrivateKeyHex: func() string {


actually, if someone forgot to set the key, and does not enable read only. Then the server can send unauthenticated data

Yes, I realized this as well, I was able to request for dispersal even with random private keys. This was confusing because I thought disperser had authentication checks with ratelimits

disperser has a free tier for testnet. but it does not have a free tier for mainnet.

the server won't be able to send unauthenticated data on the mainnet due to rate-limits, right? what's the desirable behavior in this case? (I'm going to update this random generation to use a constant address like all zeros)

see the new comments

samlaf · 2024-09-18T04:23:23Z

server/config.go

+	signerPrivateKeyHex := func() string {
+		if key := ctx.String(SignerPrivateKeyHexFlagName); key != "" {
+			return key
+		}
+		randomBytes := make([]byte, 32)
+		if _, err := rand.Read(randomBytes); err != nil {
+			return ""
+		}
+		return hex.EncodeToString(randomBytes)
+	}()


Suggested change

signerPrivateKeyHex := func() string {

if key := ctx.String(SignerPrivateKeyHexFlagName); key != "" {

return key

}

randomBytes := make([]byte, 32)

if _, err := rand.Read(randomBytes); err != nil {

return ""

}

return hex.EncodeToString(randomBytes)

}()

signerPrivateKeyHex := ctx.String(SignerPrivateKeyHexFlagName)

if signerPrivateKeyHex == "" {

randomBytes := make([]byte, 32)

if _, err := rand.Read(randomBytes); err != nil {

return ""

}

signerPrivateKeyHex = hex.EncodeToString(randomBytes)

}

Can we do something simpler like this? Really don't like inline functions when not needed.

Also not sure about returning "" when rand fails... maybe panic was right. Not super sure what the intended behavior is here so I'll leave that to you.

epociask · 2024-09-18T04:19:42Z

server/config.go

+	signerPrivateKeyHex := func() string {
+		if key := ctx.String(SignerPrivateKeyHexFlagName); key != "" {
+			return key
+		}
+		randomBytes := make([]byte, 32)
+		if _, err := rand.Read(randomBytes); err != nil {
+			return ""
+		}
+		return hex.EncodeToString(randomBytes)
+	}()


how do we handle the case in-which read mode is disable and the user didn't provide a valid key? should we fail loudly assuming this could be an error case that someone running in production would need to know about?

knit - would prefer this to live as a separate function within this file to avoid overlaying additional programming abstractions (i.e, callbacks) that aren't generally adopted within the service

epociask · 2024-09-18T04:22:20Z

store/router.go

+	if r.readonly {
+		return nil, errors.New("read-only router does not support Put")
+	}


what if I just wanna run proxy for S3 support using the keccak256 backend and would like to disable reading from eigenda?

I'm a bit confused, the read-only mode implemented here will disable all put methods no matter what. Are you saying the read-only mode should handle something more granular with the cache/fallback targets?

epociask · 2024-09-18T04:27:03Z

.env.example.holesky

@@ -87,3 +87,6 @@ EIGENDA_PROXY_SERVICE_MANAGER_ADDR=0xD4A7E1Bd8015057293f0D0A557088c286942e84b

 # endpoint for S3 storage
 # EIGENDA_PROXY_S3_ENDPOINT
+
+# set to true to disable writing to EigenDA
+# EIGENDA_PROXY_READ_ONLY=false


Thinking about this a bit more pragmatically - we have a LOT of knobs for this service with the env namespace only growing. Would it be better to process this internally using the SignerPrivateKey where we treat it as readonly <-> SignerPrivateKey="" or unset by the user?

A lil hesitant to introduce more env flags, thoughts?
cc - @samlaf, @bxue-l2

Yeah I certainly don't like the fact that we have growing flags without obvious links between themselves. I don't really like the privateKey being empty turning readonly mode though, as that is very surprising. One thing we could do eventually is move to urfave v3 and use mutuallyExclusiveFlags (https://pkg.go.dev/github.com/urfave/cli/v3#MutuallyExclusiveFlags). So basically urfave would force user to either be in readonly mode, or to provide a signerPrivateKey, but can't have both.

Regarding the UX problem of celo wanting to run in read-only mode and not having to provide a random signerPrivateKey, seems like the main source of the problem is in the eigenda-client? https://github.com/Layr-Labs/eigenda/blob/97dd2a3da09fd62ff3670afd96c2d402f1ca8453/api/clients/config.go#L55 seems like having a non-empty signerPrivateKey is enforced? Perhaps we would need a read-only eigenda-client?

Yeah, I agree with all the concerns here. I would suggest having a separate refactor effort for the flags.

For comment on eigenda-client, I thought so too, but didn't want to touch eigenda code 😬 maybe I should've taken the initiative there which makes more sense. I can go ahead and put a PR there and see what corresponding changes would be good here

reevaluating the approach. Currently EigenDAClient only contains a dispersal client that has a separate definition for RetrieveBlob than the one for retrieval client that has a more complex/decentralized retrieval logic.

thinking about read-only's UX, perhaps we want to simply create an RetrievalClient instead of an EigenDAClient and let retrieveBlob serve Get and return error for Put. But then, why don't the read-only user simply spin up a retrieval client? lmk what you guys think, can also bring this up in the sync

We didn't have time to discuss that in the sync. In the current EigenDAClient, it looks we didn't put much consideration for clients that require read-only (like op-node or nitro node which only runs derivation). Instead of creating patch here and there, it is good to start from a high level design first, which I think Hope is pointing out.

On spinning up a retrieval client. In OP, the protocol requires there to be a proxy server, so the user can't spin up a client, the proxy has to.

On refactor, we could create a client struct that wrap around both node and disperser client. What do people think

Thanks Bowen, I think taking a step back and agree on high level design makes sense. I think the current PR might be a short term temporary solution if we are okay with this, but I guess no one is rushing to use this, so I'm okay to close this.

For the client struct, are you saying this client struct would live in eigenda or in eigenda-proxy? what's the reason to wrap around a node for a read only mode 🤔? what about waiting for contract storage changes for operator set and implement distributed retrieval for the retriever first?

ignore my suggestion, it is probably too low level of a suggestion.

okay, sorry if my questions were overwhelming. I'm more than happy to learn and understand what you think is best

epociask · 2024-09-18T04:27:43Z

e2e/setup.go

+	// load signer key from environment or generate a random one
 	pk := os.Getenv(privateKey)
 	if pk == "" && !testCfg.UseMemory {
-		t.Fatal("SIGNER_PRIVATE_KEY environment variable not set")
+		t.Logf("SIGNER_PRIVATE_KEY environment variable not set. Generated random private key")
+		randomBytes := make([]byte, 32)
+		if _, err := rand.Read(randomBytes); err != nil {
+			t.Fatalf("Failed to generate random private key: %v", err)
+		}
+		pk = hex.EncodeToString(randomBytes)
 	}


What does generating a unique key do? Looking at the disperser server implementation, rate limits are only enforced when dispersing.

Also authorization headers are only signed by the client when dispersing as well

All this to say - why can't the key just be a hard-set constant?

I didn't want to hardcode a private key constant, but I guess the key could be all zeros for all the read-only mode proxy cares. I will remove the random generation and add a readonly mode conditional here for setting a constant key

epociask · 2024-09-18T04:30:54Z

server/config.go

+		&cli.BoolFlag{
+			Name:    ReadOnlyFlagName,
+			Usage:   "Whether to run the proxy in read-only mode; set to true to only allow retrieval from EigenDA.",
+			Value:   false,
+			EnvVars: prefixEnvVars("READ_ONLY"),
+		},


let's also update the readme documentation so users know how to target this functionality

hopeyen · 2024-09-20T18:28:00Z

gonna close for now and create a separate one after updating eigenda-core's EigenDAClient

hopeyen added 2 commits September 17, 2024 12:12

feat: generate a random private key if not provided

2c7e31f

feat: read-only mode

d124ef8

hopeyen requested review from epociask and bxue-l2 September 17, 2024 10:15

samlaf reviewed Sep 17, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

bxue-l2 previously approved these changes Sep 17, 2024

View reviewed changes

samlaf reviewed Sep 17, 2024

View reviewed changes

server/config.go Outdated Show resolved Hide resolved

samlaf reviewed Sep 17, 2024

View reviewed changes

bxue-l2 self-requested a review September 17, 2024 20:53

refactor: moved inline func for rand pvk gen, modify readonly usage

ad430e6

bxue-l2 reviewed Sep 17, 2024

View reviewed changes

samlaf reviewed Sep 18, 2024

View reviewed changes

epociask reviewed Sep 18, 2024

View reviewed changes

hopeyen closed this Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read only mode #126

Read only mode #126

hopeyen commented Sep 17, 2024

bxue-l2 left a comment

samlaf Sep 17, 2024

hopeyen Sep 17, 2024

bxue-l2 Sep 19, 2024

samlaf Sep 17, 2024

hopeyen Sep 17, 2024

samlaf Sep 18, 2024

bxue-l2 Sep 17, 2024

hopeyen Sep 17, 2024

bxue-l2 Sep 19, 2024

hopeyen Sep 19, 2024

samlaf Sep 18, 2024

samlaf Sep 18, 2024

epociask Sep 18, 2024

epociask Sep 18, 2024

epociask Sep 18, 2024

hopeyen Sep 18, 2024

epociask Sep 18, 2024

samlaf Sep 18, 2024

samlaf Sep 18, 2024

hopeyen Sep 18, 2024

hopeyen Sep 18, 2024 •

edited

Loading

bxue-l2 Sep 19, 2024

hopeyen Sep 19, 2024

bxue-l2 Sep 19, 2024

hopeyen Sep 19, 2024

epociask Sep 18, 2024

epociask Sep 18, 2024

epociask Sep 18, 2024

hopeyen Sep 18, 2024

epociask Sep 18, 2024

hopeyen commented Sep 20, 2024

Read only mode #126

Read only mode #126

Conversation

hopeyen commented Sep 17, 2024

bxue-l2 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hopeyen Sep 18, 2024 • edited Loading

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

hopeyen commented Sep 20, 2024

hopeyen Sep 18, 2024 •

edited

Loading