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

Include the program counter in contract-reverted messages #1593

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

Stebalien
Copy link
Member

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.

@@ -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),
Copy link
Member Author

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?

@Stebalien
Copy link
Member Author

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.
@Stebalien Stebalien force-pushed the steb/pc-counter-contract-reverted branch from a896eae to bbe431a Compare December 9, 2024 16:41
@Stebalien
Copy link
Member Author

(but I'll write a mini-FIP if desired)

@rvagg
Copy link
Member

rvagg commented Dec 9, 2024

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 Output exposed or used outside of the bundle?

@Stebalien
Copy link
Member Author

Exactly. The FVM does expose the error message that we're altering here, but via the backtrace (for debugging) not via anything on-chain.

Copy link
Member

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

@Stebalien Stebalien added this pull request to the merge queue Dec 10, 2024
@Stebalien
Copy link
Member Author

Yeah, given that this is for debugging only, I'm going to say merge it for now.

Merged via the queue into master with commit b4ed8cf Dec 10, 2024
12 checks passed
@Stebalien Stebalien deleted the steb/pc-counter-contract-reverted branch December 10, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

2 participants