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

keyring: remove lazy_static public keys hash maps #2387

Merged
merged 38 commits into from
Dec 11, 2023

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented Nov 17, 2023

The lazy_static package does not work well in no-std: it requires spin_no_std feature, which also will propagate into std if enabled. This is not what we want.

This PR removes public/private key hash-maps and replaces them with simple static byte arrays.

&T versions of AsRef/Deref/From traits implementation were removed.

Little const helper for converting hex strings into array during compile time was also added. (somewhat similar to hex_literal).

@michalkucharczyk michalkucharczyk marked this pull request as draft November 17, 2023 17:14
@michalkucharczyk michalkucharczyk changed the title keyring: remove lazy_static public hash maps keyring: remove lazy_static public keys hash maps Nov 17, 2023
@michalkucharczyk

This comment was marked as off-topic.

@michalkucharczyk michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Nov 17, 2023
@michalkucharczyk michalkucharczyk marked this pull request as ready for review November 22, 2023 15:12
Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

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

The code obviously works. There is not much to say about it.

The only thing that I still don't like (but maybe is subjective) is that there are two obvious ways to do the same thing.

That is, getting a public key using:

  • Keyring::pair().public()
  • From<Keyring> or [u8; 32]

In practice this new approach is the same as the one used in the previous iteration only that the code that previously was in a member function has not being moved in From<Keyring> for [u8; 32]`.

Again, IMO would be way better to have one way or another.

  • One which is used for std or full_crypto or whatever you want to call the feature in this crate (which also includes the Pair` and capability to sign).
  • Another where fn public internally uses this new approach of pre-constructed arrays.

If at some point you want to include this stuff in no_std you may want to remove fn pair() anyway as to sign with a Schnorr key you need randomness.

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Nov 23, 2023

IUC: the consequence of your proposal will be calling the pair().public() everytime (w/o a PUBLICK_KESY cache which was just removed) in full_crypto and std.

Having the public key in form of raw bytes prevents this call (and is closer to cached approach).

@michalkucharczyk
Copy link
Contributor Author

ok, I think I got it, you simply want this function to use predefined raw bytes, right?

pub fn public(self) -> Public {
self.pair().public()
}

@davxy
Copy link
Member

davxy commented Nov 23, 2023

IUC: the consequence of your proposal will be calling the pair().public() everytime (w/o a PUBLICK_KESY cache which was just removed) in full_crypto and std.

Having the public key in form of raw bytes prevents this call (and is closer to cached approach).

Caching is pointless here. Alice, Bob and co keyring is used for tests, templates and a like. Never in production

@davxy
Copy link
Member

davxy commented Nov 23, 2023

ok, I think I got it, you simply want this function to use predefined raw bytes, right?

The ideal is:

  • if I can compute from pair() => compute from pair
  • if I can't => use precomputed

As I said above, caching is not really important for these keys

@michalkucharczyk
Copy link
Contributor Author

IUC: the consequence of your proposal will be calling the pair().public() everytime (w/o a PUBLICK_KESY cache which was just removed) in full_crypto and std.
Having the public key in form of raw bytes prevents this call (and is closer to cached approach).

Caching is pointless here. Alice, Bob and co keyring is used for tests, templates and a like. Never in production

This will be done in chainspec generation (for all testnets, zombienets, etc...), which will be done in runtime. Why not keep this optimization?

@michalkucharczyk
Copy link
Contributor Author

Is the cost of calling pair() actually negligible?

@davxy
Copy link
Member

davxy commented Nov 23, 2023

IUC: the consequence of your proposal will be calling the pair().public() everytime (w/o a PUBLICK_KESY cache which was just removed) in full_crypto and std.
Having the public key in form of raw bytes prevents this call (and is closer to cached approach).

Caching is pointless here. Alice, Bob and co keyring is used for tests, templates and a like. Never in production

This will be done in chainspec generation (for all testnets, zombienets, etc...), which will be done in runtime. Why not keep this optimization?

  1. I'm not saying that is negligible. But for sure we do a bazilion of hashes and computations around which probably makes it negligible in the contexts where is done (I haven't done any benchmarks BTW)
  2. Haven't you already removed the cache with this PR? (i.e. PUBLIC_KEYS and PRIVATE_KEYS)

My suggestion was such that in the runtime you end up using the fn public() version which doesn't use pair(). I.e. the one which uses the pre-computed stuff. There is not real value exposing fn pair() in the runtime as you can't use fn sign() for Schnorr and ECDSA (which in general requires randomness)

@michalkucharczyk
Copy link
Contributor Author

Support for no-std (e.g removal of sign method) is added in:
#2044

@michalkucharczyk
Copy link
Contributor Author

Yeah, sorry my oversight, Obviously fn public() method should have been using the raw bytes. Fixed in 25a0a89.

@davxy
Copy link
Member

davxy commented Nov 23, 2023

Support for no-std (e.g removal of sign method) is added in: #2044

Ok I remember. But I also was thinking that with the introduction of statically computed Public having the Pair trait for no_std was not required.

What is the usage of Pair in no_std at this point? Wasn't all that work done because you were requiring to compute Public? Maybe offchain workers who doesn't want to use the keystore via HF?

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Nov 23, 2023

We can gate pair() method also. I will add it in #2044 (as it introduces support of Keyring in runtimes).

Probably there is no use case for this in runtime. On the other hand we will have this functionality for free, and if not used linker should strip it off.

@michalkucharczyk michalkucharczyk requested a review from a team November 24, 2023 15:22
@michalkucharczyk
Copy link
Contributor Author

Sorry for bikeshedding on this

It was different at the beggining, I spotted my mistake here: #2387 (comment)

@michalkucharczyk michalkucharczyk added the R0-silent Changes should not be mentioned in any release notes label Dec 6, 2023
substrate/primitives/core/src/const_hex2array.rs Outdated Show resolved Hide resolved
substrate/primitives/core/src/const_hex2array.rs Outdated Show resolved Hide resolved
@michalkucharczyk
Copy link
Contributor Author

bot update-ui

@command-bot
Copy link

command-bot bot commented Dec 8, 2023

@michalkucharczyk https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4662342 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/update-ui/update-ui.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-3f5db3cb-98b2-4490-b5c9-da31f43ceeaf to cancel this command or bot cancel to cancel all commands in this pull request.

@michalkucharczyk michalkucharczyk requested review from a team December 8, 2023 14:21
@command-bot
Copy link

command-bot bot commented Dec 8, 2023

@michalkucharczyk Command "$PIPELINE_SCRIPTS_DIR/commands/update-ui/update-ui.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4662342 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4662342/artifacts/download.

@michalkucharczyk michalkucharczyk merged commit f6548ae into master Dec 11, 2023
115 checks passed
@michalkucharczyk michalkucharczyk deleted the mku-remove-lazy-static-from-keyring branch December 11, 2023 14:21
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
The `lazy_static` package does not work well in `no-std`: it requires
`spin_no_std` feature, which also will propagate into `std` if enabled.
This is not what we want.

This PR removes public/private key hash-maps and replaces them with
simple static byte arrays.

`&T` versions of `AsRef/Deref/From` traits implementation were removed.

Little const helper for converting hex strings into array during compile
time was also added. (somewhat similar to _hex_literal_).

---------

Co-authored-by: command-bot <>
Co-authored-by: Koute <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants