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.

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

feat: Batch hash verification #39

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

epociask
Copy link
Collaborator

Pull Request Template

Fixes Issue

Fixes #

Changes proposed

Screenshots (Optional)

Note to reviewers

@@ -44,6 +47,10 @@ type Config struct {
// The blob encoding version to use when writing blobs from the high level interface.
PutBlobEncodingVersion codecs.BlobEncodingVersion

// ETH vars
EthRPC string
SvcManagerAddr string
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the readme with a table of these addresses across Holesky and Mainnet, or even better link to the canonical EigenDA contract readme where those addresses are written down.

@@ -73,21 +80,37 @@ func (c *Config) GetMaxBlobLength() (uint64, error) {
return c.maxBlobLengthBytes, nil
}

func (c *Config) KzgConfig() *kzg.KzgConfig {
func (c *Config) VerificationCfg() *verify.Config {
Copy link
Contributor

@teddyknox teddyknox Jun 18, 2024

Choose a reason for hiding this comment

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

Nit: Never understood why people abbreviate 'config' to remove 3 letters. Feel free to ignore.

@@ -199,5 +224,15 @@ func CLIFlags(envPrefix string) []cli.Flag {
Usage: "Directory path to SRS tables",
EnvVars: prefixEnvVars("TARGET_CACHE_PATH"),
},
&cli.StringFlag{
Name: EthRPCFlagName,
Usage: "JSON RPC node endpoint for the Ethereum network used for finalizing DA blobs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on putting an example RPC url in the description and a default value for a public Holesky RPC?

&cli.StringFlag{
Name: SvcManagerAddrFlagName,
Usage: "Deployed EigenDA service manager address.",
EnvVars: prefixEnvVars("SERVICE_MANAGER_ADDR"),
Copy link
Contributor

Choose a reason for hiding this comment

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

A default value for the Holesky address might be nice.

Copy link
Contributor

@teddyknox teddyknox Jun 18, 2024

Choose a reason for hiding this comment

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

I'm realizing it might be good to check into the repo certain "configuration profiles" representing good defaults for testnet and mainnet. Maybe networks/mainnet.env and networks/holesky.env. To make it the invocation of ./bin/eigenda-proxy solid state (a one liner) we would need to add a special command line flag --env-file <filepath> that let you use one of these files for settings.

@@ -28,6 +34,7 @@ func LoadStore(cfg CLIConfig, ctx context.Context, log log.Logger) (store.Store,
return store.NewMemStore(ctx, &cfg.MemStoreCfg, verifier, log, maxBlobLength)
}

log.Info("Using eigenda backend")
Copy link
Contributor

@teddyknox teddyknox Jun 18, 2024

Choose a reason for hiding this comment

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

nit: 'EigenDA' in any user-facing outputs

@@ -120,7 +120,7 @@ func (e *MemStore) Get(ctx context.Context, commit []byte, domain eigendacommon.
}

// Don't need to do this really since it's a mock store
err = e.verifier.Verify(cert.BlobHeader.Commitment, encodedBlob)
err = e.verifier.VerifyCommitment(cert.BlobHeader.Commitment, encodedBlob)
Copy link
Contributor

@teddyknox teddyknox Jun 18, 2024

Choose a reason for hiding this comment

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

Observation, not urgent: The fact that there's a decent amount of code duplication between the EigenDA store and memory store suggests to me that we could do some refactoring.

return err
}

equal := proxy_common.EqualSlices(expectedHash[:], actualHash[:])
Copy link
Contributor

@teddyknox teddyknox Jun 18, 2024

Choose a reason for hiding this comment

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

Nit: "golang.org/x/exp/slices" gives you slices.Equal(). Again, can ignore.

https://chatgpt.com/share/79d6326a-d0e3-4192-829b-ee3064dccbc9

@epociask epociask merged commit 2221b6b into main Jun 20, 2024
3 of 4 checks passed
@epociask epociask deleted the epociask--batch-hash-verification branch August 22, 2024 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants