From 07f61bdb9c365c5f51e3fd0254c5ca1b6a186c38 Mon Sep 17 00:00:00 2001 From: Boog900 Date: Fri, 7 Jun 2024 12:34:53 +0000 Subject: [PATCH] Consensus: fix panic in batch verifier (#152) * fix panic in batch verifier * docs * review comments * Update consensus/rules/src/batch_verifier.rs Co-authored-by: hinto-janai --------- Co-authored-by: hinto-janai --- Cargo.lock | 61 ++++++++++++++++++++- consensus/rules/src/batch_verifier.rs | 29 ++++++++++ consensus/rules/src/lib.rs | 1 + consensus/rules/src/transactions.rs | 6 +- consensus/rules/src/transactions/ring_ct.rs | 11 ++-- consensus/src/batch_verifier.rs | 29 +++++----- consensus/src/transactions.rs | 27 ++++----- 7 files changed, 125 insertions(+), 39 deletions(-) create mode 100644 consensus/rules/src/batch_verifier.rs diff --git a/Cargo.lock b/Cargo.lock index 0e507c41..b89874db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,16 @@ dependencies = [ "libc", ] +[[package]] +name = "async-buffer" +version = "0.1.0" +dependencies = [ + "futures", + "pin-project", + "thiserror", + "tokio", +] + [[package]] name = "async-lock" version = "3.3.0" @@ -525,6 +535,7 @@ dependencies = [ "tokio-util", "tower", "tracing", + "tracing-subscriber", ] [[package]] @@ -613,7 +624,7 @@ dependencies = [ ] [[package]] -name = "dandelion_tower" +name = "dandelion-tower" version = "0.1.0" dependencies = [ "futures", @@ -1463,6 +1474,16 @@ dependencies = [ "zeroize", ] +[[package]] +name = "nu-ansi-term" +version = "0.46.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77a8165726e8236064dbb45459242600304b42a5ea24ee2948e18e023bf7ba84" +dependencies = [ + "overload", + "winapi", +] + [[package]] name = "num-traits" version = "0.2.18" @@ -1510,6 +1531,12 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" +[[package]] +name = "overload" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" + [[package]] name = "page_size" version = "0.6.0" @@ -2136,6 +2163,15 @@ dependencies = [ "keccak", ] +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + [[package]] name = "signal-hook-registry" version = "1.4.2" @@ -2476,6 +2512,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c06d3da6113f116aaee68e4d601191614c9053067f9ab7f6edbcb161237daa54" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", ] [[package]] @@ -2484,7 +2532,12 @@ version = "0.3.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ad0f048c97dbd9faa9b7df56362b8ebcaa52adb06b498c050d2f4e32f90a7a8b" dependencies = [ + "nu-ansi-term", + "sharded-slab", + "smallvec", + "thread_local", "tracing-core", + "tracing-log", ] [[package]] @@ -2543,6 +2596,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "valuable" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" + [[package]] name = "version_check" version = "0.9.4" diff --git a/consensus/rules/src/batch_verifier.rs b/consensus/rules/src/batch_verifier.rs new file mode 100644 index 00000000..c8d3f104 --- /dev/null +++ b/consensus/rules/src/batch_verifier.rs @@ -0,0 +1,29 @@ +use multiexp::BatchVerifier as InternalBatchVerifier; + +/// This trait represents a batch verifier. +/// +/// A batch verifier is used to speed up verification by verifying multiple transactions together. +/// +/// Not all proofs can be batched and at its core it's intended to verify a series of statements are +/// each equivalent to zero. +pub trait BatchVerifier { + /// Queue a statement for batch verification. + /// + /// # Panics + /// This function may panic if `stmt` contains calls to `rayon`'s parallel iterators, e.g. `par_iter()`. + // TODO: remove the panics by adding a generic API upstream. + fn queue_statement( + &mut self, + stmt: impl FnOnce(&mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>) -> R, + ) -> R; +} + +// impl this for a single threaded batch verifier. +impl BatchVerifier for &'_ mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint> { + fn queue_statement( + &mut self, + stmt: impl FnOnce(&mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>) -> R, + ) -> R { + stmt(self) + } +} diff --git a/consensus/rules/src/lib.rs b/consensus/rules/src/lib.rs index 4beeec90..3106cbbc 100644 --- a/consensus/rules/src/lib.rs +++ b/consensus/rules/src/lib.rs @@ -1,5 +1,6 @@ use std::time::{SystemTime, UNIX_EPOCH}; +pub mod batch_verifier; pub mod blocks; mod decomposed_amount; pub mod genesis; diff --git a/consensus/rules/src/transactions.rs b/consensus/rules/src/transactions.rs index df7c6eb5..91697087 100644 --- a/consensus/rules/src/transactions.rs +++ b/consensus/rules/src/transactions.rs @@ -3,10 +3,10 @@ use std::cmp::Ordering; use monero_serai::ringct::RctType; use monero_serai::transaction::{Input, Output, Timelock, Transaction}; -use multiexp::BatchVerifier; use crate::{ - blocks::penalty_free_zone, check_point_canonically_encoded, is_decomposed_amount, HardFork, + batch_verifier::BatchVerifier, blocks::penalty_free_zone, check_point_canonically_encoded, + is_decomposed_amount, HardFork, }; mod contextual_data; @@ -606,7 +606,7 @@ pub fn check_transaction_semantic( tx_weight: usize, tx_hash: &[u8; 32], hf: &HardFork, - verifier: &mut BatchVerifier<(), dalek_ff_group::EdwardsPoint>, + verifier: impl BatchVerifier, ) -> Result { // if tx_blob_size > MAX_TX_BLOB_SIZE diff --git a/consensus/rules/src/transactions/ring_ct.rs b/consensus/rules/src/transactions/ring_ct.rs index 8b64b02d..38b56ebd 100644 --- a/consensus/rules/src/transactions/ring_ct.rs +++ b/consensus/rules/src/transactions/ring_ct.rs @@ -9,12 +9,11 @@ use monero_serai::{ transaction::{Input, Transaction}, H, }; -use multiexp::BatchVerifier; use rand::thread_rng; #[cfg(feature = "rayon")] use rayon::prelude::*; -use crate::{transactions::Rings, try_par_iter, HardFork}; +use crate::{batch_verifier::BatchVerifier, transactions::Rings, try_par_iter, HardFork}; /// This constant contains the IDs of 2 transactions that should be allowed after the fork the ringCT /// type they used should be banned. @@ -91,7 +90,7 @@ fn simple_type_balances(rct_sig: &RctSignatures) -> Result<(), RingCTError> { /// fn check_output_range_proofs( rct_sig: &RctSignatures, - verifier: &mut BatchVerifier<(), dalek_ff_group::EdwardsPoint>, + mut verifier: impl BatchVerifier, ) -> Result<(), RingCTError> { let commitments = &rct_sig.base.commitments; @@ -109,7 +108,9 @@ fn check_output_range_proofs( }), RctPrunable::MlsagBulletproofs { bulletproofs, .. } | RctPrunable::Clsag { bulletproofs, .. } => { - if bulletproofs.batch_verify(&mut thread_rng(), verifier, (), commitments) { + if verifier.queue_statement(|verifier| { + bulletproofs.batch_verify(&mut thread_rng(), verifier, (), commitments) + }) { Ok(()) } else { Err(RingCTError::BulletproofsRangeInvalid) @@ -121,7 +122,7 @@ fn check_output_range_proofs( pub(crate) fn ring_ct_semantic_checks( tx: &Transaction, tx_hash: &[u8; 32], - verifier: &mut BatchVerifier<(), dalek_ff_group::EdwardsPoint>, + verifier: impl BatchVerifier, hf: &HardFork, ) -> Result<(), RingCTError> { let rct_type = tx.rct_signatures.rct_type(); diff --git a/consensus/src/batch_verifier.rs b/consensus/src/batch_verifier.rs index 877f164e..44493a62 100644 --- a/consensus/src/batch_verifier.rs +++ b/consensus/src/batch_verifier.rs @@ -4,8 +4,6 @@ use multiexp::BatchVerifier as InternalBatchVerifier; use rayon::prelude::*; use thread_local::ThreadLocal; -use crate::ConsensusError; - /// A multithreaded batch verifier. pub struct MultiThreadedBatchVerifier { internal: ThreadLocal>>, @@ -19,19 +17,6 @@ impl MultiThreadedBatchVerifier { } } - pub fn queue_statement( - &self, - stmt: impl FnOnce( - &mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>, - ) -> Result, - ) -> Result { - let verifier_cell = self - .internal - .get_or(|| RefCell::new(InternalBatchVerifier::new(8))); - // TODO: this is not ok as a rayon par_iter could be called in stmt. - stmt(verifier_cell.borrow_mut().deref_mut()) - } - pub fn verify(self) -> bool { self.internal .into_iter() @@ -41,3 +26,17 @@ impl MultiThreadedBatchVerifier { .is_none() } } + +impl cuprate_consensus_rules::batch_verifier::BatchVerifier for &'_ MultiThreadedBatchVerifier { + fn queue_statement( + &mut self, + stmt: impl FnOnce(&mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>) -> R, + ) -> R { + let mut verifier = self + .internal + .get_or(|| RefCell::new(InternalBatchVerifier::new(32))) + .borrow_mut(); + + stmt(verifier.deref_mut()) + } +} diff --git a/consensus/src/transactions.rs b/consensus/src/transactions.rs index eefe2531..417eb487 100644 --- a/consensus/src/transactions.rs +++ b/consensus/src/transactions.rs @@ -484,26 +484,23 @@ where batch_get_ring_member_info(txs.iter().map(|(tx, _)| tx), &hf, database).await?; rayon_spawn_async(move || { - let batch_veriifier = MultiThreadedBatchVerifier::new(rayon::current_num_threads()); + let batch_verifier = MultiThreadedBatchVerifier::new(rayon::current_num_threads()); txs.par_iter() .zip(txs_ring_member_info.par_iter()) .try_for_each(|((tx, verification_needed), ring)| { // do semantic validation if needed. if *verification_needed == VerificationNeeded::SemanticAndContextual { - batch_veriifier.queue_statement(|verifier| { - let fee = check_transaction_semantic( - &tx.tx, - tx.tx_blob.len(), - tx.tx_weight, - &tx.tx_hash, - &hf, - verifier, - )?; - // make sure monero-serai calculated the same fee. - assert_eq!(fee, tx.fee); - Ok(()) - })?; + let fee = check_transaction_semantic( + &tx.tx, + tx.tx_blob.len(), + tx.tx_weight, + &tx.tx_hash, + &hf, + &batch_verifier, + )?; + // make sure monero-serai calculated the same fee. + assert_eq!(fee, tx.fee); } // Both variants of `VerificationNeeded` require contextual validation. @@ -518,7 +515,7 @@ where Ok::<_, ConsensusError>(()) })?; - if !batch_veriifier.verify() { + if !batch_verifier.verify() { return Err(ExtendedConsensusError::OneOrMoreBatchVerificationStatementsInvalid); }