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

Rename/alias sha3 to minimize confusion with SHA-3 standard #59

Open
ethers opened this Issue Jan 20, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@ethers
Copy link
Member

ethers commented Jan 20, 2016

We should rename or have an alias for Ethereum sha3 to something else, to minimize confusion with the SHA-3 standard. Even though Yellow Paper and some docs mention Keccak-256, having sha3 in Solidity code is highly misleading. Some suggestions are ksha3, keccak. Any others?

Aliasing say ksha3 to sha3 (preserves backward compatibility) and encouraging the use of the new term, and then deprecating sha3 in a future version of Solidity, seems like a practical option to move forward.

@ethers ethers changed the title Rename Ethereum sha3 to minimize confusion with SHA-3 standard Rename/alias sha3 to minimize confusion with SHA-3 standard Jan 20, 2016

@axic

This comment has been minimized.

Copy link
Member

axic commented Jan 20, 2016

I think there are multiple separate instances of Keccak/SHA3 usage and a single approach to all might not suffice. It is used at least:

  • as an opcode in EVM
  • an instruction in Solidity/etc
  • as part of the address, tx, block, etc. hashing
  • in the ABI
  • in ethash
  • in the Merkle Patricia trie for hashing the keys

It probably is unrealistic changing Keccak to SHA3 in all the above. It is fairly simple introducing a new opcode in EVM and it is really simple having an alias in Solidity. For that I would just suggest keccak and not ksha3.

However, only aliasing and renaming in Solidity might be even more misleading as one could expect the same hashing is used to create an address or ABI encoding.

@ethers

This comment has been minimized.

Copy link
Member Author

ethers commented Jan 21, 2016

It probably is unrealistic changing Keccak to SHA3 in all the above

Agree and that's a helpful list you provided. Before we consider outputting actual SHA-3 hashes, we need to start making the current naming more accurate, especially in the most visible code such as Solidity's sha3 function.

For Solidity, a new opcode isn't even needed: alias a new Solidity function to existing opcode, encourage use of new name by providing warnings in Solidity when sha3 is used, then possibly eventually compiler errors when sha3 is used in new Solidity code. Below the surface, the same existing opcode is used by Solidity.

@axic

This comment has been minimized.

Copy link
Member

axic commented Jan 21, 2016

@ethers I guess an important question is: should SHA3 be available or only Keccak. If only the latter, a simple renaming in the EVM docs without changing the opcode number and aliasing and/or renaming in solc would be sufficient.

To clarify my first comment, by the new opcode I've meant a new opcode for SHA3 and renaming the current to Keccak; eventually supporting both in Solidity.

@ethers

This comment has been minimized.

Copy link
Member Author

ethers commented Jan 21, 2016

@axic We agree on "a simple renaming in the EVM docs without changing the opcode number and aliasing and/or renaming in solc" so I filed it for Solidity. The question you ask is indeed important, but I think sooner action is better esp on Solidity front since contracts are quite visible.

ethers added a commit to ethers/yellowpaper that referenced this issue Jan 21, 2016

@Souptacular

This comment has been minimized.

Copy link
Member

Souptacular commented Jan 24, 2016

Just to demonstrate the difference, I posted a detailed comparison on this link and on this Ethereum StackExchange answer.

@emansipater

This comment has been minimized.

Copy link

emansipater commented Sep 10, 2016

Just adding my voice to reinforce how important this is. I have had to patiently explain this issue to just about every competent programmer I've ever introduced to Ethereum, and the problem is just growing over time. Actually swapping things out would be a significant challenge, but simply renaming is entirely doable and the sooner we do it the better. The longer we wait the more code, libraries, and implementations will have to be updated because they have continued to re-use the same naming. Let's create aliasing options to Keccak256 as soon soon as possible so that at least those cognisant of the issue can start tackling the issue in our own code and stop furthering the problem. And to be honest, if there's anyone using these functions who doesn't know on sight what Keccak256 is doing here when they see it, they need to learn that ASAP anyways, before they make a critical mistake because of it.

@axic

This comment has been minimized.

Copy link
Member

axic commented Oct 11, 2016

Update: Solidity now has an alias named keccak256(). (The opcode is still called SHA3.)

@ethers

This comment has been minimized.

Copy link
Member Author

ethers commented May 9, 2017

Now that Solidity has added keccak256, is it time to start considering renaming the opcode too? The opcode surfaces in things like LLL, and Viper annotations.

@fulldecent

This comment has been minimized.

Copy link
Contributor

fulldecent commented Mar 22, 2018

@Arachnid You nominated this as an "EIPs that should be merged". Can you please share your notes on that here? Is there an actual EIP to discuss?

@Arachnid Arachnid added needs-merge and removed needs-merge labels Mar 22, 2018

@Arachnid

This comment has been minimized.

Copy link
Collaborator

Arachnid commented Mar 23, 2018

This was listed as an example EIP in EIP-1 and the readme. I've removed it from there; if it's to be readded, this EIP needs to be written as such and merged into the repo.

dram added a commit to dram/sml-sha3 that referenced this issue Sep 9, 2018

Add `keccak_NNN` family functions
KECCAK-256 is needed by Ethereum, see following discussion for detail:

ethereum/EIPs#59

@axic axic referenced a pull request that will close this issue Mar 1, 2019

Open

Rename opcodes for clarity #1803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.