Beefy client generic on aduthority Id by drskalman · Pull Request #1816 · paritytech/polkadot-sdk

drskalman · 2023-10-08T00:54:15Z

Revived version of paritytech/substrate#13311 . Except Signature is not generic and is dictated by AuthorityId.

…ssfuly try to implement ByteArray for paired crypto Public

…ed crypto object in favor of avoiding copy

…me of aux traits for `paired_crypto::Signature`

… of seeds for paired crypto scheme.

…paired (ECDSA, BLS377) crypto

…_crypto.rs`. - put serialize and descerialze under `serde` feature instead of std. - in `primitives/core/src/bls.rs`. - fix documentation in `primitives/core/src/bls.rs`. - cargo fmt pair_crypto.rs

…and SCALE. Co-authored-by: Davide Galassi <[email protected]>

Co-authored-by: Davide Galassi <[email protected]>

…ZED_SIZE` instead of magic numbers

- Remove `SignaturePair` trait. - Skip encoding redundant copy of paired::Public and Signature and implement Decode

…red_crypto::Pair

…rypto type - Implement ecdsa_bls37_crypto for BEEFY primitives.

drskalman · 2023-10-08T00:57:23Z

@davxy It compiles now.

I was able to resolve the decode problem by using RuntimeAppPublic::Signature instead of AppCrypto::Signature, the former is bounded by Codec while the later is not.

Nonetheless I need to constrain associated type RuntimeAppPublic::Signature to Send and Sync and I still do not know how to do it at the definition of AuthorityIdBound so I had to constrain it all over the files whenever I needed to use AuthorityId.

…sa_bls::verify`

…test - update `substrate/primitives/consensus/beefy/src/lib.rs` accordingly

…erging-cube

Send + Sync,` constraints

davxy

LGTM. I just left one consideration about using AppCrypto vs RuntimeAppPublic

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

davxy · 2024-05-29T10:09:27Z

substrate/client/consensus/beefy/src/communication/gossip.rs

-pub(crate) enum GossipMessage<B: Block> {
+pub(crate) enum GossipMessage<B: Block, AuthorityId: AuthorityIdBound> {
 	/// BEEFY message with commitment and single signature.
-	Vote(VoteMessage<NumberFor<B>, AuthorityId, Signature>),


I just drop this here, but applies all over the place.
Does <AuthorityId as AppCrypto>::Signature work? (in place of as RuntimeAppPublic.
I can't try it right now, but if yes then to me looks more appropriate using a trait that has not being designed for usage within the runtime

We had this conversation somewhere else before, I'm sorry my PR takes ages to merge :-/ AppCrypto::Signature somehow is not bounded by Codec (perhaps it should), RuntimeAppPublic::Signature has Codec and I assume you need to encode and send the Vote. I can check nonetheless.

yep!:

error[E0599]: the variant or associated item `decode_all` exists for enum `GossipMessage<B, AuthorityId>`, but its trait bounds were not satisfied --> substrate/client/consensus/beefy/src/worker.rs:862:40 | 862 | GossipMessage::<B, AuthorityId>::decode_all(&mut &notification.message[..]) | ^^^^^^^^^^ variant or associated item cannot be called on `GossipMessage<B, AuthorityId>` due to unsatisfied trait bounds | ::: substrate/client/consensus/beefy/src/communication/gossip.rs:73:1 | 73 | pub(crate) enum GossipMessage<B: Block, AuthorityId: AuthorityIdBound> {

So I guess that we can merge this as soon as CI passes

Co-authored-by: Davide Galassi <[email protected]>

paritytech-cicd-pr · 2024-05-29T11:39:45Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6338844

…nt-generic-on-auth-id

drskalman · 2024-05-29T20:12:08Z

@acatangiu @serban300 3 non-required tests are failing. Could you check if we are mergeable or not?

serban300 · 2024-05-30T07:59:22Z

@acatangiu @serban300 3 non-required tests are failing. Could you check if we are mergeable or not?

Only gitlab-zombienet-polkadot-functional-0003-beefy-and-mmr could be related to this PR. But it succeeded on re-run. So I think we can merge. But first I will re-run it again a couple of times just to make sure.

* master: (93 commits) Fix broken windows build (#4636) Beefy client generic on aduthority Id (#1816) pallet-staking: Put tests behind `cfg(debug_assertions)` (#4620) Broker new price adapter (#4521) Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621) Update README.md (#4623) Publish `chain-spec-builder` (#4518) Add omni bencher & chain-spec-builder bins to release (#4557) Moves runtime macro out of experimental flag (#4249) Filter workspace dependencies in the templates (#4599) parachain-inherent: Make `para_id` more prominent (#4555) Add metric to measure the time it takes to gather enough assignments (#4587) Improve On_demand_assigner events (#4339) Conditional `required` checks (#4544) [CI] Deny adding git deps (#4572) [subsytem-bench] Remove redundant banchmark_name param (#4540) Add availability-recovery from systematic chunks (#1644) Remove workspace lints from templates (#4598) `sc-chain-spec`: deprecated code removed (#4410) [subsystem-benchmarks] Add statement-distribution benchmarks (#3863) ...

* master: (106 commits) [ci] Delete unused flow (#4676) Fix umbrella CI check and fix the C&P message (#4670) Add Dockerfiles to the templates (#4637) Revamp the Readme of the minimal template (#4649) Add chain-spec-builder docker image (#4655) Update Amforc bootnodes for Kusama and Polkadot (#4668) make all storage items in parachain-system public (#4645) [Pools] Refactors and runtime apis for DelegateStake (#4537) update amforc westend and its parachain bootnodes (#4641) Better error for missing index in CRV2 (#4643) Implement `XcmPaymentApi` and `DryRunApi` on all system parachains (#4634) Use Unlicense for templates (#4628) collator-protocol: remove `elastic-scaling-experimental` feature (#4595) Update `runtime_type` ref doc with the new "Associated Type Bounds" (#4624) Adds ability to specify chain type in chain-spec-builder (#4542) Fix broken windows build (#4636) Beefy client generic on aduthority Id (#1816) pallet-staking: Put tests behind `cfg(debug_assertions)` (#4620) Broker new price adapter (#4521) Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621) ...

Revived version of paritytech/substrate#13311 . Except Signature is not generic and is dictated by AuthorityId. --------- Co-authored-by: Davide Galassi <[email protected]> Co-authored-by: Robert Hambrock <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>

drskalman and others added 26 commits September 25, 2023 15:29

First definition for pair public keys

5d3dce1

Two example of implementation of pair for demonestration

be890d4

- implement paired crypto Public as tuple of two Publics - unsuce…

fb6a018

…ssfuly try to implement ByteArray for paired crypto Public

keep both public key object and their continous serialization in pair…

62ef8b1

…ed crypto object in favor of avoiding copy

implement PassBy and From<Pair> for paired_crypto

8d4e523

implement rest of aux traits for paired_crypto::Public implement so…

eea74e2

…me of aux traits for `paired_crypto::Signature`

Attempt to implement trait Pair for pair_cyrpto::Pair

f407d87

- Implement trait Pair for paired_crypto::Pair - Implement a pair…

2606cc4

… of seeds for paired crypto scheme.

implement sgin and verify for

00933da

Actually implementing paired_crypto::{Pair, Public, Signatrue} for …

e7719ab

…paired (ECDSA, BLS377) crypto

Implement and pass all test for paired_crypto

7a9b677

- move to signle seed for both schemes in `primitives/core/src/paired…

a01a814

…_crypto.rs`. - put serialize and descerialze under `serde` feature instead of std. - in `primitives/core/src/bls.rs`. - fix documentation in `primitives/core/src/bls.rs`. - cargo fmt pair_crypto.rs

replace hex! → array_bytes::hex2xx

7c02658

Apply suggestions from paired_crypto code review on type nam, hash …

7f8d958

…and SCALE. Co-authored-by: Davide Galassi <[email protected]>

Do not panic in paired::Signature::try_from

eef2cec

Co-authored-by: Davide Galassi <[email protected]>

Remove DoublePair trait.

efb215f

Co-authored-by: Davide Galassi <[email protected]>

Do not empty implement paired::Pair

65584bd

Co-authored-by: Davide Galassi <[email protected]>

Use paired_crypto::Seed instead of [u8; SECURE_SEED_LEN]

e047f75

Co-authored-by: Davide Galassi <[email protected]>

use ecdsa::PUBLIC_KEY_SERIALIZED_SIZE and `ecdsa::SIGNATURE_SERIALI…

c12b82e

…ZED_SIZE` instead of magic numbers

Remove paired::DoublePair impl as well

dba854a

- Implement BytesArray for both ecdsa and bls Signatures

5182c86

- Remove `SignaturePair` trait. - Skip encoding redundant copy of paired::Public and Signature and implement Decode

Implement encode_and_decode_(public_key/signature)_works test for pai…

17d0077

…red_crypto::Pair

cargo fmt

a740589

- Implement RuntimeAppCrypto and necessery hostApi for ecdsa_bls377 c…

79f25e9

…rypto type - Implement ecdsa_bls37_crypto for BEEFY primitives.

cargo fmt

489522f

make BEEFY Keystore and Worker generic over AuthorityId

05e08b3

drskalman added 3 commits October 27, 2023 12:21

Fix BeefyAuthorityId::verify for ecdsa_bls_crypto not to use `ecd…

0541cb0

…sa_bls::verify`

Fix comment in primitives/consensus/beefy/src/lib.rs

099b86d

- implement sign/verify_with_hasher for ecdsa_bls377 Pair with …

033ff57

…test - update `substrate/primitives/consensus/beefy/src/lib.rs` accordingly

bkchr pushed a commit that referenced this pull request Apr 10, 2024

Remove InboundPayload check (#1816)

1f727be

drskalman added 8 commits May 27, 2024 00:50

Removing constrain on <AuthorityId as RuntimeAppPublic>::Signature

5d2f33a

Merge branch 'master' into skalman--beefy-client-generic-on-auth-id-m…

6031199

…erging-cube

fix beefy tests after merging

f55e92f

unwrap beefy justification to pass test

99ab160

Remove all remaining `<AuthorityId as RuntimeAppPublic>::Signature:

8bf7a40

Send + Sync,` constraints

Do cargo +nightly fmt

7c42d8b

Remove redundant imports resolve all warnings

a326523

make @davxy happy

725fc2c

drskalman requested review from davxy and serban300 May 29, 2024 07:32

davxy approved these changes May 29, 2024

View reviewed changes

serban300 approved these changes May 29, 2024

View reviewed changes

Remove empty line from substrate/primitives/consensus/beefy/src/lib.rs

d834a1d

Co-authored-by: Davide Galassi <[email protected]>

drskalman added 3 commits May 29, 2024 12:20

add beefy_primitisto cli/service

f65096e

fmt

cfc59ed

Merge remote-tracking branch 'origin/master' into skalman--beefy-clie…

a6a1f82

…nt-generic-on-auth-id

serban300 added this pull request to the merge queue May 30, 2024

Merged via the queue into paritytech:master with commit bcab07a May 30, 2024

davxy deleted the skalman--beefy-client-generic-on-auth-id branch May 30, 2024 11:06

drskalman mentioned this pull request Jun 6, 2024

Make BEEFY Client generic over AuthorityId w3f/polkadot-sdk#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beefy client generic on aduthority Id#1816

Beefy client generic on aduthority Id#1816
serban300 merged 94 commits intoparitytech:masterfrom
w3f:skalman--beefy-client-generic-on-auth-id

drskalman commented Oct 8, 2023

Uh oh!

drskalman commented Oct 8, 2023 •

edited

Loading

Uh oh!

davxy left a comment

Uh oh!

Uh oh!

davxy May 29, 2024

Uh oh!

drskalman May 29, 2024

Uh oh!

drskalman May 29, 2024

Uh oh!

davxy May 29, 2024

Uh oh!

paritytech-cicd-pr commented May 29, 2024

Uh oh!

drskalman commented May 29, 2024 •

edited

Loading

Uh oh!

serban300 commented May 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

drskalman commented Oct 8, 2023

Uh oh!

drskalman commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davxy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davxy May 29, 2024

Choose a reason for hiding this comment

Uh oh!

drskalman May 29, 2024

Choose a reason for hiding this comment

Uh oh!

drskalman May 29, 2024

Choose a reason for hiding this comment

Uh oh!

davxy May 29, 2024

Choose a reason for hiding this comment

Uh oh!

paritytech-cicd-pr commented May 29, 2024

Uh oh!

drskalman commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serban300 commented May 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

drskalman commented Oct 8, 2023 •

edited

Loading

drskalman commented May 29, 2024 •

edited

Loading