Implement trace replay block transactions trace option #1886
Conversation
- introduce `MonitorableKeyValueStorage` - factorise code - remove metrics instanciation in `RocksDbKeyValueStorage` and `ColumnarRocksDbKeyValueStorage`
This reverts commit e2bb261.
This reverts commit e5dc3fa.
This reverts commit 15f90ea.
…ss>" This reverts commit 08cc7ca.
This reverts commit 5454265.
This reverts commit 756f52b.
- implement a strategy with 2 options - if first frame gas remaining is higher than gas remaining after transaction was processed then the gas used will be the net difference - otherwise a frame by frame cumulative gas cost will be used
…trace-replayBlockTransactions-trace-option
…trace-replayBlockTransactions-trace-option
This reverts commit 88ee6aa.
|
|
||
| public interface Trace { | ||
| @FunctionalInterface | ||
| interface ResultWriter { |
There was a problem hiding this comment.
Style: why an interface in an interface? I would expect just one interface TraceResultWriter
There was a problem hiding this comment.
Trace interface is the common type for FlatTrace and the other types of traces we will add for vmTrace and stateDiff options. So i'll keep Trace interface but as you suggest i'll move ResultWriterto a separate file.
There was a problem hiding this comment.
Does that make sense ?
There was a problem hiding this comment.
If the future is to cover vm traces and state diffs then I think it is worth some Javadoc indicating what this interface represents and the intended implementations. Right now it looks like a marker interface (like Serializable) and I don't think that is clear without documentation.
There was a problem hiding this comment.
Totally agree. i'll do that
| ? traceFrame.getMemory().get()[0].toString() | ||
| : "0x")); | ||
| ctx.markAsReturned(); | ||
| continueToPollContexts = false; |
There was a problem hiding this comment.
I wouldn't use a sentinel boolean I would just use a break statement
| // find last non returned transactionTrace | ||
| while (continueToPollContexts && (ctx = tracesContexts.pollLast()) != null) { | ||
| polledContexts.addFirst(ctx); | ||
| if (!ctx.isReturned()) { |
There was a problem hiding this comment.
I think a guard block instead of a nested if would read better since the if covers the rest of the while block.
| if (!ctx.isReturned()) { | |
| if (ctx.isReturned()) { | |
| continue; | |
| } |
Then remove the matching brace and un indent the block.
There was a problem hiding this comment.
Nice suggestion. Done.
|
|
||
| final long gasCost = cumulativeGasCost.longValue(); | ||
| // retrieve the previous transactionTrace context | ||
| Optional.ofNullable(tracesContexts.peekLast()) |
There was a problem hiding this comment.
This use of optional feels a bit clunky and creates an extra optional object. Would this work better?
if (!traceContexts.isEmpty()) {
FlatTrace.Context previousContext = traceContexts.peekLast();
// increment sub traces counter of previous transactionTrace
previousContext.getBuilder().incSubTraces();
// set gas cost of previous transactionTrace
previousContext
.getBuilder()
.getResultBuilder()
.orElse(Result.builder())
.gasUsed(Gas.of(gasCost).toHexString());
}…trace-replayBlockTransactions-trace-option
You right, i have removed setters and use only builders. |
PR description
TraceFrameand transform to parity styleFlatTraces from aggregatedTraceFrames included inTransactionTraceobjectCALLopcode to retrieve contract address required in parity formatRETURNopcode to close a subtraceFixed Issue(s)