From b0eefebc2a85fe18b5bf072910ad8c4c614548e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 12 Jul 2020 15:43:47 +0200 Subject: [PATCH] Make signature batching use specialized methods (#6616) It was a mistake to use the `*_verify` methods for signature batching. This pr move the signature batching into their own functions. This is required, because otherwise transaction signature verification infers with other signature verifications. This pr also temporarily disables signature batching. The functionality stays, but we need to make sure that all nodes have the new runtime interface, before we can bring back signature batching. --- frame/executive/src/lib.rs | 6 +- primitives/io/src/batch_verifier.rs | 74 ++++++------- primitives/io/src/lib.rs | 159 +++++++++++++--------------- 3 files changed, 109 insertions(+), 130 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index ce765cc8cab87..24dccf8b0b4a4 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -293,11 +293,13 @@ where // any initial checks Self::initial_checks(&block); - let batching_safeguard = sp_runtime::SignatureBatching::start(); + let signature_batching = sp_runtime::SignatureBatching::start(); + // execute extrinsics let (header, extrinsics) = block.deconstruct(); Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); - if !sp_runtime::SignatureBatching::verify(batching_safeguard) { + + if !signature_batching.verify() { panic!("Signature verification failed."); } diff --git a/primitives/io/src/batch_verifier.rs b/primitives/io/src/batch_verifier.rs index 6a78070b38b55..642e77504d00f 100644 --- a/primitives/io/src/batch_verifier.rs +++ b/primitives/io/src/batch_verifier.rs @@ -51,26 +51,39 @@ impl BatchVerifier { } } + /// Spawn a verification task. + /// + /// Returns `false` if there was already an invalid verification or if + /// the verification could not be spawned. fn spawn_verification_task( &mut self, f: impl FnOnce() -> bool + Send + 'static, - ) -> Result<(), ()> { + ) -> bool { // there is already invalid transaction encountered - if self.invalid.load(AtomicOrdering::Relaxed) { return Err(()); } + if self.invalid.load(AtomicOrdering::Relaxed) { return false; } let invalid_clone = self.invalid.clone(); let (sender, receiver) = oneshot::channel(); self.pending_tasks.push(receiver); - self.scheduler.spawn_obj(FutureObj::new(async move { - if !f() { - invalid_clone.store(true, AtomicOrdering::Relaxed); - } - if sender.send(()).is_err() { - // sanity - log::warn!("Verification halted while result was pending"); - invalid_clone.store(true, AtomicOrdering::Relaxed); - } - }.boxed())).map_err(drop) + self.scheduler.spawn_obj(FutureObj::new( + async move { + if !f() { + invalid_clone.store(true, AtomicOrdering::Relaxed); + } + if sender.send(()).is_err() { + // sanity + log::warn!("Verification halted while result was pending"); + invalid_clone.store(true, AtomicOrdering::Relaxed); + } + }.boxed() + )) + .map_err(|_| { + log::debug!( + target: "runtime", + "Batch-verification returns false because failed to spawn background task.", + ) + }) + .is_ok() } /// Push ed25519 signature to verify. @@ -83,17 +96,7 @@ impl BatchVerifier { pub_key: ed25519::Public, message: Vec, ) -> bool { - if self.invalid.load(AtomicOrdering::Relaxed) { return false; } - - if self.spawn_verification_task(move || ed25519::Pair::verify(&signature, &message, &pub_key)).is_err() { - log::debug!( - target: "runtime", - "Batch-verification returns false because failed to spawn background task.", - ); - - return false; - } - true + self.spawn_verification_task(move || ed25519::Pair::verify(&signature, &message, &pub_key)) } /// Push sr25519 signature to verify. @@ -111,17 +114,10 @@ impl BatchVerifier { if self.sr25519_items.len() >= 128 { let items = std::mem::take(&mut self.sr25519_items); - if self.spawn_verification_task(move || Self::verify_sr25519_batch(items)).is_err() { - log::debug!( - target: "runtime", - "Batch-verification returns false because failed to spawn background task.", - ); - - return false; - } + self.spawn_verification_task(move || Self::verify_sr25519_batch(items)) + } else { + true } - - true } /// Push ecdsa signature to verify. @@ -134,17 +130,7 @@ impl BatchVerifier { pub_key: ecdsa::Public, message: Vec, ) -> bool { - if self.invalid.load(AtomicOrdering::Relaxed) { return false; } - - if self.spawn_verification_task(move || ecdsa::Pair::verify(&signature, &message, &pub_key)).is_err() { - log::debug!( - target: "runtime", - "Batch-verification returns false because failed to spawn background task.", - ); - - return false; - } - true + self.spawn_verification_task(move || ecdsa::Pair::verify(&signature, &message, &pub_key)) } fn verify_sr25519_batch(items: Vec) -> bool { diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index dc86ff2d6ecbd..6c99a5c75195b 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -201,7 +201,6 @@ pub trait Storage { /// from within the runtime. #[runtime_interface] pub trait DefaultChildStorage { - /// Get a default child storage value for a given key. /// /// Parameter `storage_key` is the unprefixed location of the root of the child trie in the parent trie. @@ -455,68 +454,63 @@ pub trait Crypto { /// Verify `ed25519` signature. /// - /// Returns `true` when the verification is either successful or batched. - /// If no batching verification extension registered, this will return the result - /// of verification immediately. If batching verification extension is registered - /// caller should call `crypto::finish_batch_verify` to actualy check all submitted - /// signatures. + /// Returns `true` when the verification was successful. fn ed25519_verify( sig: &ed25519::Signature, msg: &[u8], pub_key: &ed25519::Public, ) -> bool { - // TODO: see #5554, this is used outside of externalities context/runtime, thus this manual - // `with_externalities`. - // - // This `with_externalities(..)` block returns Some(Some(result)) if signature verification was successfully - // batched, everything else (Some(None)/None) means it was not batched and needs to be verified. - let evaluated = sp_externalities::with_externalities(|mut instance| - instance.extension::().map( - |extension| extension.push_ed25519( - sig.clone(), - pub_key.clone(), - msg.to_vec(), - ) - ) - ); + ed25519::Pair::verify(sig, msg, pub_key) + } - match evaluated { - Some(Some(val)) => val, - _ => ed25519::Pair::verify(sig, msg, pub_key), - } + /// Register a `ed25519` signature for batch verification. + /// + /// Batch verification must be enabled by calling [`start_batch_verify`]. + /// If batch verification is not enabled, the signature will be verified immediatley. + /// To get the result of the batch verification, [`finish_batch_verify`] + /// needs to be called. + /// + /// Returns `true` when the verification is either successful or batched. + fn ed25519_batch_verify( + &mut self, + sig: &ed25519::Signature, + msg: &[u8], + pub_key: &ed25519::Public, + ) -> bool { + self.extension::().map( + |extension| extension.push_ed25519(sig.clone(), pub_key.clone(), msg.to_vec()) + ).unwrap_or_else(|| ed25519_verify(sig, msg, pub_key)) } /// Verify `sr25519` signature. /// - /// Returns `true` when the verification is either successful or batched. - /// If no batching verification extension registered, this will return the result - /// of verification immediately. If batching verification extension is registered, - /// caller should call `crypto::finish_batch_verify` to actualy check all submitted + /// Returns `true` when the verification was successful. #[version(2)] fn sr25519_verify( sig: &sr25519::Signature, msg: &[u8], pub_key: &sr25519::Public, ) -> bool { - // TODO: see #5554, this is used outside of externalities context/runtime, thus this manual - // `with_externalities`. - // - // This `with_externalities(..)` block returns Some(Some(result)) if signature verification was successfully - // batched, everything else (Some(None)/None) means it was not batched and needs to be verified. - let evaluated = sp_externalities::with_externalities(|mut instance| - instance.extension::().map( - |extension| extension.push_sr25519( - sig.clone(), - pub_key.clone(), - msg.to_vec(), - ) - ) - ); + sr25519::Pair::verify(sig, msg, pub_key) + } - match evaluated { - Some(Some(val)) => val, - _ => sr25519::Pair::verify(sig, msg, pub_key), - } + /// Register a `sr25519` signature for batch verification. + /// + /// Batch verification must be enabled by calling [`start_batch_verify`]. + /// If batch verification is not enabled, the signature will be verified immediatley. + /// To get the result of the batch verification, [`finish_batch_verify`] + /// needs to be called. + /// + /// Returns `true` when the verification is either successful or batched. + fn sr25519_batch_verify( + &mut self, + sig: &sr25519::Signature, + msg: &[u8], + pub_key: &sr25519::Public, + ) -> bool { + self.extension::().map( + |extension| extension.push_sr25519(sig.clone(), pub_key.clone(), msg.to_vec()) + ).unwrap_or_else(|| sr25519_verify(sig, msg, pub_key)) } /// Start verification extension. @@ -639,35 +633,32 @@ pub trait Crypto { /// Verify `ecdsa` signature. /// - /// Returns `true` when the verification is either successful or batched. - /// If no batching verification extension registered, this will return the result - /// of verification immediately. If batching verification extension is registered - /// caller should call `crypto::finish_batch_verify` to actualy check all submitted - /// signatures. + /// Returns `true` when the verification was successful. fn ecdsa_verify( sig: &ecdsa::Signature, msg: &[u8], pub_key: &ecdsa::Public, ) -> bool { - // TODO: see #5554, this is used outside of externalities context/runtime, thus this manual - // `with_externalities`. - // - // This `with_externalities(..)` block returns Some(Some(result)) if signature verification was successfully - // batched, everything else (Some(None)/None) means it was not batched and needs to be verified. - let evaluated = sp_externalities::with_externalities(|mut instance| - instance.extension::().map( - |extension| extension.push_ecdsa( - sig.clone(), - pub_key.clone(), - msg.to_vec(), - ) - ) - ); + ecdsa::Pair::verify(sig, msg, pub_key) + } - match evaluated { - Some(Some(val)) => val, - _ => ecdsa::Pair::verify(sig, msg, pub_key), - } + /// Register a `ecdsa` signature for batch verification. + /// + /// Batch verification must be enabled by calling [`start_batch_verify`]. + /// If batch verification is not enabled, the signature will be verified immediatley. + /// To get the result of the batch verification, [`finish_batch_verify`] + /// needs to be called. + /// + /// Returns `true` when the verification is either successful or batched. + fn ecdsa_batch_verify( + &mut self, + sig: &ecdsa::Signature, + msg: &[u8], + pub_key: &ecdsa::Public, + ) -> bool { + self.extension::().map( + |extension| extension.push_ecdsa(sig.clone(), pub_key.clone(), msg.to_vec()) + ).unwrap_or_else(|| ecdsa_verify(sig, msg, pub_key)) } /// Verify and recover a SECP256k1 ECDSA signature. @@ -1282,7 +1273,7 @@ mod tests { } #[test] - fn dynamic_extensions_work() { + fn batch_verify_start_finish_works() { let mut ext = BasicExternalities::with_tasks_executor(); ext.execute_with(|| { crypto::start_batch_verify(); @@ -1291,7 +1282,7 @@ mod tests { assert!(ext.extensions().get_mut(TypeId::of::()).is_some()); ext.execute_with(|| { - crypto::finish_batch_verify(); + assert!(crypto::finish_batch_verify()); }); assert!(ext.extensions().get_mut(TypeId::of::()).is_none()); @@ -1306,11 +1297,11 @@ mod tests { for it in 0..70 { let msg = format!("Schnorrkel {}!", it); let signature = pair.sign(msg.as_bytes()); - crypto::sr25519_verify(&signature, msg.as_bytes(), &pair.public()); + crypto::sr25519_batch_verify(&signature, msg.as_bytes(), &pair.public()); } // push invlaid - crypto::sr25519_verify( + crypto::sr25519_batch_verify( &Default::default(), &Vec::new(), &Default::default(), @@ -1321,7 +1312,7 @@ mod tests { for it in 0..70 { let msg = format!("Schnorrkel {}!", it); let signature = pair.sign(msg.as_bytes()); - crypto::sr25519_verify(&signature, msg.as_bytes(), &pair.public()); + crypto::sr25519_batch_verify(&signature, msg.as_bytes(), &pair.public()); } assert!(crypto::finish_batch_verify()); }); @@ -1333,7 +1324,7 @@ mod tests { ext.execute_with(|| { // invalid ed25519 signature crypto::start_batch_verify(); - crypto::ed25519_verify( + crypto::ed25519_batch_verify( &Default::default(), &Vec::new(), &Default::default(), @@ -1346,12 +1337,12 @@ mod tests { let pair = ed25519::Pair::generate_with_phrase(None).0; let msg = b"Important message"; let signature = pair.sign(msg); - crypto::ed25519_verify(&signature, msg, &pair.public()); + crypto::ed25519_batch_verify(&signature, msg, &pair.public()); let pair = ed25519::Pair::generate_with_phrase(None).0; let msg = b"Even more important message"; let signature = pair.sign(msg); - crypto::ed25519_verify(&signature, msg, &pair.public()); + crypto::ed25519_batch_verify(&signature, msg, &pair.public()); assert!(crypto::finish_batch_verify()); @@ -1361,9 +1352,9 @@ mod tests { let pair = ed25519::Pair::generate_with_phrase(None).0; let msg = b"Important message"; let signature = pair.sign(msg); - crypto::ed25519_verify(&signature, msg, &pair.public()); + crypto::ed25519_batch_verify(&signature, msg, &pair.public()); - crypto::ed25519_verify( + crypto::ed25519_batch_verify( &Default::default(), &Vec::new(), &Default::default(), @@ -1377,17 +1368,17 @@ mod tests { let pair = ed25519::Pair::generate_with_phrase(None).0; let msg = b"Ed25519 batching"; let signature = pair.sign(msg); - crypto::ed25519_verify(&signature, msg, &pair.public()); + crypto::ed25519_batch_verify(&signature, msg, &pair.public()); let pair = sr25519::Pair::generate_with_phrase(None).0; let msg = b"Schnorrkel rules"; let signature = pair.sign(msg); - crypto::sr25519_verify(&signature, msg, &pair.public()); + crypto::sr25519_batch_verify(&signature, msg, &pair.public()); let pair = sr25519::Pair::generate_with_phrase(None).0; let msg = b"Schnorrkel batches!"; let signature = pair.sign(msg); - crypto::sr25519_verify(&signature, msg, &pair.public()); + crypto::sr25519_batch_verify(&signature, msg, &pair.public()); assert!(crypto::finish_batch_verify()); @@ -1397,9 +1388,9 @@ mod tests { let pair = sr25519::Pair::generate_with_phrase(None).0; let msg = b"Schnorrkcel!"; let signature = pair.sign(msg); - crypto::sr25519_verify(&signature, msg, &pair.public()); + crypto::sr25519_batch_verify(&signature, msg, &pair.public()); - crypto::sr25519_verify( + crypto::sr25519_batch_verify( &Default::default(), &Vec::new(), &Default::default(),