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.
This commit is contained in:
Bastian Köcher
2020-07-12 15:43:47 +02:00
committed by GitHub
parent c03f1743c8
commit 8ab23fafdc
3 changed files with 109 additions and 130 deletions
+4 -2
View File
@@ -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.");
}
+30 -44
View File
@@ -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 {
+75 -84
View File
@@ -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::<VerificationExt>().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::<VerificationExt>().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::<VerificationExt>().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::<VerificationExt>().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::<VerificationExt>().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::<VerificationExt>().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::<VerificationExt>()).is_some());
ext.execute_with(|| {
crypto::finish_batch_verify();
assert!(crypto::finish_batch_verify());
});
assert!(ext.extensions().get_mut(TypeId::of::<VerificationExt>()).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(),