diff --git a/coordinator/tributary/src/block.rs b/coordinator/tributary/src/block.rs index e7c51bde..5c356554 100644 --- a/coordinator/tributary/src/block.rs +++ b/coordinator/tributary/src/block.rs @@ -1,7 +1,4 @@ -use std::{ - io, - collections::{HashSet, HashMap}, -}; +use std::{io, collections::HashMap}; use thiserror::Error; @@ -11,19 +8,31 @@ use ciphersuite::{Ciphersuite, Ristretto}; #[derive(Clone, PartialEq, Eq, Debug, Error)] pub enum BlockError { + /// Block was too large. + #[error("block exceeded size limit")] + TooLargeBlock, /// Header specified a parent which wasn't the chain tip. #[error("header doesn't build off the chain tip")] InvalidParent, /// Header specified an invalid transactions merkle tree hash. #[error("header transactions hash is incorrect")] InvalidTransactions, + /// A provided transaction was placed after a non-provided transaction. + #[error("a provided transaction was included after a non-provided transaction")] + ProvidedAfterNonProvided, + /// The block had a provided transaction this validator has yet to be provided. + #[error("block had a provided transaction not yet locally provided: {0:?}")] + NonLocalProvided([u8; 32]), + /// The provided transaction was distinct from the locally provided transaction. + #[error("block had a distinct provided transaction")] + DistinctProvided, /// An included transaction was invalid. #[error("included transaction had an error")] TransactionError(TransactionError), } use crate::{ - ReadWrite, TransactionError, Signed, TransactionKind, Transaction, ProvidedTransactions, merkle, + BLOCK_SIZE_LIMIT, ReadWrite, TransactionError, Signed, TransactionKind, Transaction, merkle, verify_transaction, }; @@ -89,21 +98,14 @@ impl Block { /// Create a new block. /// /// mempool is expected to only have valid, non-conflicting transactions. - pub(crate) fn new( - parent: [u8; 32], - provided: &ProvidedTransactions, - mempool: HashMap<[u8; 32], T>, - ) -> Self { - let mut txs = vec![]; - for tx in provided.transactions.values().cloned() { - txs.push(tx); - } - for tx in mempool.values().cloned() { + pub(crate) fn new(parent: [u8; 32], provided: Vec, mempool: Vec) -> Self { + let mut txs = provided; + for tx in mempool { assert!(tx.kind() != TransactionKind::Provided, "provided transaction entered mempool"); txs.push(tx); } - // Sort txs by nonces. + // Check TXs are sorted by nonce. let nonce = |tx: &T| { if let TransactionKind::Signed(Signed { nonce, .. }) = tx.kind() { *nonce @@ -111,9 +113,6 @@ impl Block { 0 } }; - txs.sort_by(|a, b| nonce(a).partial_cmp(&nonce(b)).unwrap()); - - // Check the sort. let mut last = 0; for tx in &txs { let nonce = nonce(tx); @@ -123,33 +122,62 @@ impl Block { last = nonce; } - let hashes = txs.iter().map(Transaction::hash).collect::>(); - Block { header: BlockHeader { parent, transactions: merkle(&hashes) }, transactions: txs } + let mut res = + Block { header: BlockHeader { parent, transactions: [0; 32] }, transactions: txs }; + while res.serialize().len() > BLOCK_SIZE_LIMIT { + assert!(res.transactions.pop().is_some()); + } + let hashes = res.transactions.iter().map(Transaction::hash).collect::>(); + res.header.transactions = merkle(&hashes); + res } pub fn hash(&self) -> [u8; 32] { self.header.hash() } - pub fn verify( + pub(crate) fn verify( &self, genesis: [u8; 32], last_block: [u8; 32], - mut locally_provided: HashSet<[u8; 32]>, + locally_provided: &[[u8; 32]], mut next_nonces: HashMap<::G, u32>, ) -> Result<(), BlockError> { + if self.serialize().len() > BLOCK_SIZE_LIMIT { + Err(BlockError::TooLargeBlock)?; + } + if self.header.parent != last_block { Err(BlockError::InvalidParent)?; } + let mut found_non_provided = false; let mut txs = Vec::with_capacity(self.transactions.len()); - for tx in &self.transactions { - match verify_transaction(tx, genesis, &mut locally_provided, &mut next_nonces) { + for (i, tx) in self.transactions.iter().enumerate() { + txs.push(tx.hash()); + + if tx.kind() == TransactionKind::Provided { + if found_non_provided { + Err(BlockError::ProvidedAfterNonProvided)?; + } + + let Some(local) = locally_provided.get(i) else { + Err(BlockError::NonLocalProvided(txs.pop().unwrap()))? + }; + if txs.last().unwrap() != local { + Err(BlockError::DistinctProvided)?; + } + + // We don't need to call verify_transaction since we did when we locally provided this + // transaction. Since it's identical, it must be valid + continue; + } + + found_non_provided = true; + match verify_transaction(tx, genesis, &mut next_nonces) { Ok(()) => {} Err(e) => Err(BlockError::TransactionError(e))?, } - - txs.push(tx.hash()); } if merkle(&txs) != self.header.transactions { diff --git a/coordinator/tributary/src/blockchain.rs b/coordinator/tributary/src/blockchain.rs index 7a44904f..d1d34ff9 100644 --- a/coordinator/tributary/src/blockchain.rs +++ b/coordinator/tributary/src/blockchain.rs @@ -1,11 +1,14 @@ -use std::collections::{HashSet, HashMap}; +use std::collections::HashMap; use ciphersuite::{Ciphersuite, Ristretto}; -use crate::{Signed, TransactionKind, Transaction, ProvidedTransactions, BlockError, Block, Mempool}; +use crate::{ + Signed, TransactionKind, Transaction, verify_transaction, ProvidedTransactions, BlockError, + Block, Mempool, +}; #[derive(Clone, PartialEq, Eq, Debug)] -pub struct Blockchain { +pub(crate) struct Blockchain { genesis: [u8; 32], // TODO: db block_number: u64, @@ -17,7 +20,7 @@ pub struct Blockchain { } impl Blockchain { - pub fn new(genesis: [u8; 32], participants: &[::G]) -> Self { + pub(crate) fn new(genesis: [u8; 32], participants: &[::G]) -> Self { // TODO: Reload block_number/tip/next_nonces/provided/mempool let mut next_nonces = HashMap::new(); @@ -37,44 +40,54 @@ impl Blockchain { } } - pub fn tip(&self) -> [u8; 32] { + pub(crate) fn tip(&self) -> [u8; 32] { self.tip } - pub fn block_number(&self) -> u64 { + pub(crate) fn block_number(&self) -> u64 { self.block_number } - pub fn add_transaction(&mut self, tx: T) -> bool { - self.mempool.add(&self.next_nonces, tx) + pub(crate) fn add_transaction(&mut self, internal: bool, tx: T) -> bool { + self.mempool.add(&self.next_nonces, internal, tx) } - pub fn provide_transaction(&mut self, tx: T) { - self.provided.provide(tx) + pub(crate) fn provide_transaction(&mut self, tx: T) -> bool { + // TODO: Should this check be internal to ProvidedTransactions? + if verify_transaction(&tx, self.genesis, &mut HashMap::new()).is_err() { + return false; + } + self.provided.provide(tx); + true } - /// Returns the next nonce, or None if they aren't a participant. - pub fn next_nonce(&self, key: ::G) -> Option { - self.next_nonces.get(&key).cloned() + /// Returns the next nonce for signing, or None if they aren't a participant. + pub(crate) fn next_nonce(&self, key: ::G) -> Option { + Some(self.next_nonces.get(&key).cloned()?.max(self.mempool.next_nonce(&key).unwrap_or(0))) } - pub fn build_block(&mut self) -> Block { - let block = Block::new(self.tip, &self.provided, self.mempool.block(&self.next_nonces)); + pub(crate) fn build_block(&mut self) -> Block { + let block = Block::new( + self.tip, + self.provided.transactions.iter().cloned().collect(), + self.mempool.block(&self.next_nonces), + ); // build_block should not return invalid blocks self.verify_block(&block).unwrap(); block } - pub fn verify_block(&self, block: &Block) -> Result<(), BlockError> { - let mut locally_provided = HashSet::new(); - for provided in self.provided.transactions.keys() { - locally_provided.insert(*provided); - } - block.verify(self.genesis, self.tip, locally_provided, self.next_nonces.clone()) + pub(crate) fn verify_block(&self, block: &Block) -> Result<(), BlockError> { + block.verify( + self.genesis, + self.tip, + &self.provided.transactions.iter().map(Transaction::hash).collect::>(), + self.next_nonces.clone(), + ) } /// Add a block. - pub fn add_block(&mut self, block: &Block) -> Result<(), BlockError> { + pub(crate) fn add_block(&mut self, block: &Block) -> Result<(), BlockError> { self.verify_block(block)?; // None of the following assertions should be reachable since we verified the block @@ -83,10 +96,7 @@ impl Blockchain { for tx in &block.transactions { match tx.kind() { TransactionKind::Provided => { - assert!( - self.provided.withdraw(tx.hash()), - "verified block had a provided transaction we didn't have" - ); + self.provided.complete(tx.hash()); } TransactionKind::Unsigned => {} TransactionKind::Signed(Signed { signer, nonce, .. }) => { @@ -97,6 +107,8 @@ impl Blockchain { if prev != *nonce { panic!("verified block had an invalid nonce"); } + + self.mempool.remove(&tx.hash()); } } } diff --git a/coordinator/tributary/src/lib.rs b/coordinator/tributary/src/lib.rs index 5563bc35..1bc64945 100644 --- a/coordinator/tributary/src/lib.rs +++ b/coordinator/tributary/src/lib.rs @@ -32,10 +32,10 @@ mod block; pub use block::*; mod blockchain; -pub use blockchain::*; +pub(crate) use blockchain::*; mod mempool; -pub use mempool::*; +pub(crate) use mempool::*; mod tendermint; pub(crate) use crate::tendermint::*; @@ -43,6 +43,15 @@ pub(crate) use crate::tendermint::*; #[cfg(any(test, feature = "tests"))] pub mod tests; +/// Size limit for an individual transaction. +pub const TRANSACTION_SIZE_LIMIT: usize = 50_000; +/// Amount of transactions a single account may have in the mempool. +pub const ACCOUNT_MEMPOOL_LIMIT: u32 = 50; +/// Block size limit. +// This targets a growth limit of roughly 5 GB a day, under load, in order to prevent a malicious +// participant from flooding disks and causing out of space errors in order processes. +pub const BLOCK_SIZE_LIMIT: usize = 350_000; + pub(crate) const TRANSACTION_MESSAGE: u8 = 0; pub(crate) const TENDERMINT_MESSAGE: u8 = 1; @@ -108,15 +117,19 @@ impl Tributary { Self { network, synced_block, messages } } - pub fn provide_transaction(&self, tx: T) { + pub fn provide_transaction(&self, tx: T) -> bool { self.network.blockchain.write().unwrap().provide_transaction(tx) } + pub fn next_nonce(&self, signer: ::G) -> Option { + self.network.blockchain.read().unwrap().next_nonce(signer) + } + // Returns if the transaction was valid. - pub async fn add_transaction(&self, tx: T) -> bool { + pub async fn add_transaction(&mut self, tx: T) -> bool { let mut to_broadcast = vec![TRANSACTION_MESSAGE]; tx.write(&mut to_broadcast).unwrap(); - let res = self.network.blockchain.write().unwrap().add_transaction(tx); + let res = self.network.blockchain.write().unwrap().add_transaction(true, tx); if res { self.network.p2p.broadcast(to_broadcast).await; } @@ -158,7 +171,7 @@ impl Tributary { // TODO: Sync mempools with fellow peers // Can we just rebroadcast transactions not included for at least two blocks? - self.network.blockchain.write().unwrap().add_transaction(tx) + self.network.blockchain.write().unwrap().add_transaction(false, tx) } TENDERMINT_MESSAGE => { diff --git a/coordinator/tributary/src/mempool.rs b/coordinator/tributary/src/mempool.rs index 85bb5d78..fd923293 100644 --- a/coordinator/tributary/src/mempool.rs +++ b/coordinator/tributary/src/mempool.rs @@ -1,25 +1,26 @@ -use std::collections::{HashSet, HashMap}; +use std::collections::HashMap; use ciphersuite::{Ciphersuite, Ristretto}; -use crate::{Signed, TransactionKind, Transaction, verify_transaction}; +use crate::{ACCOUNT_MEMPOOL_LIMIT, Signed, TransactionKind, Transaction, verify_transaction}; #[derive(Clone, PartialEq, Eq, Debug)] -pub struct Mempool { +pub(crate) struct Mempool { genesis: [u8; 32], txs: HashMap<[u8; 32], T>, next_nonces: HashMap<::G, u32>, } impl Mempool { - pub fn new(genesis: [u8; 32]) -> Self { + pub(crate) fn new(genesis: [u8; 32]) -> Self { Mempool { genesis, txs: HashMap::new(), next_nonces: HashMap::new() } } /// Returns true if this is a valid, new transaction. - pub fn add( + pub(crate) fn add( &mut self, blockchain_next_nonces: &HashMap<::G, u32>, + internal: bool, tx: T, ) -> bool { match tx.kind() { @@ -41,9 +42,13 @@ impl Mempool { self.next_nonces.insert(*signer, blockchain_next_nonce); } - if verify_transaction(&tx, self.genesis, &mut HashSet::new(), &mut self.next_nonces) - .is_err() - { + // If we have too many transactions from this sender, don't add this yet UNLESS we are + // this sender + if !internal && (nonce >= &(blockchain_next_nonce + ACCOUNT_MEMPOOL_LIMIT)) { + return false; + } + + if verify_transaction(&tx, self.genesis, &mut self.next_nonces).is_err() { return false; } assert_eq!(self.next_nonces[signer], nonce + 1); @@ -56,18 +61,16 @@ impl Mempool { } // Returns None if the mempool doesn't have a nonce tracked. - // The nonce to use when signing should be: - // max(blockchain.next_nonce().unwrap(), mempool.next_nonce().unwrap_or(0)) - pub fn next_nonce(&self, signer: &::G) -> Option { + pub(crate) fn next_nonce(&self, signer: &::G) -> Option { self.next_nonces.get(signer).cloned() } /// Get transactions to include in a block. - pub fn block( + pub(crate) fn block( &mut self, blockchain_next_nonces: &HashMap<::G, u32>, - ) -> HashMap<[u8; 32], T> { - let mut res = HashMap::new(); + ) -> Vec { + let mut res = vec![]; for hash in self.txs.keys().cloned().collect::>() { let tx = &self.txs[&hash]; // Verify this hasn't gone stale @@ -82,13 +85,24 @@ impl Mempool { } // Since this TX isn't stale, include it - res.insert(hash, tx.clone()); + res.push(tx.clone()); } + + // Sort res by nonce. + let nonce = |tx: &T| { + if let TransactionKind::Signed(Signed { nonce, .. }) = tx.kind() { + *nonce + } else { + 0 + } + }; + res.sort_by(|a, b| nonce(a).partial_cmp(&nonce(b)).unwrap()); + res } /// Remove a transaction from the mempool. - pub fn remove(&mut self, tx: &[u8; 32]) { + pub(crate) fn remove(&mut self, tx: &[u8; 32]) { self.txs.remove(tx); } diff --git a/coordinator/tributary/src/provided.rs b/coordinator/tributary/src/provided.rs index 32d3c68d..b86f0cfd 100644 --- a/coordinator/tributary/src/provided.rs +++ b/coordinator/tributary/src/provided.rs @@ -1,33 +1,32 @@ -use std::collections::HashMap; +use std::collections::VecDeque; use crate::{TransactionKind, Transaction}; #[derive(Clone, PartialEq, Eq, Debug)] pub struct ProvidedTransactions { - pub(crate) transactions: HashMap<[u8; 32], T>, + pub(crate) transactions: VecDeque, } impl Default for ProvidedTransactions { fn default() -> Self { - ProvidedTransactions { transactions: HashMap::new() } + ProvidedTransactions { transactions: VecDeque::new() } } } impl ProvidedTransactions { - pub fn new() -> Self { + pub(crate) fn new() -> Self { ProvidedTransactions::default() } /// Provide a transaction for inclusion in a block. - pub fn provide(&mut self, tx: T) { + pub(crate) fn provide(&mut self, tx: T) { + // TODO: Make an error out of this assert_eq!(tx.kind(), TransactionKind::Provided, "provided a non-provided transaction"); - self.transactions.insert(tx.hash(), tx); + self.transactions.push_back(tx); } - /// Withdraw a transaction, no longer proposing it or voting for its validity. - /// - /// Returns true if the transaction was withdrawn and false otherwise. - pub fn withdraw(&mut self, tx: [u8; 32]) -> bool { - self.transactions.remove(&tx).is_some() + /// Complete a provided transaction, no longer proposing it nor voting for its validity. + pub(crate) fn complete(&mut self, tx: [u8; 32]) { + assert_eq!(self.transactions.pop_front().unwrap().hash(), tx); } } diff --git a/coordinator/tributary/src/tendermint.rs b/coordinator/tributary/src/tendermint.rs index e5017b9d..39af20fb 100644 --- a/coordinator/tributary/src/tendermint.rs +++ b/coordinator/tributary/src/tendermint.rs @@ -35,8 +35,7 @@ use tendermint::{ use tokio::time::{Duration, sleep}; use crate::{ - TENDERMINT_MESSAGE, ReadWrite, Transaction, TransactionError, BlockHeader, Block, BlockError, - Blockchain, P2p, + TENDERMINT_MESSAGE, ReadWrite, Transaction, BlockHeader, Block, BlockError, Blockchain, P2p, }; fn challenge( @@ -266,9 +265,7 @@ impl NetworkTrait for Network { let block = Block::read::<&[u8]>(&mut block.0.as_ref()).map_err(|_| TendermintBlockError::Fatal)?; self.blockchain.read().unwrap().verify_block(&block).map_err(|e| match e { - BlockError::TransactionError(TransactionError::MissingProvided(_)) => { - TendermintBlockError::Temporal - } + BlockError::NonLocalProvided(_) => TendermintBlockError::Temporal, _ => TendermintBlockError::Fatal, }) } @@ -297,7 +294,7 @@ impl NetworkTrait for Network { let block_res = self.blockchain.write().unwrap().add_block(&block); match block_res { Ok(()) => break, - Err(BlockError::TransactionError(TransactionError::MissingProvided(hash))) => { + Err(BlockError::NonLocalProvided(hash)) => { log::error!( "missing provided transaction {} which other validators on tributary {} had", hex::encode(hash), diff --git a/coordinator/tributary/src/tests/block.rs b/coordinator/tributary/src/tests/block.rs index aa1c9c09..e613d417 100644 --- a/coordinator/tributary/src/tests/block.rs +++ b/coordinator/tributary/src/tests/block.rs @@ -1,9 +1,4 @@ -use std::{ - io, - collections::{HashSet, HashMap}, -}; - -use rand::{RngCore, rngs::OsRng}; +use std::{io, collections::HashMap}; use blake2::{Digest, Blake2s256}; @@ -13,9 +8,8 @@ use ciphersuite::{ }; use schnorr::SchnorrSignature; -use crate::{ - ReadWrite, TransactionError, Signed, TransactionKind, Transaction, ProvidedTransactions, Block, -}; +use crate::{ReadWrite, TransactionError, Signed, TransactionKind, Transaction, BlockError, Block}; + // A transaction solely defined by its nonce and a distinguisher (to allow creating distinct TXs // sharing a nonce). #[derive(Clone, PartialEq, Eq, Debug)] @@ -74,8 +68,8 @@ impl Transaction for NonceTransaction { fn empty_block() { const GENESIS: [u8; 32] = [0xff; 32]; const LAST: [u8; 32] = [0x01; 32]; - Block::new(LAST, &ProvidedTransactions::::new(), HashMap::new()) - .verify(GENESIS, LAST, HashSet::new(), HashMap::new()) + Block::::new(LAST, vec![], vec![]) + .verify(GENESIS, LAST, &[], HashMap::new()) .unwrap(); } @@ -87,51 +81,21 @@ fn duplicate_nonces() { // Run once without duplicating a nonce, and once with, so that's confirmed to be the faulty // component for i in [1, 0] { - let mut mempool = HashMap::new(); - let mut insert = |tx: NonceTransaction| mempool.insert(tx.hash(), tx); + let mut mempool = vec![]; + let mut insert = |tx: NonceTransaction| mempool.push(tx); insert(NonceTransaction::new(0, 0)); insert(NonceTransaction::new(i, 1)); - let res = Block::new(LAST, &ProvidedTransactions::new(), mempool).verify( + let res = Block::new(LAST, vec![], mempool).verify( GENESIS, LAST, - HashSet::new(), + &[], HashMap::from([(::G::identity(), 0)]), ); if i == 1 { res.unwrap(); } else { - assert!(res.is_err()); + assert_eq!(res, Err(BlockError::TransactionError(TransactionError::InvalidNonce))); } } } - -#[test] -fn unsorted_nonces() { - let mut mempool = HashMap::new(); - // Create a large amount of nonces so the retrieval from the HashMapis effectively guaranteed to - // be out of order - let mut nonces = (0 .. 64).collect::>(); - // Insert in a random order - while !nonces.is_empty() { - let nonce = nonces.swap_remove( - usize::try_from(OsRng.next_u64() % u64::try_from(nonces.len()).unwrap()).unwrap(), - ); - let tx = NonceTransaction::new(nonce, 0); - mempool.insert(tx.hash(), tx); - } - - // Create and verify the block - const GENESIS: [u8; 32] = [0xff; 32]; - const LAST: [u8; 32] = [0x01; 32]; - let nonces = HashMap::from([(::G::identity(), 0)]); - Block::new(LAST, &ProvidedTransactions::new(), mempool.clone()) - .verify(GENESIS, LAST, HashSet::new(), nonces.clone()) - .unwrap(); - - let skip = NonceTransaction::new(65, 0); - mempool.insert(skip.hash(), skip); - assert!(Block::new(LAST, &ProvidedTransactions::new(), mempool) - .verify(GENESIS, LAST, HashSet::new(), nonces) - .is_err()); -} diff --git a/coordinator/tributary/src/tests/blockchain.rs b/coordinator/tributary/src/tests/blockchain.rs index e027b00d..b8630204 100644 --- a/coordinator/tributary/src/tests/blockchain.rs +++ b/coordinator/tributary/src/tests/blockchain.rs @@ -1,5 +1,3 @@ -use std::collections::{HashSet, HashMap}; - use zeroize::Zeroizing; use rand::{RngCore, rngs::OsRng}; @@ -8,7 +6,7 @@ use blake2::{Digest, Blake2s256}; use ciphersuite::{group::ff::Field, Ciphersuite, Ristretto}; use crate::{ - merkle, Signed, TransactionKind, Transaction, ProvidedTransactions, Block, Blockchain, + merkle, Transaction, ProvidedTransactions, Block, Blockchain, tests::{ProvidedTransaction, SignedTransaction, random_provided_transaction}, }; @@ -69,11 +67,7 @@ fn invalid_block() { // Not a participant { // Manually create the block to bypass build_block's checks - let block = Block::new( - blockchain.tip(), - &ProvidedTransactions::new(), - HashMap::from([(tx.hash(), tx.clone())]), - ); + let block = Block::new(blockchain.tip(), vec![], vec![tx.clone()]); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); assert!(blockchain.verify_block(&block).is_err()); } @@ -83,11 +77,7 @@ fn invalid_block() { // Re-run the not a participant block to make sure it now works { - let block = Block::new( - blockchain.tip(), - &ProvidedTransactions::new(), - HashMap::from([(tx.hash(), tx.clone())]), - ); + let block = Block::new(blockchain.tip(), vec![], vec![tx.clone()]); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); blockchain.verify_block(&block).unwrap(); } @@ -95,7 +85,7 @@ fn invalid_block() { { // Add a valid transaction let mut blockchain = blockchain.clone(); - assert!(blockchain.add_transaction(tx.clone())); + assert!(blockchain.add_transaction(true, tx.clone())); let mut block = blockchain.build_block(); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); blockchain.verify_block(&block).unwrap(); @@ -109,15 +99,14 @@ fn invalid_block() { // Invalid nonce let tx = crate::tests::signed_transaction(&mut OsRng, genesis, &key, 5); // Manually create the block to bypass build_block's checks - let block = - Block::new(blockchain.tip(), &ProvidedTransactions::new(), HashMap::from([(tx.hash(), tx)])); + let block = Block::new(blockchain.tip(), vec![], vec![tx]); assert!(blockchain.verify_block(&block).is_err()); } { // Invalid signature let mut blockchain = blockchain; - assert!(blockchain.add_transaction(tx)); + assert!(blockchain.add_transaction(true, tx)); let mut block = blockchain.build_block(); blockchain.verify_block(&block).unwrap(); block.transactions[0].1.signature.s += ::F::ONE; @@ -140,49 +129,25 @@ fn signed_transaction() { let mut blockchain = new_blockchain::(genesis, &[signer]); assert_eq!(blockchain.next_nonce(signer), Some(0)); - let test = |blockchain: &mut Blockchain, - mempool: HashMap<[u8; 32], SignedTransaction>| { - let mut hashes = mempool.keys().cloned().collect::>(); - - // These transactions do need to be added, in-order, to the mempool for the blockchain to - // build a block off them - { - let mut ordered = HashMap::new(); - for (_, tx) in mempool.clone().drain() { - let nonce = if let TransactionKind::Signed(Signed { nonce, .. }) = tx.kind() { - *nonce - } else { - panic!("non-signed TX in test mempool"); - }; - ordered.insert(nonce, tx); - } - - let mut i = 0; - while !ordered.contains_key(&i) { - i += 1; - } - for i in i .. (i + u32::try_from(ordered.len()).unwrap()) { - assert!(blockchain.add_transaction(ordered.remove(&i).unwrap())); - } - } - + let test = |blockchain: &mut Blockchain, mempool: Vec| { let tip = blockchain.tip(); + for tx in mempool.clone() { + let next_nonce = blockchain.next_nonce(signer).unwrap(); + assert!(blockchain.add_transaction(true, tx)); + assert_eq!(next_nonce + 1, blockchain.next_nonce(signer).unwrap()); + } let block = blockchain.build_block(); - // The Block constructor should sort these these, and build_block should've called Block::new - assert_eq!(block, Block::new(blockchain.tip(), &ProvidedTransactions::new(), mempool)); + assert_eq!(block, Block::new(blockchain.tip(), vec![], mempool.clone())); assert_eq!(blockchain.tip(), tip); assert_eq!(block.header.parent, tip); // Make sure all transactions were included - let mut ordered_hashes = vec![]; - assert_eq!(hashes.len(), block.transactions.len()); - for transaction in &block.transactions { - let hash = transaction.hash(); - assert!(hashes.remove(&hash)); - ordered_hashes.push(hash); - } + assert_eq!(block.transactions, mempool); // Make sure the merkle was correct - assert_eq!(block.header.transactions, merkle(&ordered_hashes)); + assert_eq!( + block.header.transactions, + merkle(&mempool.iter().map(Transaction::hash).collect::>()) + ); // Verify and add the block blockchain.verify_block(&block).unwrap(); @@ -191,19 +156,13 @@ fn signed_transaction() { }; // Test with a single nonce - test(&mut blockchain, HashMap::from([(tx.hash(), tx)])); + test(&mut blockchain, vec![tx]); assert_eq!(blockchain.next_nonce(signer), Some(1)); // Test with a flood of nonces - let mut mempool = HashMap::new(); - let mut nonces = (1 .. 64).collect::>(); - // Randomize insertion order into HashMap, even though it should already have unordered iteration - while !nonces.is_empty() { - let nonce = nonces.swap_remove( - usize::try_from(OsRng.next_u64() % u64::try_from(nonces.len()).unwrap()).unwrap(), - ); - let tx = crate::tests::signed_transaction(&mut OsRng, genesis, &key, nonce); - mempool.insert(tx.hash(), tx); + let mut mempool = vec![]; + for nonce in 1 .. 64 { + mempool.push(crate::tests::signed_transaction(&mut OsRng, genesis, &key, nonce)); } test(&mut blockchain, mempool); assert_eq!(blockchain.next_nonce(signer), Some(64)); @@ -214,20 +173,24 @@ fn provided_transaction() { let mut blockchain = new_blockchain::(new_genesis(), &[]); let tx = random_provided_transaction(&mut OsRng); + + // This should be provideable let mut txs = ProvidedTransactions::new(); txs.provide(tx.clone()); + txs.complete(tx.hash()); + // Non-provided transactions should fail verification - let block = Block::new(blockchain.tip(), &txs, HashMap::new()); + let block = Block::new(blockchain.tip(), vec![tx.clone()], vec![]); assert!(blockchain.verify_block(&block).is_err()); // Provided transactions should pass verification - blockchain.provide_transaction(tx); + blockchain.provide_transaction(tx.clone()); blockchain.verify_block(&block).unwrap(); // add_block should work for verified blocks assert!(blockchain.add_block(&block).is_ok()); - let block = Block::new(blockchain.tip(), &txs, HashMap::new()); + let block = Block::new(blockchain.tip(), vec![tx], vec![]); // The provided transaction should no longer considered provided, causing this error assert!(blockchain.verify_block(&block).is_err()); // add_block should fail for unverified provided transactions if told to add them diff --git a/coordinator/tributary/src/tests/mempool.rs b/coordinator/tributary/src/tests/mempool.rs index cf569a9d..e361db50 100644 --- a/coordinator/tributary/src/tests/mempool.rs +++ b/coordinator/tributary/src/tests/mempool.rs @@ -6,7 +6,7 @@ use rand::{RngCore, rngs::OsRng}; use ciphersuite::{group::ff::Field, Ciphersuite, Ristretto}; use crate::{ - Transaction, Mempool, + ACCOUNT_MEMPOOL_LIMIT, Transaction, Mempool, tests::{SignedTransaction, signed_transaction}, }; @@ -28,17 +28,17 @@ fn mempool_addition() { // Add TX 0 let mut blockchain_next_nonces = HashMap::from([(signer, 0)]); - assert!(mempool.add(&blockchain_next_nonces, first_tx.clone())); + assert!(mempool.add(&blockchain_next_nonces, true, first_tx.clone())); assert_eq!(mempool.next_nonce(&signer), Some(1)); // Adding it again should fail - assert!(!mempool.add(&blockchain_next_nonces, first_tx.clone())); + assert!(!mempool.add(&blockchain_next_nonces, true, first_tx.clone())); // Do the same with the next nonce let second_tx = signed_transaction(&mut OsRng, genesis, &key, 1); - assert!(mempool.add(&blockchain_next_nonces, second_tx.clone())); + assert!(mempool.add(&blockchain_next_nonces, true, second_tx.clone())); assert_eq!(mempool.next_nonce(&signer), Some(2)); - assert!(!mempool.add(&blockchain_next_nonces, second_tx.clone())); + assert!(!mempool.add(&blockchain_next_nonces, true, second_tx.clone())); // If the mempool doesn't have a nonce for an account, it should successfully use the // blockchain's @@ -47,7 +47,7 @@ fn mempool_addition() { let second_signer = tx.1.signer; assert_eq!(mempool.next_nonce(&second_signer), None); blockchain_next_nonces.insert(second_signer, 2); - assert!(mempool.add(&blockchain_next_nonces, tx.clone())); + assert!(mempool.add(&blockchain_next_nonces, true, tx.clone())); assert_eq!(mempool.next_nonce(&second_signer), Some(3)); // Getting a block should work @@ -55,12 +55,35 @@ fn mempool_addition() { // If the blockchain says an account had its nonce updated, it should cause a prune blockchain_next_nonces.insert(signer, 1); - let block = mempool.block(&blockchain_next_nonces); + let mut block = mempool.block(&blockchain_next_nonces); assert_eq!(block.len(), 2); - assert!(!block.contains_key(&first_tx.hash())); - assert_eq!(mempool.txs(), &block); + assert!(!block.iter().any(|tx| tx.hash() == first_tx.hash())); + assert_eq!(mempool.txs(), &block.drain(..).map(|tx| (tx.hash(), tx)).collect::>()); // Removing should also successfully prune mempool.remove(&tx.hash()); assert_eq!(mempool.txs(), &HashMap::from([(second_tx.hash(), second_tx)])); } + +#[test] +fn too_many_mempool() { + let (genesis, mut mempool) = new_mempool::(); + + let key = Zeroizing::new(::F::random(&mut OsRng)); + let signer = signed_transaction(&mut OsRng, genesis, &key, 0).1.signer; + + // We should be able to add transactions up to the limit + for i in 0 .. ACCOUNT_MEMPOOL_LIMIT { + assert!(mempool.add( + &HashMap::from([(signer, 0)]), + false, + signed_transaction(&mut OsRng, genesis, &key, i) + )); + } + // Yet adding more should fail + assert!(!mempool.add( + &HashMap::from([(signer, 0)]), + false, + signed_transaction(&mut OsRng, genesis, &key, ACCOUNT_MEMPOOL_LIMIT) + )); +} diff --git a/coordinator/tributary/src/tests/transaction/mod.rs b/coordinator/tributary/src/tests/transaction/mod.rs index 14d2ab8d..37168feb 100644 --- a/coordinator/tributary/src/tests/transaction/mod.rs +++ b/coordinator/tributary/src/tests/transaction/mod.rs @@ -1,7 +1,4 @@ -use std::{ - io, - collections::{HashSet, HashMap}, -}; +use std::{io, collections::HashMap}; use zeroize::Zeroizing; use rand::{RngCore, CryptoRng}; @@ -19,9 +16,6 @@ use crate::{ReadWrite, Signed, TransactionError, TransactionKind, Transaction, v #[cfg(test)] mod signed; -#[cfg(test)] -mod provided; - pub fn random_signed(rng: &mut R) -> Signed { Signed { signer: ::G::random(&mut *rng), @@ -127,7 +121,7 @@ pub fn signed_transaction( ); let mut nonces = HashMap::from([(signer, nonce)]); - verify_transaction(&tx, genesis, &mut HashSet::new(), &mut nonces).unwrap(); + verify_transaction(&tx, genesis, &mut nonces).unwrap(); assert_eq!(nonces, HashMap::from([(tx.1.signer, tx.1.nonce.wrapping_add(1))])); tx diff --git a/coordinator/tributary/src/tests/transaction/provided.rs b/coordinator/tributary/src/tests/transaction/provided.rs deleted file mode 100644 index 3a80359d..00000000 --- a/coordinator/tributary/src/tests/transaction/provided.rs +++ /dev/null @@ -1,18 +0,0 @@ -use std::collections::{HashSet, HashMap}; - -use rand::rngs::OsRng; - -use crate::{Transaction, verify_transaction, tests::random_provided_transaction}; - -#[test] -fn provided_transaction() { - let tx = random_provided_transaction(&mut OsRng); - - // Make sure this works when provided - let mut provided = HashSet::from([tx.hash()]); - verify_transaction(&tx, [0x88; 32], &mut provided, &mut HashMap::new()).unwrap(); - assert_eq!(provided.len(), 0); - - // Make sure this fails when not provided - assert!(verify_transaction(&tx, [0x88; 32], &mut HashSet::new(), &mut HashMap::new()).is_err()); -} diff --git a/coordinator/tributary/src/tests/transaction/signed.rs b/coordinator/tributary/src/tests/transaction/signed.rs index 8144cd6d..b9c624f9 100644 --- a/coordinator/tributary/src/tests/transaction/signed.rs +++ b/coordinator/tributary/src/tests/transaction/signed.rs @@ -1,4 +1,4 @@ -use std::collections::{HashSet, HashMap}; +use std::collections::HashMap; use rand::rngs::OsRng; @@ -37,7 +37,6 @@ fn signed_transaction() { assert!(verify_transaction( &tx, Blake2s256::digest(genesis).into(), - &mut HashSet::new(), &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), ) .is_err()); @@ -46,26 +45,18 @@ fn signed_transaction() { { let mut tx = tx.clone(); tx.0 = Blake2s256::digest(tx.0).to_vec(); - assert!(verify_transaction( - &tx, - genesis, - &mut HashSet::new(), - &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), - ) - .is_err()); + assert!( + verify_transaction(&tx, genesis, &mut HashMap::from([(tx.1.signer, tx.1.nonce)]),).is_err() + ); } // Different signer { let mut tx = tx.clone(); tx.1.signer += Ristretto::generator(); - assert!(verify_transaction( - &tx, - genesis, - &mut HashSet::new(), - &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), - ) - .is_err()); + assert!( + verify_transaction(&tx, genesis, &mut HashMap::from([(tx.1.signer, tx.1.nonce)]),).is_err() + ); } // Different nonce @@ -73,42 +64,30 @@ fn signed_transaction() { #[allow(clippy::redundant_clone)] // False positive? let mut tx = tx.clone(); tx.1.nonce = tx.1.nonce.wrapping_add(1); - assert!(verify_transaction( - &tx, - genesis, - &mut HashSet::new(), - &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), - ) - .is_err()); + assert!( + verify_transaction(&tx, genesis, &mut HashMap::from([(tx.1.signer, tx.1.nonce)]),).is_err() + ); } // Different signature { let mut tx = tx.clone(); tx.1.signature.R += Ristretto::generator(); - assert!(verify_transaction( - &tx, - genesis, - &mut HashSet::new(), - &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), - ) - .is_err()); + assert!( + verify_transaction(&tx, genesis, &mut HashMap::from([(tx.1.signer, tx.1.nonce)]),).is_err() + ); } { let mut tx = tx.clone(); tx.1.signature.s += ::F::ONE; - assert!(verify_transaction( - &tx, - genesis, - &mut HashSet::new(), - &mut HashMap::from([(tx.1.signer, tx.1.nonce)]), - ) - .is_err()); + assert!( + verify_transaction(&tx, genesis, &mut HashMap::from([(tx.1.signer, tx.1.nonce)]),).is_err() + ); } // Sanity check the original TX was never mutated and is valid let mut nonces = HashMap::from([(tx.1.signer, tx.1.nonce)]); - verify_transaction(&tx, genesis, &mut HashSet::new(), &mut nonces).unwrap(); + verify_transaction(&tx, genesis, &mut nonces).unwrap(); assert_eq!(nonces, HashMap::from([(tx.1.signer, tx.1.nonce.wrapping_add(1))])); } @@ -119,7 +98,6 @@ fn invalid_nonce() { assert!(verify_transaction( &tx, genesis, - &mut HashSet::new(), &mut HashMap::from([(tx.1.signer, tx.1.nonce.wrapping_add(1))]), ) .is_err()); diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index 53f265ce..30e07a52 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -1,8 +1,5 @@ use core::fmt::Debug; -use std::{ - io, - collections::{HashSet, HashMap}, -}; +use std::{io, collections::HashMap}; use thiserror::Error; @@ -11,13 +8,13 @@ use blake2::{Digest, Blake2b512}; use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; use schnorr::SchnorrSignature; -use crate::ReadWrite; +use crate::{TRANSACTION_SIZE_LIMIT, ReadWrite}; #[derive(Clone, PartialEq, Eq, Debug, Error)] pub enum TransactionError { - /// A provided transaction wasn't locally provided. - #[error("provided transaction wasn't locally provided")] - MissingProvided([u8; 32]), + /// Transaction exceeded the size limit. + #[error("transaction was too large")] + TooLargeTransaction, /// This transaction's signer isn't a participant. #[error("invalid signer")] InvalidSigner, @@ -63,7 +60,13 @@ impl ReadWrite for Signed { #[allow(clippy::large_enum_variant)] #[derive(Clone, PartialEq, Eq, Debug)] pub enum TransactionKind<'a> { - /// This tranaction should be provided by every validator, solely ordered by the block producer. + /// This tranaction should be provided by every validator, in an exact order. + /// + /// The only malleability is in when this transaction appears on chain. The block producer will + /// include it when they have it. Block verification will fail for validators without it. + /// + /// If a supermajority of validators still produce a commit for a block with a provided + /// transaction which isn't locally held, the chain will sleep until it is locally provided. Provided, /// An unsigned transaction, only able to be included by the block producer. @@ -99,18 +102,16 @@ pub trait Transaction: 'static + Send + Sync + Clone + Eq + Debug + ReadWrite { pub(crate) fn verify_transaction( tx: &T, genesis: [u8; 32], - locally_provided: &mut HashSet<[u8; 32]>, next_nonces: &mut HashMap<::G, u32>, ) -> Result<(), TransactionError> { + if tx.serialize().len() > TRANSACTION_SIZE_LIMIT { + Err(TransactionError::TooLargeTransaction)?; + } + tx.verify()?; match tx.kind() { - TransactionKind::Provided => { - let hash = tx.hash(); - if !locally_provided.remove(&hash) { - Err(TransactionError::MissingProvided(hash))?; - } - } + TransactionKind::Provided => {} TransactionKind::Unsigned => {} TransactionKind::Signed(Signed { signer, nonce, signature }) => { if let Some(next_nonce) = next_nonces.get(signer) {