Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Read only mode #126

Closed
wants to merge 3 commits into from
Closed

Read only mode #126

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c7e31f
feat: generate a random private key if not provided
hopeyen Sep 13, 2024
d124ef8
feat: read-only mode
hopeyen Sep 14, 2024
ad430e6
refactor: moved inline func for rand pvk gen, modify readonly usage
hopeyen Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .env.example.holesky
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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 as readonly <-> SignerPrivateKey="" or unset by the user?

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

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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 here

Copy link
Collaborator Author

@hopeyen hopeyen Sep 18, 2024 •
edited
Loading

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 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

Copy link
Collaborator

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

Copy link
Collaborator Author

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 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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

5 changes: 4 additions & 1 deletion .env.example.mainnet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,7 @@ EIGENDA_PROXY_SERVICE_MANAGER_ADDR=0x870679E138bCdf293b7Ff14dD44b70FC97e12fc0
# EIGENDA_PROXY_S3_BUCKET=

# endpoint for S3 storage
# EIGENDA_PROXY_S3_ENDPOINT
# EIGENDA_PROXY_S3_ENDPOINT

# set to true to disable writing to EigenDA
# EIGENDA_PROXY_READ_ONLY=false
6 changes: 6 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ HOLESKYTEST = TESTNET=true go test -timeout 50m -v ./e2e -parallel 4 -deploy-co
eigenda-proxy:
env GO111MODULE=on GOOS=$(TARGETOS) GOARCH=$(TARGETARCH) go build -v $(LDFLAGS) -o ./bin/eigenda-proxy ./cmd/server

.PHONY: eigenda-proxy-codesign
proxy-build-sign-run:
env GO111MODULE=on GOOS=$(TARGETOS) GOARCH=$(TARGETARCH) go build -v $(LDFLAGS) -o ./bin/eigenda-proxy ./cmd/server
codesign --sign - --force --preserve-metadata=entitlements,requirements,flags,runtime ./bin/eigenda-proxy
samlaf marked this conversation as resolved.
Show resolved Hide resolved
ENV_PATH=.env ./bin/eigenda-proxy --addr 127.0.0.1 --port 3100

.PHONY: docker-build
docker-build:
@docker build -t $(IMAGE_NAME) .
Expand Down
10 changes: 8 additions & 2 deletions e2e/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package e2e

import (
"context"
"encoding/hex"
"fmt"
"os"
"testing"
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
34 changes: 27 additions & 7 deletions server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package server

import (
"crypto/rand"
"encoding/hex"
"fmt"
"runtime"
"time"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -179,13 +183,22 @@ func ReadConfig(ctx *cli.Context) Config {
Timeout: ctx.Duration(S3TimeoutFlagName),
},
ClientConfig: clients.EigenDAClientConfig{
RPC: ctx.String(EigenDADisperserRPCFlagName),
StatusQueryRetryInterval: ctx.Duration(StatusQueryRetryIntervalFlagName),
StatusQueryTimeout: ctx.Duration(StatusQueryTimeoutFlagName),
DisableTLS: ctx.Bool(DisableTLSFlagName),
ResponseTimeout: ctx.Duration(ResponseTimeoutFlagName),
CustomQuorumIDs: ctx.UintSlice(CustomQuorumIDsFlagName),
SignerPrivateKeyHex: ctx.String(SignerPrivateKeyHexFlagName),
RPC: ctx.String(EigenDADisperserRPCFlagName),
StatusQueryRetryInterval: ctx.Duration(StatusQueryRetryIntervalFlagName),
StatusQueryTimeout: ctx.Duration(StatusQueryTimeoutFlagName),
DisableTLS: ctx.Bool(DisableTLSFlagName),
ResponseTimeout: ctx.Duration(ResponseTimeoutFlagName),
CustomQuorumIDs: ctx.UintSlice(CustomQuorumIDsFlagName),
SignerPrivateKeyHex: func() string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if key := ctx.String(SignerPrivateKeyHexFlagName); key != "" {
return key
}
randomBytes := make([]byte, 32)
if _, err := rand.Read(randomBytes); err != nil {
panic(fmt.Errorf("failed to generate random private key: %w", err))
}
return hex.EncodeToString(randomBytes)
}(),
hopeyen marked this conversation as resolved.
Show resolved Hide resolved
PutBlobEncodingVersion: codecs.BlobEncodingVersion(ctx.Uint(PutBlobEncodingVersionFlagName)),
DisablePointVerificationMode: ctx.Bool(DisablePointVerificationModeFlagName),
},
Expand All @@ -202,6 +215,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)

Expand Down Expand Up @@ -496,6 +510,12 @@ func CLIFlags() []cli.Flag {
Value: cli.NewStringSlice(),
EnvVars: prefixEnvVars("CACHE_TARGETS"),
},
&cli.BoolFlag{
Name: ReadOnlyFlagName,
Usage: "Whether the proxy should operate in read-only mode.",
Value: false,
EnvVars: prefixEnvVars("READ_ONLY"),
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

flags = append(flags, s3Flags()...)
Expand Down
2 changes: 1 addition & 1 deletion server/load_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,5 +121,5 @@ func LoadStoreRouter(ctx context.Context, cfg CLIConfig, log log.Logger) (store.
caches := populateTargets(cfg.EigenDAConfig.CacheTargets, s3, redis)

log.Info("Creating storage router", "eigenda backend type", eigenda != nil, "s3 backend type", s3 != nil)
return store.NewRouter(eigenda, s3, log, caches, fallbacks)
return store.NewRouter(eigenda, s3, log, caches, fallbacks, cfg.EigenDAConfig.ReadOnly)
}
14 changes: 10 additions & 4 deletions store/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 [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:)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 put methods no matter what. Are you saying the read-only mode should handle something more granular with the cache/fallback targets?


var commit []byte
var err error

Expand Down
Loading