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: in-place nodes ignored in proof verification #27

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

xJonathanLEI
Copy link
Contributor

@xJonathanLEI xJonathanLEI commented Aug 13, 2024

Fixes #26.

Kinda relates to #7 and #9, but on the verification side only.

The current behavior of jumping to the next proof node does not work with in-place encoded child nodes, as when such nodes are encountered, it's always the last item of the proof, causing a mismatch of key and walked_path, and a verification failure.

This patch fixes the issue by checking whether the branch child is encoded in-place, and if so "recursively" walk the child (in reality this cannot be more than 1 level deep as otherwise it wouldn't be in-place to begin with).

The test case added is manually crafted, without using the hash builder for proof collection. This is because currently the issue of handling small nodes exist across the whole library, meaning the hash builder is faulty too. Unfortunately I only have time to fix the verification side, and hence the manual test case.

Technically this part is also prone to the issue:

trie/src/proof/verify.rs

Lines 69 to 72 in dc77954

TrieNode::Extension(extension) => {
walked_path.extend_from_slice(&extension.key);
Some(extension.child)
}

as you can't just assume the extension's child is a hash. It's an easy fix by just applying the same check as in this patch, but I didn't have time to build the test case for this code path, and I don't want to submit untested code.

@xJonathanLEI
Copy link
Contributor Author

Sorry for pushing but would be nice if we can get this in :) Thanks!

Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

thanks for PR with detailed docs

@rkrasiuk rkrasiuk merged commit 28ebb7c into alloy-rs:main Aug 20, 2024
20 checks passed
@xJonathanLEI xJonathanLEI deleted the fix/in_place_verification branch August 20, 2024 18:06
@xJonathanLEI xJonathanLEI restored the fix/in_place_verification branch August 20, 2024 18:06
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.

Proof verification failure caused by nodes encoded in-place
2 participants