Paired-key Crypto Scheme #1705

drskalman · 2023-09-25T20:39:42Z

BEEFY needs two cryptographic keys at the same time. Validators should sign BEEFY payload using both ECDSA and BLS key. The network will gossip a payload which contains a valid ECDSA key. The prover nodes aggregate the BLS keys if aggregation fails to verifies the validator which provided a valid ECDSA signature but an invalid BLS signature is subject to slashing.

As such BEEFY session should be initiated with both key. Currently there is no straight forward way of doing so, beside having a session with RuntimeApp corresponding to a crypto scheme contains both keys.

This pull request implement a generic paired_crypto scheme as well as implementing it for (ECDSA, BLS) pair.

…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

drskalman · 2023-09-27T16:12:09Z

I could change the sign algorithm to sign the second signature using the first key. This potentially might give us a 7x speed up on verification time during validators gossip of BEEFY messages:

test double::tests::test_bls_verify_signatures_chaum_pedersen  ... bench:   2,967,600 ns/iter (+/- 15,817)                                                                                                                                                    
test double::tests::test_bls_verify_signatures_simple_bls      ... bench:   4,304,517 ns/iter (+/- 6,141,991)                                                                                                                                                 
test double::tests::test_verify_EdDSA_signatures               ... bench:     352,084 ns/iter (+/- 1,802)                                                                                                                                                     
test double::tests::test_verify_Schnorr_on_secp256k1_signatures ... bench:     394,758 ns/iter (+/- 2,064)

I couldn't find an readily implemented ECDSA for Arkworks so I used Schnorr on Secp256k1 instead but I don't expect a huge difference between ECDSA and a single schnorr verification. The result is on par with this experiment showin BLS verification being 10 times slower than ECDSA.

davxy

First pass.

In the meantime I'm going to read the interesting article you shared :-)

substrate/primitives/core/src/paired_crypto.rs

…and SCALE. Co-authored-by: Davide Galassi <davxy@datawok.net>

Co-authored-by: Davide Galassi <davxy@datawok.net>

…ZED_SIZE` instead of magic numbers

drskalman · 2023-10-09T14:48:22Z

WDYT?

I think currently it gets the public key from the vote messages. Nonetheless, I could imagine that someone would take advantage of the optimization.

It is less relevant with the signatures, but we also for example do other stuff with the signatures, like aggregating them which in BLS, we need them to be in the form of points on the curve rather than the serialized form. But that is much less common than the public key reuse scenario.

So I'll try to make it infallible with constraining first.

drskalman · 2023-10-09T22:19:57Z

Doing this:

impl<LeftPublic: PublicT, RightPublic: PublicT, const LEFT_PLUS_RIGHT_LEN: usize, >                                                                           
        UncheckedFrom<[u8; LEFT_PLUS_RIGHT_LEN]> for Public<LeftPublic, RightPublic, LEFT_PLUS_RIGHT_LEN> where LeftPublic : UncheckedFrom<[u8; LeftPublic::LEN]>

Results in:

error: generic parameters may not be used in const operations                                                                                                 
   --> substrate/primitives/core/src/paired_crypto.rs:209:138                                                                                                 
    |                                                                                                                                                         
209 | ...e LeftPublic : UncheckedFrom<[u8; LeftPublic::LEN]>                                                                                                  
    |                                      ^^^^^^^^^^^^^^^ cannot perform const operation using `LeftPublic`                                                  
    |                                                                                                                                                         
    = note: type parameters may not be used in const expressions

I'm can not introduce a new const generic only for the implementation of UncheckedFrom because Rust complains that Public does not depends on:

impl<LeftPublic: PublicT, RightPublic: PublicT, const LEFT_PLUS_RIGHT_LEN: usize, const LEFT_LEN: usize>                                                      
        UncheckedFrom<[u8; LEFT_PLUS_RIGHT_LEN]> for Public<LeftPublic, RightPublic, LEFT_PLUS_RIGHT_LEN> where LeftPublic : UncheckedFrom<[u8; LEFT_LEN]>

Results in:

error[E0207]: the const parameter `LEFT_LEN` is not constrained by the impl trait, self type, or predicates                                                   
   --> substrate/primitives/core/src/paired_crypto.rs:208:83                                                                                                  
    |                                                                                                                                                         
208 | impl<LeftPublic: PublicT, RightPublic: PublicT, const LEFT_PLUS_RIGHT_LEN: usize, const LEFT_LEN: usize>                                                
    |                                                                                   ^^^^^^^^^^^^^^^^^^^^^ unconstrained const parameter                   
    |                                                                                                                                                         
    = note: expressions using a const parameter must map each value to a distinct output value                                                                
    = note: proving the result of expressions other than the parameter are unique is not supported

This also doesn't work (not even in nightly):

struct foo<const L: usize, const R: usize, > 
{
 inner : [u8; L + R]
}

as in

error: generic parameters may not be used in const operations
 --> src/main.rs:3:15
  |
3 |  inner : [u8; L + R]
  |               ^ cannot perform const operation using `L`
  |
  = help: const parameters may only be used as standalone arguments, i.e. `L`

This compiles only in nightly:

#![feature(generic_const_exprs)]
struct foo<const L: usize, const R: usize, const L_PLUS_R: usize> 
{
    inner : [u8; L_PLUS_R],
}

trait bar {
    
}

impl<const L: usize, const R: usize,> bar for foo<L, R, {L+R}>
{
}

with scary warnings about horrible consequences of using generic_const_exprs which we do not want to get involved with. Additionally it adds two more generic consts all over the code, making the code much less readable.

Note that in both of your proposed solutions we need to deal with this, because we need to finally convert Public::Inner into LeftPublic and RightPublic and (their Signature counterparts) either at inception of the object (as it is now) or when we want to sign or verify a signature. While we could return false in verification, I assume signing need to be infallible as well.

So checking Self::LEN = Left::LEN + Right::LEN during runtime and in the tests, seems to be the only option for now till the compiler matures more to handle const expressions better.

- implemented `test_length_of_paired_ecdsa_and_bls377_public_key_and_signature_is_correct`

…/polkadot-sdk into skalman-paired-crypto-scheme

drskalman · 2023-10-10T00:11:42Z

@davxy feel free to remove the debug_assert!s if you don't like them.

drskalman · 2023-10-10T13:59:22Z

@davxy I removed the internal object now it is just an array like other cryptos.

davxy

Looks good 🟢

davxy · 2023-10-11T11:10:08Z

substrate/primitives/core/src/paired_crypto.rs

+	fn derive<Iter: Iterator<Item = DeriveJunction>>(
+		&self,
+		path: Iter,
+		seed: Option<Self::Seed>,
+	) -> Result<(Self, Option<Self::Seed>), DeriveError> {
+		let path: Vec<_> = path.collect();
+
+		let left = self.left.derive(path.iter().cloned(), seed.map(|s| s.into()))?;
+		let right = self.right.derive(path.into_iter(), seed.map(|s| s.into()))?;
+
+		let seed = match (left.1, right.1) {
+			(Some(l), Some(r)) if l.as_ref() == r.as_ref() => Some(l.into()),
+			_ => None,
+		};
+
+		Ok((Self { left: left.0, right: right.0 }, seed))


Mmm... here if the two crypto schemes have different ways to derive the seed we end up with a None seed.
I don't like this a lot. But if is good for the current application I'm not going to block things for this.

Would be nice to at least write this thing explicitly over this method docs.

@davxy something like #1705 (comment)?
can also integrate directly into PairT::derive documentation, although it's specific to the implementation here.

The only way to be able to return a seed is to make the paired_crypto seed 64 bytes which is not inline with all other crypto schemes (and make it not composable i.e. you can't have a pair of paired schemes).

There is a comment in crypto.rs which says in some crypto schemes it is not possible to provide a direct seed after derivation:

https://github.com/w3f/polkadot-sdk//blob/skalman-paired-crypto-scheme/substrate/primitives/core/src/crypto.rs#L930

We can also add paired_crypto scheme to that comment perhaps.

Lederstrumpf

tACK - and thanks for removing the seed duplication (a01a814).

I recognize this change muddies the derive return though (#1705 (comment)).

substrate/primitives/core/src/paired_crypto.rs

Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

acatangiu · 2023-10-13T10:30:12Z

@davxy is this good to merge?

davxy · 2023-10-13T16:36:12Z

@davxy is this good to merge?

Yes

* master: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...

…ribution * tsv-disabling-backing: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...

Polkadot-Forum · 2023-10-17T13:10:02Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/6

…1815) Next step in process of making BEEFY being able to generate both ECDSA and BLS signature after #1705. It allows BEEFY to use a pair of ECDSA and BLS key as a AuthorityId. --------- Co-authored-by: Davide Galassi <davxy@datawok.net> Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

Polkadot-Forum · 2023-11-28T12:52:42Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/7

BEEFY needs two cryptographic keys at the same time. Validators should sign BEEFY payload using both ECDSA and BLS key. The network will gossip a payload which contains a valid ECDSA key. The prover nodes aggregate the BLS keys if aggregation fails to verifies the validator which provided a valid ECDSA signature but an invalid BLS signature is subject to slashing. As such BEEFY session should be initiated with both key. Currently there is no straight forward way of doing so, beside having a session with RuntimeApp corresponding to a crypto scheme contains both keys. This pull request implement a generic paired_crypto scheme as well as implementing it for (ECDSA, BLS) pair. --------- Co-authored-by: Davide Galassi <davxy@datawok.net> Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

…aritytech#1815) Next step in process of making BEEFY being able to generate both ECDSA and BLS signature after paritytech#1705. It allows BEEFY to use a pair of ECDSA and BLS key as a AuthorityId. --------- Co-authored-by: Davide Galassi <davxy@datawok.net> Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

drskalman added 13 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

Lederstrumpf assigned Lederstrumpf and davxy and unassigned Lederstrumpf Sep 26, 2023

andresilva requested a review from davxy September 26, 2023 09:10

Lederstrumpf unassigned Lederstrumpf and davxy Sep 26, 2023

Lederstrumpf self-requested a review September 26, 2023 09:27

Lederstrumpf added the T0-node This PR/Issue is related to the topic “node”. label Sep 26, 2023

davxy reviewed Sep 28, 2023

View reviewed changes

drskalman and others added 6 commits October 5, 2023 14:44

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

7f8d958

…and SCALE. Co-authored-by: Davide Galassi <davxy@datawok.net>

Do not panic in paired::Signature::try_from

eef2cec

Co-authored-by: Davide Galassi <davxy@datawok.net>

Remove DoublePair trait.

efb215f

Co-authored-by: Davide Galassi <davxy@datawok.net>

Do not empty implement paired::Pair

65584bd

Co-authored-by: Davide Galassi <davxy@datawok.net>

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

e047f75

Co-authored-by: Davide Galassi <davxy@datawok.net>

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

c12b82e

…ZED_SIZE` instead of magic numbers

drskalman added 3 commits October 9, 2023 20:00

- replace unwrap with expect

f568441

- implemented `test_length_of_paired_ecdsa_and_bls377_public_key_and_signature_is_correct`

Merge branch 'skalman-paired-crypto-scheme' of https://github.com/w3f…

1e3b915

…/polkadot-sdk into skalman-paired-crypto-scheme

Rename TraitPair -> PairT

f07f05a

Remove internal Left and Right Public and Signature objects.

a6da3f5

Trivial things

a069dcd

davxy approved these changes Oct 11, 2023

View reviewed changes

davxy requested a review from a team October 11, 2023 11:06

davxy reviewed Oct 11, 2023

View reviewed changes

Lederstrumpf approved these changes Oct 11, 2023

View reviewed changes

substrate/primitives/core/src/paired_crypto.rs Show resolved Hide resolved

drskalman and others added 2 commits October 11, 2023 16:09

Update substrate/primitives/core/src/paired_crypto.rs

808a53d

Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

Merge branch 'master' into skalman-paired-crypto-scheme

616f503

davxy merged commit 1b34571 into paritytech:master Oct 15, 2023
109 of 111 checks passed

drskalman mentioned this pull request Oct 16, 2023

Implement paired-crypto-scheme crypto scheme w3f/substrate#16

Closed

kiltbot mentioned this pull request Nov 7, 2023

[AUTOMATIC] Update Polkadot dependencies from 1.2.0 to 1.3.0 KILTprotocol/kilt-node#575

Closed

ahmadkaouk mentioned this pull request Nov 16, 2023

Update polkadot-sdk from v.1.1.0 to v1.3.0 moonbeam-foundation/moonbeam#2565

Closed

drskalman mentioned this pull request Nov 27, 2023

Make BEEFY able to generate data needed for Web3Sum proof generation w3f/polkadot-sdk#3

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paired-key Crypto Scheme #1705

Paired-key Crypto Scheme #1705

drskalman commented Sep 25, 2023

drskalman commented Sep 27, 2023

davxy left a comment

drskalman commented Oct 9, 2023

drskalman commented Oct 9, 2023

drskalman commented Oct 10, 2023

drskalman commented Oct 10, 2023

davxy left a comment

davxy Oct 11, 2023

Lederstrumpf Oct 11, 2023

drskalman Oct 11, 2023 •

edited

Loading

Lederstrumpf left a comment

acatangiu commented Oct 13, 2023

davxy commented Oct 13, 2023

Polkadot-Forum commented Oct 17, 2023

Polkadot-Forum commented Nov 28, 2023

Paired-key Crypto Scheme #1705

Paired-key Crypto Scheme #1705

Conversation

drskalman commented Sep 25, 2023

drskalman commented Sep 27, 2023

davxy left a comment

Choose a reason for hiding this comment

drskalman commented Oct 9, 2023

drskalman commented Oct 9, 2023

drskalman commented Oct 10, 2023

drskalman commented Oct 10, 2023

davxy left a comment

Choose a reason for hiding this comment

davxy Oct 11, 2023

Choose a reason for hiding this comment

Lederstrumpf Oct 11, 2023

Choose a reason for hiding this comment

drskalman Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Lederstrumpf left a comment

Choose a reason for hiding this comment

acatangiu commented Oct 13, 2023

davxy commented Oct 13, 2023

Polkadot-Forum commented Oct 17, 2023

Polkadot-Forum commented Nov 28, 2023

drskalman Oct 11, 2023 •

edited

Loading