From ce81c01c4f45063320a18c3360c30ab5b5cd764c Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Fri, 26 Mar 2021 10:33:54 -0400 Subject: [PATCH 1/8] Ed25519 async batch verification for JoinSplit signatures We've been verifying JoinSplitSigs one-by-one pre-ZIP-215. Now as we're post-ZIP-215, we can take advantage of the batch math to validate this signatures. I would have pumped all the joinsplits in our MAINNET_BLOCKS test vectors but these signatures are over the sighash, which needs the NU code to compute, and once we're doing all that set up, we're basically doing transaction validation, so. Resolves #1944 --- Cargo.lock | 18 ++- zebra-chain/Cargo.toml | 3 +- zebra-consensus/src/primitives.rs | 1 + zebra-consensus/src/primitives/ed25519.rs | 127 ++++++++++++++++++ .../src/primitives/ed25519/tests.rs | 72 ++++++++++ zebra-consensus/src/primitives/groth16.rs | 12 +- .../src/primitives/groth16/tests.rs | 4 +- zebra-consensus/src/primitives/redjubjub.rs | 12 +- zebra-consensus/src/transaction.rs | 21 ++- zebra-consensus/src/transaction/check.rs | 19 +-- 10 files changed, 262 insertions(+), 27 deletions(-) create mode 100644 zebra-consensus/src/primitives/ed25519.rs create mode 100644 zebra-consensus/src/primitives/ed25519/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 66de6c7b2a1..d5a03cc9bff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -935,6 +935,20 @@ dependencies = [ "thiserror", ] +[[package]] +name = "ed25519-zebra" +version = "2.2.0" +source = "git+https://github.com/ZcashFoundation/ed25519-zebra?rev=856c96500125e8dd38a525dad13dc64a0ac672cc#856c96500125e8dd38a525dad13dc64a0ac672cc" +dependencies = [ + "curve25519-dalek", + "hex", + "rand_core 0.6.2", + "serde", + "sha2", + "thiserror", + "zeroize", +] + [[package]] name = "either" version = "1.6.1" @@ -3393,7 +3407,7 @@ name = "tower-batch" version = "0.2.3" dependencies = [ "color-eyre", - "ed25519-zebra", + "ed25519-zebra 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.3.13", "futures-core", "pin-project 0.4.27", @@ -4050,7 +4064,7 @@ dependencies = [ "chrono", "color-eyre", "displaydoc", - "ed25519-zebra", + "ed25519-zebra 2.2.0 (git+https://github.com/ZcashFoundation/ed25519-zebra?rev=856c96500125e8dd38a525dad13dc64a0ac672cc)", "equihash", "futures 0.3.13", "hex", diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index 79e70536617..e786f1aa0d2 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -39,7 +39,8 @@ proptest-derive = { version = "0.3.0", optional = true } # ZF deps displaydoc = "0.2.0" -ed25519-zebra = "2" +# ed25519-zebra = "2" +ed25519-zebra = {git = "https://github.com/ZcashFoundation/ed25519-zebra", rev = "856c96500125e8dd38a525dad13dc64a0ac672cc"} equihash = "0.1" #redjubjub = "0.2" redjubjub = {git = "https://github.com/ZcashFoundation/redjubjub", rev = "8101eaff1cb2fca45334f77a65caa4c46e3d545b"} diff --git a/zebra-consensus/src/primitives.rs b/zebra-consensus/src/primitives.rs index 8fa545d7bbc..700b99e36c6 100644 --- a/zebra-consensus/src/primitives.rs +++ b/zebra-consensus/src/primitives.rs @@ -1,5 +1,6 @@ //! Asynchronous verification of cryptographic primitives. +pub mod ed25519; pub mod groth16; pub mod redjubjub; diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs new file mode 100644 index 00000000000..62b2ecfa565 --- /dev/null +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -0,0 +1,127 @@ +//! Async Ed25519 batch verifier service + +#[cfg(test)] +mod tests; + +use std::{ + future::Future, + mem, + pin::Pin, + task::{Context, Poll}, +}; + +use futures::future::{ready, Ready}; +use once_cell::sync::Lazy; +use rand::thread_rng; + +use tokio::sync::broadcast::{channel, error::RecvError, Sender}; +use tower::{util::ServiceFn, Service}; +use tower_batch::{Batch, BatchControl}; +use tower_fallback::Fallback; +use zebra_chain::primitives::ed25519::{batch, *}; + +/// Global batch verification context for Ed25519 signatures. +/// +/// This service transparently batches contemporaneous signature verifications, +/// handling batch failures by falling back to individual verification. +/// +/// Note that making a `Service` call requires mutable access to the service, so +/// you should call `.clone()` on the global handle to create a local, mutable +/// handle. +pub static VERIFIER: Lazy< + Fallback, ServiceFn Ready>>>, +> = Lazy::new(|| { + Fallback::new( + Batch::new( + Verifier::default(), + super::MAX_BATCH_SIZE, + super::MAX_BATCH_LATENCY, + ), + // We want to fallback to individual verification if batch verification + // fails, so we need a Service to use. The obvious way to do this would + // be to write a closure that returns an async block. But because we + // have to specify the type of a static, we need to be able to write the + // type of the closure and its return value, and both closures and async + // blocks have eldritch types whose names cannot be written. So instead, + // we use a Ready to avoid an async block and cast the closure to a + // function (which is possible because it doesn't capture any state). + tower::service_fn((|item: Item| ready(item.verify_single())) as fn(_) -> _), + ) +}); + +/// Ed25519 signature verifier service +pub struct Verifier { + batch: batch::Verifier, + // This uses a "broadcast" channel, which is an mpmc channel. Tokio also + // provides a spmc channel, "watch", but it only keeps the latest value, so + // using it would require thinking through whether it was possible for + // results from one batch to be mixed with another. + tx: Sender>, +} + +impl Default for Verifier { + fn default() -> Self { + let batch = batch::Verifier::default(); + let (tx, _) = channel(super::BROADCAST_BUFFER_SIZE); + Self { batch, tx } + } +} + +/// Type alias to clarify that this `batch::Item` is a `Ed25519Item` +pub type Item = batch::Item; + +impl Service> for Verifier { + type Response = (); + type Error = Error; + type Future = Pin> + Send + 'static>>; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, req: BatchControl) -> Self::Future { + match req { + BatchControl::Item(item) => { + tracing::trace!("got item"); + self.batch.queue(item); + let mut rx = self.tx.subscribe(); + Box::pin(async move { + match rx.recv().await { + Ok(result) => { + if result.is_ok() { + tracing::trace!(?result, "validated ed25519 signature"); + metrics::counter!("signatures.ed25519.validated", 1); + } else { + tracing::trace!(?result, "invalid ed25519 signature"); + metrics::counter!("signatures.ed25519.invalid", 1); + } + result + } + Err(RecvError::Lagged(_)) => { + tracing::error!( + "batch verification receiver lagged and lost verification results" + ); + Err(Error::InvalidSignature) + } + Err(RecvError::Closed) => panic!("verifier was dropped without flushing"), + } + }) + } + + BatchControl::Flush => { + tracing::trace!("got flush command"); + let batch = mem::take(&mut self.batch); + let _ = self.tx.send(batch.verify(thread_rng())); + Box::pin(async { Ok(()) }) + } + } + } +} + +impl Drop for Verifier { + fn drop(&mut self) { + // We need to flush the current batch in case there are still any pending futures. + let batch = mem::take(&mut self.batch); + let _ = self.tx.send(batch.verify(thread_rng())); + } +} diff --git a/zebra-consensus/src/primitives/ed25519/tests.rs b/zebra-consensus/src/primitives/ed25519/tests.rs new file mode 100644 index 00000000000..471b875622f --- /dev/null +++ b/zebra-consensus/src/primitives/ed25519/tests.rs @@ -0,0 +1,72 @@ +//! Tests for Ed25519 signature verification + +use std::time::Duration; + +use color_eyre::eyre::{eyre, Result}; +use futures::stream::{FuturesUnordered, StreamExt}; +use tower::ServiceExt; +use tower_batch::Batch; + +use crate::primitives::ed25519::*; + +async fn sign_and_verify(mut verifier: V, n: usize) -> Result<(), V::Error> +where + V: Service, +{ + let mut rng = thread_rng(); + let mut results = FuturesUnordered::new(); + for i in 0..n { + let span = tracing::trace_span!("sig", i); + let msg = b"BatchVerifyTest"; + + let sk = SigningKey::new(&mut rng); + let vk = VerificationKey::from(&sk); + let sig = sk.sign(&msg[..]); + verifier.ready_and().await?; + results.push(span.in_scope(|| verifier.call((vk.into(), sig, msg).into()))) + } + + while let Some(result) = results.next().await { + result?; + } + + Ok(()) +} + +#[tokio::test] +async fn batch_flushes_on_max_items_test() -> Result<()> { + batch_flushes_on_max_items().await +} + +#[spandoc::spandoc] +async fn batch_flushes_on_max_items() -> Result<()> { + use tokio::time::timeout; + + // Use a very long max_latency and a short timeout to check that + // flushing is happening based on hitting max_items. + let verifier = Batch::new(Verifier::default(), 10, Duration::from_secs(1000)); + timeout(Duration::from_secs(5), sign_and_verify(verifier, 100)) + .await? + .map_err(|e| eyre!(e))?; + + Ok(()) +} + +#[tokio::test] +async fn batch_flushes_on_max_latency_test() -> Result<()> { + batch_flushes_on_max_latency().await +} + +#[spandoc::spandoc] +async fn batch_flushes_on_max_latency() -> Result<()> { + use tokio::time::timeout; + + // Use a very high max_items and a short timeout to check that + // flushing is happening based on hitting max_latency. + let verifier = Batch::new(Verifier::default(), 100, Duration::from_millis(500)); + timeout(Duration::from_secs(5), sign_and_verify(verifier, 10)) + .await? + .map_err(|e| eyre!(e))?; + + Ok(()) +} diff --git a/zebra-consensus/src/primitives/groth16.rs b/zebra-consensus/src/primitives/groth16.rs index 0d4ad1d091b..9e9f91ed953 100644 --- a/zebra-consensus/src/primitives/groth16.rs +++ b/zebra-consensus/src/primitives/groth16.rs @@ -175,7 +175,17 @@ impl Service> for Verifier { let mut rx = self.tx.subscribe(); Box::pin(async move { match rx.recv().await { - Ok(result) => result, + Ok(result) => { + if result.is_ok() { + tracing::trace!(?result, "verified groth16 proof"); + metrics::counter!("proofs.groth16.verified", 1); + } else { + tracing::trace!(?result, "invalid groth16 proof"); + metrics::counter!("proofs.groth16.invalid", 1); + } + + result + } Err(RecvError::Lagged(_)) => { tracing::error!( "missed channel updates, BROADCAST_BUFFER_SIZE is too low!!" diff --git a/zebra-consensus/src/primitives/groth16/tests.rs b/zebra-consensus/src/primitives/groth16/tests.rs index fb79b8475a5..1496bd8684e 100644 --- a/zebra-consensus/src/primitives/groth16/tests.rs +++ b/zebra-consensus/src/primitives/groth16/tests.rs @@ -5,9 +5,7 @@ use tower::ServiceExt; use zebra_chain::{block::Block, serialization::ZcashDeserializeInto, transaction::Transaction}; -use crate::primitives::groth16; - -use super::*; +use crate::primitives::groth16::{self, *}; async fn verify_groth16_spends_and_outputs( spend_verifier: &mut V, diff --git a/zebra-consensus/src/primitives/redjubjub.rs b/zebra-consensus/src/primitives/redjubjub.rs index d3750c5c2b1..d965ecb8887 100644 --- a/zebra-consensus/src/primitives/redjubjub.rs +++ b/zebra-consensus/src/primitives/redjubjub.rs @@ -87,7 +87,17 @@ impl Service> for Verifier { let mut rx = self.tx.subscribe(); Box::pin(async move { match rx.recv().await { - Ok(result) => result, + Ok(result) => { + if result.is_ok() { + tracing::trace!(?result, "validated redjubjub signature"); + metrics::counter!("signatures.redjubjub.validated", 1); + } else { + tracing::trace!(?result, "invalid redjubjub signature"); + metrics::counter!("signatures.redjubjub.invalid", 1); + } + + result + } Err(RecvError::Lagged(_)) => { tracing::error!( "batch verification receiver lagged and lost verification results" diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 6bc7dd6f41f..82e7dc0517e 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -129,6 +129,7 @@ where let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone(); let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone(); + let mut ed25519_verifier = primitives::ed25519::VERIFIER.clone(); let mut redjubjub_verifier = primitives::redjubjub::VERIFIER.clone(); let mut script_verifier = self.script_verifier.clone(); @@ -192,7 +193,25 @@ where // correctly. // Then, pass those items to self.joinsplit to verify them. - check::validate_joinsplit_sig(joinsplit_data, shielded_sighash.as_bytes())?; + // check::validate_joinsplit_sig(joinsplit_data, shielded_sighash.as_bytes())?; + + // Consensus rule: The joinSplitSig MUST represent a + // valid signature, under joinSplitPubKey, of the + // sighash. + // + // Queue the validation of the JoinSplit signature while + // adding the resulting future to our collection of + // async checks that (at a minimum) must pass for the + // transaction to verify. + // + // https://zips.z.cash/protocol/protocol.pdf#sproutnonmalleability + // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus + let rsp = ed25519_verifier + .ready_and() + .await? + .call((joinsplit_data.pub_key, joinsplit_data.sig, &shielded_sighash).into()); + + async_checks.push(rsp.boxed()); } if let Some(shielded_data) = shielded_data { diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 084b04af957..9f1db1ae8ce 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -2,31 +2,14 @@ //! //! Code in this file can freely assume that no pre-V4 transactions are present. -use std::convert::TryFrom; - use zebra_chain::{ amount::Amount, - primitives::{ed25519, Groth16Proof}, sapling::{Output, Spend}, - transaction::{JoinSplitData, ShieldedData, Transaction}, + transaction::{ShieldedData, Transaction}, }; use crate::error::TransactionError; -/// Validate the JoinSplit binding signature. -/// -/// https://zips.z.cash/protocol/protocol.pdf#sproutnonmalleability -/// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus -pub fn validate_joinsplit_sig( - joinsplit_data: &JoinSplitData, - sighash: &[u8], -) -> Result<(), TransactionError> { - // TODO: batch verify ed25519: https://github.com/ZcashFoundation/zebra/issues/1944 - ed25519::VerificationKey::try_from(joinsplit_data.pub_key) - .and_then(|vk| vk.verify(&joinsplit_data.sig, sighash)) - .map_err(TransactionError::Ed25519) -} - /// Checks that the transaction has inputs and outputs. /// /// More specifically: From 6cc575c074ce7982ea26213c3bed65cbdece9a2d Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Sun, 28 Mar 2021 10:15:24 -0400 Subject: [PATCH 2/8] Update zebra-consensus/src/transaction.rs Co-authored-by: Alfredo Garcia --- zebra-consensus/src/transaction.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 82e7dc0517e..2b597b51efc 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -193,7 +193,6 @@ where // correctly. // Then, pass those items to self.joinsplit to verify them. - // check::validate_joinsplit_sig(joinsplit_data, shielded_sighash.as_bytes())?; // Consensus rule: The joinSplitSig MUST represent a // valid signature, under joinSplitPubKey, of the From 6e4982c0456e3c95271ff49c5bf8d641456e57d5 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 11:22:26 -0400 Subject: [PATCH 3/8] Repoint to latest ed25519-zebra commit with note to point at 3.0 when released --- zebra-chain/Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index e786f1aa0d2..976ab6097f3 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -39,8 +39,8 @@ proptest-derive = { version = "0.3.0", optional = true } # ZF deps displaydoc = "0.2.0" -# ed25519-zebra = "2" -ed25519-zebra = {git = "https://github.com/ZcashFoundation/ed25519-zebra", rev = "856c96500125e8dd38a525dad13dc64a0ac672cc"} +# TODO: upgrade ed25510-zebra to 3 when released: https://github.com/ZcashFoundation/ed25519-zebra/issues/45 +ed25519-zebra = {git = "https://github.com/ZcashFoundation/ed25519-zebra", rev = "539fad040c443302775b0f508e616418825e6c22"} equihash = "0.1" #redjubjub = "0.2" redjubjub = {git = "https://github.com/ZcashFoundation/redjubjub", rev = "8101eaff1cb2fca45334f77a65caa4c46e3d545b"} From 182e25d39d70441b3b435c995afac78a376717bb Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 11:40:05 -0400 Subject: [PATCH 4/8] Update zebra-consensus/src/primitives/ed25519.rs Co-authored-by: Alfredo Garcia --- zebra-consensus/src/primitives/ed25519.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs index 62b2ecfa565..b2fd0cdcec0 100644 --- a/zebra-consensus/src/primitives/ed25519.rs +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -82,7 +82,7 @@ impl Service> for Verifier { fn call(&mut self, req: BatchControl) -> Self::Future { match req { BatchControl::Item(item) => { - tracing::trace!("got item"); + tracing::trace!("got ed25519 item"); self.batch.queue(item); let mut rx = self.tx.subscribe(); Box::pin(async move { From 60e99a965e966d1cf60082532bda336e9a211fd9 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 11:40:39 -0400 Subject: [PATCH 5/8] Update zebra-consensus/src/primitives/ed25519.rs Co-authored-by: teor --- zebra-consensus/src/primitives/ed25519.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs index b2fd0cdcec0..1c195538b23 100644 --- a/zebra-consensus/src/primitives/ed25519.rs +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -99,7 +99,7 @@ impl Service> for Verifier { } Err(RecvError::Lagged(_)) => { tracing::error!( - "batch verification receiver lagged and lost verification results" + "ed25519 batch verification receiver lagged and lost verification results" ); Err(Error::InvalidSignature) } From e389c28a2a9fee67ef85ef2ce5b9c3fd9fd16b28 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 11:40:45 -0400 Subject: [PATCH 6/8] Update zebra-consensus/src/primitives/ed25519.rs Co-authored-by: teor --- zebra-consensus/src/primitives/ed25519.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs index 1c195538b23..7e4be74d29b 100644 --- a/zebra-consensus/src/primitives/ed25519.rs +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -103,7 +103,7 @@ impl Service> for Verifier { ); Err(Error::InvalidSignature) } - Err(RecvError::Closed) => panic!("verifier was dropped without flushing"), + Err(RecvError::Closed) => panic!("ed25519 verifier was dropped without flushing"), } }) } From bc01a7467d19179a7f82743cbedf6f0fa37f2927 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 11:40:51 -0400 Subject: [PATCH 7/8] Update zebra-consensus/src/primitives/ed25519.rs Co-authored-by: Alfredo Garcia --- zebra-consensus/src/primitives/ed25519.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs index 7e4be74d29b..39eae165310 100644 --- a/zebra-consensus/src/primitives/ed25519.rs +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -109,7 +109,7 @@ impl Service> for Verifier { } BatchControl::Flush => { - tracing::trace!("got flush command"); + tracing::trace!("got ed25519 flush command"); let batch = mem::take(&mut self.batch); let _ = self.tx.send(batch.verify(thread_rng())); Box::pin(async { Ok(()) }) From 742f19312180fa0e9effd4e30e6f8a41210a3530 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Tue, 30 Mar 2021 16:16:37 -0400 Subject: [PATCH 8/8] format --- zebra-consensus/src/primitives/ed25519.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra-consensus/src/primitives/ed25519.rs b/zebra-consensus/src/primitives/ed25519.rs index 39eae165310..82f7b988630 100644 --- a/zebra-consensus/src/primitives/ed25519.rs +++ b/zebra-consensus/src/primitives/ed25519.rs @@ -103,7 +103,9 @@ impl Service> for Verifier { ); Err(Error::InvalidSignature) } - Err(RecvError::Closed) => panic!("ed25519 verifier was dropped without flushing"), + Err(RecvError::Closed) => { + panic!("ed25519 verifier was dropped without flushing") + } } }) }