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.

Sign up for GitHub

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

Jump to bottom

feat: add FlatCallTracer #11114

Merged
merged 22 commits into from
Sep 24, 2024
Merged

feat: add FlatCallTracer #11114

merged 22 commits into from
Sep 24, 2024

Conversation

0xkrane
Copy link
Contributor

@0xkrane 0xkrane commented Sep 22, 2024 •
edited
Loading

Closes #10991

still wip but creating draft pr to get some guidance on whether this is the right path

some things to note:

  • we first convert the config into a FlatCallConfig
  • create the inspector using the from_flat_call_config (this change has been proposed here: chore: add from_flat_call_config revm-inspectors#203) , this allows us to use the precompile flags
  • then use the inspector to trace the transaction, instead of initializing a default parity tracer since that would ignore the precompile flag

trying to understand if this is the right approach to adding flatcalltracer or if i'm overcomplicating things

mattsse
mattsse requested changes Sep 23, 2024
View reviewed changes
crates/rpc/rpc/src/debug.rs Outdated
Comment on lines 401 to 404

// Now `res` is the unwrapped value, not an Option
let res = None;

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a good start, we need to replicate what we do here:

reth/crates/rpc/rpc/src/trace.rs

Lines 342 to 358 in 242a3ab

/// Returns all traces for the given transaction hash
pub async fn trace_transaction(
&self,
hash: B256,
) -> Result<Option<Vec<LocalizedTransactionTrace>>, Eth::Error> {
self.inner
.eth_api
.spawn_trace_transaction_in_block(
hash,
TracingInspectorConfig::default_parity(),
move |tx_info, inspector, _, _| {
let traces =
inspector.into_parity_builder().into_localized_transaction_traces(tx_info);
Ok(traces)
},
)
.await

and

reth/crates/rpc/rpc/src/trace.rs

Lines 366 to 374 in 242a3ab

let traces = self.inner.eth_api.trace_block_with(
block_id,
TracingInspectorConfig::default_parity(),
|tx_info, inspector, _, _, _| {
let traces =
inspector.into_parity_builder().into_localized_transaction_traces(tx_info);
Ok(traces)
},
);

I think here we can ignore the block reward stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tx_info should just be default here right since it isn't actually tx and is just a call?

(quickly update code to use default, lmk if there is something else i should be fetching)

@mattsse mattsse added the A-rpc Related to the RPC implementation label Sep 23, 2024
@mattsse mattsse marked this pull request as ready for review September 23, 2024 09:01
@mattsse mattsse added the C-enhancement New feature or request label Sep 23, 2024
@0xkrane
Copy link
Contributor Author

0xkrane commented Sep 23, 2024

quick attempt at actually doing the tracing:

  • use inspect with the inspector to actually produce the trace
  • initialize a default tx
  • convert inspector into a parity builder and give it the default tx (lmk if there is a tx hash that is relevant even tho this is a call and not a real tx)
  • pull first element out of vector, return

@0xkrane 0xkrane changed the title feat: add flattracer feat: add FlatCallTracer Sep 23, 2024
mattsse
mattsse approved these changes Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, lgtm, needs fmt

you can tackle the other call in a followup or add it here

@0xkrane
Copy link
Contributor Author

0xkrane commented Sep 23, 2024

added other, need from_flat_call_config here, will wait for bump on that

@mattsse
Copy link
Collaborator

mattsse commented Sep 23, 2024

on revm-inspectors?

this is available in the new release so you can bump the dep here

@mattsse mattsse mentioned this pull request Sep 24, 2024
Merged
mattsse
mattsse requested changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

style nits,

  • remove the feature gated _transaction_context and use into for conversion with revm-inspector 0.7.7

crates/rpc/rpc/src/debug.rs Outdated
Comment on lines 795 to 802
let tx_info = TransactionInfo {
hash: {
#[cfg(not(feature = "js-tracer"))]
{
_transaction_context.unwrap().tx_hash
}
#[cfg(feature = "js-tracer")]
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets get rid of this entirely, we can now also remove the feature gated argument because we use it now

also releasing this which makes this conversion a simple into() paradigmxyz/revm-inspectors#204

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed transaction_conext feature gating in the function. updated conversion to not have feature gate as well. left the feature gating on js-tracer since that seems unrelated

@mattsse
Copy link
Collaborator

mattsse commented Sep 24, 2024

we can merge this with the pops and then simply remove them once we bump alloy, this way we don't need to rebase in case there are changes

@0xkrane 0xkrane mentioned this pull request Sep 24, 2024
Closed
mattsse
mattsse approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, needs smol update once alloy is bumped but this wont compile and we'll know

merging because there is currently more wip on this file to avoid conflcits

@mattsse mattsse added this pull request to the merge queue Sep 24, 2024
@0xkrane
Copy link
Contributor Author

0xkrane commented Sep 24, 2024

yeah can use #11152 to remove the pop or include the alloy bump in the PR if needed

Merged via the queue into paradigmxyz:main with commit 33d88a4 Sep 24, 2024
36 checks passed
@0xkrane 0xkrane deleted the krane/flattracer branch September 24, 2024 10:48
@0xkrane 0xkrane mentioned this pull request Sep 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@emhane emhane Awaiting requested review from emhane emhane is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Assignees
No one assigned
Labels
A-rpc Related to the RPC implementation C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for FlatCallTracer
2 participants
@0xkrane @mattsse