Skip to content
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

Merged
merged 10 commits into from Feb 16, 2024
Merged

Better tracing alignment #6525

merged 10 commits into from Feb 16, 2024

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Feb 6, 2024

PR description

Update tracing and evm tool

  • Intrinsic gas is optional in EVMTool
  • For call series, also charge the gas given to the next call level

Fixed Issue(s)

Fixes #6523

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>
Copy link

github-actions bot commented Feb 6, 2024

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests
  • I thought about running CI.
  • If I did not run CI, I ran as much locally as possible before pushing.

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>
Comment on lines +391 to +392
addressList.add(sender);
addressList.add(contract);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia left a 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.

Copy link
Contributor

@ahamlat ahamlat left a 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));
}

Copy link
Contributor

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.

Copy link
Contributor Author

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>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon shemnon enabled auto-merge (squash) February 16, 2024 15:51
@shemnon shemnon merged commit 44c8052 into hyperledger:main Feb 16, 2024
78 checks passed
suraneti pushed a commit to suraneti/besu that referenced this pull request Feb 24, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trace transaction - Investigate gasCost difference
3 participants