feat: Integrate WeaveVM as a secondary backend in EigenDA proxy #198
Conversation
feat: send eigen blobs to wvm
docs: update README
docs: add PUT & GET workflows
docs: add archive pool info
Merge upstream
allnil
changed the title
…onfigs and client, add e2e test
allnil
force-pushed
the
feat/eigenda-wvm-code-integration
branch
from
158053a to
bee623c
Compare
allnil
changed the title
e2e/server_test.go
Outdated
| require.NoError(t, err) | ||
| require.Equal(t, expectedBlob, actualBlob) | ||
|
|
||
| requireSimpleClientSetGet(t, ts, e2e.RandBytes(1_000_000)) |
There was a problem hiding this comment.
iiuc you're doing the same assertion logic twice
There was a problem hiding this comment.
hmmm
frankly speaking I took test which was made for S3 and prepared weave vm config+backend type
so the core logic of the test suit is the same as it was
| @@ -19,6 +19,7 @@ Features: | |||
| * Performs DA certificate verification during retrieval to ensure that data represented by bad DA certificates do not become part of the canonical chain. | |||
| * Compatibility with Optimism's alt-da commitment type with eigenda backend. | |||
| * Compatibility with Optimism's keccak-256 commitment type with S3 storage. | |||
| * Blobs permanent backup storage option via [WeaveVM](./store/precomputed_key/wvm). | |||
|
|
|||
| In order to disperse to the EigenDA network in production, or at high throughput on testnet, please register your authentication ethereum address through [this form](https://forms.gle/3QRNTYhSMacVFNcU8). Your EigenDA authentication keypair address should not be associated with any funds anywhere. | |||
There was a problem hiding this comment.
can you please update the table to also reflect the WeaveVM cfg params?
e2e/server_test.go
Outdated
| privateKey := os.Getenv("WVM_PRIV_KEY") | ||
| if privateKey == "" { | ||
| t.Skip("Skipping test as WVM_PRIV_KEY has not been set") | ||
| } |
There was a problem hiding this comment.
how is this private key channeled to the server config?
There was a problem hiding this comment.
yep..I didn't supply it through cli package
I'll add it
flags/flags.go
Outdated
| @@ -24,6 +25,7 @@ const ( | |||
| S3Category = "S3 Cache/Fallback" | |||
| VerifierCategory = "KZG and Cert Verifier" | |||
| VerifierDeprecatedCategory = "DEPRECATED VERIFIER FLAGS -- THESE WILL BE REMOVED IN V2.0.0" | |||
| WVMCategory = "WVM Fallback/Perm Storage option" | |||
There was a problem hiding this comment.
can we explicitly prefix "weave"? most users won't know the term wvm
| @@ -210,6 +210,7 @@ require ( | |||
| github.com/opencontainers/image-spec v1.1.0 // indirect | |||
| github.com/opencontainers/runtime-spec v1.2.0 // indirect | |||
| github.com/opentracing/opentracing-go v1.2.0 // indirect | |||
| github.com/patrickmn/go-cache v2.1.0+incompatible // indirect | |||
There was a problem hiding this comment.
nice only one new dependency
store/precomputed_key/wvm/cli.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| EnabledFlagName = withFlagPrefix("enabled") |
There was a problem hiding this comment.
is there no env var for the wvm account private key?
| signer Signer | ||
| } | ||
|
|
||
| func NewWvmRPCClient(log log.Logger, cfg *wvmtypes.Config, signer Signer) (*RPCClient, error) { |
There was a problem hiding this comment.
is this shadowing an existing client implementation or is it written from scratch?
There was a problem hiding this comment.
mmm,
to make standard calls this package utilizes go-ethereum/ethclient and types
cause our node is a fork of ethereum client(in this case - reth) we are free from rewriting this part from scratch
everything else written around it
| return ethRPCClient, nil | ||
| } | ||
|
|
||
| func (rpc *RPCClient) SendTransaction(ctx context.Context, to string, data []byte) (string, error) { |
There was a problem hiding this comment.
just to verify - is there a maximum data size that can be submitted per tx?
| msg := ethereum.CallMsg{ | ||
| From: fromAddress, | ||
| To: &toAddr, | ||
| Gas: 0x00, |
There was a problem hiding this comment.
mmm, in this case just makes things explicit
as CallMsg says:
Gas uint64 // if 0, the call executes with near-infinite gas
| func (rpc *RPCClient) createRawTransaction(ctx context.Context, to string, data string, gasLimit uint64) (string, error) { | ||
| baseFee, err := rpc.client.SuggestGasPrice(ctx) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| fromAddress, err := rpc.signer.GetAccount(ctx) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get an account from signer: %w", err) | ||
| } | ||
| nonce, err := rpc.client.PendingNonceAt(ctx, fromAddress) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| signData := wvmtypes.SignData{To: to, Data: data, GasLimit: gasLimit, GasFeeCap: baseFee, Nonce: nonce} | ||
| return rpc.signer.SignTransaction(ctx, &signData) |
There was a problem hiding this comment.
are there async collisions that can occur if this function is called twice in parallel? e.g, incorrect nonce mgmt
There was a problem hiding this comment.
checked,
nonce from ethclient.PendingNonceAt call should be pretty reliable cause considers 'pending' transactions too
and if there will be any error during "send" the retry mechanism can repeat and resolve it
iiuc daproxy shouldn't have load of hundreds/thousands blobs which will transform in hundreds/thousands txs for the particular address
…o feat/eigenda-wvm-code-integration
…o feat/eigenda-wvm-code-integration
| TransactionTime string `json:"transactionTime,omitempty"` | ||
| TransactionCost string `json:"transactionCost,omitempty"` | ||
| } | ||
|
|
There was a problem hiding this comment.
can we please change the package name to snake-case weave_vm to ensure compatibility with existing module naming schema?
There was a problem hiding this comment.
renamed package weavevm => weave_vm
| fromAddress, err := rpc.signer.GetAccount(ctx) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to estimate gas, no signer") | ||
| } |
There was a problem hiding this comment.
should the underlying error also be wrapped here?
store/precomputed_key/weaveVM/wvm.go
Outdated
|
|
||
| // Store...wraps weaveVM client, ethclient and concurrent internal cache | ||
| type Store struct { | ||
| weaveVMClient WeaveVM |
There was a problem hiding this comment.
why not just call this client?
store/precomputed_key/weaveVM/wvm.go
Outdated
| type Store struct { | ||
| weaveVMClient WeaveVM | ||
| log log.Logger | ||
| txCache *cache.Cache |
There was a problem hiding this comment.
what's the purpose of this cache? Does it change hardware requirements and could it lead to OOM exceptions with high usage?
There was a problem hiding this comment.
right now we use it to store a mapping between commitment key and weave vm transaction hash
it's only temporary solution and should be changed alongside with arbitrary blob sizes
with rough estimations of 1 commitment per second and 15 days TTL + some overhead this cache shouldn't be more than 145MB
I tried to pick the most simple and "essentials" implementation which is thread-safety, has ttl expiry ,tested
store/store.go
Outdated
| @@ -52,6 +55,18 @@ func (cfg *Config) Check() error { | |||
| return fmt.Errorf("redis password is set, but endpoint is not") | |||
| } | |||
|
|
|||
| // NOTE: we take the MaxBlobLengthBytes from verify package as it is done in memstore | |||
| // 8Mb in bytes is 8_388_608 | |||
| if cfg.WeaveVMConfig.Enabled && verify.MaxBlobLengthBytes > 8_388_608 { | |||
There was a problem hiding this comment.
knit - let's add a constant var for 8_388_608
store/store.go
Outdated
| // NOTE: we take the MaxBlobLengthBytes from verify package as it is done in memstore | ||
| // 8Mb in bytes is 8_388_608 | ||
| if cfg.WeaveVMConfig.Enabled && verify.MaxBlobLengthBytes > 8_388_608 { | ||
| return fmt.Errorf("current max blob size with weavevm secondary backend enabled is 8Mb") |
| @@ -52,6 +55,18 @@ func (cfg *Config) Check() error { | |||
| return fmt.Errorf("redis password is set, but endpoint is not") | |||
| } | |||
|
|
|||
| // NOTE: we take the MaxBlobLengthBytes from verify package as it is done in memstore | |||
There was a problem hiding this comment.
what about the case where the user specifies both a private key and a remote signer config?
There was a problem hiding this comment.
in this case we will create a client which will use web3signer
Are you suggesting to fail check and explicitly tell a user to pass params for only one of them?
There was a problem hiding this comment.
I added a check
if there will be explicitly allowed to have multiple different signers at the same time it will be another story
store/precomputed_key/weaveVM/wvm.go
Outdated
| return "", fmt.Errorf("weaveVM backend: tx hash for provided commitment not found") | ||
| } | ||
|
|
||
| weaveVM.log.Info("weaveVM backned: tx hash using provided commitment FOUND", "provided key", string(key)) |
There was a problem hiding this comment.
does this warrant an info log?
…vided at the same time
Hey! PR from WeaveVM team to integrate our chain as secondary backend
Awaiting for review, notes and additional requests! :)
Changes proposed
Integrates weave-vm as a secondary storage
It implements required interfaces and adds cli/env configuration
Note to reviewers
so it shouldn't break the CI
https://gateway.wvm.dev/calldata/0xbc9ff1082e36595ea14f1109c817bcf35a73ad566f7a38d8dbdda2d82b50d5ab
https://explorer.wvm.dev/tx/0xbc9ff1082e36595ea14f1109c817bcf35a73ad566f7a38d8dbdda2d82b50d5ab
https://explorer.wvm.dev/tx/0x7bdd17fdfee0190499c78a78586aa82366d52d5636e939e7e35eb5df2c630195