diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 4b86d80c8..4576d4d19 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -31,7 +31,9 @@ use tokio::{ time::sleep, }; -use ::tributary::{ProvidedError, TransactionKind, TransactionTrait, Block, Tributary}; +use ::tributary::{ + ProvidedError, TransactionKind, TransactionError, TransactionTrait, Block, Tributary, +}; mod tributary; use crate::tributary::{ @@ -150,10 +152,16 @@ async fn publish_signed_transaction( .await .expect("we don't have a nonce, meaning we aren't a participant on this tributary"), ) { - // TODO: Assert if we didn't create a valid transaction // We need to return a proper error here to enable that, due to a race condition around // multiple publications - tributary.add_transaction(tx).await; + match tributary.add_transaction(tx.clone()).await { + Ok(_) => {} + // Some asynchonicity if InvalidNonce, assumed safe to deterministic nonces + Err(TransactionError::InvalidNonce) => { + log::warn!("publishing TX {tx:?} returned InvalidNonce. was it already added?") + } + Err(e) => panic!("created an invalid transaction: {e:?}"), + } } } @@ -630,10 +638,10 @@ async fn handle_processor_message( } TransactionKind::Unsigned => { log::trace!("publishing unsigned transaction {}", hex::encode(tx.hash())); - // Ignores the result since we can't differentiate already in-mempool from - // already on-chain from invalid - // TODO: Don't ignore the result - tributary.add_transaction(tx).await; + match tributary.add_transaction(tx.clone()).await { + Ok(_) => {} + Err(e) => panic!("created an invalid unsigned transaction: {e:?}"), + } } TransactionKind::Signed(_) => { log::trace!("getting next nonce for Tributary TX in response to processor message"); diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index aa8ea7c94..433faad34 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/coordinator/src/tests/tributary/dkg.rs @@ -56,7 +56,7 @@ async fn dkg_test() { // Publish all commitments but one for (i, tx) in txs.iter().enumerate().skip(1) { - assert!(tributaries[i].1.add_transaction(tx.clone()).await); + assert_eq!(tributaries[i].1.add_transaction(tx.clone()).await, Ok(true)); } // Wait until these are included @@ -104,7 +104,7 @@ async fn dkg_test() { // Publish the last commitment let block_before_tx = tributaries[0].1.tip().await; - assert!(tributaries[0].1.add_transaction(txs[0].clone()).await); + assert_eq!(tributaries[0].1.add_transaction(txs[0].clone()).await, Ok(true)); wait_for_tx_inclusion(&tributaries[0].1, block_before_tx, txs[0].hash()).await; sleep(Duration::from_secs(Tributary::::block_time().into())).await; @@ -181,7 +181,7 @@ async fn dkg_test() { let block_before_tx = tributaries[0].1.tip().await; for (i, tx) in txs.iter().enumerate().skip(1) { - assert!(tributaries[i].1.add_transaction(tx.clone()).await); + assert_eq!(tributaries[i].1.add_transaction(tx.clone()).await, Ok(true)); } for tx in txs.iter().skip(1) { wait_for_tx_inclusion(&tributaries[0].1, block_before_tx, tx.hash()).await; @@ -205,7 +205,7 @@ async fn dkg_test() { // Publish the final set of shares let block_before_tx = tributaries[0].1.tip().await; - assert!(tributaries[0].1.add_transaction(txs[0].clone()).await); + assert_eq!(tributaries[0].1.add_transaction(txs[0].clone()).await, Ok(true)); wait_for_tx_inclusion(&tributaries[0].1, block_before_tx, txs[0].hash()).await; sleep(Duration::from_secs(Tributary::::block_time().into())).await; @@ -296,7 +296,7 @@ async fn dkg_test() { } let block_before_tx = tributaries[0].1.tip().await; for (i, tx) in txs.iter().enumerate() { - assert!(tributaries[i].1.add_transaction(tx.clone()).await); + assert_eq!(tributaries[i].1.add_transaction(tx.clone()).await, Ok(true)); } for tx in txs.iter() { wait_for_tx_inclusion(&tributaries[0].1, block_before_tx, tx.hash()).await; diff --git a/coordinator/src/tests/tributary/tx.rs b/coordinator/src/tests/tributary/tx.rs index 3030834d7..0d2124dfb 100644 --- a/coordinator/src/tests/tributary/tx.rs +++ b/coordinator/src/tests/tributary/tx.rs @@ -43,7 +43,7 @@ async fn tx_test() { Transaction::DkgCommitments(attempt, commitments.clone(), Transaction::empty_signed()); tx.sign(&mut OsRng, spec.genesis(), &key, 0); - assert!(tributaries[sender].1.add_transaction(tx.clone()).await); + assert_eq!(tributaries[sender].1.add_transaction(tx.clone()).await, Ok(true)); let included_in = wait_for_tx_inclusion(&tributaries[sender].1, block_before_tx, tx.hash()).await; // Also sleep for the block time to ensure the block is synced around before we run checks on it sleep(Duration::from_secs(Tributary::::block_time().into())).await; diff --git a/coordinator/tributary/src/blockchain.rs b/coordinator/tributary/src/blockchain.rs index 93d328a2a..3be658e01 100644 --- a/coordinator/tributary/src/blockchain.rs +++ b/coordinator/tributary/src/blockchain.rs @@ -10,7 +10,7 @@ use tendermint::ext::{Network, Commit}; use crate::{ ReadWrite, ProvidedError, ProvidedTransactions, BlockError, Block, Mempool, Transaction, - transaction::{Signed, TransactionKind, Transaction as TransactionTrait}, + transaction::{Signed, TransactionKind, TransactionError, Transaction as TransactionTrait}, }; #[derive(Debug)] @@ -165,7 +165,7 @@ impl Blockchain { internal: bool, tx: Transaction, schema: N::SignatureScheme, - ) -> bool { + ) -> Result { let db = self.db.as_ref().unwrap(); let genesis = self.genesis; diff --git a/coordinator/tributary/src/lib.rs b/coordinator/tributary/src/lib.rs index 0d4d96ce3..854c042b3 100644 --- a/coordinator/tributary/src/lib.rs +++ b/coordinator/tributary/src/lib.rs @@ -256,10 +256,10 @@ impl Tributary { self.network.blockchain.read().await.next_nonce(signer) } - // Returns if the transaction was new and valid. + // Returns Ok(true) if new, Ok(false) if an already present unsigned, or the error. // Safe to be &self since the only meaningful usage of self is self.network.blockchain which // successfully acquires its own write lock - pub async fn add_transaction(&self, tx: T) -> bool { + pub async fn add_transaction(&self, tx: T) -> Result { let tx = Transaction::Application(tx); let mut to_broadcast = vec![TRANSACTION_MESSAGE]; tx.write(&mut to_broadcast).unwrap(); @@ -268,7 +268,7 @@ impl Tributary { tx, self.network.signature_scheme(), ); - if res { + if res == Ok(true) { self.network.p2p.broadcast(self.genesis, to_broadcast).await; } res @@ -339,8 +339,8 @@ impl Tributary { tx, self.network.signature_scheme(), ); - log::debug!("received transaction message. valid new transaction: {res}"); - res + log::debug!("received transaction message. valid new transaction: {res:?}"); + res == Ok(true) } Some(&TENDERMINT_MESSAGE) => { diff --git a/coordinator/tributary/src/mempool.rs b/coordinator/tributary/src/mempool.rs index 988702e6a..00d182a4b 100644 --- a/coordinator/tributary/src/mempool.rs +++ b/coordinator/tributary/src/mempool.rs @@ -8,7 +8,9 @@ use tendermint::ext::{Network, Commit}; use crate::{ ACCOUNT_MEMPOOL_LIMIT, ReadWrite, - transaction::{Signed, TransactionKind, Transaction as TransactionTrait, verify_transaction}, + transaction::{ + Signed, TransactionKind, TransactionError, Transaction as TransactionTrait, verify_transaction, + }, tendermint::tx::verify_tendermint_tx, Transaction, }; @@ -92,7 +94,7 @@ impl Mempool { res } - /// Returns true if this is a valid, new transaction. + // Returns Ok(true) if new, Ok(false) if an already present unsigned, or the error. pub(crate) fn add( &mut self, blockchain_next_nonces: &HashMap<::G, u32>, @@ -101,7 +103,7 @@ impl Mempool { schema: N::SignatureScheme, unsigned_in_chain: impl Fn([u8; 32]) -> bool, commit: impl Fn(u32) -> Option>, - ) -> bool { + ) -> Result { match &tx { Transaction::Tendermint(tendermint_tx) => { // All Tendermint transactions should be unsigned @@ -109,13 +111,11 @@ impl Mempool { // check we have the tx in the pool/chain if self.unsigned_already_exist(tx.hash(), unsigned_in_chain) { - return false; + return Ok(false); } // verify the tx - if verify_tendermint_tx::(tendermint_tx, schema, commit).is_err() { - return false; - } + verify_tendermint_tx::(tendermint_tx, schema, commit)?; } Transaction::Application(app_tx) => { match app_tx.kind() { @@ -123,7 +123,7 @@ impl Mempool { // Get the nonce from the blockchain let Some(blockchain_next_nonce) = blockchain_next_nonces.get(signer).cloned() else { // Not a participant - return false; + Err(TransactionError::InvalidSigner)? }; // If the blockchain's nonce is greater than the mempool's, use it @@ -140,32 +140,28 @@ impl Mempool { // 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; + Err(TransactionError::TooManyInMempool)?; } - if verify_transaction(app_tx, self.genesis, &mut self.next_nonces).is_err() { - return false; - } + verify_transaction(app_tx, self.genesis, &mut self.next_nonces)?; debug_assert_eq!(self.next_nonces[signer], nonce + 1); } TransactionKind::Unsigned => { // check we have the tx in the pool/chain if self.unsigned_already_exist(tx.hash(), unsigned_in_chain) { - return false; + return Ok(false); } - if app_tx.verify().is_err() { - return false; - } + app_tx.verify()?; } - TransactionKind::Provided(_) => return false, + TransactionKind::Provided(_) => Err(TransactionError::ProvidedAddedToMempool)?, } } } // Save the TX to the pool self.save_tx(tx); - true + Ok(true) } // Returns None if the mempool doesn't have a nonce tracked. diff --git a/coordinator/tributary/src/tendermint/mod.rs b/coordinator/tributary/src/tendermint/mod.rs index 2861b2591..67e0021b3 100644 --- a/coordinator/tributary/src/tendermint/mod.rs +++ b/coordinator/tributary/src/tendermint/mod.rs @@ -349,7 +349,8 @@ impl Network for TendermintNetwork true, Transaction::Tendermint(tx), self.signature_scheme(), - ) { + ) == Ok(true) + { self.p2p.broadcast(signer.genesis, to_broadcast).await; } } diff --git a/coordinator/tributary/src/tests/blockchain.rs b/coordinator/tributary/src/tests/blockchain.rs index 43d7ad608..70a8c48c3 100644 --- a/coordinator/tributary/src/tests/blockchain.rs +++ b/coordinator/tributary/src/tests/blockchain.rs @@ -105,11 +105,9 @@ fn invalid_block() { { // Add a valid transaction let (_, mut blockchain) = new_blockchain(genesis, &[tx.1.signer]); - assert!(blockchain.add_transaction::( - true, - Transaction::Application(tx.clone()), - validators.clone() - )); + blockchain + .add_transaction::(true, Transaction::Application(tx.clone()), validators.clone()) + .unwrap(); let mut block = blockchain.build_block::(validators.clone()); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); blockchain.verify_block::(&block, validators.clone(), false).unwrap(); @@ -130,11 +128,9 @@ fn invalid_block() { { // Invalid signature let (_, mut blockchain) = new_blockchain(genesis, &[tx.1.signer]); - assert!(blockchain.add_transaction::( - true, - Transaction::Application(tx), - validators.clone() - )); + blockchain + .add_transaction::(true, Transaction::Application(tx), validators.clone()) + .unwrap(); let mut block = blockchain.build_block::(validators.clone()); blockchain.verify_block::(&block, validators.clone(), false).unwrap(); match &mut block.transactions[0] { @@ -170,11 +166,9 @@ fn signed_transaction() { panic!("tendermint tx found"); }; let next_nonce = blockchain.next_nonce(signer).unwrap(); - assert!(blockchain.add_transaction::( - true, - Transaction::Application(tx), - validators.clone() - )); + blockchain + .add_transaction::(true, Transaction::Application(tx), validators.clone()) + .unwrap(); assert_eq!(next_nonce + 1, blockchain.next_nonce(signer).unwrap()); } let block = blockchain.build_block::(validators.clone()); @@ -363,11 +357,9 @@ async fn tendermint_evidence_tx() { let Transaction::Tendermint(tx) = tx else { panic!("non-tendermint tx found"); }; - assert!(blockchain.add_transaction::( - true, - Transaction::Tendermint(tx), - validators.clone() - )); + blockchain + .add_transaction::(true, Transaction::Tendermint(tx), validators.clone()) + .unwrap(); } let block = blockchain.build_block::(validators.clone()); assert_eq!(blockchain.tip(), tip); @@ -475,7 +467,7 @@ async fn block_tx_ordering() { let signed_tx = Transaction::Application(SignedTx::Signed(Box::new( crate::tests::signed_transaction(&mut OsRng, genesis, &key, i), ))); - assert!(blockchain.add_transaction::(true, signed_tx.clone(), validators.clone())); + blockchain.add_transaction::(true, signed_tx.clone(), validators.clone()).unwrap(); mempool.push(signed_tx); let unsigned_tx = Transaction::Tendermint( @@ -485,7 +477,7 @@ async fn block_tx_ordering() { ) .await, ); - assert!(blockchain.add_transaction::(true, unsigned_tx.clone(), validators.clone())); + blockchain.add_transaction::(true, unsigned_tx.clone(), validators.clone()).unwrap(); mempool.push(unsigned_tx); let provided_tx = diff --git a/coordinator/tributary/src/tests/mempool.rs b/coordinator/tributary/src/tests/mempool.rs index 20b69cc7c..3d3215078 100644 --- a/coordinator/tributary/src/tests/mempool.rs +++ b/coordinator/tributary/src/tests/mempool.rs @@ -10,7 +10,7 @@ use tendermint::ext::Commit; use serai_db::MemDb; use crate::{ - transaction::Transaction as TransactionTrait, + transaction::{TransactionError, Transaction as TransactionTrait}, tendermint::{TendermintBlock, Validators, Signer, TendermintNetwork}, ACCOUNT_MEMPOOL_LIMIT, Transaction, Mempool, tests::{SignedTransaction, signed_transaction, p2p::DummyP2p, random_evidence_tx}, @@ -43,69 +43,85 @@ async fn mempool_addition() { // Add TX 0 let mut blockchain_next_nonces = HashMap::from([(signer, 0)]); - assert!(mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Application(first_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); + assert!(mempool + .add::( + &blockchain_next_nonces, + true, + Transaction::Application(first_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ) + .unwrap()); assert_eq!(mempool.next_nonce(&signer), Some(1)); // add a tendermint evidence tx let evidence_tx = random_evidence_tx::(Signer::new(genesis, key.clone()).into(), TendermintBlock(vec![])) .await; - assert!(mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Tendermint(evidence_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); + assert!(mempool + .add::( + &blockchain_next_nonces, + true, + Transaction::Tendermint(evidence_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ) + .unwrap()); // Test reloading works assert_eq!(mempool, Mempool::new(db, genesis)); - // Adding it again should fail - assert!(!mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Application(first_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); - assert!(!mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Tendermint(evidence_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); + // Adding them again should fail + assert_eq!( + mempool.add::( + &blockchain_next_nonces, + true, + Transaction::Application(first_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ), + Err(TransactionError::InvalidNonce) + ); + assert_eq!( + mempool.add::( + &blockchain_next_nonces, + true, + Transaction::Tendermint(evidence_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ), + Ok(false) + ); // Do the same with the next nonce let second_tx = signed_transaction(&mut OsRng, genesis, &key, 1); - assert!(mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Application(second_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); + assert_eq!( + mempool.add::( + &blockchain_next_nonces, + true, + Transaction::Application(second_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ), + Ok(true) + ); assert_eq!(mempool.next_nonce(&signer), Some(2)); - assert!(!mempool.add::( - &blockchain_next_nonces, - true, - Transaction::Application(second_tx.clone()), - validators.clone(), - unsigned_in_chain, - commit, - )); + assert_eq!( + mempool.add::( + &blockchain_next_nonces, + true, + Transaction::Application(second_tx.clone()), + validators.clone(), + unsigned_in_chain, + commit, + ), + Err(TransactionError::InvalidNonce) + ); // If the mempool doesn't have a nonce for an account, it should successfully use the // blockchain's @@ -114,14 +130,16 @@ async 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, - true, - Transaction::Application(tx.clone()), - validators.clone(), - unsigned_in_chain, - commit - )); + assert!(mempool + .add::( + &blockchain_next_nonces, + true, + Transaction::Application(tx.clone()), + validators.clone(), + unsigned_in_chain, + commit + ) + .unwrap()); assert_eq!(mempool.next_nonce(&second_signer), Some(3)); // Getting a block should work @@ -159,22 +177,32 @@ fn too_many_mempool() { // We should be able to add transactions up to the limit for i in 0 .. ACCOUNT_MEMPOOL_LIMIT { - assert!(mempool.add::( + assert!(mempool + .add::( + &HashMap::from([(signer, 0)]), + false, + Transaction::Application(signed_transaction(&mut OsRng, genesis, &key, i)), + validators.clone(), + unsigned_in_chain, + commit, + ) + .unwrap()); + } + // Yet adding more should fail + assert_eq!( + mempool.add::( &HashMap::from([(signer, 0)]), false, - Transaction::Application(signed_transaction(&mut OsRng, genesis, &key, i)), + Transaction::Application(signed_transaction( + &mut OsRng, + genesis, + &key, + ACCOUNT_MEMPOOL_LIMIT + )), validators.clone(), unsigned_in_chain, commit, - )); - } - // Yet adding more should fail - assert!(!mempool.add::( - &HashMap::from([(signer, 0)]), - false, - Transaction::Application(signed_transaction(&mut OsRng, genesis, &key, ACCOUNT_MEMPOOL_LIMIT)), - validators.clone(), - unsigned_in_chain, - commit, - )); + ), + Err(TransactionError::TooManyInMempool) + ); } diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index 63271aeab..c9c5ba1a6 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -31,6 +31,12 @@ pub enum TransactionError { /// Transaction's content is invalid. #[error("transaction content is invalid")] InvalidContent, + /// Transaction's signer has too many transactions in the mempool. + #[error("signer has too many transactions in the mempool")] + TooManyInMempool, + /// Provided Transaction added to mempool. + #[error("provided transaction added to mempool")] + ProvidedAddedToMempool, } /// Data for a signed transaction. diff --git a/coordinator/tributary/tendermint/src/ext.rs b/coordinator/tributary/tendermint/src/ext.rs index 02c6dd28b..1f95362d1 100644 --- a/coordinator/tributary/tendermint/src/ext.rs +++ b/coordinator/tributary/tendermint/src/ext.rs @@ -282,7 +282,6 @@ pub trait Network: Sized + Send + Sync { /// Trigger a slash for the validator in question who was definitively malicious. /// /// The exact process of triggering a slash is undefined and left to the network as a whole. - // TODO: We need to provide some evidence for this. async fn slash(&mut self, validator: Self::ValidatorId, slash_event: SlashEvent); /// Validate a block.