[Merged by Bors] - Added Merkle Proof Generation for Beacon State #3674

Giulio2002 · 2022-10-31T14:32:00Z

Issue Addressed

This PR addresses partially #3651

Proposed Changes

This PR adds the following methods:

a new method to trait TreeHash, hash_tree_leaves which returns all the Merkle leaves of the ssz object.
a new method to BeaconState: compute_merkle_proof which generates a specific merkle proof for given depth and index by using the hash_tree_leaves as leaves function.

Additional Info

Now here is some rationale on why I decided to go down this route: adding a new function to commonly used trait is a pain but was necessary to make sure we have all merkle leaves for every object, that is why I just added hash_tree_leaves in the trait and not compute_merkle_proof as well. although it would make sense it gives us code duplication/harder review time and we just need it from one specific object in one specific usecase so not worth the effort YET. In my humble opinion.

Giulio2002 · 2022-10-31T14:33:59Z

CC @pawanjay176 @michaelsproul please give it a quick look, i still need to work some things out and test it properly and in the meanwhile I would like a general opinion on it and if hash_tree_leaves impls makes sense :). also sorry for the big PR but TreeHash trait is everywhere :(.

Giulio2002 · 2022-10-31T15:19:41Z

just kidding it just need some unit tests, do anyone knows of something already avaiable or do I have to try making the test myself?

Giulio2002 · 2022-11-01T15:12:30Z

On second thought, I think it is not feasible to write unit tests for Beacon State as you would need something real from nimbus (Which I have no idea where to start as I never coded in Nim) and the struct itself is too big, I think it would be better if something breaks when I simulate gossip/resp req

michaelsproul

I've left a sketch of how I think we should proceed in a code comment.

Re: testing, the BeaconStates are actually quite small (<100MB) and there are unit tests available from the specs repo here: https://github.com/ethereum/consensus-specs/blob/dev/tests/formats/light_client/single_merkle_proof.md.

Our code that integrates the spec tests is here: https://github.com/sigp/lighthouse/tree/stable/testing/ef_tests. To run the new tests we'd need to add a new case runner and handler.

I'm off sick this week, but can get stuck into this more with you next week.

consensus/types/src/beacon_state.rs

Giulio2002 · 2022-11-04T01:11:56Z

I need to get Finality Branch EF test to work thats the one missing, all of the others are working at the first try due probably to some miracle

Giulio2002 · 2022-11-05T00:30:18Z

Okay so this is interesting, all the tests for finality have a proof that starts with 0x0000000000000000000000000000000000000000000000000000000000000000, and then it goes on normally? also it seems that we generate the correct proof if we discard this weird thing, like the rest of the proof match. is that even possible?

Giulio2002 · 2022-11-05T00:42:20Z

I just checked against Erigon lightclient we do not indeed get any finality branch starting with 0 hash

michaelsproul · 2022-11-05T00:49:21Z

I was wondering if maybe we were appending the extra node at the wrong end of the proof? The 0x0 value could be the epoch value that is the sibling of the finalized root

Giulio2002 · 2022-11-05T01:08:04Z

okay i tried to prepend the extra leaf and now everything passed

Giulio2002 · 2022-11-05T23:06:22Z

Consider it ready for review now

michaelsproul

looking great on the whole, just a few minor things then we can merge

cargo fmt is currently failing too, you can fix it up with cargo fmt --all

thanks!

consensus/types/src/beacon_state.rs

consensus/types/src/light_client_bootstrap.rs

consensus/types/src/light_client_finality_update.rs

consensus/types/src/light_client_update.rs

Giulio2002 · 2022-11-06T22:58:01Z

I seee execution-engine-integration-ubuntu fail which seems weird as I did not touch anything related to engine API, that is in CI, it fails at compilation and it compiles for me? uh weird.

Giulio2002 · 2022-11-06T23:02:32Z

nevermind i think it was an old test

michaelsproul · 2022-11-07T00:12:30Z

you can run the linter locally with make lint btw ;)

michaelsproul · 2022-11-07T00:16:24Z

running make lint will also update Cargo.lock which will fix this error from the execution engine integration tests:

error: the lock file /home/runner/work/lighthouse/lighthouse/Cargo.lock needs to be updated but --locked was passed to prevent this

michaelsproul

LGTM! Will try to get this merged assuming CI cooperates

michaelsproul · 2022-11-08T01:30:19Z

bors r+

bors · 2022-11-08T01:30:22Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

consensus/types/src/beacon_state.rs

michaelsproul · 2022-11-08T01:58:00Z

bors retry

bors · 2022-11-08T01:58:12Z

Stopped waiting for PR status (GitHub check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests.

## Issue Addressed This PR addresses partially #3651 ## Proposed Changes This PR adds the following methods: * a new method to trait `TreeHash`, `hash_tree_leaves` which returns all the Merkle leaves of the ssz object. * a new method to `BeaconState`: `compute_merkle_proof` which generates a specific merkle proof for given depth and index by using the `hash_tree_leaves` as leaves function. ## Additional Info Now here is some rationale on why I decided to go down this route: adding a new function to commonly used trait is a pain but was necessary to make sure we have all merkle leaves for every object, that is why I just added `hash_tree_leaves` in the trait and not `compute_merkle_proof` as well. although it would make sense it gives us code duplication/harder review time and we just need it from one specific object in one specific usecase so not worth the effort YET. In my humble opinion. Co-authored-by: Michael Sproul <micsproul@gmail.com>

bors · 2022-11-08T04:24:20Z

Pull request successfully merged into unstable.

Build succeeded:

## Issue Addressed This PR addresses partially sigp#3651 ## Proposed Changes This PR adds the following methods: * a new method to trait `TreeHash`, `hash_tree_leaves` which returns all the Merkle leaves of the ssz object. * a new method to `BeaconState`: `compute_merkle_proof` which generates a specific merkle proof for given depth and index by using the `hash_tree_leaves` as leaves function. ## Additional Info Now here is some rationale on why I decided to go down this route: adding a new function to commonly used trait is a pain but was necessary to make sure we have all merkle leaves for every object, that is why I just added `hash_tree_leaves` in the trait and not `compute_merkle_proof` as well. although it would make sense it gives us code duplication/harder review time and we just need it from one specific object in one specific usecase so not worth the effort YET. In my humble opinion. Co-authored-by: Michael Sproul <micsproul@gmail.com>

Giulio2002 marked this pull request as draft October 31, 2022 14:32

Giulio2002 marked this pull request as ready for review November 1, 2022 15:12

michaelsproul reviewed Nov 2, 2022

View reviewed changes

consensus/types/src/beacon_state.rs Outdated Show resolved Hide resolved

michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Nov 2, 2022

Giulio2002 added 2 commits November 2, 2022 21:28

added compute_merkle_proof specialized

3133c3c

added integration tests

2dc3ed4

michaelsproul changed the title ~~Added Merkle Proof Generation for Beacon State [DO NOT MERGE, BUT PLEASE GIVE ME SOME FEEDBACK]~~ Added Merkle Proof Generation for Beacon State Nov 4, 2022

michaelsproul added work-in-progress PR is a work-in-progress and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 4, 2022

Giulio2002 added 3 commits November 5, 2022 02:08

prepend

571ee8c

added all tests

496a66d

lint

42f5621

michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 6, 2022

pass clippy

4dbc353

michaelsproul requested changes Nov 6, 2022

View reviewed changes

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 6, 2022

Giulio2002 added 3 commits November 6, 2022 23:42

lint again, please be fixed

c42b8b3

comments

f00237c

comments again

cc35619

please

9e9a6b7

update Cargo.lock

1c47fe0

michaelsproul approved these changes Nov 8, 2022

View reviewed changes

michaelsproul added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 8, 2022

michaelsproul changed the base branch from unstable to stable November 8, 2022 01:34

michaelsproul changed the base branch from stable to unstable November 8, 2022 01:34

michaelsproul reviewed Nov 8, 2022

View reviewed changes

consensus/types/src/beacon_state.rs Outdated Show resolved Hide resolved

Update comment to trigger CI re-run

3e295e5

bors bot changed the title ~~Added Merkle Proof Generation for Beacon State~~ [Merged by Bors] - Added Merkle Proof Generation for Beacon State Nov 8, 2022

bors bot closed this Nov 8, 2022

michaelsproul mentioned this pull request Nov 10, 2022

[Merged by Bors] - Adding light_client gossip topics #3693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Added Merkle Proof Generation for Beacon State #3674

[Merged by Bors] - Added Merkle Proof Generation for Beacon State #3674

Giulio2002 commented Oct 31, 2022

Giulio2002 commented Oct 31, 2022 •

edited

Loading

Giulio2002 commented Oct 31, 2022

Giulio2002 commented Nov 1, 2022

michaelsproul left a comment

Giulio2002 commented Nov 4, 2022 •

edited

Loading

Giulio2002 commented Nov 5, 2022 •

edited

Loading

Giulio2002 commented Nov 5, 2022

michaelsproul commented Nov 5, 2022

Giulio2002 commented Nov 5, 2022

Giulio2002 commented Nov 5, 2022

michaelsproul left a comment •

edited

Loading

Giulio2002 commented Nov 6, 2022 •

edited

Loading

Giulio2002 commented Nov 6, 2022

michaelsproul commented Nov 7, 2022

michaelsproul commented Nov 7, 2022

michaelsproul left a comment

michaelsproul commented Nov 8, 2022

bors bot commented Nov 8, 2022

michaelsproul commented Nov 8, 2022

bors bot commented Nov 8, 2022

bors bot commented Nov 8, 2022

[Merged by Bors] - Added Merkle Proof Generation for Beacon State #3674

[Merged by Bors] - Added Merkle Proof Generation for Beacon State #3674

Conversation

Giulio2002 commented Oct 31, 2022

Issue Addressed

Proposed Changes

Additional Info

Giulio2002 commented Oct 31, 2022 • edited Loading

Giulio2002 commented Oct 31, 2022

Giulio2002 commented Nov 1, 2022

michaelsproul left a comment

Choose a reason for hiding this comment

Giulio2002 commented Nov 4, 2022 • edited Loading

Giulio2002 commented Nov 5, 2022 • edited Loading

Giulio2002 commented Nov 5, 2022

michaelsproul commented Nov 5, 2022

Giulio2002 commented Nov 5, 2022

Giulio2002 commented Nov 5, 2022

michaelsproul left a comment • edited Loading

Choose a reason for hiding this comment

Giulio2002 commented Nov 6, 2022 • edited Loading

Giulio2002 commented Nov 6, 2022

michaelsproul commented Nov 7, 2022

michaelsproul commented Nov 7, 2022

michaelsproul left a comment

Choose a reason for hiding this comment

michaelsproul commented Nov 8, 2022

bors bot commented Nov 8, 2022

michaelsproul commented Nov 8, 2022

bors bot commented Nov 8, 2022

bors bot commented Nov 8, 2022

Giulio2002 commented Oct 31, 2022 •

edited

Loading

Giulio2002 commented Nov 4, 2022 •

edited

Loading

Giulio2002 commented Nov 5, 2022 •

edited

Loading

michaelsproul left a comment •

edited

Loading

Giulio2002 commented Nov 6, 2022 •

edited

Loading