Implement eigenDA client remaining features #3243

juanbono · 2024-11-08T22:09:32Z

What ❔

This PR adds extra functionality to the eigen client:

Non authenticated blob dispersal feat(eigen-client-extra-features): non auth dispersal lambdaclass/zksync-era#318
RLP encoding for blobs feat(eigen-client-extra-feature): rlp encoding lambdaclass/zksync-era#319
Get inclusion data feat(eigen-client-extra-features): get inclusion data lambdaclass/zksync-era#324
Memstore feat(eigen-client-extra-features): Eigen client memstore lambdaclass/zksync-era#321
Blob size limit feat(eigen-client-extra-features): blob size limit lambdaclass/zksync-era#325
Soft confirmations feat(eigen-client-extra-features): Add soft confirmations lambdaclass/zksync-era#322
Verification logic feat(eigen-client-extra-features): Verifier lambdaclass/zksync-era#326
EigenDA Documentation feat(eigen-client-extra-features): Eigen client documentation lambdaclass/zksync-era#327

Why ❔

These features are needed to have a full featured client and to have the same features as when communicating with the EigenDA proxy.

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zkstack dev fmt and zkstack dev lint.

impl rlp encoding

Implement non auth blob dispersal for eigen client --------- Co-authored-by: Gianbelinche <[email protected]>

feat(eigen-client-extra-features): get inclusion data

add test for remote disperser cfg

* initial commit * Add tests * Add memstore * Add assert to tests * Add rest of memstore * Address pr comments * Remove to retriable error * Fix conflicts * Document memstore * Fix typo --------- Co-authored-by: Juan Munoz <[email protected]>

blob size limit --------- Co-authored-by: Gianbelinche <[email protected]>

* initial commit * Add tests * Add memstore * Add assert to tests * Add rest of memstore * Add soft confirmations * Remove print * Address pr comments * Remove to retriable error * Fix conflicts * Add inclusion data * Change status query timeout to millis * Document memstore * Fix typo * Fix typo * Format --------- Co-authored-by: Juan Munoz <[email protected]>

* initial commit * Add tests * Add memstore * Add assert to tests * Add rest of memstore * Address pr comments * Remove to retriable error * Fix conflicts * Add verifier * Fix verifier * Add path to points to config * Fix typo * Fix eigenda env test * Fix verifier test * Move eigendaservicemanager to generated * Remove unneeded imports * Document verifier * Modify errors * Address comments * Fix conflicts --------- Co-authored-by: Juan Munoz <[email protected]>

* initial commit * move file location * fix step

* Add eth confirmation depth * Fix error

#329) * Remove unused custom quorum numbers * Add serial to tests

feat(eigen-client-extra-features): fix clippy and add doc

feat(eigen-client-extra-features): merge main

…dule-update feat(eigen-client-extra-features): update contracts submodule

core/node/da_clients/src/eigen/verifier.rs

dimazhornyk · 2024-11-14T10:49:41Z

core/node/da_clients/src/eigen/verifier.rs

+    fn test_verify_commitment() {
+        let verifier = super::Verifier::new(super::VerifierConfig {
+            verify_certs: true,
+            rpc_url: "https://ethereum-holesky-rpc.publicnode.com".to_string(),


this looks like the test relies on the external RPC, which means it could be blocking the CI if RPC is unavailable
let's rework this to remove the external dependency

yeah, the thing is that we can't test it locally since there is no working local setup for EigenDA. So we had to test on testnet directly.

can we use mocks instead of external calls?

dimazhornyk · 2024-11-15T06:24:14Z

core/node/da_clients/src/eigen/sdk.rs


+        let verifier_config = VerifierConfig {
+            verify_certs: true,


maybe let's make it a part of the config

what part are you referring to?

I initially thought about verify_certs variable, but now that I think about it, it shouldn't exist at all
to verify the inclusion or not should be defined by the use_dummy_inclusion_data variable, if it is true - the verifier won't even be used

dimazhornyk · 2024-11-15T06:34:06Z

core/node/da_clients/src/eigen/sdk.rs

+            rpc_url: config.eigenda_eth_rpc.clone(),
+            svc_manager_addr: config.eigenda_svc_manager_address.clone(),
+            max_blob_size: config.blob_size_limit,
+            path_to_points: config.path_to_points.clone(),


Do I understand correctly that these points have to be present every time this code is run? This would add an ops load that we would ideally want to avoid. Is there a way to get rid of the points file?

Yes, we need it every time that code is run. One solution can be to have the contents of the file bundled with the binary. The downside of that approach is that it will increase the size of the resulting binary in ~30mb

hmm, I see
none of it is a great option tbh, let's do something to optimize this
maybe we could ask the EigenDA team to publish it to the S3/GCS bucket managed by them, and then in case the client is started with the verification enabled - it will fetch the points from the bucket

core/node/da_clients/src/eigen/sdk.rs

core/node/da_clients/src/eigen/memstore.rs

core/node/da_clients/Cargo.toml

core/lib/protobuf_config/src/proto/config/da_client.proto

* Remove kzgpad * Add empty pad and unpad test

* Remove kzgpad * Move dependencies

* Add backon instead loops * Add constant for avg block time

* Remove extra config * Use bn254 crate

* Add zksync client * Call service manager with zksync client * Add rest of calls * Remove alloy * Remove unwraps * Remove todos * Harcode function selectors * Remove service manager json * Fix clippy

…merge-main Eigen client extra features merge main

dimazhornyk · 2024-11-25T09:07:51Z

core/lib/config/src/configs/da_client/eigen.rs

+    pub disperser_rpc: String,
+    /// Block height needed to reach in order to consider the blob finalized
+    /// a value less or equal to 0 means that the disperser will not wait for finalization
+    pub eth_confirmation_depth: i32,


it's better to use l1_ or settlement_layer_ here instead of eth_ because the settlement layer is not necessarily an Ethereum mainnet

dimazhornyk · 2024-11-25T09:09:33Z

core/lib/config/src/configs/da_client/eigen.rs

+    /// a value less or equal to 0 means that the disperser will not wait for finalization
+    pub eth_confirmation_depth: i32,
+    /// URL of the Ethereum RPC server
+    pub eigenda_eth_rpc: String,


we can reuse the RPC that's used for sending txs to L1, it is already provided in the secrets

dimazhornyk · 2024-11-25T09:12:43Z

core/lib/config/src/configs/da_client/eigen.rs

+    /// Address of the service manager contract
+    pub eigenda_svc_manager_address: String,
+    /// Maximum size permitted for a blob in bytes
+    pub blob_size_limit: u32,


this shouldn't be a part of the config, it is defined by the "protocol" i.e. the max value that EigenDA can handle and there is a check in da_dispatcher that max_pubdata_per_batch is lower than that value

dimazhornyk · 2024-11-25T09:15:30Z

core/lib/config/src/configs/da_client/eigen.rs

+    /// Path to the file containing the points used for KZG
+    pub path_to_points: String,
+    /// Chain ID of the Ethereum network
+    pub chain_id: u64,


seems like it is unused

dimazhornyk · 2024-11-25T09:17:21Z

core/lib/protobuf_config/src/proto/config/da_client.proto

@@ -37,8 +37,19 @@ message CelestiaConfig {
 }

 message EigenConfig {
-  optional string rpc_node_url = 1;
-  optional uint64 inclusion_polling_interval_ms = 2;
+  reserved 1,2;


please move it to the bottom of the message for better readability and let's also reserve the field name, not only the index

dimazhornyk · 2024-11-25T10:06:51Z

core/node/da_clients/src/eigen/sdk.rs

+        (|| async { self.verifier.verify_certificate(blob_info.clone()).await })
+            .retry(
+                &ConstantBuilder::default()
+                    .with_delay(Duration::from_secs(AVG_BLOCK_TIME))


you shouldn't use 12s delay here
if we assume this func is called with some interval (defined by da_dispatcher) - we shouldn't be doing any sleeps here at all, only retries in case the request to L1 failed

dimazhornyk · 2024-11-25T10:07:21Z

core/node/da_clients/src/eigen/sdk.rs

+            .verify_commitment(blob_info.blob_header.commitment.clone(), data)
+            .map_err(|_| anyhow::anyhow!("Failed to verify commitment"))?;
+
+        self.loop_verify_certificate(blob_info.clone(), disperse_elapsed)


same here, dispatch shouldn't be blocked by inclusion

dimazhornyk · 2024-11-25T10:11:25Z

core/node/da_clients/src/eigen/verifier.rs

+}
+
+impl Verifier {
+    const DEFAULT_PRIORITY_FEE_PER_GAS: u64 = 100;


why is it needed if the client won't be sending any txs?

dimazhornyk · 2024-11-25T10:13:09Z

core/node/da_clients/src/eigen/verifier.rs

+
+impl Verifier {
+    const DEFAULT_PRIORITY_FEE_PER_GAS: u64 = 100;
+    const BATCH_ID_TO_METADATA_HASH_FUNCTION_SELECTOR: [u8; 4] = [236, 203, 191, 201];


this is going to be difficult to maintain, could you please use something like this?

just recalled that there is an easier way where you don't need to include the contracts in our codebase, example:

let func_selector = ethabi::short_signature( "validatePubdata", &[ ParamType::FixedBytes(32), ParamType::FixedBytes(32), ParamType::FixedBytes(32), ParamType::FixedBytes(32), ParamType::Bytes, ], );

dimazhornyk · 2024-11-25T10:15:42Z

core/node/da_clients/src/eigen/verifier.rs

+    fn test_verify_commitment() {
+        let verifier = super::Verifier::new(super::VerifierConfig {
+            verify_certs: true,
+            rpc_url: "https://ethereum-holesky-rpc.publicnode.com".to_string(),


can we use mocks instead of external calls?

* Move inclusion logic * Remove unused import

* Move inclusion logic * Remove verify cert

* Change eth conf depth for settlement layer * Remove blob size limit * Move reserved * Change function name * Add constants to lib * Fix compilation

* initial commit * remove anyhow err * Format code * Fix compilation --------- Co-authored-by: Gianbelinche <[email protected]>

* Add option to download points * Fix test * Fix clippy * Change name to points source * Fix test * Fix test

…s with mock tests (#356) * initial commit * create verifier client trait * remove unwraps * Update core/node/da_clients/src/eigen/verifier.rs Ignore test Co-authored-by: Gianbelinche <[email protected]> * Update core/node/da_clients/src/eigen/sdk.rs Co-authored-by: Gianbelinche <[email protected]> * change ignore test comment * Format code --------- Co-authored-by: Gianbelinche <[email protected]>

…on docs (#360) * Remove steps to run from integration docs * Combine readme * Clarify readme

…and-main feat(eigen-client-extra-features): Merge Main

dimazhornyk · 2024-12-04T11:02:42Z

core/lib/config/src/configs/da_client/eigen.rs

+
+impl Default for PointsSource {
+    fn default() -> Self {
+        PointsSource::Path("".to_string())


I assume that this was added to allow #[derive(Default)] for EigenConfig, but if someone actually uses it - it won't work for them
can we publish the points to some storage, e.g. GCS, and refer to that storage by default

dimazhornyk · 2024-12-04T11:04:38Z

core/lib/config/src/configs/da_client/eigen.rs

+    /// Authenticated dispersal
+    pub authenticated: bool,
+    /// Verify the certificate of dispatched blobs
+    pub verify_cert: bool,


is it used anywhere?

dimazhornyk · 2024-12-04T11:13:21Z

core/lib/protobuf_config/src/proto/config/da_client.proto

+  optional bool authenticated = 10;
+  optional bool verify_cert = 11;
+  oneof points_source {
+    Path path = 12;


you can do it like this

oneof points_source { string path = 12; string link = 13; }

oneof assumes that it is optional by default

dimazhornyk · 2024-12-04T11:17:41Z

core/node/da_clients/src/eigen/README.md

-The generated files are received by compiling the `.proto` files from EigenDA repo using the following function:
+There a 3 milestones defined, we are currently on the first one.
+
+### M0: Read and Write integration


as I understand, that's more of a Lambda's milestone here, MatterLabs has slightly different ones
let's consider either removing this or making it more generic like

create an MVP client

improve the client //we are here

enable verification on L1

optimize

dimazhornyk · 2024-12-04T11:19:53Z

core/node/da_clients/src/eigen/README.md

+
+For this milestone the scope is to replace the mocked L1 verification logic with EigenDA compatible verifier. It should
+integrate EigenDA certificate verification, and use it as the equivalent part for 4844 versioned hash. More importantly
+modify the equivalence proof in Zksync L1 contract such that the proof can be verified correctly with respect to the


if I understand it correctly - you are saying you want to integrate the EigenDA inclusion verification in our circuits
I doubt that it is possible, most likely a separate proof will be used for it

dimazhornyk · 2024-12-04T12:53:22Z

core/node/da_clients/src/eigen/verifier.rs

+impl Verifier {
+    pub const DEFAULT_PRIORITY_FEE_PER_GAS: u64 = 100;
+    async fn save_points(link: String) -> Result<String, VerificationError> {
+        let url_g1 = format!("{}{}", link, "/g1.point");


let's better use the Url package for this, to avoid issues when someone provides a path with trailing slash and it all fails

dimazhornyk · 2024-12-04T12:55:17Z

core/node/da_clients/src/eigen/verifier.rs

+            &format!("{}{}", path, "/g1.point"),
+            "",
+            &format!("{}{}", path, "/g2.point.powerOf2"),
+            268435456, // 2 ^ 28


let's maybe use constant for it?

dimazhornyk · 2024-12-04T13:01:17Z

core/node/da_clients/src/eigen/verifier.rs

+        let mut data = BATCH_ID_TO_METADATA_HASH_FUNCTION_SELECTOR.to_vec();
+        let mut batch_id_vec = [0u8; 32];
+        U256::from(cert.blob_verification_proof.batch_id).to_big_endian(&mut batch_id_vec);
+        data.append(batch_id_vec.to_vec().as_mut());
+
+        let call_request = CallRequest {
+            to: Some(
+                H160::from_str(&self.cfg.svc_manager_addr)
+                    .map_err(|_| VerificationError::ServiceManagerError)?,
+            ),
+            data: Some(zksync_basic_types::web3::Bytes(data)),
+            ..Default::default()
+        };
+
+        let res = self
+            .signing_client
+            .as_ref()
+            .call_contract_function(
+                call_request,
+                Some(BlockId::Number(BlockNumber::Number(context_block.into()))),
+            )
+            .await
+            .map_err(|_| VerificationError::ServiceManagerError)?;
+
+        let expected_hash = res.0.to_vec();


all of this can be moved to the separate func for better readability

dimazhornyk · 2024-12-04T13:03:44Z

core/node/da_clients/src/eigen/verifier.rs

+        }
+    }
+
+    /// Mock struct for the Verifier


same here with tests, can they be moved to a separate file?

dimazhornyk · 2024-12-04T13:04:17Z

resources/g1.point

as per comment above, let's remove those and use some remote storage as default

juan518munoz and others added 23 commits October 31, 2024 17:14

add integration doc

fcecc81

Reimplement eigen config (#320)

2bcf78f

feat(eigen-client-extra-feature): rlp encoding (#319)

537cf92

impl rlp encoding

feat(eigen-client-extra-features): non auth dispersal (#318)

f58e0bf

Implement non auth blob dispersal for eigen client --------- Co-authored-by: Gianbelinche <[email protected]>

initial commit

04b93e6

add spaces

c121eca

Revert wrong push

4a38ee2

initial commit

e64cc2e

Merge pull request #324 from lambdaclass/eigen-client-get-inclusion-data

5b8665b

feat(eigen-client-extra-features): get inclusion data

fix(eigen-client-extra-features): fix eigen config test (#323)

aa9f1b2

add test for remote disperser cfg

feat(eigen-client-extra-features): blob size limit (#325)

b164870

blob size limit --------- Co-authored-by: Gianbelinche <[email protected]>

feat(eigen-client-extra-features): Eigen client documentation (#327)

3b9e766

* initial commit * move file location * fix step

feat(eigen-client-extra-features): Eth confirmation depth (#328)

6668ab9

* Add eth confirmation depth * Fix error

feat(eigen-client-extra-features): Remove unused custom quorum numbers (

cc9e2be

#329) * Remove unused custom quorum numbers * Add serial to tests

doc & cargo check all

9769a17

clippy

91bb79b

Merge branch 'main' into eigen-client-merge-main

cbe124e

update contracts submodule

9c73b3f

Merge pull request #330 from lambdaclass/eigen-client-clippy-and-doc

94ecb52

feat(eigen-client-extra-features): fix clippy and add doc

Merge pull request #331 from lambdaclass/eigen-client-merge-main

342b476

feat(eigen-client-extra-features): merge main

github-actions bot added the external-contribution label Nov 8, 2024

juanbono changed the title ~~Eigen client extra features~~ Implement eigenDA client remaining features Nov 8, 2024

juanbono and others added 2 commits November 8, 2024 19:14

Merge branch 'main' into eigen-client-extra-features

077bc13

Update integration readme (#335)

c98726a

juan518munoz mentioned this pull request Nov 11, 2024

feat(main): add eigen client extra features lambdaclass/zksync-era#317

Closed

12 tasks

juan518munoz and others added 2 commits November 12, 2024 11:02

update contracts submodule

991c340

Merge pull request #337 from lambdaclass/eigen-client-contracts-submo…

f55a424

…dule-update feat(eigen-client-extra-features): update contracts submodule

dimazhornyk requested changes Nov 15, 2024

View reviewed changes

gianbelinche added 13 commits November 15, 2024 18:19

feat(eigen-client-extra-features): Remove kzgpad (#339)

f2fb12c

* Remove kzgpad * Add empty pad and unpad test

Create eigendaservicemanager type (#340)

b825d4f

Change match to if (#341)

9561bc3

feat(eigen-client-eigen-features): Move dependencies (#342)

986a1d2

* Remove kzgpad * Move dependencies

feat(eigen-client-extra-features): Backon (#344)

09ecfdd

* Add backon instead loops * Add constant for avg block time

Remove memstore (#346)

a2ddcc3

Add reserved to proto (#343)

7d369a8

Remove extra config (#351)

a9c31a7

feat(eigen-client-extra-features): Kzg bn254 crate (#352)

6e1de82

* Remove extra config * Use bn254 crate

feat(eigen-client-extra-features): Remove Alloy Dependency (#349)

8d0b908

* Add zksync client * Call service manager with zksync client * Add rest of calls * Remove alloy * Remove unwraps * Remove todos * Harcode function selectors * Remove service manager json * Fix clippy

Merge branch 'main' into eigen-client-extra-features-merge-main

28c826c

Fix cargo lock

68abc97

Merge pull request #354 from lambdaclass/eigen-client-extra-features-…

99b153a

…merge-main Eigen client extra features merge main

dimazhornyk requested changes Nov 25, 2024

View reviewed changes

gianbelinche mentioned this pull request Nov 26, 2024

docs: add eigenda steps to run matter-labs/zksync-docs#271

Open

gianbelinche and others added 11 commits November 28, 2024 15:38

feat(eigen-client-extra-features): Move inclusion logic (#357)

01b50ed

* Move inclusion logic * Remove unused import

feat(eigen-client-extra-features): Remove verify cert (#359)

daace34

* Move inclusion logic * Remove verify cert

feat(eigen-client-extra-features): Small changes (#358)

99c6d42

* Change eth conf depth for settlement layer * Remove blob size limit * Move reserved * Change function name * Add constants to lib * Fix compilation

feat(eigen-client-extra-features): minimal client changes (#362)

163c41a

* initial commit * remove anyhow err * Format code * Fix compilation --------- Co-authored-by: Gianbelinche <[email protected]>

feat(eigen-client-extra-features): Add option to download points (#361)

cef9a89

* Add option to download points * Fix test * Fix clippy * Change name to points source * Fix test * Fix test

feat(eigen-client-extra-features): Remove steps to run from integrati…

7a61574

…on docs (#360) * Remove steps to run from integration docs * Combine readme * Clarify readme

Merge branch 'main' into eigen-client-extra-features-and-main

bc90cc6

Update Cargo.lock

87d450f

Merge pull request #363 from lambdaclass/eigen-client-extra-features-…

e2d4dab

…and-main feat(eigen-client-extra-features): Merge Main

Merge branch 'main' into eigen-client-extra-features

4d1fadb

dimazhornyk requested changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement eigenDA client remaining features #3243

Implement eigenDA client remaining features #3243

juanbono commented Nov 8, 2024 •

edited

Loading

dimazhornyk Nov 14, 2024

juanbono Nov 15, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 15, 2024

juanbono Nov 15, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 15, 2024

juanbono Nov 15, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Nov 27, 2024

dimazhornyk Nov 25, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

dimazhornyk Dec 4, 2024

Implement eigenDA client remaining features #3243

Are you sure you want to change the base?

Implement eigenDA client remaining features #3243

Conversation

juanbono commented Nov 8, 2024 • edited Loading

What ❔

Why ❔

Checklist

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

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

juanbono commented Nov 8, 2024 •

edited

Loading