-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add Precompile for secp256r1 conditionally based on ArbOS version #303
Conversation
…ereum into secp256r1-precompile
…ereum into secp256r1-precompile
core/vm/contracts.go
Outdated
return PrecompiledAddressesArbitrum | ||
var ret []common.Address | ||
if rules.ArbOSVersion >= 30 { | ||
ret = append(ret, PrecompiledAddressesP256Verify...) |
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.
- This is called every tx, so I think we should consider caching this as PrecompiledAddressesArbOS30 or such
- We also need to update the EVM's
precompile
method in core/vm/evm.go
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.
Good catch, updated evm's precompile as well and introduced PrecompiledAddresses[/Contracts]ArbOS30 that is populated in geth hook (OffchainLabs/nitro#2239).
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.
looks good, just might need to remove PrecompiledAddressesP256Verify
if it isn't used
core/vm/contracts.go
Outdated
PrecompiledAddressesIstanbul []common.Address | ||
PrecompiledAddressesByzantium []common.Address | ||
PrecompiledAddressesHomestead []common.Address | ||
PrecompiledAddressesP256Verify []common.Address |
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.
do we need this? looks like we include those addresses in PrecompiledAddressesArbOS30
instead
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.
Removed, I think it might've been needed in an older version of how precompile activation worked
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
Main changes forked from: https://github.com/ulerdogan/go-ethereum/pull/1/files