-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add new tracing API #11100
Add new tracing API #11100
Conversation
61dcd9c
to
cb63de6
Compare
064990a
to
0355557
Compare
From our discussion, we have a few edge cases we need to handle... quite a few. Ideally, we'd handle all of these cases inside the ethereum json-rpc API module in lotus without touching the FVM (except to, maybe, add additional tracing information where necessary). The FVM is otherwise supposed to have no idea that the EVM exists (except for a few hacks that will hopefully go away in the near future). I hope I included everything... but, to be honest, we'll probably find more edge cases and I likely underspecified a few things. Native calls(both from EVM actors and from native actors) In general (exceptions noted in the EVM section), we need to format native calls in a way that makes sense to Ethereum tooling. That means, if we see a call not on with InvokeEVM method number (use the constant, we need to convert the inputs & outputs into solidity "ABI". Luckily, we already have a mechanism for this. Specifically, when an EVM actor is invoked with a method number above 1023 that's not Here, we need to:
Basically, we want to make native calls look like calls to a contract like our FilecoinFallback test. Native actor creationIn Filecoin, actor creation happens by sending messages (calling actors):
Unfortunately, it's going to be difficult to make this look like an EVM contract creation but we can hack it... Personally, I'd:
EVM contract creationWith EVM contracts, there's an extra step
In the EVM trace, we need to make these look like EVM call special casingAs noted above, there are some exceptions to the rule where we convert "native" calls into "solidity ABI" calls. Specifically, any outbound call from an EVM actor on methods 1-1023. These calls are side-effects from EVM instructions and should be dropped from the trace. EVM -> EVM callsAs we discussed, we need to handle EVM -> EVM calls:
|
@Stebalien for the EVM -> EVM calls specifically for 2) regarding DelegateCall, when you mention that we need to "search backwards in the trace for a call to another actor (A) on method 3" does that mean we need to look up the call hierarchy or could should this be in a previous subcall at the same level (probably the latter since that is the only thing I see in the traces but just wanted to confirm) ? |
Same level and it should be the adjacent call. Really, I'd memoize the last call on method 3 when handling calls from the EVM. |
fd52be7
to
f48463d
Compare
@fridrik01 Did you consider simply adding this into the existing "EthAPI"? This seems to fundamentally be a restatement of existing Lotus APIs like I might be missing something. |
Yes, I think I started actually with the implementation first in the EthAPI but soon refactored it to keep them separate as its very tracing specific and introduces multiple types which would add complexity to an already huge file (it would blow up to 3K+ LOC with this PR). |
Hmm, so I definitely agree that that file is quite large already. Would you be open to doing the following:
I do think we should avoid adding a new module if we can. |
1d6c3ad
to
0843d8c
Compare
@arajasek I removed the module and refactored the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial pass. I'll continue tomorrow.
Writing down what we discussed in the sync: We need to make everything "look" like an Ethereum call/transaction. This means:
Concretely:
|
After changing in prev commit to use to ethereum addresses the comparison does not make sense against builtin actors. This fixes that by storing also the filecoin addresses in each trace Also renamed filecoin related fields to Filecoin prefix. Also remove requirement call to InvokeContract needed to come from a evm actor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo one comment, this looks mergable (as long as we make it clear that it's still experimental and has some missing features).
@fridrik01 could you file issues for the remaining parts:
|
Updated the CHANGELOG and added this as a highly experimental feature |
@fridrik01 can we merge this one now, since we created follow-up tasks from our TO-DOs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this LGTM
Related Issues
Related: filecoin-project/ref-fvm#1793 and https://github.com/filecoin-project/fvm-pm/issues/613
Relevant PR: filecoin-project/ref-fvm#1823 and filecoin-project/filecoin-ffi#411
Proposed Changes
This PR adds a new tracing API to lotus RPC which should unblock Blockscout/ngram which rely on this feature.
The following two RPC methods will be exported alongside other eth rpc methods:
I used the OpenEthereum/Nethermind/Erigon client version since I liked that API more than what Geth implements and implemented the two required trace methods as listed by Blockscout.
Additional Info
Note that I thought about adding a new lotus config that enables/disables the tracing API, but for now just made it be set any time
EnableEthRPC
is set.Test plan
Testing trace_block
curl -s http://localhost:1234/rpc/v1 -X POST -H "Content-Type: application/json" --data '{"method":"trace_block", "params":["3166855"],"id":1,"jsonrpc":"2.0"}'|jq
Testing trace_replayBlockTransactions
curl -s http://localhost:1234/rpc/v1 -X POST -H "Content-Type: application/json" --data '{"method":"trace_replayBlockTransactions", "params":["3166855", ["trace"]],"id":1,"jsonrpc":"2.0"}'|jq
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps