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

fix: use alloy-trie for eth_getProof #7546

Merged
merged 10 commits into from
Apr 8, 2024
Merged

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Apr 2, 2024

Motivation

Closes #5004
Closes #5577

Solution

Changes eth_getProof logic to use alloy-trie crate. Changes MaybeHashDatabase trait to MaybeFullDatabase as currently the only scenario in which we can obtain proofs is when using in-memory database and building trie from full state available in it.

I've been testing proofs manually and they seem correct, however, the tests for them won't pass because they are still using parity libraries which seem to rely on different encoding making it impossible to decode correct eth_getProof outputs out-of-the box (ref #5004 (comment))

I'll probably change tests to concrete fixtures as it's done in Reth: https://github.com/paradigmxyz/reth/blob/112c8a3f1e1bcab1a1418e6150400012bc195548/crates/trie/src/proof.rs#L165

@klkvr klkvr marked this pull request as draft April 2, 2024 16:46
@klkvr klkvr changed the title fix: use alloy-trie for eth_getProof [wip] fix: use alloy-trie for eth_getProof Apr 2, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

amazing!

testing these is a bit hard...
I think we should get rid of all the tests/it/proof.rs helper code, and instead perhaps get some test vecs from somewhere else.

ptal @rkrasiuk @DaniPopes

@@ -5440,31 +5456,6 @@ dependencies = [
"winapi",
]

[[package]]
name = "parity-util-mem"
Copy link
Member

Choose a reason for hiding this comment

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

this should allow enabling jemalloc for anvil

Copy link
Member

Choose a reason for hiding this comment

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

crates/anvil/src/eth/backend/mem/state.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/state.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/state.rs Outdated Show resolved Hide resolved
@klkvr klkvr marked this pull request as ready for review April 5, 2024 16:40
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Looks fine

Does this fix #5577, #7039?

crates/anvil/core/Cargo.toml Outdated Show resolved Hide resolved
@klkvr klkvr changed the title [wip] fix: use alloy-trie for eth_getProof fix: use alloy-trie for eth_getProof Apr 5, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

so much work for one endpoint -.-

last dep nit, lgtm

Copy link
Member

Choose a reason for hiding this comment

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

amazing!

@klkvr
Copy link
Member Author

klkvr commented Apr 5, 2024

It does fix #5577, not sure about #7039, do we think that most of the overhead is coming from keccak? will do some testing

@klkvr
Copy link
Member Author

klkvr commented Apr 5, 2024

Tested on #7039 repro and don't see any differences, TPS is still drops significantly for low number of transactions in block

@DaniPopes
Copy link
Member

DaniPopes commented Apr 5, 2024

Can you share a samply profile if you have it? Anyway can be in a follow up

@adraffy
Copy link

adraffy commented Apr 6, 2024

looking forward to this fix!

@klkvr
Copy link
Member Author

klkvr commented Apr 7, 2024

@DaniPopes yeah, the overhead is definitely coming from state root calculation. It is negligible while the state is small but becomes much higher when it grows. https://share.firefox.dev/3VMJ8QM - this is a profile for a run which mined 1000 blocks with 10 simple CREATE transactions each, thus state at the end of execution contained 10.000 addresses

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

sick

@klkvr klkvr merged commit 61f046d into foundry-rs:master Apr 8, 2024
20 checks passed
@Arachnid
Copy link

You absolute hero. I owe you a beer - or several - @klkvr.

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.

Anvil crashes when calling eth_getProof eth_getProof on anvil node returns wrong RLP encoding
6 participants