fix(interpreter): use IntoAddress in pop_address to avoid const-eval panic - #3735
Conversation
…panic
`StackTr::pop_address` converted the popped word with
`Address::from(value.to_be_bytes())`. Since `Address: From<[u8; 20]>`, type
inference forces ruint's const-generic `Uint::<256, 4>::to_be_bytes::<20>()`,
whose `const { Self::assert_bytes(BYTES) }` requires `BYTES == Self::BYTES`
(32 for U256). That unreachable const block is only sometimes dead-code
eliminated; build profiles that actually evaluate it (cargo-fuzz coverage,
release codegen) hit `error[E0080]: BYTES must be equal to Self::BYTES`,
breaking downstream builds (bluealloy#3728).
Use the crate's existing `IntoAddress` converter (`value.into_address()`),
which does `Address::from_word(B256::from(value.to_be_bytes()))`. Its
`to_be_bytes()` infers the correct `[u8; 32]` size, so there is no fragile
const-generic instantiation, and `pop_address` now reuses the canonical
U256->Address conversion instead of inlining its own.
Add a regression test asserting `pop_address` truncates a full 32-byte word to
its low 20 bytes and returns `None` on an empty stack.
Closes bluealloy#3728
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors pop_address to use the IntoAddress utility trait instead of constructing an Address directly from big-endian bytes, and adds a unit test verifying the truncation behavior.
Changes:
- Replace inline
Address::from(value.to_be_bytes())withvalue.into_address()inStackTr::pop_address. - Import
IntoAddressfrominstructions::utility. - Add a test covering both the truncation-to-low-20-bytes behavior and the empty-stack case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/interpreter/src/interpreter_types.rs | Use IntoAddress trait to convert popped U256 to Address. |
| crates/interpreter/src/interpreter/stack.rs | Add test asserting pop_address returns low 20 bytes and None on empty stack. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn pop_address_truncates_to_low_20_bytes() { | ||
| use primitives::Address; | ||
|
|
||
| // A full 32-byte word whose high 12 bytes are set and must be discarded, | ||
| // and whose low 20 bytes are the address. This mirrors how the EVM reads an | ||
| // address from a stack word (e.g. BALANCE/EXTCODESIZE/EXTCODECOPY targets). | ||
| let word = U256::from_be_bytes::<32>([ | ||
| 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x11, 0x22, | ||
| 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00, | ||
| 0x12, 0x34, 0x56, 0x78, | ||
| ]); | ||
| let expected = Address::from([ | ||
| 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, | ||
| 0xff, 0x00, 0x12, 0x34, 0x56, 0x78, | ||
| ]); | ||
|
|
||
| run(|stack| { | ||
| assert!(stack.push(word)); | ||
| assert_eq!(stack.pop_address(), Some(expected)); | ||
| }); | ||
|
|
||
| // Empty stack returns None rather than panicking. | ||
| run(|stack| { | ||
| assert_eq!(stack.pop_address(), None); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
No need for this
| #[test] | |
| fn pop_address_truncates_to_low_20_bytes() { | |
| use primitives::Address; | |
| // A full 32-byte word whose high 12 bytes are set and must be discarded, | |
| // and whose low 20 bytes are the address. This mirrors how the EVM reads an | |
| // address from a stack word (e.g. BALANCE/EXTCODESIZE/EXTCODECOPY targets). | |
| let word = U256::from_be_bytes::<32>([ | |
| 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x11, 0x22, | |
| 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00, | |
| 0x12, 0x34, 0x56, 0x78, | |
| ]); | |
| let expected = Address::from([ | |
| 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, | |
| 0xff, 0x00, 0x12, 0x34, 0x56, 0x78, | |
| ]); | |
| run(|stack| { | |
| assert!(stack.push(word)); | |
| assert_eq!(stack.pop_address(), Some(expected)); | |
| }); | |
| // Empty stack returns None rather than panicking. | |
| run(|stack| { | |
| assert_eq!(stack.pop_address(), None); | |
| }); | |
| } |
rakita
left a comment
There was a problem hiding this comment.
Hey lets remove the test, it is not needed
Remove `pop_address_truncates_to_low_20_bytes` as requested in review on bluealloy#3735 — the truncation behaviour is covered by the shared `IntoAddress` conversion path and the dedicated test is not needed.
@rakita , i've removed the test. One small issue - The CI workflows are still showing awaiting approval, would you mind approving the runs so the workflow can run? |
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
StackTr::pop_addressconverts the popped stack word withAddress::from(value.to_be_bytes()). BecauseAddress: From<[u8; 20]>, typeinference forces ruint's const-generic
Uint::<256, 4>::to_be_bytes::<20>(),whose
const { Self::assert_bytes(BYTES) }requiresBYTES == Self::BYTES(32 for
U256).That const block is unreachable for valid inputs, so the code compiles whenever
the compiler dead-code-eliminates it — but the Rust Reference only guarantees such
a block may or may not be evaluated. Build profiles that do evaluate it
(cargo-fuzz coverage instrumentation, release codegen) hit
error[E0080]: evaluation panicked: BYTES must be equal to Self::BYTES,breaking downstream builds. Fixes #3728.
Fix
Reuse the crate's existing canonical converter,
IntoAddress for U256(
crates/interpreter/src/instructions/utility.rs), viavalue.into_address().It does
Address::from_word(B256::from(value.to_be_bytes())), whoseto_be_bytes()infers the correct[u8; 32]size — identical EVM word→addresstruncation (keep the low 20 bytes) with no fragile const-generic instantiation.
This also removes the inlined conversion in favour of the shared helper. The same
problematic idiom is present on
main, so this is not release-specific.Testing
Added
pop_address_truncates_to_low_20_bytes(truncates a full 32-byte word to itslow 20 bytes; returns
Noneon an empty stack). Verified on rustc 1.96:cargo test -p revm-interpreter --all-features→ 45 passedcargo clippy -p revm-interpreter --all-targets --all-features→ cleancargo fmt --all --check→ cleancargo build -p revm-interpreter --releaseand--no-default-features→ both build(these are the profiles that previously hit the
E0080)Closes #3728