[WIP] Add block trace RPC methods (#1055) #1088
Conversation
| import com.fasterxml.jackson.annotation.JsonPropertyOrder; | ||
|
|
||
| @JsonPropertyOrder({"gas", "structLogs"}) | ||
| public class DebugTraceBlockResult { |
There was a problem hiding this comment.
It's missing value property (it's listed in issue), any hints how block's value can be computed ?
There was a problem hiding this comment.
The issue description is missing an important element here unforunately - the result should be an array of transaction trace results. So:
[
{
gas: 999,
returnValue: "",
structLogs: [{
depth: 1,
error: "",
gas: 99999,
gasCost: 2,
memory: null,
op: "PUSH1",
pc: 0,
stack: [],
storage: {}
},
... (more transactions)
]
The returnValue is then the return value of the transaction the same way DebugTraceTransactionResult extracts it. In fact, I suspect DebugTraceBlockResult isn't required and the success response is just return new JsonRpcSuccessResponse(request.getId(), listOfDebugTraceTransactionResult)
There was a problem hiding this comment.
Thanks, i'll change it.
| final String input = parameters.required(request.getParams(), 0, String.class); | ||
| final Block block = | ||
| Block.readFrom( | ||
| RLP.input(BytesValue.fromHexString(input)), MainnetBlockHashFunction::createHash); |
There was a problem hiding this comment.
We probably want to avoid hard coding MainnetBlockHashFunction here and instead use a ScheduleBasedBlockHashFunction so that it automatically selects the right hash function based on the protocol schedule. My suggestion would be to add a BlockHashFunction parameter to the constructor and create the ScheduleBasedBlockHashFunction back up in JsonRpcMethodsFactory so DebugTraceBlock avoids having a direct dependency on ProtocolSchedule.
There was a problem hiding this comment.
Absolutely, I wasn't aware of ScheduleBasedBlockHashFunction and had to pass something.
| RLP.input(BytesValue.fromHexString(input)), MainnetBlockHashFunction::createHash); | ||
| DebugTraceBlockResult result = | ||
| blockTracer.trace(block).map(DebugTraceBlockResult::new).orElse(null); | ||
| return new JsonRpcSuccessResponse(request.getId(), result); |
There was a problem hiding this comment.
We probably should return an error response with a message if the block parent isn't on the block chain rather than just success with a null result.
| .collect(Collectors.toList()); | ||
| return Optional.of(new BlockTrace(transactionTraces)); | ||
| }) | ||
| .orElse(null); |
There was a problem hiding this comment.
I suspect we could avoid duplication through this class by setting up the versions of methods that accept a Hash to just lookup the block by hash and if it's present delegate to the version that accepts a Block rather than duplicating code.
|
|
||
| private final BlockReplay.Action<TransactionTrace> replayAction = | ||
| (transaction, header, blockchain, mutableWorldState, transactionProcessor) -> { | ||
| DebugOperationTracer tracer = new DebugOperationTracer(TraceOptions.DEFAULT); |
There was a problem hiding this comment.
Would be good to have the TraceOptions come from an optional extra param to the JSON-RPC calls rather than hard coding to default.
|
@ajsutton thanks, I really apprecieate your comments and I'll follow Your suggestions, new commits comming soon. |
kziemianek
force-pushed
the
debug_block_rpc
branch
from
ea1c5c8
to
bef9040
Compare
| public JsonRpcResponse response(final JsonRpcRequest request) { | ||
| final String input = parameters.required(request.getParams(), 0, String.class); | ||
| final Block block = | ||
| Block.readFrom(RLP.input(BytesValue.fromHexString(input)), this.blockHashFunction); |
There was a problem hiding this comment.
This may throw RLPException if the input isn't valid RLP which we should probably catch and return a meaningful error. Currently it would result in an internal server error response.
|
|
||
| public interface DebugTraceJsonRpcMethod { | ||
|
|
||
| default TraceOptions getTraceOptions( |
There was a problem hiding this comment.
This would probably make more sense as a static method rather than a default method. It's probably clearer to pass in the transactionTraceparamsParameterIndex as a param to the method than using the second default method.
| .orElse(Optional.empty()); | ||
| } | ||
|
|
||
| private <T> Optional<T> performActionPerBlockTransaction( |
There was a problem hiding this comment.
This method name is a bit misleading. It sounds like it's looping through calling action for each transaction in the block but actually appears to just call the action once. So maybe performActionWithBlock would be a better name?
| transaction, blockHeader, blockchain, worldState, transactionProcessor); | ||
| }); | ||
| @FunctionalInterface | ||
| private interface BlockTransactionAction<T> { |
There was a problem hiding this comment.
Maybe this should be BlockAction?
| } | ||
|
|
||
| @FunctionalInterface | ||
| public interface Action<T> { |
There was a problem hiding this comment.
And maybe this is now TransactionAction?
| } | ||
|
|
||
| public Optional<BlockTrace> trace(final Block block, final DebugOperationTracer tracer) { | ||
| return Optional.of(blockReplay.block(block, this.prepareReplayAction(tracer))); |
There was a problem hiding this comment.
nit: the this. prefix isn't required here.
| } | ||
|
|
||
| @Test | ||
| public void shouldReturnErrorResponseWhenParenBlockMissing() { |
There was a problem hiding this comment.
nit: missing t at the end of Paren.
|
Sorry @ajsutton , I'm very busy lately. Thanks for commit and merge. |
|
@kziemianek Thank you for doing all the hard work. It's great to have this APIs available. |
Implements debug_traceBlock, debug_traceBlockByHash and debug_traceBlockByNumber methods.
Implements debug_traceBlock, debug_traceBlockByHash and debug_traceBlockByNumber methods.
PR description
Adds rpc methods to debug block by providing block number, block hash or block itself encoded by RLP.
Fixed Issue(s)
fixes: #1055