Skip to content

Commit

Permalink
BFT-457: Remove secp256k1::AggregateSignature
Browse files Browse the repository at this point in the history
  • Loading branch information
aakoshh committed Jun 12, 2024
1 parent ce1f480 commit 8a1f741
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 157 deletions.
90 changes: 0 additions & 90 deletions node/libs/crypto/src/secp256k1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,96 +175,6 @@ impl Ord for Signature {
}
}

/// Seclp256k1 multi-sig
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Default)]
pub struct AggregateSignature(Vec<Signature>);

impl AggregateSignature {
/// Add a signature to the aggregate.
pub fn add(&mut self, sig: Signature) {
self.0.push(sig)
}

/// Verifies an aggregated signature for multiple messages against the provided list of public keys.
///
/// Verification fails if the total number of messages and signatures do not match.
///
/// This method was added to mimic the bn254 version, which used BLS signature aggregation and did
/// not rely on message and key ordering; it supported different messages from each signatory as well.
///
/// Here we have a simple list of signatures, and we cannot rely on them having been added in the same
/// order in which they are going to be verified, therefore we have to try every message against every
/// signature.
///
/// The method assumes that there are no repeated pairs in the input, ie. that every signature is used exactly once.
///
/// The method inherited its non-generic never-inlined nature from the bn254 version, which was marked as such
/// to allow the crate level optimisations to take place, which in turn were required for tests to run fast.
#[inline(never)]
pub fn verify_hash(&self, hashes_and_pks: &[(&[u8], &PublicKey)]) -> anyhow::Result<()> {
if hashes_and_pks.len() != self.0.len() {
bail!(
"wrong number of inputs vs signatures: expected {}, got {}",
self.0.len(),
hashes_and_pks.len()
);
}
// Keep track of which signatures have been verified.
let mut verified = vec![false; self.0.len()];

'inputs: for (i, (hash, pk)) in hashes_and_pks.iter().enumerate() {
'signatures: for (j, sig) in self.0.iter().enumerate() {
if verified[j] {
continue 'signatures;
}
if sig.verify_hash(hash, pk).is_ok() {
verified[j] = true;
continue 'inputs;
}
}
bail!("failed to verify message {i} against any of the signatures in the aggregate");
}
Ok(())
}

/// Verify messages after hashing them with Keccak256
pub fn verify<'a>(
&self,
msgs_and_pks: impl Iterator<Item = (&'a [u8], &'a PublicKey)>,
) -> anyhow::Result<()> {
let hashes_and_pks = msgs_and_pks
.map(|(msg, pk)| (Keccak256::new(msg), pk))
.collect::<Vec<_>>();

let hashes_and_pks = hashes_and_pks
.iter()
.map(|(hash, pk)| (hash.as_bytes().as_slice(), *pk))
.collect::<Vec<_>>();

self.verify_hash(&hashes_and_pks)
}
}

impl ByteFmt for AggregateSignature {
fn decode(bytes: &[u8]) -> anyhow::Result<Self> {
anyhow::ensure!(
bytes.len() % SIGNATURE_LENGTH == 0,
"unexpected aggregate signature length"
);

let sigs = bytes
.chunks_exact(SIGNATURE_LENGTH)
.map(Signature::decode)
.collect::<Result<Vec<_>, _>>()?;

Ok(Self(sigs))
}

fn encode(&self) -> Vec<u8> {
self.0.iter().flat_map(|s| s.encode()).collect()
}
}

/// Normalize the V in signatures from Ethereum tooling.
///
/// Based on <https://github.com/gakonst/ethers-rs/blob/51fe937f6515689b17a3a83b74a05984ad3a7f11/ethers-core/src/types/signature.rs#L202>
Expand Down
12 changes: 1 addition & 11 deletions node/libs/crypto/src/secp256k1/testonly.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::secp256k1::{AggregateSignature, PublicKey, SecretKey, Signature};
use crate::secp256k1::{PublicKey, SecretKey, Signature};
use elliptic_curve::ScalarPrimitive;
use k256::ecdsa::RecoveryId;
use rand::{
Expand Down Expand Up @@ -50,13 +50,3 @@ impl Distribution<Signature> for Standard {
Signature { sig, recid }
}
}

impl Distribution<AggregateSignature> for Standard {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> AggregateSignature {
let mut agg = AggregateSignature::default();
for _ in 0..rng.gen_range(1..=5) {
agg.add(rng.gen());
}
agg
}
}
56 changes: 0 additions & 56 deletions node/libs/crypto/src/secp256k1/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::fmt::Debug;
use rand::{
distributions::{Distribution, Standard},
rngs::StdRng,
seq::SliceRandom,
Rng, SeedableRng,
};

Expand Down Expand Up @@ -59,11 +58,6 @@ fn prop_sig_format() {
prop_byte_format::<Signature>();
}

#[test]
fn prop_aggsig_format() {
prop_byte_format::<AggregateSignature>();
}

#[test]
fn prop_sign_verify() {
let rng = &mut make_rng();
Expand Down Expand Up @@ -103,56 +97,6 @@ fn prop_sign_verify_wrong_msg_fail() {
}
}

#[test]
fn prop_sign_verify_agg() {
let rng = &mut make_rng();

for _ in 0..10 {
let mut agg = AggregateSignature::default();
let mut inputs = Vec::new();
for _ in 0..rng.gen_range(0..5) {
let sk = rng.gen::<SecretKey>();
let msg = gen_msg(rng);
let sig = sk.sign(&msg).unwrap();
agg.add(sig);
inputs.push((msg, sk.public()));
}

// Verification should work for any order of messages and signatures.
agg.0.shuffle(rng);
inputs.shuffle(rng);

agg.verify(inputs.iter().map(|(msg, pk)| (msg.as_slice(), pk)))
.unwrap();
}
}

#[test]
fn prop_sign_verify_agg_fail() {
let rng = &mut make_rng();

for _ in 0..10 {
let mut agg = AggregateSignature::default();
let mut inputs = Vec::new();
// Minimum two signatures so that we can pop something.
for _ in 0..=rng.gen_range(2..5) {
let sk = rng.gen::<SecretKey>();
let msg = gen_msg(rng);
let sig = sk.sign(&msg).unwrap();
agg.add(sig);
inputs.push((msg, sk.public()));
}
// Do something to mess it up.
if rng.gen_bool(0.5) {
inputs.pop();
} else {
agg.0.pop();
}
agg.verify(inputs.iter().map(|(msg, pk)| (msg.as_slice(), pk)))
.unwrap_err();
}
}

/// Test vectors from <https://web3js.readthedocs.io/en/v1.2.0/web3-eth-accounts.html#eth-accounts-signtransaction>
///
/// The same example has been used in <https://github.com/gakonst/ethers-rs/blob/ba00f549/ethers-signers/src/wallet/private_key.rs#L197>
Expand Down

0 comments on commit 8a1f741

Please sign in to comment.