-
Notifications
You must be signed in to change notification settings - Fork 973
feat: Implement EIP-8024 for Amsterdam #3223
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
Conversation
| 0xE6 => DUPN => stack_io(0, 1), immediate_size(1); | ||
| 0xE7 => SWAPN => stack_io(0, 0), immediate_size(1); | ||
| 0xE8 => EXCHANGE => stack_io(0, 0), immediate_size(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stack_io values are not accurate here, they depend on the immediate, unless we want to put the worst case values here. I don't know what this is used for.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test cases are the same listed in the EIP.
| Self::Internal(InternalResult::InvalidExtDelegateCallTarget) | ||
| } | ||
| InstructionResult::InvalidImmediateEncoding => { | ||
| Self::Halt(HaltReason::OpcodeNotFound.into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to add a new HaltReason for this because IIUC it would be a breaking change for implementers of From<HaltReason>.
CodSpeed Performance ReportMerging #3223 will not alter performanceComparing Summary
|
|
I can't reproduce the nightly test failures. |
Interesting error, jumpdest analysis found that the last opcode is 0x00, but it is jumped over, so it concluded it does not need to append zero bytes. Will see how to do this in an efficient way in |
|
Imo |
|
Unbelievably bullish change |
|
It will take a few months before this becomes active so will move this PR to draft |
rakita
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, as this is expected in devnet-2 will merge it to main
| 1 | ||
| }; | ||
| // SAFETY: Iterator access range is checked in the while loop | ||
| iterator = unsafe { iterator.add(skip) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this can generate a different jumptable for previous bytecode, as we are now jumping over one more immediate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will need to rework this so jump table is same as previous bytecodes.
CFI'd as of last week.