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

Prefetch JUMPDESTs through RPC #427

Open
wants to merge 121 commits into
base: develop
Choose a base branch
from

Conversation

einar-polygon
Copy link
Contributor

@einar-polygon einar-polygon commented Jul 23, 2024

resolves #290

@einar-polygon einar-polygon added performance Performance improvement related changes crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Jul 23, 2024
@einar-polygon einar-polygon self-assigned this Jul 23, 2024
@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Jul 23, 2024
@Nashtare Nashtare added this to the Performance Tuning milestone Jul 24, 2024
@einar-polygon einar-polygon force-pushed the einar/prefetch_transaction_jumps/pr branch 2 times, most recently from 7ad89de to 18d5da4 Compare July 30, 2024 12:36
@einar-polygon einar-polygon force-pushed the einar/prefetch_transaction_jumps/pr branch 2 times, most recently from 6255f00 to 6613802 Compare August 7, 2024 15:25
evm_arithmetization/Cargo.toml Outdated Show resolved Hide resolved
evm_arithmetization/Cargo.toml Outdated Show resolved Hide resolved
zero_bin/rpc/Cargo.toml Outdated Show resolved Hide resolved
zero_bin/rpc/Cargo.toml Outdated Show resolved Hide resolved
@Nashtare Nashtare requested a review from 4l0n50 August 8, 2024 13:26
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
@einar-polygon einar-polygon force-pushed the einar/prefetch_transaction_jumps/pr branch from bdcf935 to 270256f Compare August 18, 2024 23:50
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Thanks Einar! Aside from the inline comments, a miss in this PR is the application of JD pre-fetching for jerigon as well, not only the native tracer.

I haven't tested it yet as I write this, but I think your current branch would crash on any jerigon payload (from the ZeroTracer) because the RPC will provide a Vec<ZeroTxResult> which contains TxnInfo, i.e. with the jumpdest table field already processed, as opposed to the TxnMeta from which you're starting the processing.

I think we're also missing fallback mechanisms, wherever the procedure fails for some reason, then we should default to the current behavior, i.e. no provided JDT and let the prover do the work.

evm_arithmetization/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/tests/add11.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/prover_input.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/prover_input.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/native/txn.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

You also need to address the format change on all hardcoded witnesses, either by re-generating them, or adding default empty JDTs to each of them, and leaving the prover handle them internally as before.

@einar-polygon
Copy link
Contributor Author

You also need to address the format change on all hardcoded witnesses, either by re-generating them, or adding default empty JDTs to each of them, and leaving the prover handle them internally as before.

Indeed. Which solution do you prefer and who could I talk to about this CI setup?

@Nashtare
Copy link
Collaborator

You also need to address the format change on all hardcoded witnesses, either by re-generating them, or adding default empty JDTs to each of them, and leaving the prover handle them internally as before.

Indeed. Which solution do you prefer and who could I talk to about this CI setup?

I'm not sure I understand the relation between the broken tests and the CI. We don't have to update them all (especially because some dev blocks are from an outdated devchain IIRC), but at least have them have proper format so that they can run correctly. If your question was how to test this logic of JDT pre-fetching in the CI, we could do this with the Jerigon Integration related jobs, we're testing both native and jerigon tracers there.

@Nashtare
Copy link
Collaborator

@einar-polygon Has it been tested against some endpoint? Should I run it against a few dozen blocks to see how it behaves? (ideally after conflicts are addressed)

@einar-polygon
Copy link
Contributor Author

@einar-polygon Has it been tested against some endpoint? Should I run it against a few dozen blocks to see how it behaves? (ideally after conflicts are addressed)

I checked that the JUMPDEST_tables were identical for the small Calcun block and a few others against the paid Quicknode endpoint. But it would be good to have a set of interesting blocks with exceptions, reverts and what not.

Here is a start:

txn type 0-3
20548415
20240058
19665756

I will add a bash script to iterate over them while we test.

@Nashtare
Copy link
Collaborator

I'll launch a run against the devchain once it's rebased (it has a conflicting Erigon version that requires some latest PRs here)

@einar-polygon einar-polygon force-pushed the einar/prefetch_transaction_jumps/pr branch 2 times, most recently from a0f0438 to 05e065a Compare August 28, 2024 13:58
@Nashtare
Copy link
Collaborator

I got a panic with your endpoint on block 0x13adb68
at line let lower_bytes = U160::from(callee_raw); in zk_evm/zero_bin/rpc/src/native/txn.rs:

Uint conversion error: Value is too large for Uint<160>

@Nashtare
Copy link
Collaborator

Nashtare commented Aug 29, 2024

So some more data with your endpoint. I tried 140 consecutive blocks independently.

  • 140/140 failed
  • 36 valid witnesses, failing when batching transactions (tested with -b 2)
  • 32 failures due to unreachable code, all from failed transactions -> cf discussion on slack
  • 72 failures due to Uint conversion error: Value is too large for Uint<160> as shared above

trace_decoder/src/decoding.rs Outdated Show resolved Hide resolved
trace_decoder/src/decoding.rs Outdated Show resolved Hide resolved
trace_decoder/src/decoding.rs Outdated Show resolved Hide resolved
@einar-polygon einar-polygon force-pushed the einar/prefetch_transaction_jumps/pr branch from a9b13a3 to e351ff9 Compare September 2, 2024 17:54
evm_arithmetization/src/generation/jumpdest.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/generation/jumpdest.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jumpdest.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jumpdest.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jumpdest.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jumpdest.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jerigon.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/main.rs Outdated Show resolved Hide resolved
zero_bin/rpc/src/jumpdest.rs Outdated Show resolved Hide resolved
@einar-polygon
Copy link
Contributor Author

Quid of the large test chain?

I don't understand this question.

The first testnet containing 688 blocks is contained in the other chain (6555) blocks (at the start of the chain), but the first blocks don't fail, below are the failing ones (all Jerigon related).

Oh, I didn't realise this was the case. Well, then assuming nothing has changed since you verified those, then that leaves:

678, 679, 680, 681, 697, 737.

Anyway, I just merged in latest develop.

@Nashtare
Copy link
Collaborator

Nashtare commented Nov 4, 2024

Quid of the large test chain?

I don't understand this question.

I was just asking the results of running this branch over the large test chain (6555 blocks), given this one is much more extensive (containing the previous blocks + all matrixed variants of the Ethereum test suite).

@einar-polygon
Copy link
Contributor Author

For the record, here is the last run over the full chain from last Wednesday https://gist.github.com/einar-polygon/c5bbe371587dc0002b6789722b5c22f8

I will post a fresh one tomorrow or whenever it finishes.

@Nashtare Nashtare removed the request for review from muursh November 5, 2024 15:42
@einar-polygon
Copy link
Contributor Author

Latest full run: https://gist.github.com/einar-polygon/d238ecad89307132b92c9b920327b1df

Caveat: Stats has a bug. The presence of a 124-error causes one 1-error, one 2-error and one 4-error to be detected. This will be fixed in next run.

@Nashtare
Copy link
Collaborator

Nashtare commented Nov 12, 2024

Note that if you regenerate your witnesses you won't have any "expected" failures (the Jerigon endpoint has been updated). -> I just can't read properly.
As for the timeout issues, what's the timeout value? Is it timing out on witness (JDT) fetching or at the proving stage?
Also why are you getting a extension collapse error? Aren't you running with --rpc-type jerigon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates. performance Performance improvement related changes
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Prefetch transaction jumps through opcode tracer
5 participants