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

Introduce trace processing for multiple storage tries #157

Closed
wants to merge 11 commits into from

Conversation

frisitano
Copy link
Contributor

Work in progress

@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Apr 10, 2024
@Nashtare Nashtare added the enhancement New feature or request label Apr 18, 2024
@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Apr 29, 2024
BGluth and others added 6 commits May 3, 2024 22:52
…xPolygonZero#197)

* First rough plan on trait structure

* Now conditionally uses evm types at compile time

- Created type aliases for all `evm_arithmetization` types that we
  need to swap out depending on whether or not we are compiling for
  `mpt` or `smt`.

* Removed the default feature of "mpt"

- Realized that there is no clean way to disable a default feature when
  it absolutely needs to be disabled (eg. when "smt" is enabled).
- Also added a compile time check to enforce that one of these features
  is enabled.

* Renamed `processed_block_trace.rs` --> `processed_block_trace_mpt.rs`

- Prep for `mpt`/`smt` logic feature gating.

* SQUASH!!

Rough draft of SMT & continuation support

- Still need to remove feature gating stuff.

* Removed feature gating for MPT/SMT

- Realized that we can link again both versions of the library at once
  without conditional compilation.

* A bit more cleanup

* Hack to avoid jemalloc failure

* Requested changes for PR 0xPolygonZero#197

* Fixed all clippy errors
* Fix access lists pointers

* Update evm_arithmetization/src/cpu/kernel/asm/core/access_lists.asm

Co-authored-by: Robin Salen <[email protected]>

* Address reviews

---------

Co-authored-by: Robin Salen <[email protected]>
* feat: add a few usability functions to the interface

* fix: fix clippy issue

* feat: add usize to nibbles

* docs: update changelog.md

---------

Co-authored-by: Vladimir Trifonov <[email protected]>
Co-authored-by: Ben <[email protected]>
@Nashtare
Copy link
Collaborator

Hey @frisitano, #197 has been removed from develop to be part of feat/type2 instead. As such, the force-push kinda messed-up the display of this PR. Would you mind cleaning it up so we can move on reviewing this and the companion PR on zero-bin?

Also (cc @muursh @cpubot), we don't have a priority integration queue at the zkEVM project level (yet?), but I'd suggest waiting slightly before merging #31 into develop, because of the alloy vs ethers-core topic mentioned in #229 and related issues. Specifically, I'd suggest merging in order:

  • @frisitano's work for the native tracer (2 PRs)
  • @0xaatif's work on alloy migration (currently done on eth-tx-proof, would need to be migrated to the native tracer of zero-bin instead)
  • then merge Feat/cancun #31 into develop

Any objection / suggestion?

@0xaatif
Copy link
Contributor

0xaatif commented May 21, 2024

So, to confirm my understanding of the path for the alloy/ethers path you're suggesting:

  • eth-tx-proof#22 is closed or otherwise de-prioritised
  • (I) open a new PR with similar changes in zero-bin, as a high priority because it blocks other work

This makes sense, given that (as far as I understand) eth-tx-proof has an unclear lifespan/was prototype-y

Nod from @cpubot was that there's a lot of duplicate code between zero-bin and eth-tx-proof and WIBNI there was only one repo

@frisitano
Copy link
Contributor Author

Sure thing. I'll see to it in the morning.

@Nashtare
Copy link
Collaborator

@0xaatif Yes that's correct. I'm sorry that you actually did implement it on eth-tx-proof as it is aimed at being deprecated in favor of @frisitano's work on zero-bin's native tracer 😬 We were only keeping it alive because zero-bin was only supporting jerigon until very recently (and because partners are using it at the moment for testing).

@frisitano
Copy link
Contributor Author

@Nashtare I've been working on this today but I'm running into some issues in regards to proof generation. From what I can see the transaction traces and trie data looks correct however the transaction kernel always fails on post transaction checks relating to trie modification and consistency checks. This is the zero-bin branch I'm working on - https://github.com/fractal-zkp/zero-bin/tree/tx-proof-rpc-new. If you could take a look to see if there is something I'm missing that would be appreciated.

Maybe we can cross reference against some traces produced by jerigon to see if there is any discrepancies. I may be missing something obvious but a second set of eyes would be appreciated.

@Nashtare
Copy link
Collaborator

Hey @frisitano, any specific changes since last time? AFAIR, we had almost a 100% success rate with your native tracer. Or would it be coming from latest develop changes you merged?

@Nashtare
Copy link
Collaborator

Nashtare commented May 22, 2024

Here are two traces to compare. I think txns are processed in reverse order (first one in native but last (13) one in jerigon). I've trimmed part of it for the upload, but relevant discrepancies are still there.

b19240705_jerigon.log
b19240705_native.log

@Nashtare
Copy link
Collaborator

On the zero-bin side, in process_transactions() within rpc/native module, we need to tweak:

-     for tx_hash in &block.transactions {
+     for tx_hash in block.transactions.iter().rev() {

This will make part of the block provable. For block 19240705, I got 8 txns proven, with a Kernel Panic on the 9th one. Still investigating.

@frisitano
Copy link
Contributor Author

This is replaced by #246

@Nashtare
Copy link
Collaborator

Closing as per #157 (comment).

@Nashtare Nashtare closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: trace_decoder Anything related to the trace_decoder crate. enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants