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

feat: trie diff tool #630

Merged
merged 2 commits into from
Sep 24, 2024
Merged

feat: trie diff tool #630

merged 2 commits into from
Sep 24, 2024

Conversation

atanmarko
Copy link
Contributor

@atanmarko atanmarko commented Sep 16, 2024 •
edited
Loading

Resolves #415

@github-actions github-actions bot added crate: trace_decoder Anything related to the trace_decoder crate. crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Sep 16, 2024
@atanmarko atanmarko self-assigned this Sep 17, 2024
@atanmarko atanmarko force-pushed the feat/trie-diff-tool branch 2 times, most recently from d7ec054 to c45ed15 Compare September 17, 2024 14:03
@github-actions github-actions bot added the crate: mpt_trie Anything related to the mpt_trie crate. label Sep 18, 2024
@atanmarko atanmarko changed the title [wip] feat: trie diff tool feat: trie diff tool Sep 18, 2024
@atanmarko atanmarko marked this pull request as ready for review September 18, 2024 14:25
@Nashtare Nashtare added this to the System strengthening milestone Sep 18, 2024
0xaatif
0xaatif reviewed Sep 19, 2024
View reviewed changes
evm_arithmetization/src/generation/mod.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/mod.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/mod.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/segments.rs Show resolved Hide resolved
evm_arithmetization/src/generation/mod.rs Show resolved Hide resolved
trace_decoder/src/observer.rs Outdated Show resolved Hide resolved
trace_decoder/src/observer.rs Outdated Show resolved Hide resolved
trace_decoder/src/typed_mpt.rs Outdated Show resolved Hide resolved
zero/src/bin/trie_diff.rs Outdated Show resolved Hide resolved
zero/src/bin/trie_diff.rs Show resolved Hide resolved
@atanmarko atanmarko force-pushed the feat/trie-diff-tool branch 3 times, most recently from 03e9033 to 26e547a Compare September 20, 2024 15:51
muursh
muursh reviewed Sep 23, 2024
View reviewed changes
zero/src/trie_diff/mod.rs Outdated Show resolved Hide resolved
@atanmarko
Copy link
Contributor Author

Squashing the PR as it gets quite tedious to rebase on top of the develop with ongoing refactors.

@atanmarko
feat: trie diff tool
e19f780 
feat: extract tries from the prover

fix: implement trace decoder observer

update: trie diff main

fix: refactor

fix: debug

save tries to disk

add comparison and printout

fix: cleanup 1

fix: cleanup 2

fix: fmt

fix: printout

fix: review 1

fix: review 2 - commments

fix: review 3 - collect_debug_tries

fix: review - refactor of errors

fix: cleanup error fields

fix: rebase
@atanmarko
fix: review
8825ea8
0xaatif
0xaatif approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

no blockers, nice work on the error refactoring

evm_arithmetization/src/generation/mod.rs
cpu_res?;
};

cpu_res?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think we need to bind a variable at all - just inline the ? to the expression above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove in the next PR on this topic (#655 )

evm_arithmetization/src/generation/segments.rs Show resolved Hide resolved
trace_decoder/src/observer.rs Show resolved Hide resolved
trace_decoder/src/observer.rs
Comment on lines +18 to +19
fn collect_tries(
&mut self,
Copy link
Contributor

Choose a reason for hiding this comment

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

want to return the data from the entrypoint function, not just to print them out or similar. I can not do that with the closure.

I'm not sure I follow - can you help me understand?

In my head what you've just said is trivial to implement as follows:

fn foo<T>(f: impl FnOnce() -> T) -> Result<usize, T> { .. }

zero/src/bin/trie_diff.rs
@@ -0,0 +1,172 @@
//! This binary is a debugging tool used to compare
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd move this to Cli, so it shows up with --help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do in the next PR on this topic, I am currently working on.

@atanmarko atanmarko merged commit 56e7d08 into develop Sep 24, 2024
16 of 17 checks passed
@atanmarko atanmarko deleted the feat/trie-diff-tool branch September 24, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muursh muursh muursh approved these changes

@0xaatif 0xaatif 0xaatif approved these changes

@Nashtare Nashtare Awaiting requested review from Nashtare Nashtare is a code owner

@LindaGuiga LindaGuiga Awaiting requested review from LindaGuiga LindaGuiga is a code owner

Assignees

@atanmarko atanmarko

Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: mpt_trie Anything related to the mpt_trie crate. crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Zero EVM
Status: Done
Milestone
System strengthening
Development

Successfully merging this pull request may close these issues.

Add trie diff tool for evm_arithmetization's CPU
4 participants
@atanmarko @muursh @Nashtare @0xaatif