-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
There was a problem hiding this 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.
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Outdated
Show resolved
Hide resolved
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. |
Co-authored-by: Hamy Ratoanina <[email protected]>
@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. |
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/linked_list.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/linked_list.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/linked_list.asm
Outdated
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/linked_list.asm
Outdated
Show resolved
Hide resolved
Co-authored-by: Hamy Ratoanina <[email protected]>
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/initial_tries.asm
Show resolved
Hide resolved
evm_arithmetization/src/cpu/kernel/asm/mpt/linked_list/linked_list.asm
Outdated
Show resolved
Hide resolved
Co-authored-by: Robin Salen <[email protected]>
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 listsstate trie. This change saves around10%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.