Use debug traces to extract the input for settlement::Observer #3245
Conversation
| return Some(call_frame); | ||
| } | ||
| // Add all nested calls to the queue | ||
| queue.extend(&call_frame.calls); |
There was a problem hiding this comment.
I think if you were to use a Vec for the stack these callframes should be added to the stack in reverse order.
Otherwise you'd first decent down the last child call - not the first one, right?
There was a problem hiding this comment.
Yeah. So bottom line, using queue with pop-ing from front is breadth-first, while using vec with extending in reverse order would be depth-first search, if i'm not wrong. With that, I am not sure which one has a higher chance of finding the calldata faster, so both of them seem fine to me, so I went with breadth-first.
Maybe breadth-first is faster since settle calldata will probably be in one of the first layers of subcalls.
There was a problem hiding this comment.
Hmm, interesting. Didn't even occur to me to use BFS to analyze a call stack since DFS is the natural flow of the program. But I agree that BFS would probably find the important stack frame faster. 👍
Adding a small comment about the BFS approach would be nice, though.
Also while thinking about the differences between DFS and BFS I realized that an SC solver could execute 2 settlements in a single call. I believe this would probably trip up our indexing logic. Would be good to check what needs to be changed to accommodate that edge case.
There was a problem hiding this comment.
Multiple settlements per transaction are not supported by our system (this is not the only place where it would be an issue). If you are worried about it, I would suggest defining a circuit breaker rule that would forbid it.
| }) | ||
| } | ||
|
|
||
| /// Taken from alloy::rpc::types::trace::geth::CallFrame | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Deserialize)] | ||
| pub struct CallFrame { |
There was a problem hiding this comment.
This exact type exists twice. Once in the infra module and once in the domain module. I suspect we only need one of them.
There was a problem hiding this comment.
I don't want to have deserialization specifics in the domain.
There was a problem hiding this comment.
Hmm, I see. I wonder if this and the debug_trace logic might not be better placed in the ethrpc crate.
We might as well make this easily reusable with an extension trait. It's a bit awkward that the web3 crate does not support the debug module out of the box but AFAICS we could add a new namespace for debug which can have the trace_transaction functionality.
There was a problem hiding this comment.
Note that what I have right now in the code is not actually reusable in general. It's actually the pure minimum of code needed to satisfy the autopilot's needs. The reasons:
1.CallFrame only contains subset of data that is needed for functionality used in autopilot.
2. I use only one type of calltracer here.
In general, to fully implement debug_traceTransaction function so it could be reused with all of it's different input's and outputs would require a lot more code which would be a copy paste from alloy crate.
There was a problem hiding this comment.
Moved the current impl code into ethrpc
|
Test transactions succeeded on all networks. |
Description
A second attempt to resolve #3177
(first attempt was here: #3233)
Now, we try to use
debug_traceTransactioninstead oftrace_transaction, to find call traces to extract the right one -> settle call to settlement contract.For a difference from first attempt, review just the commits after "revert".
Changes
debug_traceTransactionto find /settle call instead of usingtransaction.inputdirectly.How to test
e2e tests show that the code is correct and that deserialization works properly.
manually tested the
debug_traceTransactionon different networks and rpc nodes. (reth ok, alchemy ok, arbitrum ovh nodes ok, just to verify arbitrum nitro)