-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Read only mode #126
Read only mode #126
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package e2e | |
|
||
import ( | ||
"context" | ||
"encoding/hex" | ||
"fmt" | ||
"os" | ||
"testing" | ||
|
@@ -93,10 +94,15 @@ type TestSuite struct { | |
func CreateTestSuite(t *testing.T, testCfg *Cfg) (TestSuite, func()) { | ||
ctx := context.Background() | ||
|
||
// load signer key from environment | ||
// 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) | ||
} | ||
Comment on lines
+97
to
106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does generating a unique key do? Looking at the disperser server implementation, rate limits are only enforced when dispersing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also authorization headers are only signed by the client when dispersing as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this to say - why can't the key just be a hard-set constant? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
// load node url from environment | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,8 @@ | ||||||||||||||||||||||||||||||||||||||
package server | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||
"crypto/rand" | ||||||||||||||||||||||||||||||||||||||
"encoding/hex" | ||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||
"runtime" | ||||||||||||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||||||||||||
|
@@ -28,6 +30,7 @@ const ( | |||||||||||||||||||||||||||||||||||||
SignerPrivateKeyHexFlagName = "eigenda-signer-private-key-hex" | ||||||||||||||||||||||||||||||||||||||
PutBlobEncodingVersionFlagName = "eigenda-put-blob-encoding-version" | ||||||||||||||||||||||||||||||||||||||
DisablePointVerificationModeFlagName = "eigenda-disable-point-verification-mode" | ||||||||||||||||||||||||||||||||||||||
ReadOnlyFlagName = "read-only" | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// kzg flags | ||||||||||||||||||||||||||||||||||||||
G1PathFlagName = "eigenda-g1-path" | ||||||||||||||||||||||||||||||||||||||
|
@@ -75,6 +78,7 @@ var ( | |||||||||||||||||||||||||||||||||||||
type Config struct { | ||||||||||||||||||||||||||||||||||||||
// eigenda | ||||||||||||||||||||||||||||||||||||||
ClientConfig clients.EigenDAClientConfig | ||||||||||||||||||||||||||||||||||||||
ReadOnly bool | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// the blob encoding version to use when writing blobs from the high level interface. | ||||||||||||||||||||||||||||||||||||||
PutBlobEncodingVersion codecs.BlobEncodingVersion | ||||||||||||||||||||||||||||||||||||||
|
@@ -161,6 +165,17 @@ func (cfg *Config) VerificationCfg() *verify.Config { | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// ReadConfig ... parses the Config from the provided flags or environment variables. | ||||||||||||||||||||||||||||||||||||||
func ReadConfig(ctx *cli.Context) Config { | ||||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+168
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Comment on lines
+168
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can we do something simpler like this? Really don't like inline functions when not needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
cfg := Config{ | ||||||||||||||||||||||||||||||||||||||
RedisCfg: store.RedisConfig{ | ||||||||||||||||||||||||||||||||||||||
Endpoint: ctx.String(RedisEndpointFlagName), | ||||||||||||||||||||||||||||||||||||||
|
@@ -185,7 +200,7 @@ func ReadConfig(ctx *cli.Context) Config { | |||||||||||||||||||||||||||||||||||||
DisableTLS: ctx.Bool(DisableTLSFlagName), | ||||||||||||||||||||||||||||||||||||||
ResponseTimeout: ctx.Duration(ResponseTimeoutFlagName), | ||||||||||||||||||||||||||||||||||||||
CustomQuorumIDs: ctx.UintSlice(CustomQuorumIDsFlagName), | ||||||||||||||||||||||||||||||||||||||
SignerPrivateKeyHex: ctx.String(SignerPrivateKeyHexFlagName), | ||||||||||||||||||||||||||||||||||||||
SignerPrivateKeyHex: signerPrivateKeyHex, | ||||||||||||||||||||||||||||||||||||||
PutBlobEncodingVersion: codecs.BlobEncodingVersion(ctx.Uint(PutBlobEncodingVersionFlagName)), | ||||||||||||||||||||||||||||||||||||||
DisablePointVerificationMode: ctx.Bool(DisablePointVerificationModeFlagName), | ||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||
|
@@ -202,6 +217,7 @@ func ReadConfig(ctx *cli.Context) Config { | |||||||||||||||||||||||||||||||||||||
MemstorePutLatency: ctx.Duration(MemstorePutLatencyFlagName), | ||||||||||||||||||||||||||||||||||||||
FallbackTargets: ctx.StringSlice(FallbackTargets), | ||||||||||||||||||||||||||||||||||||||
CacheTargets: ctx.StringSlice(CacheTargets), | ||||||||||||||||||||||||||||||||||||||
ReadOnly: ctx.Bool(ReadOnlyFlagName), | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
cfg.ClientConfig.WaitForFinalization = (cfg.EthConfirmationDepth < 0) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -496,6 +512,12 @@ func CLIFlags() []cli.Flag { | |||||||||||||||||||||||||||||||||||||
Value: cli.NewStringSlice(), | ||||||||||||||||||||||||||||||||||||||
EnvVars: prefixEnvVars("CACHE_TARGETS"), | ||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||
&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"), | ||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+515
to
+520
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's also update the readme documentation so users know how to target this functionality |
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
flags = append(flags, s3Flags()...) | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,10 @@ type IRouter interface { | |
|
||
// Router ... storage backend routing layer | ||
type Router struct { | ||
log log.Logger | ||
eigenda KeyGeneratedStore | ||
s3 PrecomputedKeyStore | ||
log log.Logger | ||
eigenda KeyGeneratedStore | ||
s3 PrecomputedKeyStore | ||
readonly bool | ||
|
||
caches []PrecomputedKeyStore | ||
cacheLock sync.RWMutex | ||
|
@@ -37,11 +38,12 @@ type Router struct { | |
} | ||
|
||
func NewRouter(eigenda KeyGeneratedStore, s3 PrecomputedKeyStore, l log.Logger, | ||
caches []PrecomputedKeyStore, fallbacks []PrecomputedKeyStore) (IRouter, error) { | ||
caches []PrecomputedKeyStore, fallbacks []PrecomputedKeyStore, readonly bool) (IRouter, error) { | ||
return &Router{ | ||
log: l, | ||
eigenda: eigenda, | ||
s3: s3, | ||
readonly: readonly, | ||
caches: caches, | ||
cacheLock: sync.RWMutex{}, | ||
fallbacks: fallbacks, | ||
|
@@ -117,6 +119,10 @@ func (r *Router) Get(ctx context.Context, key []byte, cm commitments.CommitmentM | |
|
||
// Put ... inserts a value into a storage backend based on the commitment mode | ||
func (r *Router) Put(ctx context.Context, cm commitments.CommitmentMode, key, value []byte) ([]byte, error) { | ||
if r.readonly { | ||
return nil, errors.New("read-only router does not support Put") | ||
} | ||
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Also thought about making a separate router type that contains exactly this super simple Please suggest and I'm happy to learn and redo:) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if I just wanna run proxy for S3 support using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused, the read-only mode implemented here will disable all |
||
|
||
var commit []byte | ||
var err error | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 asreadonly <-> SignerPrivateKey=""
or unset by the user?A lil hesitant to introduce more env flags, thoughts?
cc - @samlaf, @bxue-l2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reevaluating the approach. Currently
EigenDAClient
only contains a dispersal client that has a separate definition forRetrieveBlob
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 anEigenDAClient
and letretrieveBlob
serveGet
and return error forPut
. 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 syncThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ineigenda-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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore my suggestion, it is probably too low level of a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, sorry if my questions were overwhelming. I'm more than happy to learn and understand what you think is best