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

EIP-7212: precompile for secp256r1 curve #27540

Closed
wants to merge 15 commits into from

Conversation

ulerdogan
Copy link

The PR is about EIP-7212 which proposes the addition of a new precompiled contract to the EVM that allows signature verifications in the “secp256r1” elliptic curve by given parameters of message hash, r - s components of the signature, and x - y coordinates of the public key.

@fjl fjl changed the title New EIP: Precompiled for secp256r1 Curve Support EIP-7212: precompile for secp256r1 curve Jun 22, 2023
crypto/secp256r1/verifier.go Outdated Show resolved Hide resolved
core/vm/contracts.go Outdated Show resolved Hide resolved
@ulerdogan ulerdogan requested a review from fjl June 22, 2023 22:14
Copy link

@nlok5923 nlok5923 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution can you look into the comments

// Run executes the precompiled contract, returning the output and the used gas
func (c *p256Verify) Run(input []byte) ([]byte, error) {
// Required input length is 160 bytes
const p256VerifyInputLength = 160

Choose a reason for hiding this comment

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

Can we put some constraints here to make sure the input provided should scope to 160 bytes only to avoid unnecessary execution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you mean to halt precompile execution in an exceptional state? The EIP doesn't specify this behavior so it shouldn't be added here.

Copy link

Choose a reason for hiding this comment

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

it seems clean for the precompile to always return 1 or 0.

might be worth specifying that it return 0 if the input is not exactly 160 bytes. any other input length indicates an error in the calling contract.

also, what happens if a nonzero value is included with the call?

cc @ulerdogan

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, we do not care about value transmitted to precompiles via call. None of the other precompiles check for it.

core/vm/contracts.go Show resolved Hide resolved
core/vm/contracts.go Outdated Show resolved Hide resolved
@@ -21,17 +15,7 @@ func Verify(hash []byte, r, s, x, y *big.Int) bool {
return false
}

// Check the malleability issue
if checkMalleability(s) {
Copy link

Choose a reason for hiding this comment

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

👍

@holiman
Copy link
Contributor

holiman commented Nov 28, 2023

As far as I know, this is not slated for any upcoming hardfork. I'll close this, feel free to reopen if it becomes scheduled.

@holiman holiman closed this Nov 28, 2023
@ulerdogan
Copy link
Author

Hey @holiman, thanks for following up! Yes, the EIP is not scheduled for any hard forks, so I think good to keep as a closed PR.

The proposal is currently being discussed to re-created as an RIP and then, most of the rollups are planning to integrate into their chains. So, I will apply any specification changes to this PR to create a reference. For example, I will start by changing the implementation address (got advised from 'lightclient' to apply to another address in the RIP roadmap) when the update PR get merged.

Comment on lines +125 to +130
// PrecompiledContractsP256Verify contains the precompiled Ethereum
// contract specified in EIP-7212. This is exported for testing purposes.
var PrecompiledContractsP256Verify = map[common.Address]PrecompiledContract{
common.BytesToAddress([]byte{19}): &p256Verify{},
}

Copy link
Author

Choose a reason for hiding this comment

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

The implementation address updated to 0x0b due to the EIP specification in my branch with this commit, but have not synced into this branch as it's closed.

// PrecompiledContractsP256Verify contains the precompiled Ethereum
// contract specified in EIP-7212. This is exported for testing purposes.
var PrecompiledContractsP256Verify = map[common.Address]PrecompiledContract{
	common.BytesToAddress([]byte{0x0b}): &p256Verify{},
}

Copy link
Author

Choose a reason for hiding this comment

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

The implementation address updated to 0x100 again with this commit as the EIP moved into the RIPs repo and this range has been attached for the RIP precompiles.

// PrecompiledContractsP256Verify contains the precompiled Ethereum
// contract specified in RIP-7212. This is exported for testing purposes.
var PrecompiledContractsP256Verify = map[common.Address]PrecompiledContract{
	common.BytesToAddress([]byte{0x01, 0x00}): &p256Verify{},
}

yperbasis added a commit to erigontech/erigon that referenced this pull request Jan 17, 2024
Initial support of the upcoming Napoli hard fork on Polygon – see
[PIP-33](https://forum.polygon.technology/t/pip-33-napoli-upgrade). Per
[PIP-31](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-31.md),
it parallels the
[Cancun](https://github.com/ethereum/execution-specs/blob/master/network-upgrades/mainnet-upgrades/cancun.md)
upgrade of Ethereum, but does not include
[EIP-4788](https://eips.ethereum.org/EIPS/eip-4788),
[EIP-4844](https://eips.ethereum.org/EIPS/eip-4844),
[EIP-7516](https://eips.ethereum.org/EIPS/eip-7516). In other words,
Napoli includes [EIP-1153](https://eips.ethereum.org/EIPS/eip-1153),
[EIP-5656](https://eips.ethereum.org/EIPS/eip-5656),
[EIP-6780](https://eips.ethereum.org/EIPS/eip-6780) from Cancun.

This PR implements
[PIP-31](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-31.md),
[PIP-16: Transaction Dependency
Data](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-16.md)
(by merging `ParallelUniverseBlock` into `NapoliBlock`; the bulk of
PIP-16 was implemented in PR #8037), and [PIP-27: Precompiled for
secp256r1 Curve
Support](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-27.md)
([EIP-7212](https://eips.ethereum.org/EIPS/eip-7212); see also
maticnetwork/bor#1069 &
ethereum/go-ethereum#27540).

---------

Co-authored-by: Anshal Shukla <shukla.anshal85@gmail.com>
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.

6 participants