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.

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

cheats: fix assume not precompile #594

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Aug 6, 2024

After the cancun hardfork, the point evaluation precompile was added at address(10) which the assumeNotPrecompile implementation did not take into account. Pectra is scheduled to include BLS precompiles and likely more precompiles will be introduced in other future hardforks. We might as well reserve the least significant byte for Ethereum precompiles, as the address space is large and there is no need to make calls from these addresses, and in return code will not break on new EVM specs.

This was causing test failures in the Optimism test suite when we updated the EVM version to cancun.

After the cancun hardfork, the point evaluation precompile was added
at `address(10)` which the `assumeNotPrecompile` implementation
did not take into account. Pectra is scheduled to include BLS
precompiles and likely more precompiles will be introduced in
other future hardforks. We might as well reserve the least significant
byte for Ethereum precompiles, as the address space is large and there
is no need to make calls from these addresses, and in return code
will not break on new EVM specs.

This was causing test failures in the Optimism test suite when
we updated the EVM version to cancun.
// These should be present on all EVM-compatible chains.
vm.assume(addr < address(0x1) || addr > address(0x9));
// These are reserved by Ethereum and may be on all EVM-compatible chains.
vm.assume(addr < address(0x1) || addr > address(0x100));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the larger upper bound so this is more future-proof. Do you have a source for the 0x100 value? I've only seen https://eips.ethereum.org/EIPS/eip-1352 which reserves through 0xffff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The genesis for testnets includes 1 wei for the precompiles in the range address(0) to address(0xff). See https://github.com/eth-clients/holesky/blob/main/metadata/genesis.json

I meant to follow this standard, will update the PR to use address(0xff) rather than address(0x100)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in dab1435

Use the address range of precomiples that testnets use,
see https://github.com/eth-clients/holesky/blob/main/metadata/genesis.json
Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mds1 mds1 merged commit bf66061 into foundry-rs:master Aug 6, 2024
3 checks passed
@tynes tynes deleted the fix/assume-not-precompile branch August 6, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants