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

feat: SMT support in trace_decoder ignores storage #693

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented Oct 3, 2024

You can now give trace_decoder an encoded SMT, and have it do... something.

Changes

Note this code is incorrect given #707, and we're probably doing the hash conversions wrong, but it does finalize the external API, and is a meaningful feature increment over the previous code.

fyi @hratoanina iteration of trait StateTrie

@github-actions github-actions bot added crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Oct 3, 2024
@0xaatif 0xaatif force-pushed the 0xaatif/messy-smt2 branch from b9f107c to f27bbdf Compare October 6, 2024 00:59
@0xaatif 0xaatif changed the title 0xaatif/messy smt2 feat: SMT support in trace_decoder ignores storage Oct 7, 2024
@0xaatif 0xaatif marked this pull request as ready for review October 7, 2024 14:12
@0xaatif 0xaatif self-assigned this Oct 7, 2024
@Nashtare Nashtare added this to the Type 2 - CDK Erigon (Q4 2024) milestone Oct 9, 2024
zero/src/prover.rs Show resolved Hide resolved
trace_decoder/src/core.rs Outdated Show resolved Hide resolved
trace_decoder/src/core.rs Outdated Show resolved Hide resolved
trace_decoder/src/tries.rs Outdated Show resolved Hide resolved
trace_decoder/src/tries.rs Show resolved Hide resolved
trace_decoder/src/tries.rs Outdated Show resolved Hide resolved
@@ -377,31 +465,30 @@ impl From<StateMpt> for HashedPartialTrie {
}
}

// TODO(0xaatif): https://github.com/0xPolygonZero/zk_evm/issues/706
Copy link
Member

Choose a reason for hiding this comment

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

One or two sentences about what is StateSmt struct and why are we using it would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to defer this to #722

@@ -411,7 +498,101 @@ impl StateTrie for StateSmt {
.map(|(addr, acct)| (keccak_hash::keccak(addr), *acct))
}
fn root(&self) -> H256 {
todo!()
conv_hash::smt2eth(self.as_smt().root)
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking: This seems like a quite expensive operation just to get the root of the trie (which may be called often in the code handling the state tries). Maybe we should internally cache smt?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to go the caching route then I'd vote leaving it for now and opening an issue for it.

fn visit(
hashes: &mut BTreeMap<SmtKey, H256>,
leaves: &mut BTreeMap<Address, CollatedLeaf>,
path: Stack<bool>,
Copy link
Member

Choose a reason for hiding this comment

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

Any compelling reason why common Vec could not be used as a stack, but external stackstack dependency is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It removes a footgun, and limits misuse:

fn visit(path: &mut Vec<bool>) {
    path.push(true);
    visit(path);
    path.pop(); // often forgotten
}

It also happens to have no heap usage in this case too

trace_decoder/src/tries.rs Outdated Show resolved Hide resolved
trace_decoder/src/tries.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ci label Oct 11, 2024
@0xaatif 0xaatif force-pushed the 0xaatif/messy-smt2 branch from c111680 to adeaa4e Compare October 11, 2024 10:26
Copy link
Contributor

@muursh muursh left a comment

Choose a reason for hiding this comment

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

We should open an issue for the caching improvement Marko mentioned but I'm good with this.

@0xaatif 0xaatif merged commit fa1e2f9 into develop Oct 16, 2024
20 checks passed
@0xaatif 0xaatif deleted the 0xaatif/messy-smt2 branch October 16, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants