From 710b06824cd2933defa525a8703a9f486fa4fb27 Mon Sep 17 00:00:00 2001 From: Boog900 <54e72d8a-345f-4599-bd90-c6b9bc7d0ec5@aleeas.com> Date: Thu, 6 Jun 2024 16:25:03 +0100 Subject: [PATCH] fix panic in batch verifier --- Cargo.lock | 61 ++++++++++++++++++++- consensus/rules/src/batch_verifier.rs | 17 ++++++ 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 | 29 +++++----- 7 files changed, 119 insertions(+), 35 deletions(-) create mode 100644 consensus/rules/src/batch_verifier.rs diff --git a/Cargo.lock b/Cargo.lock index 0e507c411..b89874dbd 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 000000000..cae70093a --- /dev/null +++ b/consensus/rules/src/batch_verifier.rs @@ -0,0 +1,17 @@ +use multiexp::BatchVerifier as InternalBatchVerifier; + +pub trait BatchVerifier { + fn queue_statement( + &mut self, + stmt: impl FnOnce(&mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>) -> R, + ) -> R; +} + +impl BatchVerifier for 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 4beeec909..3106cbbc7 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 df7c6eb59..91697087f 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 8b64b02d8..38b56ebde 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 877f164ef..b1f03efe3 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,17 +17,12 @@ impl MultiThreadedBatchVerifier { } } - pub fn queue_statement( - &self, - stmt: impl FnOnce( - &mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>, - ) -> Result, - ) -> Result { - let verifier_cell = self + pub fn queue_worker(&self) -> QueueWorker<'_> { + let verifier = 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()) + + QueueWorker { verifier } } pub fn verify(self) -> bool { @@ -41,3 +34,17 @@ impl MultiThreadedBatchVerifier { .is_none() } } + +pub struct QueueWorker<'a> { + verifier: &'a RefCell>, +} + +impl<'a> cuprate_consensus_rules::batch_verifier::BatchVerifier for QueueWorker<'a> { + fn queue_statement( + &mut self, + stmt: impl FnOnce(&mut InternalBatchVerifier<(), dalek_ff_group::EdwardsPoint>) -> R, + ) -> R { + let mut verifier = self.verifier.borrow_mut(); + stmt(verifier.deref_mut()) + } +} diff --git a/consensus/src/transactions.rs b/consensus/src/transactions.rs index eefe25311..bc540d0a9 100644 --- a/consensus/src/transactions.rs +++ b/consensus/src/transactions.rs @@ -484,26 +484,25 @@ 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 queue_worker = batch_verifier.queue_worker(); + + let fee = check_transaction_semantic( + &tx.tx, + tx.tx_blob.len(), + tx.tx_weight, + &tx.tx_hash, + &hf, + queue_worker, + )?; + // make sure monero-serai calculated the same fee. + assert_eq!(fee, tx.fee); } // Both variants of `VerificationNeeded` require contextual validation. @@ -518,7 +517,7 @@ where Ok::<_, ConsensusError>(()) })?; - if !batch_veriifier.verify() { + if !batch_verifier.verify() { return Err(ExtendedConsensusError::OneOrMoreBatchVerificationStatementsInvalid); }