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

core,eth/tracers: don't panic in OnTxEnd if receipt is nil #30109

Closed
wants to merge 1 commit into from

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Jul 2, 2024

There are two issues currently with the tracer's OnTxEnd invocation in state_processor

  1. Because it is deferred and uses named return values, the receipt is not yet set in the first return after ApplyTransactionWithEVM. OnTxEnd expects the receipt to be non-nil, so this causes a panic.
  2. The error from result is never set into the named return value err. So the error reporting for OnTxEnd isn't really correct.

This PR fixes both of these issues.

cc @s1na

@lightclient lightclient marked this pull request as ready for review July 2, 2024 15:41
@s1na
Copy link
Contributor

s1na commented Jul 2, 2024

  1. Hm I thought it is expected that in case of error the receipt will be null. It can be that the tracers are not handling this well?
  2. Yep so here the OnTxEnd captures tx-level errors like insufficient intrinsic gas etc. The "vm errors" will be caught by lower level hooks like OnExit or OnFault.

@lightclient
Copy link
Member Author

Okay I see, I updated the PR to just fix 1). Thanks

@lightclient lightclient changed the title core,eth/tracers: correctly report trace if tx fails to apply core,eth/tracers: don't panic in OnTxEnd if receipt is nil Jul 2, 2024
@@ -268,7 +268,9 @@ func (l *StructLogger) OnTxEnd(receipt *types.Receipt, err error) {
}
return
}
l.usedGas = receipt.GasUsed
if receipt != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised you've seen a panic here. As far as I can see in state processor either receipt is nil and error is non-nil (in which case struct logger returns) or there is a receipt.

Copy link
Member Author

@lightclient lightclient Jul 3, 2024

Choose a reason for hiding this comment

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

Ah you're right. The issue issue is actually that err is redeclared. Sorry it actually wasn't a problem in the state_processor, it's in the test runner.

@lightclient lightclient closed this Jul 3, 2024
@lightclient
Copy link
Member Author

Closing this for now as my change doesn't address the issue and the current code in state_processor works. The issue is there are some call sites for OnTxEnd which do not pass a receipt, e.g.

evm.Config.Tracer.OnTxEnd(nil, err)

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.

3 participants