Make some BEEFY keystore logic more generic #10763

serban300 · 2026-01-09T16:10:57Z

This PR:

makes some BEEFY keystore methods more generic:

sign()
public_keys()
This is done by implementing the specific logic in the BeefyAuthorityId.

Removes the BeefyAuthorityId::SignatureHasher since for some algorithms it doesn't make sense to have a hasher.

Also since now the BeefyAuthorityId implements both the signing and the verification logic, we should have better consistency.

Related to #8707 (comment)

The BEEFY keystore doesn't support simple BLS keys

substrate/client/consensus/beefy/src/keystore.rs

drskalman · 2026-01-14T13:43:53Z

substrate/primitives/consensus/beefy/src/lib.rs

+			let msg_hash = keccak_256(msg);
+			let public = ecdsa::Public::try_from(self.as_slice()).unwrap();
+
+			let maybe_sig = store.ecdsa_sign_prehashed(BEEFY_KEY_TYPE, &public, &msg_hash)?;


I assume that you can do this in no_std and when it gets called actually in no_std it panics? Is it acceptable behavoir?

Added a #[cfg(feature = "std")] guard for try_sign().

substrate/primitives/consensus/beefy/src/lib.rs

drskalman · 2026-01-14T18:08:46Z

substrate/primitives/consensus/beefy/src/lib.rs

-		));
+		let blake2_256_signature: ecdsa_crypto::Signature =
+			pair.as_inner_ref().sign_prehashed(&blake2_256(msg)).into();
+		assert!(!BeefyAuthorityId::verify(&pair.public(), &blake2_256_signature, msg));


What is the point of keeping this? The test was showing that signing with different hash works, now it doesn't, do we need it to demonestrate that something doesn't work?

drskalman · 2026-01-14T18:11:54Z

substrate/primitives/consensus/beefy/src/lib.rs

-
-	#[test]
-	#[cfg(feature = "bls-experimental")]
-	fn bls_beefy_verify_works() {


Why are we removing bls tests?

Added back the support for bls_crypto

drskalman · 2026-01-14T18:15:21Z

substrate/primitives/consensus/beefy/src/test_utils.rs

-{
-	fn sign_with_hasher(&self, message: &[u8]) -> <Self as AppCrypto>::Signature {
-		let hashed_message = <MsgHash as Hash>::hash(message).into();
+impl BeefySignerAuthority for <ecdsa_crypto::AuthorityId as AppCrypto>::Pair {


We don't need to define BeefySignerAuthority because apparently BeefyAuthority can sign itself. Right?

I think we still need it because these are different use cases. BeefySignerAuthority signs using the private key. BeefyAuthorityId signs using a keystore. Also renamed BeefyAuthority::try_sign() to try_sign_with_store()

I see that BeefySignerAuthority is used in the tests and we don't always have the store in those scenarios. Maybe we could try to modify the tests to always use a key store, but sounds like some work and I don't know if it's worth it.

Ok let makes that explicit in the comment so it is less confusing.

substrate/primitives/consensus/beefy/src/test_utils.rs

drskalman · 2026-01-14T18:23:20Z

substrate/client/consensus/beefy/src/keystore.rs

+		let raw_signature = BeefyAuthorityId::try_sign(public, store, message)
+			.map_err(|e| error::Error::Keystore(e.to_string()))?
+			.ok_or_else(|| {
+				error::Error::Signature("BeefyAuthorityId::try_sign() returned None".to_string())


It seems to me that error messages were more expressive before. Now we only getting "returned None" with no further clarification.

drskalman · 2026-01-14T18:28:09Z

substrate/primitives/consensus/beefy/src/lib.rs

-pub trait BeefyAuthorityId<MsgHash: Hash>: RuntimeAppPublic {
+pub trait BeefyAuthorityId: RuntimeAppPublic {
+	/// Get all the public keys of the current type from a provided `Keystore`.
+	fn get_all_from_store(store: KeystorePtr) -> Vec<impl AsRef<[u8]>>;


Suggested change

fn get_all_from_store(store: KeystorePtr) -> Vec<impl AsRef<[u8]>>;

fn get_all_public_keys_from_store(store: KeystorePtr) -> Vec<impl AsRef<[u8]>>;

I could not figure out the puprose of the function from "get_all_from_store"

substrate/primitives/consensus/beefy/src/lib.rs

This reverts commit d6f0d09.

serban300 · 2026-01-16T07:58:32Z

@drskalman thank you for the review ! I think I addressed all your comments. Can you PTAL when you have time ?

drskalman

I actually liked your using of BEEFY_KEY_TYPE it was more expressive but I'm on going to dwell on that :-)

substrate/primitives/consensus/beefy/src/lib.rs

substrate/primitives/consensus/beefy/src/test_utils.rs

drskalman · 2026-01-23T18:29:37Z

substrate/primitives/consensus/beefy/src/test_utils.rs

-{
-	fn sign_with_hasher(&self, message: &[u8]) -> <Self as AppCrypto>::Signature {
-		let hashed_message = <MsgHash as Hash>::hash(message).into();
+impl BeefySignerAuthority for <ecdsa_crypto::AuthorityId as AppCrypto>::Pair {


Ok let makes that explicit in the comment so it is less confusing.

serban300 · 2026-01-26T08:58:56Z

I actually liked your using of BEEFY_KEY_TYPE it was more expressive but I'm on going to dwell on that :-)

Renamed it back to BEEFY_KEY_TYPE

serban300 · 2026-01-26T15:15:43Z

/cmd prdoc --audience node_dev --bump patch

…e_dev --bump patch'

drskalman

Thank you very much! LGTM 👍

serban300 self-assigned this Jan 9, 2026

serban300 added the T15-bridges This PR/Issue is related to bridges. label Jan 9, 2026

serban300 changed the title ~~Make BEEFY keystore more generic~~ Make some BEEFY keystore logic more generic Jan 9, 2026

serban300 force-pushed the small_fixes_beefy branch 2 times, most recently from d74c2d6 to ca1d586 Compare January 9, 2026 18:17

serban300 added 4 commits January 9, 2026 21:20

Remove unused BeefyAuthorityId

d6f0d09

The BEEFY keystore doesn't support simple BLS keys

Expose generic try_sign() in BeefyAuthorityId

d2cc0c8

Expose generic get_all_from_store() in BeefyAuthorityId

92a3472

Remove SignatureHasher

ccf1be1

serban300 force-pushed the small_fixes_beefy branch from ca1d586 to ccf1be1 Compare January 9, 2026 19:20

serban300 requested a review from acatangiu as a code owner January 9, 2026 19:20

paritytech-review-bot bot requested a review from a team January 9, 2026 19:21

serban300 mentioned this pull request Jan 9, 2026

Add support of additional crypto types and hashing algorithms for Beefy #8707

Open

acatangiu approved these changes Jan 13, 2026

View reviewed changes

drskalman requested changes Jan 14, 2026

View reviewed changes

serban300 added 7 commits January 15, 2026 12:37

SignatureHasher leftovers

4ca9d2f

CR comments

d5c9216

Revert "Remove unused BeefyAuthorityId"

c6f7267

This reverts commit d6f0d09.

bls_crypto implement new BeefyAuthorityId methods

b8d6832

simplification

3e6164e

Fix build warnings

c10c21f

Fix build warnings

2944e4e

drskalman requested changes Jan 23, 2026

View reviewed changes

serban300 added 3 commits January 26, 2026 10:30

Update comments

557d8bf

Merge remote-tracking branch 'upstream/master' into small_fixes_beefy

a3bdd44

KEY_TYPE -> BEEFY_KEY_TYPE

134caf8

github-actions bot and others added 2 commits January 26, 2026 15:18

Update from github-actions[bot] running command 'prdoc --audience nod…

04b54c7

…e_dev --bump patch'

Fix prdoc

5ff7258

drskalman self-requested a review January 29, 2026 17:01

drskalman approved these changes Jan 29, 2026

View reviewed changes

serban300 added this pull request to the merge queue Jan 29, 2026

Merged via the queue into paritytech:master with commit a6082b3 Jan 29, 2026
237 of 241 checks passed

serban300 deleted the small_fixes_beefy branch January 29, 2026 17:53

	fn get_all_from_store(store: KeystorePtr) -> Vec<impl AsRef<[u8]>>;
	fn get_all_public_keys_from_store(store: KeystorePtr) -> Vec<impl AsRef<[u8]>>;

Make some BEEFY keystore logic more generic #10763

Make some BEEFY keystore logic more generic #10763

Uh oh!

Conversation

serban300 commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serban300 Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serban300 commented Jan 16, 2026

Uh oh!

drskalman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serban300 commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serban300 commented Jan 26, 2026

Uh oh!

drskalman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serban300 commented Jan 9, 2026 •

edited

Loading

serban300 Jan 15, 2026 •

edited

Loading

serban300 commented Jan 26, 2026 •

edited

Loading