-
Notifications
You must be signed in to change notification settings - Fork 897
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
Better tracing alignment #6525
Better tracing alignment #6525
Conversation
Update tracing and evm tool * Intrinsic gas is optional in EVMTool * For call series, also charge the gas given to the next call level Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
|
evm/src/main/java/org/hyperledger/besu/evm/tracing/StandardJsonTracer.java
Show resolved
Hide resolved
Wire into the debug_ series calls, but not the trace_ series calls. Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
addressList.add(sender); | ||
addressList.add(contract); |
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.
What does this change mean in this PR?
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.
EIP-2929 says the sender and contract address are pre-warmed. The transaction handler does that, but the evm tool has it's own bootstrap logic and needs to pre-warm them.
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.
LGTM, Would you be able to add some comments on how additionalCallGas
works either in the AbstractTraceCall
or in the DebugOperationTracer
and/or when it makes sense to use it? It could help future development to use this flag correctly.
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.
LGTM. From what I see, this pull request makes two separate improvements. The change in tracing affects only calls frames in debug trace RPC methods, not "Parity" style RPC tracing calls. I'm assuming this difference is more about the design or specification choice, correct?
traceFrame(validCallFrame(), new TraceOptions(false, false, false), true); | ||
assertThat(traceFrame.getGasCost()).isEqualTo(OptionalLong.of(1020L)); | ||
} | ||
|
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.
It could be interesting to add a test with a frame that is not a call frame, set additionalCallGas to true and verify that gasCost in the frame is not updated with additionalCallGas.
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.
test added. validMessageFrame with opposite flags, asserting it's the same.
documentation tests Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Update tracing and evm tool * Intrinsic gas is optional in EVMTool * For call series, also charge the gas given to the next call level for debug_ series calls. Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
PR description
Update tracing and evm tool
Fixed Issue(s)
Fixes #6523