This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Make BEEFY client able to generate either ECDSA or (ECDSA,BLS) Finality proof. #13311
cargo fmt beefy primitives
- Loading branch information
commit 42c894b0079090781b42111282cbf29037d9826e
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| // This file is part of Substrate. | ||
|
|
||
| // Copyright (C) 2021-2022 Parity Technologies (UK) Ltd. | ||
|
|
@@ -66,8 +65,7 @@ impl<TBlockNumber, TAggregatedSignature> | |
| aggregator: TSignatureAggregator, | ||
| ) -> (Self, Vec<Option<TSignature>>) | ||
| where | ||
| TSignatureAggregator: FnOnce(&[Option<TSignature>]) -> TAggregatedSignature, | ||
| { | ||
| let SignedCommitment { commitment, signatures } = signed; | ||
| let signed_by = signatures.iter().map(|s| s.is_some()).collect(); | ||
|
|
@@ -86,23 +84,25 @@ mod tests { | |
| use super::*; | ||
| use codec::Decode; | ||
|
|
||
| use crate::{ | ||
| bls_crypto::Signature as BLSSignature, ecdsa_crypto, known_payloads, Payload, KEY_TYPE, | ||
| }; | ||
| use bls_like::{ | ||
| pop::SignatureAggregatorAssumingPoP, EngineBLS, SerializableToBytes, Signed, BLS377, | ||
| }; | ||
|
There was a problem hiding this comment. I see We do not want to hash-to-G2. We want to both hash-to-G1 and also run apk proofs on G1. This requires that public keys be on both G1 and G2 and contain a DLEQ proof between them. This is the point of https://eprint.iacr.org/2022/1611 |
||
|
|
||
| type TestCommitment = Commitment<u128>; | ||
|
|
||
| ///types for ecdsa signed commitment | ||
| type TestSignedCommitment = SignedCommitment<u128, ecdsa_crypto::Signature>; | ||
| type TestSignedCommitmentWitness = | ||
| SignedCommitmentWitness<u128, Vec<Option<ecdsa_crypto::Signature>>>; | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, codec::Encode, codec::Decode)] | ||
| struct ECDSABLSSignaturePair(ecdsa_crypto::Signature, BLSSignature); | ||
|
|
||
| ///types for commitment containing bls signature along side ecdsa signature | ||
| type TestBLSSignedCommitment = SignedCommitment<u128, ECDSABLSSignaturePair>; | ||
| type TestBLSSignedCommitmentWitness = | ||
| SignedCommitmentWitness<u128, [u8; BLS377::SIGNATURE_SERIALIZED_SIZE]>; | ||
|
|
||
|
|
@@ -130,7 +130,7 @@ mod tests { | |
|
|
||
| ///generates mock aggregatable bls signature for generating test commitment | ||
| ///BLS signatures | ||
| fn mock_bls_signatures() -> (BLSSignature, BLSSignature) { | ||
| let store: SyncCryptoStorePtr = KeyStore::new().into(); | ||
|
|
||
| let mut alice = sp_core::bls::Pair::from_string("//Alice", None).unwrap(); | ||
|
|
@@ -157,10 +157,7 @@ mod tests { | |
|
|
||
| let sigs = mock_ecdsa_signatures(); | ||
|
|
||
| SignedCommitment { commitment, signatures: vec![None, None, Some(sigs.0), Some(sigs.1)] } | ||
| } | ||
|
|
||
| fn ecdsa_and_bls_signed_commitment() -> TestBLSSignedCommitment { | ||
|
|
@@ -174,8 +171,12 @@ mod tests { | |
|
|
||
| SignedCommitment { | ||
| commitment, | ||
| signatures: vec![ | ||
| None, | ||
| None, | ||
| Some(ECDSABLSSignaturePair(ecdsa_sigs.0, bls_sigs.0)), | ||
| Some(ECDSABLSSignaturePair(ecdsa_sigs.1, bls_sigs.1)), | ||
| ], | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -185,12 +186,11 @@ mod tests { | |
| let signed = ecdsa_signed_commitment(); | ||
|
|
||
| // when | ||
| let (witness, signatures) = TestSignedCommitmentWitness::from_signed::< | ||
| _, | ||
| _, | ||
| Vec<Option<ecdsa_crypto::Signature>>, | ||
| >(signed, |sigs| sigs.to_vec()); | ||
|
|
||
| // then | ||
| assert_eq!(witness.aggregated_signature, signatures); | ||
|
|
@@ -202,30 +202,45 @@ mod tests { | |
| let signed = ecdsa_and_bls_signed_commitment(); | ||
|
|
||
| // when | ||
| let (witness, signatures) = TestBLSSignedCommitmentWitness::from_signed::< | ||
| _, | ||
| _, | ||
| [u8; BLS377::SIGNATURE_SERIALIZED_SIZE], | ||
| >(signed, |sigs| { | ||
| //we are going to aggregate the signatures here | ||
| let mut aggregatedsigs: SignatureAggregatorAssumingPoP<BLS377> = | ||
| SignatureAggregatorAssumingPoP::new(); | ||
| sigs.iter().filter_map(|sig| { | ||
|
Contributor
Author
There was a problem hiding this comment. not only I assumed the commitment is the aggregated signature, I tought is should be computed only by the prover not each validator. I think @AlistairStewart says that this is the commitment and it should be computed and sign by all validators:
Contributor
Author
There was a problem hiding this comment. I added the commitment to the set of public keys here: https://github.com/paritytech/substrate/pull/13311/files#diff-90d0377841064e4bcca6f0956bf16bd71e11159f9dd6d5975b91f2b4d901b0a4R200 It is not clear for me where it should be actually computed and updated. |
||
| sig.clone().map(|sig| { | ||
| aggregatedsigs.add_signature( | ||
| &(bls_like::Signature::from_bytes( | ||
| <BLSSignature as AsRef<[u8]>>::as_ref(&sig.1.clone()) | ||
| .try_into() | ||
| .unwrap(), | ||
| )) | ||
| .unwrap(), | ||
| ) | ||
| }) | ||
| }); | ||
| (&aggregatedsigs).signature().to_bytes() | ||
| }); | ||
| // then | ||
| BLSSignature::try_from(witness.aggregated_signature.as_slice()).unwrap(); | ||
| //, signatures.iter().filter_map(|sig| sig.map(|sig| | ||
| //, (bls_like::Signature::<BLS377>::from_bytes(<BLSSignature as | ||
| //, AsRef<[u8]>>::as_ref(&sig.1).try_into().unwrap())).unwrap())). | ||
| //, collect::<Vec<bls_like::Signature::<BLS377>>>().iter().sum()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_encode_and_decode_witness() { | ||
| // given | ||
| let signed = ecdsa_signed_commitment(); | ||
| let (witness, _) = | ||
| TestSignedCommitmentWitness::from_signed::<_, _, Vec<Option<ecdsa_crypto::Signature>>>( | ||
| signed, | ||
| |sigs: &[std::option::Option<ecdsa_crypto::Signature>]| sigs.to_vec(), | ||
| ); | ||
|
|
||
| // when | ||
| let encoded = codec::Encode::encode(&witness); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: concatenation ECDSABLS is hard to decipher. I'd either go with
ECDSAnBLS/BLSnECDSA, as you've done here and here, or break style convention with#[allow(non_camel_case_types)]&BLS_ECDSA_SignaturePair, which I find useful for crypto acronyms.