Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Crypto Pair trait refactory #13657

Merged
merged 10 commits into from
Mar 22, 2023
Merged

Crypto Pair trait refactory #13657

merged 10 commits into from
Mar 22, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Mar 20, 2023

While preparing the integration of BLS curves I noticed some duplicated code that can be directly part of the Pair trait default implementation.

Note 1. I've left a temporary test just to check that the result was the one expected for an implementation that was essentially doing the same stuff but in a fancier way (I'll remove it before merge)

Note 2. I would also like to take the opportunity to hear your opinion about substrate-bip39 repo/crate.
Is a library containing only 2 functions (both of few lines), and one of these now ends up being unused.
Maybe we can just embed the seed_from_entropy function in the core code (keeping the tests obviously) Any drawback?

@davxy davxy self-assigned this Mar 20, 2023
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 20, 2023
@davxy davxy requested review from bkchr, andresilva and a team March 20, 2023 16:38
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

primitives/core/src/sr25519.rs Outdated Show resolved Hide resolved
@davxy davxy requested a review from a team March 21, 2023 09:56
Comment on lines 900 to 901
let minlen = seed_slice.len().min(big_seed.len());
seed_slice[0..minlen].copy_from_slice(&big_seed[0..minlen]);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add some debug assert that big_seed.len() > seed_slice.len()? Otherwise there may will be some implementation that will just use some zeros?

Copy link
Member Author

@davxy davxy Mar 21, 2023

Choose a reason for hiding this comment

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

Yeah probably would be better to fail than filling the tail of the seed with zeroes.

But what about embedding the only function that substrate-bip39 offers directly into substrate?
As I said that library provides 1 function (the other is not used anymore after this pr).

The function is this:

pub fn seed_from_entropy(entropy: &[u8], password: &str) -> Result<[u8; 64], Error> {
    if entropy.len() < 16 || entropy.len() > 32 || entropy.len() % 4 != 0 {
        return Err(Error::InvalidEntropy);
    }
    let mut salt = String::with_capacity(8 + password.len());
    salt.push_str("mnemonic");
    salt.push_str(password);
    let mut seed = [0u8; 64];
    pbkdf2::<Hmac<Sha512>>(entropy, salt.as_bytes(), 2048, &mut seed);
    salt.zeroize();
    Ok(seed)
}

Sounds a bit bombastic to define a library only to provide a function...

Furthermore, we can then easily modify it to fill a mut slice (passed as parameter) to fill it with an arbitrary number of bytes... Thus there would be no need to check if we can fill the seed buffer because we always can

Copy link
Member

Choose a reason for hiding this comment

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

I have no real opinion here. We can copy the function, I don't really care. I don't know the history behind the substrate-bip39 crate.

@davxy davxy merged commit 2b42fbd into master Mar 22, 2023
@davxy davxy deleted the davxy-crypto-pair-refactory branch March 22, 2023 10:09
breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
* Crypto pair refactory

* Remove unused method

* Apply review suggestions

* Remove leftovers

* Associated type is not really required

* Fix after refactory

* Fix benchmark-ui test

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Crypto pair refactory

* Remove unused method

* Apply review suggestions

* Remove leftovers

* Associated type is not really required

* Fix after refactory

* Fix benchmark-ui test

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants