feat: add initial ethereum light client #150

gjermundgaraba · 2024-12-05T19:03:27Z

Description

closes: #142

What is not covered here:

Documentation (Document 08-wasm ethereum light client #146)
Perfect code (we can add things to do later in Code cleanup of 08-wasm etheruem light client #147)
Replacing the current wasm binary in the e2e test (Integrate 08-wasm etheruem light client into e2e test #143)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
Wrote unit and integration tests.
Added relevant natspec and godoc comments.
Provide a conventional commit message to follow the repository standards.
Re-reviewed Files changed in the GitHub PR explorer.
Review SonarCloud Report in the comment section below once CI passes.

…-light-client

codecov · 2024-12-05T19:07:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.83%. Comparing base (66bed7c) to head (b1e90e2).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #150   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files          11       11           
  Lines         537      537           
=======================================
  Hits          520      520           
  Misses         17       17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

srdtrk

This is not really a review but some early suggestions

packages/ethereum-light-client/src/config.rs

Cargo.toml

packages/tree_hash/Cargo.toml

…-light-client

gjermundgaraba

Added some comments and explanations + in the PR itself described what is covered and not.

Cargo.toml

gjermundgaraba · 2024-12-07T16:52:29Z

Cargo.toml

+# dev-dependencies
+cw-multi-test = { version = "2.2.0", default-features = false }
+milagro_bls   = { git = "https://github.com/Snowfork/milagro_bls", rev = "bc2b5b5e8d48b7e2e1bfaa56dc2d93e13cb32095", default-features = false } # Only used for testing, not to be used in production!
+
 [patch.crates-io]


Should these patches actually be moved to where they are needed only? Not sure if it's a great idea to do SP1 specific patching on the workspace level?

I think patches only work in the top level. Happy to move it lower if a PR can make that work

Yeah, seems like it is not possible. We should be very sure that this will not create any issues because it seems less than ideal to have to pin to these patches for all packages here :/

It shouldn't because sp1 is the most security critical part of our code. Given that we would use these patches in the sp1 programs, it doesn't matter whether other apps use these or not. They shouldn't be changing functionality if the target isn't riscv anyway

gjermundgaraba · 2024-12-07T16:53:39Z

e2e/interchaintestv8/e2esuite/utils.go

@@ -439,3 +440,12 @@ func (s *TestSuite) CreateTMClientHeader(
 		TrustedValidators: oldHeader.TrustedValidators,
 	}
 }
+
+func (s *TestSuite) GetTopLevelTestName() string {


Used to create the rust fixture file names.

gjermundgaraba · 2024-12-07T16:54:51Z

e2e/interchaintestv8/ibc_eureka_test.go

-	generateFixtures bool
+	// Whether to generate fixtures for tests or not
+	generateSolidityFixtures bool
+	rustFixtureGenerator     *types.RustFixtureGenerator


I decided to create a "persistent" generator so that I could create fixtures that are numbered in the order they happen (to be able to run them like that in unit tests as well).

gjermundgaraba · 2024-12-07T16:59:00Z

justfile

@@ -116,23 +116,23 @@ deploy-sp1-ics07: genesis-sp1-ics07
 generate-fixtures-solidity: clean


I'll add the rust fixtures here in #143

Did not want to add them just yet because the output is not 100% compatible until the issue over is done.

Hmm. Can we still add a separate recipe generate-fixtures-rust

Yeah, I did mean adding it as a separate recipe in #143

Can we add it in this PR? Given that the fixtures are added in this PR

I'll add them in #143 because I don't know which ones we will need and how much tweaking is needed for them to be fully functional. They will also be compressed like we talked about, so we don't have to have all those files. The recipe doesn't add any value at this point.

gjermundgaraba · 2024-12-07T17:02:46Z

packages/ethereum-light-client/src/config.rs

@@ -0,0 +1,382 @@
+use core::{


This file is copied in from union for convenience, but it should be cleaned up or removed eventually. Tracking that in #147

gjermundgaraba · 2024-12-07T17:05:47Z

packages/ethereum-light-client/src/error.rs

+    depth: {depth}, index: {index}, root: {root}, found: {found})",
+    branch = .branch.iter().map(ToString::to_string).collect::<Vec<_>>().join(", ")
+)]
+pub struct InvalidMerkleBranch {


I am a bit ambivalent about this one. It is very convenient when debugging issues, but it is quite a large error. I added this a long time ago after some long painful debugging sessions with the union light client.

Maybe we should have like a "debug" feature flag or something that enables more output and info. Not sure...

Why is this not a part of the enum

Mostly just to be able to box it, since it is big ^^ Does not need to be separate for any other reason

Can we add it into the enum then

Not in its current form, it causes a warning for too big error type - so that is why the box is there.

gjermundgaraba · 2024-12-07T17:16:15Z

packages/ethereum-light-client/src/types/wrappers.rs

@@ -0,0 +1,162 @@
+use alloy_primitives::{aliases::B32, Bloom, Bytes, FixedBytes, B256};


A lot of these wrapped types are added to enable tree_hash_root in special corner cases where tree_hash_derive did not work properly for some reason. I think it might be possible to have way fewer of these, but I didn't manage in a reasonable time and didn't want to spend too much time on it right now. Tracking in #147

packages/ethereum-utils/src/base64.rs

gjermundgaraba · 2024-12-07T17:26:03Z

programs/08-wasm-eth/src/contract.rs

@@ -0,0 +1,649 @@
+use std::convert::Into;


Quite a lot of things to improve here, but I mostly added it to be able to test building and checking the wasm file to make sure I didn't have any bad dependencies and so on. It will be cleaned up and done proper in #143

That goes for all the files in programs/08-wasm-eth, really. It does seem to work pretty well already though :)

srdtrk

I did my first pass. I found some structural improvements, some inaccuracies related to the ibc-go's 08-wasm interface.

The lack of documentation made it difficult to review at times, currently, in our project, all other crates require documentation on public interfaces. And it will become more difficult to introduce docs as time passes (since codebase grows). So I think it would be good to address it sooner rather than later.

Given the fact that it doesn't fully function yet, missing docs, and uncertainties around some crates. It seems it would be better to have a feature branch until POS e2e's pass and docs are added. I also want to start contributing to the feature branch if we have one. What do you think?

programs/08-wasm-eth/src/state.rs

programs/08-wasm-eth/src/msg.rs

Cargo.toml

srdtrk · 2024-12-09T03:27:24Z

.github/workflows/rust.yml

+        uses: actions-rs/cargo@v1
+        with:
+          command: run-script
+          args: build-08-wasm-eth


I think we shouldn't be building with docker in this CI, and use the rust toolchain instead. I'm also not a fan of using cargo scripts. justfile should suffice

We need the docker build to actually verify that we can build the optimized wasm file and then verify it. I don't see any issues with building with docker in CI here?

There aren't any issues. the docker uses a specific version of rust whereas I'd like us to use the latest stable. That's all

srdtrk · 2024-12-09T06:02:16Z

programs/08-wasm-eth/src/contract.rs

+        QueryMsg::VerifyClientMessage(verify_client_message_msg) => {
+            verify_client_message(deps, env, verify_client_message_msg)
+        }
+        QueryMsg::CheckForMisbehaviour(_) => check_for_misbehaviour(),
+        QueryMsg::TimestampAtHeight(_) => timestamp_at_height(env),
+        QueryMsg::Status(_) => status(),


These should all be moved to a query submodule

Suggested change

QueryMsg::VerifyClientMessage(verify_client_message_msg) => {

verify_client_message(deps, env, verify_client_message_msg)

}

QueryMsg::CheckForMisbehaviour(_) => check_for_misbehaviour(),

QueryMsg::TimestampAtHeight(_) => timestamp_at_height(env),

QueryMsg::Status(_) => status(),

QueryMsg::VerifyClientMessage(verify_client_message_msg) => {

query::verify_client_message(deps, env, verify_client_message_msg)

}

QueryMsg::CheckForMisbehaviour(_) => query::check_for_misbehaviour(),

QueryMsg::TimestampAtHeight(_) => query::timestamp_at_height(env),

QueryMsg::Status(_) => query::status(),

Yes, but not necessary in this PR. The wasm client itself will be cleaned up and made proper in #143

programs/08-wasm-eth/src/contract.rs

srdtrk · 2024-12-09T06:04:10Z

programs/08-wasm-eth/src/custom_query.rs

+
+#[derive(serde::Serialize, serde::Deserialize, Clone)]
+#[serde(rename_all = "snake_case")]
+pub enum EthereumCustomQuery {


What is this?

This is for the custom query to bls verification (BlsVerify)

This sucks. Should we create an issue to try and do this fully in wasm?

Would love that. I don't want us to write this ourselves, but created an issue for it: #163

If Union didn't want to do it rewrite it themselves, I doubt we should :D

There is a rust library we use for tests (same that Union uses for their tests): https://github.com/Snowfork/milagro_bls, but it is not production ready (big warning on the repo about that).

programs/08-wasm-eth/src/error.rs

programs/08-wasm-eth/src/msg.rs

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

gjermundgaraba · 2024-12-09T19:12:56Z

I believe the most relevant issues have been fixed at this point. I have commented on those that will be covered in separate issues so we can get this merged and not sit with a big PR over a long time that will get conflicts and get out of hand to both review and keep track of all the comments.

srdtrk

Thanks for the updates. I think we can merge this before all the POS e2es are passing, but it'd be great to address a couple more issues before merging. Some of the issues I commented on and also:

Updating repo README.md to include a bit of info about the eth light client and mentioning that it is WIP (⏳)
Adding a README.md in programs/cw-ics08-wasm-eth/ which gives more info about the light client and mentions that it is WIP.
Yes, adding docs on everything will not be necessary. Since we expect large changes in ethereum-utils, tree_hash, and others, so it makes sense to not add docs there. But I think we should still add docs on ethereum-light-client and cw-ics08-wasm-eth to make it easier to read.
It'd also be great if we could reduce the number of fixtures (by merging them), but we can defer it if it is somehow difficult.

srdtrk · 2024-12-10T03:30:04Z

programs/08-wasm-eth/src/custom_query.rs

+
+#[derive(serde::Serialize, serde::Deserialize, Clone)]
+#[serde(rename_all = "snake_case")]
+pub enum EthereumCustomQuery {


This sucks. Should we create an issue to try and do this fully in wasm?

srdtrk · 2024-12-10T03:33:08Z

packages/ethereum-light-client/src/error.rs

+    depth: {depth}, index: {index}, root: {root}, found: {found})",
+    branch = .branch.iter().map(ToString::to_string).collect::<Vec<_>>().join(", ")
+)]
+pub struct InvalidMerkleBranch {


Can we add it into the enum then

srdtrk · 2024-12-10T03:39:48Z

Cargo.toml

+# dev-dependencies
+cw-multi-test = { version = "2.2.0", default-features = false }
+milagro_bls   = { git = "https://github.com/Snowfork/milagro_bls", rev = "bc2b5b5e8d48b7e2e1bfaa56dc2d93e13cb32095", default-features = false } # Only used for testing, not to be used in production!
+
 [patch.crates-io]


It shouldn't because sp1 is the most security critical part of our code. Given that we would use these patches in the sp1 programs, it doesn't matter whether other apps use these or not. They shouldn't be changing functionality if the target isn't riscv anyway

srdtrk · 2024-12-10T03:41:28Z

programs/cw-ics08-wasm-eth/Cargo.toml

Can we add a README.md in this directory to provide some info about cw-ics08-wasm-eth and mention that it is work-in-progress.

Yeah, that makes sense, will do that 👍

srdtrk · 2024-12-10T03:49:44Z

packages/ethereum-light-client/src/types/sync_committee.rs

+    pub fn get_active_sync_committee(&self) -> ActiveSyncCommittee {
+        if let Some(sync_committee) = &self.current_sync_committee {
+            ActiveSyncCommittee::Current(sync_committee.clone())
+        } else if let Some(sync_committee) = &self.next_sync_committee {
+            ActiveSyncCommittee::Next(sync_committee.clone())
+        } else {
+            ActiveSyncCommittee::default()
+        }
+    }


It is not an enum in TrustedSyncCommittee, instead it is a pair of options which we only expect one to be present, right?

srdtrk · 2024-12-10T03:51:23Z

packages/ethereum-light-client/src/types/sync_committee.rs

+    //
+    // https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#process_light_client_update
+    pub fn validate_signature_supermajority(&self) -> bool {
+        self.num_sync_committe_participants() * 3 >= self.sync_committee_bits.len() * 8 * 2


It seems you merged my suggestion. So are we sure I was correct? I'm a bit confused about whether this comes from union or where?

It did, but the types have changed, which is where the problem came from. It was correct, but it still needs better unit testing which will be done later

Added unit tests for it, your fix was correct

srdtrk · 2024-12-10T03:57:10Z

packages/ethereum-light-client/src/verify.rs

+    bls_verifier: V,
+) -> Result<(), EthereumIBCError> {
+    // Verify sync committee has sufficient participants
+    ensure(


Then we would have to use this in the entire project (not just the ethereum light client) for consistency. I don't want to have multiple ways of asserting things in the repo. Moreover, something like this would make more sense as a macro rather than a function.

Did you base this upon an example from another repo?

srdtrk · 2024-12-10T07:19:24Z

packages/ethereum-light-client/src/verify.rs

+                .clone()
+                .unwrap_or_default()
+                .into(),
+            floorlog2(NEXT_SYNC_COMMITTEE_INDEX),


Don't you already have a constant for this NEXT_SYNC_COMMITTEE_BRANCH_SIZE

Yup! Have moved them all and using them everywhere now.

srdtrk · 2024-12-10T07:24:28Z

packages/ethereum-light-client/src/verify.rs

+    validate_merkle_branch(
+        get_lc_execution_root(client_state, header),
+        header.execution_branch.0.into(),
+        floorlog2(EXECUTION_PAYLOAD_INDEX),


There is a constant for this EXECUTION_BRANCH_SIZE right?

Good point. Will go through them and see if I can consolidate and get rid of floorlog2

I just noticed that floorlog2 has some extra safety added in. Not sure if we want to have that?

/// Convenience function safely to call [`u64::ilog2`] and convert the result into a usize. #[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))]

I kept the floorlog2, but only using it inside the config.rs which needs to be cleaned up anyway. I think this is fine for now at least. The rest of the code does not use it.

srdtrk · 2024-12-10T07:32:49Z

packages/ethereum-light-client/src/types/light_client.rs

+    wrappers::{WrappedBloom, WrappedBranch, WrappedBytes},
+};
+
+const EXECUTION_BRANCH_SIZE: usize = floorlog2(EXECUTION_PAYLOAD_INDEX);


Some of the places where floorlog2 is used could actually reuse these constants. Made comments about those as well

srdtrk · 2024-12-10T07:45:19Z

.github/workflows/rust.yml

+      # checks that the wasm binary is a proper cosmwasm smart contract
+      # it checks for things like memories, exports, imports, available capabilities, and non-determinism
+      - name: Check cosmwasm file
+        run: cosmwasm-check artifacts/cw_08_wasm_etheruem_light_client.wasm


Name of the file should have changed

gjermundgaraba · 2024-12-10T19:25:52Z

@srdtrk Take a look now. I think this can be merged unless there are any big issues.

srdtrk

Lgtm now. I refactored and added full rustdocs to:

ethereum-light-client
ethereum-trie-db
cw-ics08-wasm-eth

So, I'd like you to review my changes. Make improvements to docs if you see the need, and then you can merge the PR

gjermundgaraba · 2024-12-11T09:35:10Z

I reviewed the changes and I think it looks good! Thank you for the effort you put in!

I will want to have a discussion at some point on documentation, because I think we end up with a lot of things like this, which is adding noice with no value, as far as I can tell:

/// the value name
pub value_name: Type

But this goes for the entire repo, so it's a broader discussion, and for now it's better to be consistent :)

srdtrk · 2024-12-11T10:43:11Z

Yes but in general, all public interfaces should have rustdocs. There are also many other fields that are very ambiguous without rustdocs and I find hard to review. I don't think we should every attempt to change this @gjermundgaraba

And sometimes, the variable names are not fully self explanotary. I'd argue that the example you gave is still not very well documented since I have no idea what it is referring to from the snippet you gave. The documentation I wrote could still be improved a lot, for example, I found:

    /// The time of genesis
    pub genesis_time: u64,

But the docs should actually be mentioning whether it is in seconds or nanoseconds or something else.

So we can do another pass once the crate is more finalized

gjermundgaraba · 2024-12-11T11:15:08Z

That is fair. Perhaps the solution is indeed to just have docs that actually bring some useful information. :)

gjermundgaraba added 5 commits December 4, 2024 23:07

current state minus cosmwasm

a577c12

testing and cleanup

bb1a04f

fork tree_hash for now

39c0414

lint

8052da9

Merge branch 'main' into gjermund/142-create-initial-08-wasm-ethereum…

c657b38

…-light-client

gjermundgaraba linked an issue Dec 5, 2024 that may be closed by this pull request

Create initial 08-wasm Ethereum Light Client #142

Closed

srdtrk reviewed Dec 6, 2024

View reviewed changes

gjermundgaraba added 13 commits December 6, 2024 09:58

add run-script

699cdf4

Merge branch 'main' into gjermund/142-create-initial-08-wasm-ethereum…

14ebe5a

…-light-client

add wasm target to cosmwasm build in ci

9022dfa

move features out of top-level cargo

2387472

Merge branch 'main' into gjermund/142-create-initial-08-wasm-ethereum…

d95dd17

…-light-client

lint and fix just lint command

a3854e4

fix cosmwasm ci build

ee4df25

cr fixes

60730c2

Merge branch 'main' into gjermund/142-create-initial-08-wasm-ethereum…

26cb328

…-light-client

cargo lock

e9c1626

fix e2e test in pow mode

8577c38

add cosmwasm-check to ci

b9d0ef4

added unit tests on cosmwasm contract

2d1c2b2

gjermundgaraba commented Dec 7, 2024

View reviewed changes

some minor cleanup before review

abf2ef9

gjermundgaraba marked this pull request as ready for review December 7, 2024 17:31

srdtrk reviewed Dec 9, 2024

View reviewed changes

gjermundgaraba and others added 3 commits December 9, 2024 11:39

Update packages/ethereum-light-client/src/types/sync_committee.rs

34df628

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

Update programs/08-wasm-eth/src/msg.rs

1026bd2

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

add source errors to error messages

001807f

gjermundgaraba mentioned this pull request Dec 9, 2024

Integrate 08-wasm etheruem light client into e2e test #143

Closed

remove From trait for json serializaiton

4021f54

remove export metadata

9dd789a

gjermundgaraba requested a review from srdtrk December 9, 2024 19:11

srdtrk reviewed Dec 10, 2024

View reviewed changes

gjermundgaraba added 6 commits December 10, 2024 08:02

update wasm ci file name for check

e0d216e

move and use branch consts everywhere

68e99f1

add readme with WIP warnings and acknowledgements

c9de44d

change ensure to macro

fc9958a

added unit tests for supermajority check

a4ac4aa

move rust fixtures into a single file per test

68a50d2

gjermundgaraba requested a review from srdtrk December 10, 2024 19:25

srdtrk added 10 commits December 11, 2024 12:05

docs: updated readmes

9cae4fa

docs: fully linted and documented wasm client

a56859d

refactor: custom query

c792160

docs: update README

b99018f

docs: refactored and added docs to all of ethereum-light-client

52dfc31

refactor: use match instead of if

13caeb1

lint: fixed clippy complaint

0442375

docs: added linting and docs to triedb

373d094

deps: added std feature for error impl

2b5eaa0

imp: removed ethereum-utils::base64

b1e90e2

srdtrk approved these changes Dec 11, 2024

View reviewed changes

gjermundgaraba merged commit a30f974 into main Dec 11, 2024
53 checks passed

gjermundgaraba deleted the gjermund/142-create-initial-08-wasm-ethereum-light-client branch December 11, 2024 09:35

gjermundgaraba mentioned this pull request Dec 17, 2024

test: integrate 08 wasm etheruem light client into e2e test #183

Merged

6 tasks

		@@ -116,23 +116,23 @@ deploy-sp1-ics07: genesis-sp1-ics07
		generate-fixtures-solidity: clean

		@@ -0,0 +1,162 @@
		use alloy_primitives::{aliases::B32, Bloom, Bytes, FixedBytes, B256};

feat: add initial ethereum light client #150

feat: add initial ethereum light client #150

Conversation

gjermundgaraba commented Dec 5, 2024 • edited Loading

Description

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

srdtrk left a comment

Choose a reason for hiding this comment

gjermundgaraba left a 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

gjermundgaraba Dec 10, 2024 • edited Loading

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

srdtrk left a 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

gjermundgaraba commented Dec 9, 2024

srdtrk left a 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

gjermundgaraba commented Dec 10, 2024

srdtrk left a comment

Choose a reason for hiding this comment

gjermundgaraba commented Dec 11, 2024

srdtrk commented Dec 11, 2024 • edited Loading

gjermundgaraba commented Dec 11, 2024

gjermundgaraba commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

gjermundgaraba Dec 10, 2024 •

edited

Loading

srdtrk commented Dec 11, 2024 •

edited

Loading