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.

Sign up for GitHub

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

Jump to bottom

Fix withdrawals accesses in state trie #176

Merged
merged 2 commits into from
Apr 20, 2024
Merged

Fix withdrawals accesses in state trie #176

merged 2 commits into from
Apr 20, 2024

Conversation

Nashtare
Copy link
Collaborator

#168 didn't include withdrawals accesses into the initial payload state trie, causing issues when processing the last transactions. Tested against John's block witnesses.

This fixes it by adding them while doing the initial preprocessing. Also updates some related outdated comments.

@Nashtare Nashtare requested a review from BGluth April 20, 2024 15:17
@Nashtare Nashtare self-assigned this Apr 20, 2024
@Nashtare Nashtare requested a review from muursh as a code owner April 20, 2024 15:17
@Nashtare Nashtare mentioned this pull request Apr 20, 2024
Closed
@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Apr 20, 2024
BGluth
BGluth reviewed Apr 20, 2024
View reviewed changes
trace_decoder/src/processed_block_trace.rs Outdated
Comment on lines 92 to 93
let last_tx_idx = self.txn_info.len() - 1;

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... Can this underflow if we have an empty block? Maybe saturating_sub would fix this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm and actually I think I need to handle withdrawals on empty payloads. Not that it matters in real life much but for test chains this may be a pb. will fix both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case the last payload is a dummy, it hasn't been processed by the incoming jerigon payload so I added logic to insert withdrawal state accesses. Also fixed the underflow here (although it wouldn't even enter the loop so wouldn't have been a big deal, but I like it better out of consistency).

trace_decoder/src/processed_block_trace.rs Outdated Show resolved Hide resolved
BGluth
BGluth approved these changes Apr 20, 2024
View reviewed changes
Copy link
Contributor

@BGluth BGluth left a comment

Choose a reason for hiding this comment

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

LGTM!

@Nashtare Nashtare merged commit 6033b3a into develop Apr 20, 2024
6 checks passed
@Nashtare Nashtare deleted the fix-with branch April 20, 2024 20:09
@BGluth BGluth mentioned this pull request Apr 22, 2024
Merged
LindaGuiga added a commit that referenced this pull request Apr 26, 2024
@LindaGuiga @BGluth
@hratoanina @wborgeaud @vladimir-trifonov @lastminutedev @Nashtare @muursh @shuklaayush
Merge with develop (#195)
79df636 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BGluth BGluth BGluth approved these changes

@muursh muursh muursh approved these changes

Assignees

@Nashtare Nashtare

Labels
crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Zero EVM
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nashtare @muursh @BGluth