Mpt trie panic refactor #118

vladimir-trifonov · 2024-03-22T11:00:53Z

Use ThisError instead of panicking on various places in mtr_trie.

muursh

All in all a good start. We're going in the right direction

trace_decoder/src/decoding.rs

mpt_trie/examples/simple.rs

mpt_trie/src/debug_tools/stats.rs

mpt_trie/src/partial_trie.rs

mpt_trie/src/trie_hashing.rs

mpt_trie/src/trie_ops.rs

BGluth

So this looks good (and you got this done pretty quick as well!), and I just have a few thoughts.

I guess this is probably too big to put into a single PR, but a lot of the methods/functions in nibbles.rs can also panic, and I guess we should probably change those to Results as well.

I mentioned this in a few of my comments, but I think we should avoid duplicating expects that are identical throughout the codebase (@muursh want your thoughts on this). This is also a nit, but I think it's a bit cleaner to avoid using map_or(Ok(None)) and instead wrap the block in an Ok() since the duplication is a bit better this way. This is a bit subjective, so I'm good either way here.

mpt_trie/examples/ethereum_trie.rs

mpt_trie/src/trie_ops.rs

BGluth · 2024-03-22T23:33:59Z

mpt_trie/src/trie_ops.rs

+            delete_intern(&children[nibble as usize], curr_k)?.map_or(Ok(None),
                |(updated_child, value_deleted)| {
                    // If the child we recursively called is deleted, then we may need to reduce
                    // this branch to an extension/leaf.


Small nit, but wdyt about instead replacing the map_or with map and then wrapping delete_intern with an Ok()? I guess this is a bit subjective, but this means we can ignore the Result type except for the Ok() that wraps everything.

but we have:

updated_children[nibble as usize] = try_collapse_if_extension(updated_child)?;

inside the map_or

mpt_trie/src/trie_ops.rs

wborgeaud

LGTM otherwise

mpt_trie/examples/simple.rs

mpt_trie/src/debug_tools/diff.rs

mpt_trie/src/debug_tools/stats.rs

sonarqubecloud · 2024-03-25T11:45:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

muursh

I'm happy with this

BGluth

Yeah LGTM! Merge-er!

* Update plonky2 dependencies (#119) * Update plonky2 dependencies * Modify changelog * Charge gas before SLOAD and refactor `insert_accessed_storage_keys` (#117) * Charge gas before SLOAD and refactor `insert_accessed_storage_keys` * fmt * Only store value for cold access * PR feedback --------- Co-authored-by: BGluth <gluthb@gmail.com> * Increased the public interface for `trie_tools` (#123) - `trie_tools` needs to access a bit of currently private logic. Specifically, it needs to be able to process compact bytecode into `mpt_tries` directly. - I think this change is actually reasonable. I can see realistic use cases where we don't need to process an entire block trace but instead just want to decode some compact bytecode. - With the current public interface, the caller can only pass in an entire block trace to process. * Mpt trie panic refactor (#118) * refactor: refactoring mpt_trie to use more Results * fix: replace anyhow with this-error for mpt_trie * style: formatting * fix: fix results * fix: pr fixes * fix: fix error message * fix: format * fix: fix unusefull return type * fix: fix formatting * fix: pr fixes * fix: pr fixes * fix: pr fixes * tests: refactor some tests --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * refactor: remove some reallocations from decoder (#126) Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Charge cold access cost in *CALL* before accessing state (#124) * Charge cold access cost in CALL before accessing state * PR feedback --------- Co-authored-by: BGluth <gluthb@gmail.com> * chore: add debug function for better logging in development (#134) * chore: add debug function for better logging in development * chore: fix clippy issue --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Make test_receipt_encoding more meaningful. (#131) * Make test_receipt_encoding more meaningful. * Apply comment * Add a getter for the KERNEL codehash (#136) * Update CHANGELOG * Version bump for next release (#137) * Bumped sub-crate versions for a new release * Added in missing change log entries - Other PRs missed updating `CHANGELOG.md` * feat: swap out the internal U512 inside nibbles (#132) * feat: swap out the internal U512 inside nibbles * fix: comment fix * fix: fix clippy pr issues * fix: fix clippy issue * fix: fix pr comments * docs: update changelog * fix: update impl_to_nibbles --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Some clippy fixes (#149) * MAX fixes for clippy * fix transmut without annotations * please the fmt gods * Remove interpreter-specific preinialization logic from State trait (#139) * Remove interpreter specific methods from State trait * Changelog * Make some more functions constant (#154) * Make some more functions constant * Update changelog * fix(keccak-sponge): properly constrain padding bytes (#158) * fix(keccak-sponge): properly constrain padding bytes * fix: block bytes offset * fix: constrain zero padding bytes * fix: use collect_vec * feat: replace is_final_input_len with is_padding_byte * fix: remove unnecessary iterators * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * fix: remove redundant constraint * docs: define padding byte in comment --------- Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Reduce verbosity in logs (#160) * Reduce verbosity in logs * CHANGELOG * Add entry for 158 in CHANGELOG * Bump with latest starky (#161) * Bump with latest plonky2 * CHANGELOG * feat: decouple trace_decoder and proof_gen (#163) Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Simplify withdrawals logic (#168) * Simplify withdrawals logic * Update CHANGELOG * Clippy * feat: extend trace decoder err info (#148) * feat: extend trace decoder err info * fix: fix clippy issue * feat: swap out the internal U512 inside nibbles (#132) * feat: swap out the internal U512 inside nibbles * fix: comment fix * fix: fix clippy pr issues * fix: fix clippy issue * fix: fix pr comments * docs: update changelog * fix: update impl_to_nibbles --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Some clippy fixes (#149) * MAX fixes for clippy * fix transmut without annotations * please the fmt gods * fix: add pr comments fixes * fix: add pr comments fix * fix: add pr comment fix * docs: update changelog --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> Co-authored-by: Ben <bmarsh94@gmail.com> * Moved `Unreleased` changes to `0.3.0` (#173) - Also added two missing PRs --------- Co-authored-by: Hamy Ratoanina <hamy.ratoanina@toposware.com> Co-authored-by: wborgeaud <williamborgeaud@gmail.com> Co-authored-by: Vladimir Trifonov <vladimir-trifonov@users.noreply.github.com> Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> Co-authored-by: Linda Guiga <101227802+LindaGuiga@users.noreply.github.com> Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> Co-authored-by: Robin Salen <salenrobin@gmail.com> Co-authored-by: Ben <bmarsh94@gmail.com> Co-authored-by: Ayush Shukla <shuklaayush247@gmail.com>

* refactor: refactoring mpt_trie to use more Results * fix: replace anyhow with this-error for mpt_trie * style: formatting * fix: fix results * fix: pr fixes * fix: fix error message * fix: format * fix: fix unusefull return type * fix: fix formatting * fix: pr fixes * fix: pr fixes * fix: pr fixes * tests: refactor some tests --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com>

* Moved `Unreleased` changes to `0.3.0` (#173) - Also added two missing PRs * `main` --> `0.3.0` (#138) * Update plonky2 dependencies (#119) * Update plonky2 dependencies * Modify changelog * Charge gas before SLOAD and refactor `insert_accessed_storage_keys` (#117) * Charge gas before SLOAD and refactor `insert_accessed_storage_keys` * fmt * Only store value for cold access * PR feedback --------- Co-authored-by: BGluth <gluthb@gmail.com> * Increased the public interface for `trie_tools` (#123) - `trie_tools` needs to access a bit of currently private logic. Specifically, it needs to be able to process compact bytecode into `mpt_tries` directly. - I think this change is actually reasonable. I can see realistic use cases where we don't need to process an entire block trace but instead just want to decode some compact bytecode. - With the current public interface, the caller can only pass in an entire block trace to process. * Mpt trie panic refactor (#118) * refactor: refactoring mpt_trie to use more Results * fix: replace anyhow with this-error for mpt_trie * style: formatting * fix: fix results * fix: pr fixes * fix: fix error message * fix: format * fix: fix unusefull return type * fix: fix formatting * fix: pr fixes * fix: pr fixes * fix: pr fixes * tests: refactor some tests --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * refactor: remove some reallocations from decoder (#126) Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Charge cold access cost in *CALL* before accessing state (#124) * Charge cold access cost in CALL before accessing state * PR feedback --------- Co-authored-by: BGluth <gluthb@gmail.com> * chore: add debug function for better logging in development (#134) * chore: add debug function for better logging in development * chore: fix clippy issue --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Make test_receipt_encoding more meaningful. (#131) * Make test_receipt_encoding more meaningful. * Apply comment * Add a getter for the KERNEL codehash (#136) * Update CHANGELOG * Version bump for next release (#137) * Bumped sub-crate versions for a new release * Added in missing change log entries - Other PRs missed updating `CHANGELOG.md` * feat: swap out the internal U512 inside nibbles (#132) * feat: swap out the internal U512 inside nibbles * fix: comment fix * fix: fix clippy pr issues * fix: fix clippy issue * fix: fix pr comments * docs: update changelog * fix: update impl_to_nibbles --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Some clippy fixes (#149) * MAX fixes for clippy * fix transmut without annotations * please the fmt gods * Remove interpreter-specific preinialization logic from State trait (#139) * Remove interpreter specific methods from State trait * Changelog * Make some more functions constant (#154) * Make some more functions constant * Update changelog * fix(keccak-sponge): properly constrain padding bytes (#158) * fix(keccak-sponge): properly constrain padding bytes * fix: block bytes offset * fix: constrain zero padding bytes * fix: use collect_vec * feat: replace is_final_input_len with is_padding_byte * fix: remove unnecessary iterators * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Update evm_arithmetization/src/keccak_sponge/keccak_sponge_stark.rs Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * fix: remove redundant constraint * docs: define padding byte in comment --------- Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> * Reduce verbosity in logs (#160) * Reduce verbosity in logs * CHANGELOG * Add entry for 158 in CHANGELOG * Bump with latest starky (#161) * Bump with latest plonky2 * CHANGELOG * feat: decouple trace_decoder and proof_gen (#163) Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Simplify withdrawals logic (#168) * Simplify withdrawals logic * Update CHANGELOG * Clippy * feat: extend trace decoder err info (#148) * feat: extend trace decoder err info * fix: fix clippy issue * feat: swap out the internal U512 inside nibbles (#132) * feat: swap out the internal U512 inside nibbles * fix: comment fix * fix: fix clippy pr issues * fix: fix clippy issue * fix: fix pr comments * docs: update changelog * fix: update impl_to_nibbles --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> * Some clippy fixes (#149) * MAX fixes for clippy * fix transmut without annotations * please the fmt gods * fix: add pr comments fixes * fix: add pr comments fix * fix: add pr comment fix * docs: update changelog --------- Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> Co-authored-by: Ben <bmarsh94@gmail.com> * Moved `Unreleased` changes to `0.3.0` (#173) - Also added two missing PRs --------- Co-authored-by: Hamy Ratoanina <hamy.ratoanina@toposware.com> Co-authored-by: wborgeaud <williamborgeaud@gmail.com> Co-authored-by: Vladimir Trifonov <vladimir-trifonov@users.noreply.github.com> Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> Co-authored-by: Linda Guiga <101227802+LindaGuiga@users.noreply.github.com> Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> Co-authored-by: Robin Salen <salenrobin@gmail.com> Co-authored-by: Ben <bmarsh94@gmail.com> Co-authored-by: Ayush Shukla <shuklaayush247@gmail.com> * Fix withdrawals accesses in state trie (#176) * Bumped `trace_decoder` version for new `zk_evm` release (#177) * Bumped `trace_decoder` version for new `zk_evm` release * Updated `CHANGELOG.md` * Revert "`main` --> `0.3.0` (#138)" (#179) This reverts commit 88d75ed. * Some cleanup (#190) * Cleanup perform_op * Move interpreter test methods to test module * Some more cleanup and removal * more cleanup * More cleanup misc and doc * Remove outdated arg * Fix clippy --------- Co-authored-by: BGluth <gluthb@gmail.com> Co-authored-by: Hamy Ratoanina <hamy.ratoanina@toposware.com> Co-authored-by: wborgeaud <williamborgeaud@gmail.com> Co-authored-by: Vladimir Trifonov <vladimir-trifonov@users.noreply.github.com> Co-authored-by: Vladimir Trifonov <trifonov.vp@gmail.com> Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com> Co-authored-by: Robin Salen <salenrobin@gmail.com> Co-authored-by: Ben <bmarsh94@gmail.com> Co-authored-by: Ayush Shukla <shuklaayush247@gmail.com>

lastminutedev added 2 commits March 21, 2024 19:02

refactor: refactoring mpt_trie to use more Results

1df1350

fix: replace anyhow with this-error for mpt_trie

301e9a2

vladimir-trifonov requested review from muursh and Nashtare as code owners March 22, 2024 11:00

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 Mar 22, 2024

muursh requested a review from BGluth March 22, 2024 11:02

style: formatting

4e3df65

muursh force-pushed the mpt_trie_panic_refactor branch from 57ea909 to 4e3df65 Compare March 22, 2024 11:54

muursh reviewed Mar 22, 2024

View reviewed changes

fix: fix results

f2ccdd5

vladimir-trifonov requested a review from wborgeaud as a code owner March 22, 2024 12:12

github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Mar 22, 2024

vgnom added this to the Type 1 - Q1 2024 milestone Mar 22, 2024

lastminutedev and others added 6 commits March 22, 2024 16:41

fix: pr fixes

0c3a75c

fix: fix error message

d5be973

fix: format

af13dc4

fix: fix unusefull return type

475a6c6

fix: fix formatting

2225181

Merge branch 'develop' into mpt_trie_panic_refactor

8048d2e

vladimir-trifonov requested a review from muursh March 22, 2024 15:51

muursh reviewed Mar 22, 2024

View reviewed changes

mpt_trie/src/trie_ops.rs Show resolved Hide resolved

BGluth reviewed Mar 22, 2024

View reviewed changes

wborgeaud approved these changes Mar 25, 2024

View reviewed changes

mpt_trie/examples/simple.rs Outdated Show resolved Hide resolved

mpt_trie/src/debug_tools/diff.rs Outdated Show resolved Hide resolved

mpt_trie/src/debug_tools/stats.rs Outdated Show resolved Hide resolved

lastminutedev added 4 commits March 25, 2024 11:46

fix: pr fixes

737030f

fix: pr fixes

4f1807a

fix: pr fixes

dd9d8e1

tests: refactor some tests

196cdaf

vladimir-trifonov requested a review from muursh March 25, 2024 13:16

vladimir-trifonov requested review from BGluth and wborgeaud March 25, 2024 13:16

muursh approved these changes Mar 25, 2024

View reviewed changes

BGluth approved these changes Mar 25, 2024

View reviewed changes

muursh merged commit 71cd5b5 into develop Mar 25, 2024
7 checks passed

muursh deleted the mpt_trie_panic_refactor branch March 25, 2024 19:59

BGluth mentioned this pull request Apr 3, 2024

Version bump for next release #137

Merged

BGluth mentioned this pull request Apr 16, 2024

fixed warning #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mpt trie panic refactor #118

Mpt trie panic refactor #118

vladimir-trifonov commented Mar 22, 2024

muursh left a comment

BGluth left a comment

BGluth Mar 22, 2024 •

edited

Loading

vladimir-trifonov Mar 25, 2024

wborgeaud left a comment

sonarqubecloud bot commented Mar 25, 2024

muursh left a comment

BGluth left a comment

Mpt trie panic refactor #118

Mpt trie panic refactor #118

Conversation

vladimir-trifonov commented Mar 22, 2024

muursh left a comment

Choose a reason for hiding this comment

BGluth left a comment

Choose a reason for hiding this comment

BGluth Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

vladimir-trifonov Mar 25, 2024

Choose a reason for hiding this comment

wborgeaud left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 25, 2024

Quality Gate passed

muursh left a comment

Choose a reason for hiding this comment

BGluth left a comment

Choose a reason for hiding this comment

BGluth Mar 22, 2024 •

edited

Loading