Skip to content

Conversation

georgehao
Copy link
Member

…EIP-7951: Precompile for secp256r1 Curve Support

…EIP-7951: Precompile for secp256r1 Curve Support
Copy link
Collaborator

@greged93 greged93 left a comment

Choose a reason for hiding this comment

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

just a couple of nits, otherwise lgtm!


// Galileo should accept this (no 32-byte limit)
let galileo_result = galileo_run(&input, gas_limit);
assert!(galileo_result.is_ok(), "Galileo should accept base_len > 32");
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we check the result is the expected value?

Copy link
Member Author

Choose a reason for hiding this comment

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

added 8f5ba7d

src/spec.rs Outdated
Comment on lines 45 to 46
Self::FEYNMAN |
Self::Galileo => SpecId::SHANGHAI,
Copy link
Contributor

@Thegaram Thegaram Oct 16, 2025

Choose a reason for hiding this comment

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

For the CLZ opcode we might run into the same "selectively enable EIP" issue as before. @greged93

But we can address this in a separate PR.

Copy link
Collaborator

@greged93 greged93 Oct 16, 2025

Choose a reason for hiding this comment

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

right, I had the same thing in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get the issue here, is there more context for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically once you set SpecId::OSAKA, that will enable all Osaka upgrade changes. But in our case we only want to enable some (e.g. CLZ opcode) and not others. Revm currently does not allow selectively enabling EIPs. We ran into this with 7702.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, get it. I'll open a pr to enable CLZ

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.

3 participants