Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions primitives/io/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1402,6 +1402,43 @@ mod tests {
);

assert!(!crypto::finish_batch_verify());

// 1 valid ecdsa signature
crypto::start_batch_verify();

let pair = ecdsa::Pair::generate_with_phrase(None).0;
let msg = b"ECDSA!!1";
let signature = pair.sign(msg);
crypto::ecdsa_batch_verify(&signature, msg, &pair.public());

assert!(crypto::finish_batch_verify());

// 1 invalid ecdsa signature
crypto::start_batch_verify();

crypto::ecdsa_batch_verify(
&Default::default(),
&Vec::new(),
&Default::default(),
);

assert!(!crypto::finish_batch_verify());

// 1 valid, 1 invalid ecdsa signature
crypto::start_batch_verify();

let pair = ecdsa::Pair::generate_with_phrase(None).0;
let msg = b"ECDSA!!1";
let signature = pair.sign(msg);
crypto::ecdsa_batch_verify(&signature, msg, &pair.public());

crypto::ecdsa_batch_verify(
&Default::default(),
&Vec::new(),
&Default::default(),
);

assert!(!crypto::finish_batch_verify());
});
}
}
4 changes: 2 additions & 2 deletions primitives/runtime/src/generic/unchecked_extrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ for
where
Address: Member + MaybeDisplay,
Call: Encode + Member,
Signature: Member + traits::Verify,
Signature: Member + traits::BatchVerify,
<Signature as traits::Verify>::Signer: IdentifyAccount<AccountId=AccountId>,
Extra: SignedExtension<AccountId=AccountId>,
AccountId: Member + MaybeDisplay,
Expand All @@ -128,7 +128,7 @@ where
Some((signed, signature, extra)) => {
let signed = lookup.lookup(signed)?;
let raw_payload = SignedPayload::new(self.function, extra)?;
if !raw_payload.using_encoded(|payload| signature.verify(payload, &signed)) {
if !raw_payload.using_encoded(|payload| signature.batch_verify(payload, &signed)) {
return Err(InvalidTransaction::BadProof.into())
}

Expand Down
42 changes: 41 additions & 1 deletion primitives/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub use either::Either;
/// bypasses this problem.
pub type Justification = Vec<u8>;

use traits::{Verify, Lazy};
use traits::{Verify, BatchVerify, Lazy};

/// A module identifier. These are per module and should be stored in a registry somewhere.
#[derive(Clone, Copy, Eq, PartialEq, Encode, Decode)]
Expand Down Expand Up @@ -340,6 +340,24 @@ impl Verify for MultiSignature {
}
}

impl BatchVerify for MultiSignature {
fn batch_verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &AccountId32) -> bool {
match (self, signer) {
(MultiSignature::Ed25519(ref sig), who) => sig.batch_verify(msg, &ed25519::Public::from_slice(who.as_ref())),
(MultiSignature::Sr25519(ref sig), who) => sig.batch_verify(msg, &sr25519::Public::from_slice(who.as_ref())),
(MultiSignature::Ecdsa(ref sig), who) => {
let m = sp_io::hashing::blake2_256(msg.get());
match sp_io::crypto::secp256k1_ecdsa_recover_compressed(sig.as_ref(), &m) {
Ok(pubkey) =>
&sp_io::hashing::blake2_256(pubkey.as_ref())
== <dyn AsRef<[u8; 32]>>::as_ref(who),
_ => false,
}
}
}
}
}

/// Signature verify that can work with any known signature types..
#[derive(Eq, PartialEq, Clone, Default, Encode, Decode, RuntimeDebug)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
Expand Down Expand Up @@ -867,6 +885,28 @@ mod tests {
assert!(multi_sig.verify(msg, &multi_signer.into_account()));
}

#[test]
fn ecdsa_batch_verify_works() {
let mut ext = sp_state_machine::BasicExternalities::default();
ext.register_extension(
sp_core::traits::TaskExecutorExt::new(sp_core::testing::TaskExecutor::new()),
);

ext.execute_with(move || {
let msg = &b"test-message"[..];
let (pair, _) = ecdsa::Pair::generate();

let signature = pair.sign(&msg);

let multi_sig = MultiSignature::from(signature.clone());
let multi_signer = MultiSigner::from(pair.public());

let batching = SignatureBatching::start();
assert!(signature.batch_verify(msg, &pair.public()));
assert!(multi_sig.batch_verify(msg, &multi_signer.into_account()));
assert!(batching.verify());
});
}

#[test]
#[should_panic(expected = "Signature verification has not been called")]
Expand Down
8 changes: 7 additions & 1 deletion primitives/runtime/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ impl traits::Verify for TestSignature {
}
}

impl traits::BatchVerify for TestSignature {
fn batch_verify<L: traits::Lazy<[u8]>>(&self, msg: L, signer: &u64) -> bool {
traits::Verify::verify(self, msg, signer)
}
}

/// Digest item
pub type DigestItem = generic::DigestItem<H256>;

Expand All @@ -161,7 +167,7 @@ pub type Digest = generic::Digest<H256>;

/// Block Header
pub type Header = generic::Header<u64, BlakeTwo256>;

impl Header {
/// A new header with the given number and default hash for all other fields.
pub fn new_from_number(number: <Self as traits::Header>::Number) -> Self {
Expand Down
39 changes: 38 additions & 1 deletion primitives/runtime/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,24 @@ pub trait Verify {
type Signer: IdentifyAccount;
/// Verify a signature.
///
/// Return `true` if signature is valid for the value.
/// Should return `true` if signature is valid for the value.
fn verify<L: Lazy<[u8]>>(&self, msg: L, signer: &<Self::Signer as IdentifyAccount>::AccountId) -> bool;
}

/// Means of signature batch verification.
///
/// Should only be used when there is a batching context registered on the host (through instantiating
/// helper struct `SingatureBatching` or via 'crypto::start_batch_verify`).
///
/// If there were at least one call to this function, `crypto::finish_batch_verify` (or `SingatureBatching::verify`)
/// then should be called and code must panic if either returns false.
pub trait BatchVerify: Verify {
/// Verify a signature using available batcher.
///
/// Should return `false` if batching failed or a previous signature already evaluated as invalid.
fn batch_verify<L: Lazy<[u8]>>(&self, msg: L, signer: &<Self::Signer as IdentifyAccount>::AccountId) -> bool;
}

impl Verify for sp_core::ed25519::Signature {
type Signer = sp_core::ed25519::Public;

Expand All @@ -96,6 +110,13 @@ impl Verify for sp_core::ed25519::Signature {
}
}


impl BatchVerify for sp_core::ed25519::Signature {
fn batch_verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &sp_core::ed25519::Public) -> bool {
sp_io::crypto::ed25519_batch_verify(self, msg.get(), signer)
}
}

impl Verify for sp_core::sr25519::Signature {
type Signer = sp_core::sr25519::Public;

Expand All @@ -104,6 +125,12 @@ impl Verify for sp_core::sr25519::Signature {
}
}

impl BatchVerify for sp_core::sr25519::Signature {
fn batch_verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &sp_core::sr25519::Public) -> bool {
sp_io::crypto::sr25519_batch_verify(self, msg.get(), signer)
}
}

impl Verify for sp_core::ecdsa::Signature {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not for ecdsa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ecdsa is a bit tricky, since it dominates the verification (takes much more time compared to transaction execution)

Node can be forced to do much more work before first ecdsa is resolved as invalid

Copy link
Member

@bkchr bkchr Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other signatures probably also take more time to verify than a simple transfer. This feature is ridiculous if you draw the line here. Than we don't need async verification at all. If this would be a problem in the future, we could disable async signature verification when importing blocks on the tip of the chain. However, for syncing we want the best performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, you can trick node to verify 1000 or so heavy signatures for free until first one is resolved and possibly completely stop some validators, not going to add it here until some proof that this is safe provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also force you to verify 3000 sr25519 signatures. Where it the difference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, you basically just want early exit of the scheduled verifications? So that we abort all, if one fails? Should not be that hard to implement, just make the future select on an exit signal.

And ECDSA is also not orders of magnitudes slower, on my machine it was factor 3 slower.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can support Ed25519 batch verification eventually I think, but not worth the time right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ECDSA performance is better than I had impression of, indeed just 3x thanks @bkchr, #6697

So should be no problem adding it also

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevertheless, I like the idea of adding an early bail out for these verification tasks if one failed. This should prevent the problems you have described here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added implementation for standalone ECDSA, but I think the way it works it can't be used in MultiSignature, since it uses AccountId32, which can store only blake2(public), but not public itself

type Signer = sp_core::ecdsa::Public;
fn verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &sp_core::ecdsa::Public) -> bool {
Expand All @@ -117,6 +144,16 @@ impl Verify for sp_core::ecdsa::Signature {
}
}

impl BatchVerify for sp_core::ecdsa::Signature {
fn batch_verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &sp_core::ecdsa::Public) -> bool {
sp_io::crypto::ecdsa_batch_verify(
self,
msg.get(),
signer,
)
}
}

/// Means of signature verification of an application key.
pub trait AppVerify {
/// Type of the signer.
Expand Down