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

Refactor linked lists initial hashing #581

Merged
merged 46 commits into from
Sep 18, 2024
Merged

Conversation

4l0n50
Copy link
Contributor

@4l0n50 4l0n50 commented Sep 2, 2024

This PR changes the logic for hashing the initial tries using linked lists. Instead of traversing the initial state trie and setting all the payloads, it inserts all the nodes into the initial linked lists state trie. This change saves around 10% 15% of CPU cycles. Even though #467 considered also refactoring the final hashing, it turned out that the current implementation of the final hash is more efficient.
Closes #467.

Copy link
Contributor

@hratoanina hratoanina left a comment

Choose a reason for hiding this comment

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

Looks good!

Just a general question about soundness not specific to this PR:
For the initial trie, we check consistency of the state MPT by adding all the initial accounts of the linked list in order. Since we don't use the next pointers of the items and just advance by @ACCOUNTS_LINKED_LISTS_NODE_SIZE, what happens if a malicious prover provides duplicate accounts? I can't see a way to abuse it but I'm not sure it's completely safe either.

mpt_trie/src/nibbles.rs Outdated Show resolved Hide resolved
@4l0n50
Copy link
Contributor Author

4l0n50 commented Sep 6, 2024

For the initial trie, we check consistency of the state MPT by adding all the initial accounts of the linked list in order. Since we don't use the next pointers of the items and just advance by @ACCOUNTS_LINKED_LISTS_NODE_SIZE, what happens if a malicious prover provides duplicate accounts? I can't see a way to abuse it but I'm not sure it's completely safe either.

Good point! I think this is indeed a problem. Even though the only account in the trie would be the last one, It seems possible to have multiple accounts with different states (e.g. different balance, storage trie) and choose any of them when writing/reading from the state. This might certainly allow you to take the last account from the duplicates to an incorrect state, getting a corrupted final hash.
I think the solution is to add the check addr_key < next_addr_key so that you never read/write from the duplicated accounts/slot.

@github-actions github-actions bot removed the crate: mpt_trie Anything related to the mpt_trie crate. label Sep 11, 2024
@4l0n50
Copy link
Contributor Author

4l0n50 commented Sep 12, 2024

@hratoanina I addressed the soundness issue we discussed. Now it's checking well formedness of the initial next node ptrs, as well as checking that the initial keys are strictly increasing.

@Nashtare Nashtare requested a review from atanmarko as a code owner September 16, 2024 14:55
@github-actions github-actions bot added the crate: zero_bin Anything related to the zero-bin subcrates. label Sep 16, 2024
@github-actions github-actions bot removed the crate: zero_bin Anything related to the zero-bin subcrates. label Sep 18, 2024
@Nashtare Nashtare merged commit 6ef8827 into develop Sep 18, 2024
18 checks passed
@Nashtare Nashtare deleted the refactor_ll_initial_hashing branch September 18, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor linked list hashing
4 participants