fix: ignore unknown opcodes in source maps #764
Conversation
🦋 Changeset detectedLatest commit: 00d9527 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fvictorio
had a problem deploying
to
github-action-benchmark
GitHub Actions
Error
fvictorio
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
| @@ -159,7 +159,10 @@ pub fn decode_instructions( | |||
| let source_map = &source_maps[instructions.len()]; | |||
|
|
|||
| let pc = bytes_index; | |||
| let opcode = OpCode::new(bytecode[pc]).expect("Invalid opcode"); | |||
| let opcode = match OpCode::new(bytecode[pc]) { | |||
There was a problem hiding this comment.
I don't think it's advisable to fail silently. It opens a whole can of worms when this should actually be panicking.
Both approaches have downsides:
- Failing silently requires a lot of effort on the user side, which is frustrating
- Panicking on an invalid opcode is too harsh/restrictive
I'd propose:
Minimal change:
- Log that we detected an invalid opcode
Ideally, also:
2. Add a boolean in the configuration that sets whether to ignore_invalid_opcodes. If that's enabled, we don't panic, but log an error.
3. Add documentation to Hardhat describing this option
There was a problem hiding this comment.
Thanks @Wodann, I agree that silent failing could be dangerous/frustrating.
Do we have any ability to do debug level logging (to be used when ignore_invalid_opcodes is enabled)?
Regardless, can we at least add the invalid op code to part of the panic message?
There was a problem hiding this comment.
@Wodann I think you might have the wrong impression of what's going on here. I should've explained better the context of the change in the description, sorry about that.
this should actually be panicking
I'm pretty sure it shouldn't. This code is part of the error inference heuristics, which "fail silently" all the time. From a user perspective, this means that you might get a plain "transaction reverted" instead of, say, "tried to send funds to non-payable function", or a "<unknown function>" label somewhere in a stack trace. Again, that's much better that a panic.
Failing silently requires a lot of effort on the user side, which is frustrating
I'm not sure what you have in mind here. Maybe you think that an execution will have the wrong behavior, like an opcode being skipped instead of reverting? But, again, that's not the case here. Users would have less useful information about a revert, but the execution behavior (the fact that a tx reverted, the receipt, etc.) will be exactly the same.
I do agree that it's desirable to know that this is happening. I'm not sure that logging a warning is necessarily the best option though; sometimes (like here!) there's not much we can do, and that only leads to non-actionable user reports. I know because it's what happened in Hardhat for a long time, when those kinds of warnings reached diminishing returns after we fixed the actionable stuff.
In any case, I think this a telemetry question, and I wouldn't block this PR on that.
There was a problem hiding this comment.
Discussed this offline. We'll add a log::debug for future reference.
Closes #763.
As far as I can tell, this can only happens when solc produces source maps that have an error, so I think just ignoring this is fine. I'm not adding a test for this because it seems to only happen in very special circumstances, with older versions of solc.