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

[pallet-revive] add tracing support (3/3) #6727

Open
wants to merge 23 commits into
base: pg/add-tracing-support-2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f3b3d0e
Add new rpc API
pgherveou Jan 14, 2025
7c06a39
rm extra spaces
pgherveou Jan 15, 2025
c17c94c
Improve doc
pgherveou Jan 15, 2025
b66aec2
fix runtime impl
pgherveou Jan 16, 2025
db77731
fix tx_traces runtime impl
pgherveou Jan 16, 2025
e5314b2
rm extra logs
pgherveou Jan 16, 2025
807ebd6
Adding note regarding usage and response size
pgherveou Jan 16, 2025
3fbcaed
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 16, 2025
ce2ff1e
fix up merge
pgherveou Jan 16, 2025
92ae9ad
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 16, 2025
2e16534
fix
pgherveou Jan 16, 2025
a37a5ef
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 16, 2025
0189fb8
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 16, 2025
8b34beb
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 16, 2025
01c2ebc
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 17, 2025
c484137
update
pgherveou Jan 17, 2025
0641eeb
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 17, 2025
b09394f
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 17, 2025
505d89f
update new names
pgherveou Jan 17, 2025
d9b0b2c
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 17, 2025
dbed695
Update comment
pgherveou Jan 17, 2025
e356429
Merge branch 'pg/add-tracing-support-2' into pg/evm-debug
pgherveou Jan 23, 2025
3adbc1a
Merge remote-tracking branch 'refs/remotes/origin/pg/evm-debug' into …
pgherveou Jan 23, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3303,18 +3303,49 @@ impl_runtime_apis! {
}

fn trace_block(
_block: Block,
_config: pallet_revive::evm::TracerConfig
block: Block,
config: pallet_revive::evm::TracerConfig
) -> Result<Vec<(u32, pallet_revive::evm::EthTraces)>, sp_runtime::DispatchError> {
unimplemented!()
let mut tracer = pallet_revive::debug::Tracer::from_config(config);
let mut traces = vec![];

pallet_revive::using_tracer(&mut tracer, || {
Executive::initialize_block(block.header());

for (index, ext) in block.extrinsics().into_iter().enumerate() {
let _ = Executive::apply_extrinsic(ext.clone());
pgherveou marked this conversation as resolved.
Show resolved Hide resolved
pallet_revive::with_tracer(|tracer| {
if !tracer.has_traces() {
pgherveou marked this conversation as resolved.
Show resolved Hide resolved
let tx_traces = tracer.collect_traces().as_eth_traces::<Runtime>();
traces.push((index as u32, tx_traces));
pgherveou marked this conversation as resolved.
Show resolved Hide resolved
}
});
}
});

Ok(traces)
}

fn trace_tx(
_block: Block,
_tx_index: u32,
_config: pallet_revive::evm::TracerConfig
block: Block,
tx_index: u32,
config: pallet_revive::evm::TracerConfig
) -> Result<pallet_revive::evm::EthTraces, sp_runtime::DispatchError> {
unimplemented!()
let mut tracer = pallet_revive::debug::Tracer::from_config(config);
Executive::initialize_block(block.header());

for (index, ext) in block.extrinsics().into_iter().enumerate() {
if index as u32 == tx_index {
pallet_revive::using_tracer(&mut tracer, || {
let _ = Executive::apply_extrinsic(ext.clone());
pgherveou marked this conversation as resolved.
Show resolved Hide resolved
});
break;
} else {
let _ = Executive::apply_extrinsic(ext.clone());
pgherveou marked this conversation as resolved.
Show resolved Hide resolved
}
}

Ok(tracer.collect_traces().as_eth_traces::<Runtime>())
}
}

Expand Down
43 changes: 43 additions & 0 deletions substrate/client/rpc-api/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,49 @@ pub trait StateApi<Hash> {
#[method(name = "state_call", aliases = ["state_callAt"], blocking)]
fn call(&self, name: String, bytes: Bytes, hash: Option<Hash>) -> Result<Bytes, Error>;

/// Retrieve the block using the specified hash and invoke a runtime API method from the parent
/// state of this block. This allows the method to replay the extrinsics from the parent state
/// of the block.
///
/// This is typically used to replay transactions with tracing enabled. For example,
/// `pallet_revive` uses this to capture smart-contract call traces.
///
/// # Parameters
/// - `name`: The name of the runtime API method to call.
/// - `block`: The hash of the block to replay.
/// - `bytes`: Additional, encoded data to pass to the runtime API method, after the block data.
///
/// # `curl` example
///
/// - Call `pallet_revive` [`trace_block`](https://paritytech.github.io/polkadot-sdk/master/pallet_revive/trait.ReviveApi.html#method.trace_block)
Copy link
Member

@niklasad1 niklasad1 Jan 15, 2025

Choose a reason for hiding this comment

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

Do you know "ballpark size" of the response for ReviveApi_trace_block?

The reason why I'm asking is because --rpc-max-response-limit==10MB by default and maybe worth adding a comment about it whether one needs a bigger limit for this RPC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I will mention it in the doc
People running this will probably do it on a dedicated "tracing node" where they can tune this up.

Similarly, can the wasm heap memory @kianenigma be tuned up for this kind of "offchain" call?
For validators, I understand this is defined on-chain, but I want people running such "tracing node" to be able to adjust it.

/// to replay a block and capture call traces.
///
/// ```text
/// curl http://localhost:9944 \
/// -H 'Content-Type: application/json' \
/// -d '{
/// "id":1, "jsonrpc":"2.0", "method":"state_debugBlock", \
/// "params": ["ReviveApi_trace_block", "0xb246acf1adea1f801ce15c77a5fa7d8f2eb8fed466978bcee172cc02cf64e264"]
/// }'
/// }'
/// ```
///
/// - Call `pallet_revive` [`trace_tx`](https://paritytech.github.io/polkadot-sdk/master/pallet_revive/trait.ReviveApi.html#method.trace_block)
/// to replay a block and capture the call trace of the transaction at the given index.
///
/// ```text
/// curl http://localhost:9944 \
/// -H 'Content-Type: application/json' \
/// -d '{
/// "id":1, "jsonrpc":"2.0", "method":"state_debugBlock", \
/// "params": ["ReviveApi_trace_tx",
/// "0xb246acf1adea1f801ce15c77a5fa7d8f2eb8fed466978bcee172cc02cf64e264", "0x2a000000"]
/// }'
/// }'
/// ```
#[method(name = "state_debugBlock", blocking, with_extensions)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we build this entirely with the new RPC APIs instead? We plan to revisit the legacy ones soon and deprecate as much as possible.

One way that this could work without introducing the debug_block RPC:

  • Step 1 (maybe optional): Let the user call into archive_unstable_header. This maybe needed because the implementation below calls a runtime API at the parent hash of the desired block. If the user knows the parent block (ie because they might have subscribed to chainHead_follow API), then this call is entirely optional.

  • Step 2: Let the user call archive_unstable_call with the newly added ReviveApi_trace_tx runtime method

Overall the code looks good 🙏 I feel like debug_block is a tiny wrapper that could live in a separate repo, or the users can rely on archive_unstable_call entirely. Would love to get your thoughts on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @lexnv thanks for the review, sorry for the slow response, I was trying to test this end to end and it took more time than expected.

I wasn't aware of the new "unstable" APIs.
So all I need is a mean to call my runtime api from "parent_hash" and pass it the block so I can replay the block with the executive pallet inside an environmental context that will set the tracer first so that we can capture traces.

If we can port the "debug_block" fn to the new unstable APIs then it looks like this should be a simple port.

I feel like debug_block is a tiny wrapper that could live in a separate repo, or the users can rely on archive_unstable_call entirely. Would love to get your thoughts on this

Do you mean polkadot-sdk crate / rpc folder ? This will be a core feature of Plaza and needs to live in the mono-repo

Copy link
Member

@niklasad1 niklasad1 Jan 17, 2025

Choose a reason for hiding this comment

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

No, the idea is that someone could implement this on the "client-side" like CLI tool which under the hood would do two RPC calls (not adding a new server API called debug_block):

  1. archive_unstable_header
  2. archive_unstable_call

For example you could add it eth revive proxy instead since it already is using subxt... but we need to add the "archive API" in subxt which is missing but that should be trivial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok going with that so we don't have to touch the legacy endpoints, will update the PR
thank you both for the review

fn debug_block(&self, name: String, block: Hash, bytes: Bytes) -> Result<Bytes, Error>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why you should pass the name: String here? If this RPC is only to call ReviveApi_trace_tx, it should hardcode it?

Copy link
Contributor

Choose a reason for hiding this comment

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

also asking better docs as to what each param is in general, RPC docs are important and get propagated into PJS/PAPI.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah maybe this is not the one that is meant to be used by end users?

Copy link
Contributor Author

@pgherveou pgherveou Jan 14, 2025

Choose a reason for hiding this comment

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

+1 for the docs, added more details.

I don't get why you should pass the name: String here?

A few reason, We have 2 runtime methods that make use of this. One that replay an entire block, the other that replay a single transaction. Replaying a single transaction involve that you replay all the transactions that precede
to ensure that it's replayed from the same state that when it was executed.

Also potentially other Runtime API could make use of this, so hard coding it does not make much sense here.


/// Returns the keys with prefix, leave empty to get all the keys.
#[method(name = "state_getKeys", blocking)]
#[deprecated(since = "2.0.0", note = "Please use `getKeysPaged` with proper paging support")]
Expand Down
20 changes: 20 additions & 0 deletions substrate/client/rpc/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ where
call_data: Bytes,
) -> Result<Bytes, Error>;

/// Call the runtime method at the block's parent state.
/// and pass the block to be replayed, followed by the call data.
fn debug_block(
Copy link
Member

@niklasad1 niklasad1 Jan 15, 2025

Choose a reason for hiding this comment

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

I wonder whether these could be pallet specific RPCs/runtime APIs for pallet-revive instead of "state_debug_block" because from my understanding it supposed to be used for "ReviveApi_trace_tx" and "ReviveApi_trace_block"?

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 thought it was pretty generic and potentially useful for other pallets that could setup tracing features in a similar way.

It can't just be a runtime API as it need to load the runtime and the blocks before passing it over to the runtime api that will handle this data

Copy link
Member

Choose a reason for hiding this comment

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

In general I'm quite reluctant to add new stuff to the legacy RPCs but this is fine by me.

You didn't look into implementing this on-top of https://paritytech.github.io/json-rpc-interface-spec/api/archive_unstable_body.html?

&self,
method: String,
block: Block::Hash,
extra_call_data: Bytes,
) -> Result<Bytes, Error>;

/// Returns the keys with prefix, leave empty to get all the keys.
fn storage_keys(
&self,
Expand Down Expand Up @@ -209,6 +218,17 @@ where
self.backend.call(block, method, data).map_err(Into::into)
}

fn debug_block(
&self,
ext: &Extensions,
method: String,
block: Block::Hash,
data: Bytes,
) -> Result<Bytes, Error> {
check_if_safe(ext)?;
self.backend.debug_block(method, block, data).map_err(Into::into)
}

fn storage_keys(
&self,
key_prefix: StorageKey,
Expand Down
33 changes: 33 additions & 0 deletions substrate/client/rpc/src/state/state_full.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,39 @@ where
.map_err(client_err)
}

fn debug_block(
&self,
method: String,
block: Block::Hash,
extra_call_data: Bytes,
) -> std::result::Result<Bytes, Error> {
use sp_runtime::traits::Header;
let header = self
.client
.header(block)
.map_err(|err| Error::Client(Box::new(err)))?
.ok_or_else(|| Error::Client("Block hash not found".into()))?;

let parent_hash = header.parent_hash().clone();
let extrinsics = self
.client
.block_body(block)
.map_err(|err| Error::Client(Box::new(err)))?
.ok_or_else(|| Error::Client("Block hash not found".into()))?;

let call_data = Block::new(header, extrinsics)
.encode()
.into_iter()
.chain(extra_call_data.0.into_iter())
.collect::<Vec<u8>>();

self.client
.executor()
.call(parent_hash, &method, &call_data, CallContext::Offchain)
.map(Into::into)
.map_err(client_err)
}

// TODO: This is horribly broken; either remove it, or make it streaming.
fn storage_keys(
&self,
Expand Down
Loading