-
Notifications
You must be signed in to change notification settings - Fork 80
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
Include the program counter in contract-reverted messages #1593
Conversation
@@ -181,7 +181,7 @@ where | |||
} | |||
Outcome::Revert => Err(ActorError::unchecked_with_data( | |||
EVM_CONTRACT_REVERTED, | |||
"contract reverted".to_string(), | |||
format!("contract reverted at {0}", output.pc), |
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.
We could consider adding additional information here. E.g., self destructed v. reverted? Or maybe the ID/address of the actor?
Note: this is a consensus critical change solely because it touches the actors. The message altered here is for debugging only and isn't available on-chain (it only shows up in the "backtrace"). |
This makes it possible to tell on which instruction the contract reverted where as previously the message wasn't all that useful. The runtime cost should be negligible.
a896eae
to
bbe431a
Compare
(but I'll write a mini-FIP if desired) |
This is really only consensus-critical in a similar way that rebuilding the actors bundle ends up being consensus critical though, right? It's "breaking" if a downstream consumer is consuming this message and decoding it. But is the |
Exactly. The FVM does expose the error message that we're altering here, but via the backtrace (for debugging) not via anything on-chain. |
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.
As long as return_data
remains unmolested this doesn't seem to impact anything that I can find. I don't have opinions about what else to include, this logging is too low-level for me to have experienced yet.
Yeah, given that this is for debugging only, I'm going to say merge it for now. |
This makes it possible to tell on which instruction the contract reverted where as previously the message wasn't all that useful. The runtime cost should be negligible.