Update EIP-8024: Switch to branchless normalization and extend EXCHANGE#11306
Conversation
github-actions
Bot
added
c-update
|
✅ All reviewers have approved. |
chfast
left a comment
chfast
left a comment
There was a problem hiding this comment.
Big thanks for materializing this idea into the spec update.
| return (x + 145) & 0xff | ||
|
|
||
| def decode_pair(x: int) -> tuple[int, int]: | ||
| assert 0 <= x <= 79 or 128 <= x <= 255 |
There was a problem hiding this comment.
Why the range of valid byte values is different here?
There was a problem hiding this comment.
If we use the same domain as decode_single, we either have a discontinuity in the pairs that can be represented1, or there would be two ways to encode the same pair. So the range is reduced to keep this a bijection. We could try to map the extra available domain to some other continuous section but I figured the complexity wasn't worth it.
Footnotes
-
You'd be able to swap 11,16 and 11,18 but not 11,17. ↩
There was a problem hiding this comment.
I found a way to extend EXCHANGE with two more pairs, making 14,15 and 14,16 swappable.
frangio
force-pushed
the
8024-branchless
branch
from
1db8fbf to
d4f4578
Compare
frangio
changed the title
frangio
force-pushed
the
8024-branchless
branch
from
faa51b0 to
2304fef
Compare
Update decode_single/decode_pair to use branchless arithmetic per ethereum/EIPs#11306: modular add for single values, XOR for pairs. Extends EXCHANGE to support 2 additional swappable pairs (n up to 14).
| 3. If `79 < x < 128`, halt with exceptional failure. | ||
| 3. If `81 < x < 128`, halt with exceptional failure. | ||
| 4. Let `n, m = decode_pair(x)`. | ||
| 5. If `m + 1 > len(stack)`, halt with exceptional failure. |
There was a problem hiding this comment.
Thanks a lot @frangio :)
I have a question about the EXCHANGE depth check. If I understand correctly, the forbidden range implicitly ensures that decode_pair always produces n < m (with max n = 14, max m = 29). This invariant can be inferred from the encode_pair precondition, but not as a postcondition of decode_pair. Since the depth check only validates m, I believe it would be valuable for implementers to know that decode_pair also guarantees n < m, otherwise stack[top - n] could be an out-of-bounds access.
Would it make sense to either change the depth check to be defensive
| 5. If `m + 1 > len(stack)`, halt with exceptional failure. | |
| 5. If `max(n, m) + 1 > len(stack)`, halt with exceptional failure. |
Or, since decode_pair should always produce n < m, maybe add an assert to make the invariant explicit like in encode_pair?
def decode_pair(x: int) -> tuple[int, int]:
# ...
if q < r:
n, m = q + 1, r + 1
else:
n, m = r + 1, 29 - q
assert 1 <= n < m <= 29 and n + m <= 30
return n, mThere was a problem hiding this comment.
Oh sorry, my mistake. I thought the diff showed you removing the assertion on the input x rather than modifying it. So asserting the result seems unnecessary.
There was a problem hiding this comment.
I still think there is an opportunity to make this clearer, though I prefer to keep the instruction spec as it is now. See #11351
By the way I realized with the new EXCHANGE it's much simpler to check whether a pair is swappable. Basically just n + m <= 30.
eth-bot
left a comment
eth-bot
left a comment
There was a problem hiding this comment.
All Reviewers Have Approved; Performing Automatic Merge...
Align DUPN, SWAPN, and EXCHANGE decoding with the updated EIP-8024 spec (ethereum/EIPs#11306). decode_single now uses (x + 145) % 256 instead of branching, and decode_pair uses XOR 0x8F instead of conditional subtraction. EXCHANGE valid range extended from 0x00-0x4F to 0x00-0x51, adding two new swappable pairs (14,15) and (14,16).
Align DUPN, SWAPN, and EXCHANGE decoding with the updated EIP-8024 spec (ethereum/EIPs#11306). decode_single now uses (x + 145) % 256 instead of branching, and decode_pair uses XOR 0x8F instead of conditional subtraction. EXCHANGE valid range extended from 0x00-0x4F to 0x00-0x51, adding two new swappable pairs (14,15) and (14,16).
For bal-devnet-3 we need to update the EIP-8024 implementation to the latest spec changes: ethereum/EIPs#11306 > Note: I deleted tests not specified in the EIP bc maintaining them through EIP changes is too error prone.
* feat(interpreter): update EIP-8024 encoding to branchless normalization Update decode_single/decode_pair to use branchless arithmetic per ethereum/EIPs#11306: modular add for single values, XOR for pairs. Extends EXCHANGE to support 2 additional swappable pairs (n up to 14). * feat(primitives): rename EIP-7708 Selfdestruct event to Burn Rename the EIP-7708 selfdestruct log event from `Selfdestruct(address,uint256)` to `Burn(address,uint256)` per EIP PR #11311. This updates the topic hash and makes the event name more generic for any future ETH burn mechanism. * feat(primitives): add EIP-7954 contract size limits for Amsterdam Add spec-aware max_code_size (0x8000) and max_initcode_size (0x10000) defaults for AMSTERDAM+, falling back to EIP-170/EIP-3860 for earlier specs. * style: apply rustfmt formatting * chore(scripts): temporarily skip devnet tests in run-tests.sh
6 participants
Changes the encodings to slightly simpler ones that normalize the immediate to a contiguous range in a branchless way, and adds two more swappable pairs to
EXCHANGE.Suggested by @chfast in https://ethereum-magicians.org/t/eip-8024-backward-compatible-swapn-dupn-exchange/25486/7.