Make signature batching use specialized methods (paritytech#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.
KILTprotocol · Jul 12, 2020 · b0eefeb · b0eefeb
1 parent b5280cf
commit b0eefeb
Show file tree

Hide file tree

Showing 3 changed files with 109 additions and 130 deletions.
diff --git 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
@@ -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<u8>,
 	) -> 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<u8>,
 	) -> 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<Sr25519BatchItem>) -> bool {