Introduce native tracer support #81

frisitano · 2024-05-24T12:31:04Z

This PR introduces native tracer support such that blocks can be prover using standard rpc methods. This is a replacement for #51. This is currently blocked as alchemy is returning incorrect data for prestate tracer requests. I have opened a ticket with the support team and they are looking into it.

Merge steps:

Merge companion PR Native trace processing support zk_evm#246
Repoint zk_evm dependencies in cargo to either the git repo or a new release.
Merge this PR into develop.

frisitano · 2024-05-27T12:12:11Z

I have tested this over the block range of 19240650 - 19240850 and 199 blocks completed successfully with one failure. This yields a 99.5% success rate.

muursh · 2024-05-27T14:22:38Z

I have tested this over the block range of 19240650 - 19240850 and 199 blocks completed successfully with one failure. This yields a 99.5% success rate.

What was the failure? Which block and do you know why?

frisitano · 2024-05-27T15:15:35Z

I have tested this over the block range of 19240650 - 19240850 and 199 blocks completed successfully with one failure. This yields a 99.5% success rate.

What was the failure? Which block and do you know why?

It's block 19240734. The error is:

Error: Failed to get proof for account: JsonRpcClientError(TimeoutError)

I have not had a chance to investigate the root cause of this but @Nashtare and I had concluded that the success rate was sufficiently high to merge as is and then leave this investigation / fix for a follow up issue / PR.

Cargo.toml

Nashtare · 2024-05-28T22:22:33Z

tools/prove_blocks.sh

@@ -5,6 +5,7 @@
 # 2 --> End block index (inclusive)
 # 3 --> Rpc endpoint:port (eg. http://35.246.1.96:8545)
 # 4 --> Ignore previous proofs (boolean)
+# 5 --> Rpc type (eg. jerigon / native)


Can this be set before Ignore previous proofs? So that it goes in pair with the RPC endpoint, and we can keep the boolean as optional with default value. Not really blocking though

Nashtare · 2024-06-03T20:09:41Z

rpc/src/rpc/native/mod.rs

+        debug!("Got block result: {:?}", rpc_block_metadata.block_by_number);
+        debug!("Got trace result: {:?}", block_trace);
+        debug!("Got chain_id: {:?}", rpc_block_metadata.chain_id);


nit: maybe remove? Not sure this brings much

Nashtare · 2024-06-03T20:11:00Z

rpc/src/rpc/native/txn.rs

+) -> Result<Vec<TxnInfo>> {
+    let mut futures_ordered = FuturesOrdered::new();
+
+    for tx_hash in &block.transactions {


Last time we tested blocks, I recall we needed to reverse the order here. Has this been tested since @frisitano?

Yes, I tested this on 200 blocks with a 99.5% success rate. The change was made below on line 30 in which we push_back instead of the prior push_front, see ref.

Awesome, thanks!
The failure being due to the branch collapsing bug mentioned in the related issue I presume?

No that issue has been fixed (hacked) by fetching proofs after block execution and inserting short nodes (extension and leafs) into the trie builder.

This failure is due to an error when trying to fetch a proof for an account - see #81 (comment). I haven't had an opportunity to look into it.

Nashtare · 2024-06-03T20:12:46Z

rpc/src/rpc/native/trie.rs

+pub const EMPTY_TRIE_HASH: H256 = H256([
+    0x56, 0xe8, 0x1f, 0x17, 0x1b, 0xcc, 0x55, 0xa6, 0xff, 0x83, 0x45, 0xe6, 0x92, 0xc0, 0xf8, 0x6e,
+    0x5b, 0x48, 0xe0, 0x1b, 0x99, 0x6c, 0xad, 0xc0, 0x01, 0x62, 0x2f, 0xb5, 0xe3, 0x63, 0xb4, 0x21,
+]);


non-blocking, as not exposed (yet) but we should try to group all these common constants somewhere (probably somewhere under zk_evm crates) and reuse/re-exports them instead of always redefining them

Agreed, I will add a TODO. I was also wondering if this module would be better positioned in the zk_evm/mpt_trie crate for reuse?

yeah that'd make sense, it's a bit too specific on MPTs to be inside zero-bin I'd say.
cc @0xaatif @BGluth

Yeah I think inside mpt_trie probably makes the most sense.

frisitano · 2024-06-03T21:39:45Z

I've opened a PR in zk_evm repo to migrate the PartialTrieBuilder. I've repointed deps of this PR to the new branch and also addressed the feedback. Once that PR is merged I will repoint deps and squash commits to make it ready for merge.

0xaatif

The most important change is keeping with alloy from now on.

I'd like to re-review after markups

0xaatif · 2024-06-04T10:30:23Z

.gitignore

@@ -13,3 +13,6 @@ proofs/
 # Serialized generated prover & verifier state used by plonky2
 prover_state_*
 verifier_state_*
+
+# System files
+.DS_Store


nit: newline

README.md

rpc/src/cli.rs

rpc/src/rpc/jerigon.rs

0xaatif · 2024-06-04T10:57:02Z

rpc/src/rpc/native/mod.rs

+
+/// A static retry policy that always retries.
+#[derive(Debug, Default)]
+pub struct StaticRetryPolicy;


Library code should never care about this - that's why we should be abstract over Provider

0xaatif · 2024-06-04T10:58:43Z

rpc/src/rpc/native/state.rs

+) -> Result<
+    (
+        Vec<(H160, EIP1186ProofResponse)>,
+        Vec<(H160, EIP1186ProofResponse)>,
+    ),
+    anyhow::Error,
+> {


nit

Suggested change

) -> Result<

(

Vec<(H160, EIP1186ProofResponse)>,

Vec<(H160, EIP1186ProofResponse)>,

),

anyhow::Error,

> {

) -> anyhow::Result<[Vec<(H160, EIP1186ProofResponse)>; 2]> {

0xaatif · 2024-06-04T11:02:26Z

rpc/src/rpc/native/state.rs

+                        keys.into_iter().collect(),
+                        Some((block_number - 1).into()),
+                    )
+                    .map_err(|e| anyhow!("Failed to get proof for account: {:?}", e))


use anyhow::Context as __;

Suggested change

.map_err(|e| anyhow!("Failed to get proof for account: {:?}", e))

.context("Failed to get proof for account")

Anyhow will preserve the source error in it's cause chain. This is the whole point of using anyhow

0xaatif · 2024-06-04T11:03:25Z

rpc/src/rpc/native/txn.rs

+        futures::try_join!(tx_fut, tx_receipt_fut, pre_trace_fut, diff_trace_fut,)?;
+
+    Ok((
+        tx.ok_or_else(|| anyhow!("Transaction not found."))?,


use anyhow::Context as _;

Suggested change

tx.ok_or_else(|| anyhow!("Transaction not found."))?,

tx.context("Transaction not found.")?,

Also works on Options

0xaatif · 2024-06-04T11:04:09Z

tools/debug_blocks.sh

+    current_a=$((initial_a + i))
+    echo "Running debug block script with block=$current_a rpc=$b"
+    "$script_dir/debug_block.sh" $current_a $b $rpc_type
+done


frisitano · 2024-06-05T14:55:48Z

The most important change is keeping with alloy from now on.

I'd like to re-review after markups

I've been trying to migrate to alloy but I'm running into an issue when trying to rlp encode the ReceiptEnvelope to populate trace_decoder::trace_protocol::TxnMeta. I would expect the following to work but it doesn't:

    let tx_receipt = provider.get_transaction_receipt(tx_hash).await?.unwrap();
    let tx_receipt_rlp = tx_receipt.inner.encoded_2718();

If someone could take a look at this to assess what I may be doing wrong that would be appreciated.

0xaatif · 2024-06-05T15:02:34Z

Does any of this help?
https://github.com/0xPolygonZero/eth-tx-proof/blob/3805968cf6c1d88fa40ca10a37e9bba00c05faf3/leader/src/lib.rs#L521-L527

/// There are two representations of domain objects in ethereum:
/// - RPC (JSON), in [`alloy::rpc`].
/// - RLP (binary), in [`alloy::consensus`].
///
/// This module provides best-effort [RLP encoding](alloy::rlp::Encodable) for
/// RPC types.

frisitano · 2024-06-05T15:06:28Z

Does any of this help?

https://github.com/0xPolygonZero/eth-tx-proof/blob/3805968cf6c1d88fa40ca10a37e9bba00c05faf3/leader/src/lib.rs#L521-L527
/// There are two representations of domain objects in ethereum:

/// - RPC (JSON), in [`alloy::rpc`].

/// - RLP (binary), in [`alloy::consensus`].

///

/// This module provides best-effort [RLP encoding](alloy::rlp::Encodable) for

/// RPC types.

It certainly looks like it could, thanks!

frisitano · 2024-06-07T11:45:49Z

I've implemented an initial migration to alloy but I have hit a blocker. Previously we were using a retry policy to mitigate the request failures due to rate limiting from rpc providers. ethers shipped with an out the box retry client. I have done some investigation to see how we can implement a retry policy for alloy. alloy does ship with a ClientBuilder struct that wraps tower::ServiceBuilder. It is suggested that we should be able to introduce a retry layer using this builder however my efforts (and those of others) have not been successful thus far. Without the retry logic I am unable to test against rpc providers.

This PR still needs a hygiene sweep but I would like to address the retry issue first.

frisitano · 2024-06-08T14:41:25Z

This is ready for review along with 0xPolygonZero/zk_evm#258.

rpc/src/retry.rs

frisitano · 2024-06-17T12:11:22Z

I've completed the merge with develop and have tested native. It may be wise to test jerigon again but I don't have access to a jergion rpc.

atanmarko · 2024-06-17T12:13:45Z

EDIT: obligatory parameter to select jerigon or native before test_only and ignore previous proofs

@frisitano You can probably update tools/prove_jerigon.sh to prove_rpc.sh with the default as jerigon client and optional parameter as native

frisitano · 2024-06-17T12:16:26Z

@frisitano You can probably update tools/prove_jerigon.sh to prove_rpc.sh with the default as jerigon client and optional parameter as native

Good catch! However, I don't think we will be able to make the rpc type optional as we can only have one optional argument (the last positional argument). Currently test_only is being used as the optional argument. I am happy to introduce it as a required argument, what do you think?

atanmarko · 2024-06-17T12:34:49Z

Tested prove_jerigon.sh with 371ca61, no regressions.

frisitano · 2024-06-17T12:38:38Z

EDIT: obligatory parameter to select jerigon or native before test_only and ignore previous proofs

@frisitano You can probably update tools/prove_jerigon.sh to prove_rpc.sh with the default as jerigon client and optional parameter as native

I've updated the script. Let me know your thoughts.

atanmarko · 2024-06-17T12:43:07Z

EDIT: obligatory parameter to select jerigon or native before test_only and ignore previous proofs
@frisitano You can probably update tools/prove_jerigon.sh to prove_rpc.sh with the default as jerigon client and optional parameter as native

I've updated the script. Let me know your thoughts.

@frisitano Script is OK. Please fix also the docs and the CI.

frisitano · 2024-06-17T12:51:11Z

EDIT: obligatory parameter to select jerigon or native before test_only and ignore previous proofs
@frisitano You can probably update tools/prove_jerigon.sh to prove_rpc.sh with the default as jerigon client and optional parameter as native

I've updated the script. Let me know your thoughts.

@frisitano Script is OK. Please fix also the docs and the CI.

README updated. As discussed on slack, no changes required to CI.

rpc/src/jerigon.rs

atanmarko · 2024-06-17T14:39:03Z

@frisitano We want to fix alloy version, so we will use recently released tag v0.1.1.
Here is the PR. Could you please regression test it with native tracer? Jerigon works without regressions with v0.1.1.

atanmarko · 2024-06-17T16:08:08Z

@Nashtare @BGluth Any additional comments/requests or we are ready to merge?

Nashtare

@atanmarko Only did a synthetic review over the latest changes but I'm good to merge.

frisitano · 2024-06-18T04:52:29Z

@frisitano We want to fix alloy version, so we will use recently released tag v0.1.1. Here is the PR. Could you please regression test it with native tracer? Jerigon works without regressions with v0.1.1.

Regression tested latest develop with the native tracer. No regressions, working as expected.

This was referenced May 24, 2024

Native trace processing support 0xPolygonZero/zk_evm#246

Merged

Introduce native rpc support for witness generation #51

Closed

frisitano marked this pull request as ready for review May 27, 2024 12:08

frisitano requested review from cpubot, muursh and Nashtare as code owners May 27, 2024 12:08

Nashtare added the enhancement New feature or request label May 28, 2024

muursh reviewed May 29, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

frisitano force-pushed the tx-proof-rpc-new branch 4 times, most recently from 4bbad04 to 5234bc9 Compare May 30, 2024 15:57

Nashtare reviewed Jun 3, 2024

View reviewed changes

0xaatif self-requested a review June 4, 2024 10:00

0xaatif suggested changes Jun 4, 2024

View reviewed changes

0xaatif mentioned this pull request Jun 18, 2024

CLI documentation in README should be autogenerated, else it'll just get out of date 0xPolygonZero/zk_evm#289

Open

Nashtare mentioned this pull request Jun 5, 2024

Unify shared constants into a common public module 0xPolygonZero/zk_evm#261

Closed

vgnom added this to the Cancun - Q2 2024 milestone Jun 5, 2024

frisitano force-pushed the tx-proof-rpc-new branch 2 times, most recently from 73f310c to 7cd3d62 Compare June 8, 2024 14:40

frisitano requested a review from 0xaatif June 8, 2024 14:43

frisitano added 3 commits June 17, 2024 19:24

Introduce native tracer support

6f4695c

remove arc and refactor code

cc81f4d

merge upstream

07cea87

frisitano force-pushed the tx-proof-rpc-new branch from 8752d77 to 07cea87 Compare June 17, 2024 11:38

frisitano added 2 commits June 17, 2024 19:41

update README

ab4aff6

remove unecessary tools

41bcbbd

atanmarko reviewed Jun 17, 2024

View reviewed changes

rpc/src/retry.rs Show resolved Hide resolved

use reference for provider

371ca61

frisitano added 2 commits June 17, 2024 20:36

refactor prove_rpc tool

0c2dd1a

remove prove_jerigon tool

ec1a99d

update README for prove_rpc.sh script changes

0fd74c0

atanmarko mentioned this pull request Jun 17, 2024

Fixes related to nightly and alloy #101

Merged

atanmarko approved these changes Jun 17, 2024

View reviewed changes

atanmarko reviewed Jun 17, 2024

View reviewed changes

rpc/src/jerigon.rs Outdated Show resolved Hide resolved

remove dead code

5f4cdb4

Nashtare approved these changes Jun 17, 2024

View reviewed changes

atanmarko removed the request for review from cpubot June 17, 2024 16:53

BGluth approved these changes Jun 17, 2024

View reviewed changes

atanmarko merged commit 23fff4d into 0xPolygonZero:develop Jun 17, 2024
5 checks passed

Nashtare mentioned this pull request Jun 18, 2024

Support fetching witnesses via eth-tx-proof #33

Closed

	.map_err(\|e\| anyhow!("Failed to get proof for account: {:?}", e))
	.context("Failed to get proof for account")

	tx.ok_or_else(\|\| anyhow!("Transaction not found."))?,
	tx.context("Transaction not found.")?,

Introduce native tracer support #81

Introduce native tracer support #81

Conversation

frisitano commented May 24, 2024

frisitano commented May 27, 2024 • edited Loading

muursh commented May 27, 2024

frisitano commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frisitano commented Jun 3, 2024

0xaatif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frisitano commented Jun 5, 2024

0xaatif commented Jun 5, 2024

frisitano commented Jun 5, 2024

frisitano commented Jun 7, 2024

frisitano commented Jun 8, 2024

frisitano commented Jun 17, 2024

atanmarko commented Jun 17, 2024 • edited Loading

frisitano commented Jun 17, 2024

atanmarko commented Jun 17, 2024

frisitano commented Jun 17, 2024

atanmarko commented Jun 17, 2024

frisitano commented Jun 17, 2024

atanmarko commented Jun 17, 2024 • edited Loading

atanmarko commented Jun 17, 2024

Nashtare left a comment

Choose a reason for hiding this comment

frisitano commented Jun 18, 2024

frisitano commented May 27, 2024 •

edited

Loading

atanmarko commented Jun 17, 2024 •

edited

Loading

atanmarko commented Jun 17, 2024 •

edited

Loading