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

Fix scale-codec for Multihash #140

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Fix scale-codec for Multihash #140

merged 1 commit into from
Sep 22, 2021

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Sep 17, 2021

waiting for #139 to be merged firstly

This PR is a breaking change.

BREAKING CHANGE: Write the digest of multihash directly to dest when encoding multihash, since we have known the size of digest.

We do not choose to encode &[u8] directly, because it will add extra bytes
(the compact length of digest) at the beinging of encoding result.
For a valid multihash, the length of digest must equal to size field of multihash.
Therefore, we can only read raw bytes whose length is equal to size when decoding.

And the digest of Multihash<U64> is of type [u8; 64], taking Code::Sha2_256 as an example,
if the digest is treated as &[u8] when encoding, useless data will be added to
the encoding result.

@koushiro
Copy link
Contributor Author

@vmx PTAL

@vmx
Copy link
Member

vmx commented Sep 20, 2021

@koushiro I know very little about the scale-codec, I haven't really used it. Could you perhaps put some information into the commit message why it is a breaking change?

BREAKING CHANGE: Write the digest of multihash directly to dest when encoding
multihash, since we have known the size of digest.

We do not choose to encode &[u8] directly, because it will add extra bytes
(the compact length of digest) at the beinging of encoding result.
For a valid multihash, the length of digest must equal to `size` field of multihash.
Therefore, we can only read raw bytes whose length is equal to `size` when decoding.

And the digest of Multihash<U64> is of type [u8; 64], taking Code::Sha2_256 as an example,
if the digest is treated as &[u8] when encoding, useless data will be added to
the encoding result.

Signed-off-by: koushiro <[email protected]>
@koushiro
Copy link
Contributor Author

koushiro commented Sep 20, 2021

Could you perhaps put some information into the commit message why it is a breaking change?

Updated, and you could compare the difference between the encoding results of multihash in test_scale before and after the change

@koushiro
Copy link
Contributor Author

@vmx What do you think? Any suggestions?

@koushiro
Copy link
Contributor Author

koushiro commented Sep 22, 2021

Example:

let mh1 = Multihash::<crate::U32>::wrap(Code::Sha2_256.into(), Sha2_256::digest(b"hello world").as_ref()).unwrap();

let mh3: Multihash<crate::U64> = Code::Sha2_256.digest(b"hello world");

Before:

mh1: code = 18, size = 32, digest = [185, 77, 39, 185, 147, 77, 62, 8, 165, 46, 82, 215, 218, 125, 171, 250, 196, 132, 239, 227, 122, 83, 128, 238, 144, 136, 247, 172, 226, 239, 205, 233] (0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9)
Multihash<U32> scale-codec encoding: 120000000000000020b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9

mh3: code = 18, size = 32, digest = [185, 77, 39, 185, 147, 77, 62, 8, 165, 46, 82, 215, 218, 125, 171, 250, 196, 132, 239, 227, 122, 83, 128, 238, 144, 136, 247, 172, 226, 239, 205, 233] (0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9)
Multihash<U64> scale-codec encoding: 120000000000000020b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde90000000000000000000000000000000000000000000000000000000000000000

After:

mh1: code = 18, size = 32, digest = [185, 77, 39, 185, 147, 77, 62, 8, 165, 46, 82, 215, 218, 125, 171, 250, 196, 132, 239, 227, 122, 83, 128, 238, 144, 136, 247, 172, 226, 239, 205, 233] (0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9)
Multihash<U32> scale-codec encoding: 120000000000000020b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9

mh3: code = 18, size = 32, digest = [185, 77, 39, 185, 147, 77, 62, 8, 165, 46, 82, 215, 218, 125, 171, 250, 196, 132, 239, 227, 122, 83, 128, 238, 144, 136, 247, 172, 226, 239, 205, 233] (0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9)
Multihash<U64> scale-codec encoding: 120000000000000020b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9

@vmx vmx merged commit 59064a6 into multiformats:master Sep 22, 2021
@vmx
Copy link
Member

vmx commented Sep 22, 2021

Sorry for the delay, and thanks for providing an example, that was really helpful.

@koushiro koushiro deleted the fix-scale-codec branch September 22, 2021 09:30
@koushiro
Copy link
Contributor Author

koushiro commented Sep 22, 2021

@vmx Could you release a new version?

Maybe the new version can include #135

@vmx
Copy link
Member

vmx commented Sep 22, 2021

@koushiro would it be OK if I do a release next week? I'd like to also get #138 in if possible (if it isn't ready by next week, I'll do a release without it).

@koushiro
Copy link
Contributor Author

would it be OK if I do a release next week? I'd like to also get #138 in if possible (if it isn't ready by next week, I'll do a release without it).

Ok, thanks!

@vmx
Copy link
Member

vmx commented Oct 4, 2021

@koushiro sorry for the delay. 0.15 is now released.

Stebalien added a commit to Stebalien/sp-multihash that referenced this pull request Oct 15, 2021
The number of digest bytes written needs to match the size.

See the upstream issue here: multiformats/rust-multihash#140

The one change is to not allocate.
Stebalien added a commit to Stebalien/sp-multihash that referenced this pull request Oct 15, 2021
The number of digest bytes written needs to match the size.

See the upstream issue here: multiformats/rust-multihash#140

All credit goes to @koushiro. My one change is to not allocate.
Stebalien added a commit to Stebalien/sp-multihash that referenced this pull request Oct 15, 2021
The number of digest bytes written needs to match the size.

See the upstream issue here: multiformats/rust-multihash#140

All credit goes to @koushiro. My one change is to not allocate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants