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

Fix Stylus Tracer in v1.14.0 Merge #2725

Merged

Conversation

gligneul
Copy link
Contributor

@gligneul gligneul commented Oct 7, 2024

fixes NIT-2881

@gligneul gligneul requested a review from amsanghi October 7, 2024 18:12
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 7, 2024
@gligneul gligneul marked this pull request as ready for review October 7, 2024 18:26
@@ -385,6 +390,10 @@ func evmOpcodesGasUsage(ctx context.Context, rpcClient rpc.ClientInterface, tx *
op := vm.StringToOp(result.StructLogs[i].Op)
gasUsed := uint64(0)
if op == vm.CALL || op == vm.STATICCALL || op == vm.DELEGATECALL || op == vm.CREATE || op == vm.CREATE2 {
if result.StructLogs[i].GasCost == 0 {
// ignore mock call emitted by arbos
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to inject these logs into normal EVM execution, unless a precompile call was part of execution or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. These happen at the end of the execution, regardless of whether the contract was an EVM contract or a stylus one. They were not being injected before geth 1.14. I wonder why it changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the extra call logged comes from arbos/util/transfer.go TransferBalance: info.MockCall.
It seems like a problem in the condition (if/else) but I want to learn more about the code there.

@@ -36,7 +36,7 @@ func TransferBalance(
return errors.New("tracing scenario mismatch")
}

if scenario != TracingDuringEVM {
if scenario != TracingDuringEVM && tracer.CaptureArbitrumTransfer != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that && makes the "else" branch be taken when it previously wasn't

Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though one question on if a change is still necessary

@@ -127,6 +119,9 @@ func (t *stylusTracer) OnEnter(depth int, typ byte, from common.Address, to comm
if t.interrupt.Load() {
return
}
if depth == 0 {
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary now that @tsahee's fix was applied?

Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It'd be good to figure out why the if statement is required but it seems to work and is definitely an improvement.

@PlasmaPower PlasmaPower merged commit 59bd01e into update-gethpin-v1.14.0 Oct 31, 2024
8 of 9 checks passed
@PlasmaPower PlasmaPower deleted the gligneul/fix-update-gethpin-v1.14.0 branch October 31, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants