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

Add collapse strategy to PartialTrie variants #501

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

Nashtare
Copy link
Collaborator

Add a collapsing strategy for the edge case of a branch node with two children being collapsed into an extension node after the deletion of one the children.

cf #202 and #455 for reference

@0xaatif I don't consider the PR ready yet, especially some more doc/clarification would be good but I'm opening it to get some feedback regarding the approach taken and if it matches how you were envisioning it following our chat.

@Nashtare Nashtare self-assigned this Aug 15, 2024
@github-actions github-actions bot added crate: trace_decoder Anything related to the trace_decoder crate. crate: mpt_trie Anything related to the mpt_trie crate. labels Aug 15, 2024
@Nashtare Nashtare added this to the Type 1 - Q3 2024 milestone Aug 15, 2024
trace_decoder/src/type1.rs Outdated Show resolved Hide resolved
Co-authored-by: BGluth <gluthb@gmail.com>
@BGluth
Copy link
Contributor

BGluth commented Aug 15, 2024

Hey just for some more info, how are we handling this in the decoder? Isn't this something that must be done in the full node? Like if the decoder realizes that an extension will collapse into a hash node, doesn't it not know anything about what node was hashed, since the only case where this is safe is if the hash node was a branch node IIRC?

@Nashtare
Copy link
Collaborator Author

Nashtare commented Aug 15, 2024

Jerry confirmed that this is always handled by Jerigon, hence the rationale behind ignoring these edge-cases from Jerigon payloads and only trigger errors from native client ones.
I don't see how unmodified nodes can alleviate this though, as relying only on regular RPCs like eth_getStorageAt or eth_getProof won't suffice

@Nashtare Nashtare requested a review from 0xaatif August 16, 2024 13:03
mpt_trie/src/partial_trie.rs Outdated Show resolved Hide resolved
mpt_trie/src/partial_trie.rs Outdated Show resolved Hide resolved
@Nashtare Nashtare enabled auto-merge (squash) August 16, 2024 17:41
@Nashtare Nashtare merged commit 0e55293 into develop Aug 16, 2024
15 checks passed
@Nashtare Nashtare deleted the feat/collapse_strategy branch August 16, 2024 17:55
@BGluth
Copy link
Contributor

BGluth commented Aug 16, 2024

Jerry confirmed that this is always handled by Jerigon, hence the rationale behind ignoring these edge-cases from Jerigon payloads and only trigger errors from native client ones.

@Nashtare Ah ok. So, if I'm understanding this right, even if Jerigon is handling this, under normal circumstances, we should never trigger the logic to collapse a extension node into a hash node right? Isn't it maybe better to still treat that as an invalid state even if we should still never be able to reach it (eg. like the unreachable! macro in rust)?

@Nashtare
Copy link
Collaborator Author

Not quite, with jerigon payloads, Jerry guaranteed that having this orphaned hash node converted to an extension node will never cause inconsistent trie root, unlike what we were having on frisitano's issue. I have Jerigon payload examples that were triggering these false positives, but were sane to handle for hte prover.

@BGluth
Copy link
Contributor

BGluth commented Aug 16, 2024

Ah gotcha, makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: mpt_trie Anything related to the mpt_trie crate. crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants