Make witness generation conform to draft specs#8
Make witness generation conform to draft specs#8jsign wants to merge 22 commits intoparadigmxyz:mainfrom
Conversation
…ince sorting is done at generation
Signed-off-by: jsign <jsign.uy@gmail.com>
Resolve merge conflicts between jsign/witness-generation-testing and main: - Cargo.toml: merge workspace members (add crates/tries, crates/zeth-mpt, testing/runner) - Cargo.lock: regenerate with all workspace members - Auto-merged: two-pass trie logic applied to crates/tries/src/default.rs, FixtureExecutionWitness + witness comparison in models.rs, TestCaseFailed error wrapping + witness assertion in blockchain_test.rs Amp-Thread-ID: https://ampcode.com/threads/T-019d42a6-d255-707f-9cc5-d228f1236413 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d42a6-d255-707f-9cc5-d228f1236413 Co-authored-by: Amp <amp@ampcode.com>
Signed-off-by: jsign <jsign.uy@gmail.com>
|
@mattsse, @figtracer, after this original PR creation we worked on official specs and tests. The official specs and test live in execution-spec@projects/zkevm. Now there are also official tests released too, so I think after the Reth PR gets merged, we should probably switch to using these official releases. I'm not sure yet if you have some opinion on how this can be managed in Reth, since the releases we are doing are basically rebasing the BTW, we can create a TG group if helps to discuss/chat about these things. |
|
@jsign thanks for the update. we've created a long-running branch https://github.com/paradigmxyz/stateless/tree/bal-devnet-3 pointing to reth@bal-devnet-3, which runs all adjacent fixtures. regarding the Reth PR, since part of the trie was completely rewritten ever since, you would need to rebase it paradigmxyz/reth#22289 (comment) then we can just add these improvements on top of stateless later. +i've also added the missing |
|
@figtracer, thanks! I'll work soon on rebasing paradigmxyz/reth#22289, and coming back to this PR to see how things look and we can keep iterating to move this forward. |
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
|
@jsign bumped reth to main, you can rebase now |
Signed-off-by: jsign <jsign.uy@gmail.com>
jsign
left a comment
There was a problem hiding this comment.
@figtracer, I think this is ready for some eyes again:
- I resolved previous comments so you see new comments as relevant ones.
- Added some extra flag helpers in
ef-runnersdebug stuff.
I think we still need to wait for paradigmxyz/reth#22289 to do a last Cargo.toml update and consider merging.
| fn test_names(&self) -> Vec<&str>; | ||
|
|
||
| /// Keep only the test cases whose name contains the given substring. | ||
| fn filter_by_name(&mut self, filter: &str); |
There was a problem hiding this comment.
@figtracer, I added a new --filter to the ef-runner so it is easier to potentially see if a particular test is problematic.
Example: EF_TEST_TRIE=default cargo run -p ef-test-runner --release -- ./fixtures --filter 'test_abcd[fork_PragueToOsakaAtTime15k-..]
| reth-ethereum-primitives = { git = "https://github.com/paradigmxyz/reth", rev = "adc960162f", default-features = false } | ||
| reth-evm = { git = "https://github.com/paradigmxyz/reth", rev = "adc960162f", default-features = false } | ||
| reth-evm-ethereum = { git = "https://github.com/paradigmxyz/reth", rev = "adc960162f" } | ||
| reth-chainspec = { git = "https://github.com/paradigmxyz/reth", rev = "6dac6974b4337822e2ce0dfe6d06fcc6c2b30a64", default-features = false } |
There was a problem hiding this comment.
This is pointing to the latest commit in paradigmxyz/reth#22289
But actually this still pointing to paradigmyxz/reth was a mistake, not sure how this works. Looks like my fork branch commit is observable somehow as a detached commit in the repo.
| name: Execution Witness tests | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| env: | ||
| RUST_LOG: info | ||
| RUST_BACKTRACE: 1 | ||
| EF_TEST_TRIE: default | ||
| EXECUTION_WITNESS_TESTS_URL: https://github.com/jsign/artifacts/releases/download/execution_witness_v0.0.1/execution_witness_tests_409fae8.tar.gz | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| cache-on-failure: true | ||
| - name: Run Execution Witness tests | ||
| run: cargo run -p ef-test-runner --release -- --skip testing/runner/execution-witness-skip.txt $EXECUTION_WITNESS_TESTS_URL |
There was a problem hiding this comment.
As I mentioned in a comment, there are now official spec and tests.
But since they are meant to run on top of hte latest BAL devnet EL branches, for this PR I decided to keep these older execution witness tests, just temporarily.
Not all the tests pass though, and the reason why is explained in the execution-witness-skip.txt file. I believe teh reason for it will also be a problem whenever the official tests are run.
I would say to:
- Merge this PR with this CI as is, since it can provide guarantees on a lot of passing tests until official releases are used.
- I can create a issue on this repo to explain in more detail that using official spec tests should be done, and provide more details that can help do that work.
There was a problem hiding this comment.
we're running these official fixtures at stateless@bal-devnet-3
main...bal-devnet-3#diff-e723b0fac5c1b72ac28debd4e719da6375ed298123f05186e64f5088677235fbR33
still some things to do regarding the ability to debug the tests properly (some don't pass) but most of the reth@bal-devnet-3 patching has been done
Merge this PR with this CI as is, since it can provide guarantees on a lot of passing tests until official releases are used.
we should wait for upstream first imo, the detached commit problem would also be solved
I can create a issue on this repo to explain in more detail that using official spec tests should be done, and provide more details that can help do that work.
would be very appreciated :)
There was a problem hiding this comment.
Nice, sounds great you're doing progress in that branch 🔥
I'll create the issue to explain things in more detail later/soon today.
| # The following tests are skipped since they all fail with a message: | ||
| # > Witness generation mismatch: expected X nodes, generated X+1 | ||
| # Note that `X` is a concrete number for each case. | ||
| # | ||
| # As far as investigated, the reason for this is that the new ProofNodeV2 in Reth | ||
| # bundles ExtensionNodes with their BranchNode children in a single virtual node. | ||
| # When the witness is generated, this encoded node always include both tnodes. | ||
| # | ||
| # This is not correct for witness generation, since if a proof of absence terminal node | ||
| # is the ExtensionNode, then the proof of absence should finish there and not include the | ||
| # BranchNode since isn't required. | ||
| # | ||
| # This makes any fixture that has a proof of absence ending in an ExtensionNode to fail since | ||
| # it is adding this +1 BranchNode when it shouldn't. |
There was a problem hiding this comment.
@mediocregopher, I dug a bit into Reth new V2 stuff, and I believe this is the reason for the failures. Trying to help a bit, having some line of thought whenever work continues to happen in making the tests pass.
Prob you have the inner workings more clear in your mind and can have a sense if this makes sense to you.
There was a problem hiding this comment.
Hi @jsign yes this makes sense and is correct. In proof V2 we did made an optimization to always reveal the extension's branch in the sparse trie, as we do sometimes need it for certain cases to update our DB properly, and this saves us from having to make another round-trip to the proof workers in those cases. Even in the old proof code we were always fully calculating the branch even if we didn't retain it, so this was simply a net-win for performance.
From the witness's perspective I understand it's not ideal though. Here's my thoughts on a possible solution, lmk if I'm missing something.
-
For the remove phase this situation shouldn't arise, because it's not possible that an account/slot was removed in a block without it being there in the first place, so an exclusion proof isn't relevant.
-
For the update phase it should be possible to do a post-processing on the proof nodes which were calculated for that phase, such branches whose short key is splitting a target only retain the extension with that short key, skipping the branch itself.
|
@figtracer, @mediocregopher, reg the failing CI can be quickly reproduced by: Looks like all these cases are a particular limitation of the default trie verification of the witness, when I'm not sure there is an easy fix here. Do you have any suggestions? |
|
@jsign @figtracer if you all want to start a group in TG feel free, my username is the same there |
For context, please see paradigmxyz/reth#22289 PR description.
This PR contains some extra fixes that now live in this repo. It temporarily adds a patch in crates to use that Reth branch.
I'll create PR comments to explain better the changes.
Trying to get some feelings about this PR plus the Reth mentioned one!