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

core/vm: Rename SHA3 instruction to KECCAK256 #23976

Merged
merged 1 commit into from Nov 30, 2021
Merged

Conversation

axic
Copy link
Member

@axic axic commented Nov 25, 2021

Also rename helper functions.

This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.

The only concern could be if there is some kind of intput/output geth produces where this makes a difference.

There is also a field which is serialised as sha3Uncles -- not sure what relies on it, but of course haven't touched that here.

Also rename helper functions.
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Screenshot from latest YP, the op is technically still named SHA3.

Screenshot 2021-11-25 at 20-02-52 Ethereum Yellow Paper a formal specification of Ethereum, a programmable blockchain - pap

I don't really care much either way, but slightly feel that SHA3 is most correct, since it aligns with the YP

@@ -254,7 +254,7 @@ var opCodeToString = map[OpCode]string{
MULMOD: "MULMOD",

// 0x20 range - crypto.
SHA3: "SHA3",
KECCAK256: "KECCAK256",
Copy link
Contributor

Choose a reason for hiding this comment

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

This will 'break' the API for differential tracing. It's pretty easy to fix in the tracer shims though, so not a blocker for me

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have some links, I can try submitting support pre-emptively. Note that evmone already outputs KECCAK256 so the shims should be updated to accept both in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think e.g. this just need an alias: https://github.com/holiman/goevmlab/blob/0d79a9a3f8e4f25dcac009f1e4b7f0e1d458c238/ops/operations.go#L304 . It's not a biggie, I can fix whenever I need it next

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's my attempt: holiman/goevmlab#46

@axic
Copy link
Member Author

axic commented Nov 26, 2021

Screenshot from latest YP, the op is technically still named SHA3.

This is solved: ethereum/yellowpaper#825 😅

@holiman
Copy link
Contributor

holiman commented Nov 26, 2021

Oh my, we're out of spec!!
Screenshot 2021-11-26 at 11-32-16 Ethereum Yellow Paper a formal specification of Ethereum, a programmable blockchain - pap

:shipit:

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.10.14 milestone Nov 30, 2021
@holiman holiman merged commit a69d4b2 into ethereum:master Nov 30, 2021
@holiman holiman deleted the sha3 branch November 30, 2021 09:34
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.
yperbasis added a commit to ledgerwatch/erigon that referenced this pull request Oct 28, 2022
Cherry-pick ethereum/go-ethereum#23976

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
banteg added a commit to banteg/storage-layout that referenced this pull request Aug 4, 2023
banteg added a commit to banteg/evm-trace that referenced this pull request Aug 4, 2023
the opcode was renamed in geth and erigon, but still called sha3 in reth
ethereum/go-ethereum#23976
ledgerwatch/erigon#5890
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Feb 23, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Feb 23, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Feb 23, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Feb 26, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Feb 27, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Feb 27, 2024
This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Feb 27, 2024
This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Mar 1, 2024
This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Mar 1, 2024
This was proposed in 2016, Solidity uses this since 2017, and evmone and other VMs use the keccak256 name. This brings geth in line with those.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants