-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
Use blake2b everywhere #9262
Use blake2b everywhere #9262
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@pchrysochoidis could you update the wallet in this PR also (re #9248) to use Blake2b? |
Just to repeat here this comment. Wallet does't do any hashing (as far as I know). It uses ts sdk that does the work for us. here, here and here are a few places I found but might be more. Ideally someone from devX should have a better idea. |
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.
@joyqvq are we going to add the TS changes we explored (+ what @pchrysochoidis points out) in this PR?
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.
@pchrysochoidis all mentioned impact in the TS files you mentioned has been addressed. Please review
74d0f40
to
c895b42
Compare
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.
Let's rebase as well
|
||
/// Default hash function for non-user-facing purposes, namely the ones not done by the wallet. | ||
pub type InternalHash = Blake2b256; | ||
pub type DefaultHash = Blake2b256; |
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.
Add comment:
/// Blake2b_256 (output size = 32 bytes) is the default hash function for all Sui related operations including consensus and user-facing digests, such as publicKey to address and transactionID derivation.
c895b42
to
de9c2c2
Compare
de9c2c2
to
c8f6e28
Compare
c8f6e28
to
743234a
Compare
a88dc0a
to
7de92cb
Compare
7de92cb
to
a4ffc34
Compare
a4ffc34
to
312c2be
Compare
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.
amazing work ethics here, thanks!
@@ -83,13 +70,6 @@ describe('Ed25519PublicKey', () => { | |||
expect(new Ed25519PublicKey(key.toBytes()).equals(key)).toBe(true); | |||
}); | |||
|
|||
it('toSuiAddress', () => { |
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.
Curious why this test was removed?
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.
there are a bunch of tests here already for the same purpose
## Description Follow-up for #9262, removes a package that's no longer used. ## Test Plan Tests should pass.
Description
Closing #9225
Previously, sui address is computed as the first 32 bytes of the SHA3-256 hash of
flag || pubkey
, this changes usesfirst 32 bytes of the Blake2bhash of
flag || pubkey
instead. This is for consideration of speed optimization.In Typescript, see
sui/sdk/typescript/src/cryptography/ed25519-publickey.ts
Line 71 in 312c2be
sui/sdk/typescript/src/cryptography/secp256k1-publickey.ts
Line 71 in 312c2be
In Rust see:
sui/crates/sui-types/src/base_types.rs
Line 466 in 312c2be
Test Plan
All tests passes.
Release notes
We now use the BLAKE2b-256 cryptographic hash function everywhere instead of Sha3-256. It is considered to be just as secure as Sha3 but is about three times faster. See also https://www.blake2.net.