Skip to content

fix: use alloy-trie for eth_getProof#7546

Merged
klkvr merged 10 commits into
foundry-rs:masterfrom
klkvr:klkvr/fix-proof
Apr 8, 2024
Merged

fix: use alloy-trie for eth_getProof#7546
klkvr merged 10 commits into
foundry-rs:masterfrom
klkvr:klkvr/fix-proof

Conversation

@klkvr

@klkvr klkvr commented Apr 2, 2024

Copy link
Copy Markdown
Member

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

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

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

Comment thread Cargo.lock
]

[[package]]
name = "parity-util-mem"

Copy link
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread crates/anvil/src/eth/backend/mem/state.rs Outdated
Comment thread crates/anvil/src/eth/backend/mem/state.rs Outdated
Comment thread crates/anvil/src/eth/backend/mem/state.rs Outdated
@klkvr klkvr marked this pull request as ready for review April 5, 2024 16:40

@DaniPopes DaniPopes left a comment

Copy link
Copy Markdown
Member

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?

Comment thread crates/anvil/core/Cargo.toml Outdated
@klkvr klkvr changed the title [wip] fix: use alloy-trie for eth_getProof fix: use alloy-trie for eth_getProof Apr 5, 2024

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

amazing!

@klkvr

klkvr commented Apr 5, 2024

Copy link
Copy Markdown
Member Author

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

klkvr commented Apr 5, 2024

Copy link
Copy Markdown
Member Author

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

@DaniPopes

DaniPopes commented Apr 5, 2024

Copy link
Copy Markdown
Member

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

@adraffy

adraffy commented Apr 6, 2024

Copy link
Copy Markdown

looking forward to this fix!

@klkvr

klkvr commented Apr 7, 2024

Copy link
Copy Markdown
Member Author

@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

@Evalir Evalir left a comment

Copy link
Copy Markdown
Contributor

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
@Arachnid

Copy link
Copy Markdown

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