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

[crypto] SuiAddress derivation via sha3 #1481

Merged
merged 2 commits into from
Apr 20, 2022
Merged

Conversation

kchalkias
Copy link
Collaborator

@kchalkias kchalkias commented Apr 20, 2022

We currently mix sha2 (pubKey->address) and sha3 (objects/transaction) and this is an attempt to unify our hash-digest functionality into a single behavior and scheme.
This is very important for a) auditing purposes b) consistency c) less dependencies (-sha2)

@kchalkias kchalkias force-pushed the kostas-sha3-everything branch 2 times, most recently from 5fc3869 to f257c9b Compare April 20, 2022 05:28
@kchalkias kchalkias force-pushed the kostas-sha3-everything branch from f257c9b to afb713a Compare April 20, 2022 06:02
@kchalkias kchalkias requested review from lxfind and patrickkuo April 20, 2022 06:54
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Since we need to keep the admin address in sync with the hashing function, why not add a test that checks it?
If we do not want to reveal the preimage of the admin address, instead of testing sha3(foo) == ADMIN_ADDRESS, we can have a useless #[cfg(test)] constant ADMIN_SENTINEL defined on the preceding line, do the test on that variable, and precede it by a comment saying "If you're updating this because a test broke, please update the admin address too".

@gdanezis
Copy link
Collaborator

I think that in the ed25519 we implicitly also use sha512? Worth checking?

@kchalkias
Copy link
Collaborator Author

I think that in the ed25519 we implicitly also use sha512? Worth checking?

Good point, but this is explicit to the internal signature RFC specs (hidden from our repo), not a Sui protocol hash selection decision.

@kchalkias kchalkias force-pushed the kostas-sha3-everything branch from da0535a to c181c4c Compare April 20, 2022 15:40
@kchalkias
Copy link
Collaborator Author

@huitseeker back to you, per your recommendation, a slightly more generic test has been introduced that will help us spot any changes to the SuiAddress derivation algorithm in the future.

@kchalkias kchalkias merged commit 512f2ed into main Apr 20, 2022
@kchalkias kchalkias deleted the kostas-sha3-everything branch April 20, 2022 18:31
Comment on lines +483 to +491
fn test_admin_address_consistency() {
use hex;
let (admin_address, _) = make_admin_account();
assert_eq!(admin_address.to_vec(), hex::decode(ADMIN_SENTINEL).expect("Decoding failed"),
"If this test broke, then the algorithm for deriving addresses from public keys has \
changed. Please compute the new admin address in hex format from `make_admin_account`\
and update both the ADMIN_SENTINEL const above, but also the ADMIN address to \
Hero.move with the new admin account hex value.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants