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.

Sign up for GitHub

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: ignore unknown opcodes in source maps #764

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: ignore unknown opcodes in source maps #764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fec9fca
fix: ignore unknown opcodes in source maps
fvictorio Jan 6, 2025
00d9527
Create chatty-roses-itch.md
fvictorio Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chatty-roses-itch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nomicfoundation/edr": patch
---

fix: ignore unknown opcodes in source maps
5 changes: 4 additions & 1 deletion crates/edr_solidity/src/source_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Failing silently requires a lot of effort on the user side, which is frustrating
  2. Panicking on an invalid opcode is too harsh/restrictive

I'd propose:

Minimal change:

  1. 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

Copy link
Member

@nebasuke nebasuke Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

We have debug logging using the log crate. For that you need to run with RUST_LOG=warning your-command in the CLI. I don't think we currently enable the logger for Hardhat, though.

We also use a logger to print things to Hardhat's logger.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed this offline. We'll add a log::debug for future reference.

Some(opcode) => opcode,
None => continue,
};

let push_data = if opcode.is_push() {
let push_data = &bytecode[bytes_index..][..1 + opcode.info().immediate_size() as usize];
Expand Down
Loading