diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index f97a2237..2171ba24 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -649,6 +649,18 @@ async fn handle_processor_messages( log::trace!("providing transaction {}", hex::encode(tx.hash())); let res = tributary.tributary.provide_transaction(tx).await; if !(res.is_ok() || (res == Err(ProvidedError::AlreadyProvided))) { + if res == Err(ProvidedError::LocalMismatchesOnChain) { + // Spin, since this is a crit for this Tributary + loop { + log::error!( + "{}. tributary: {}, provided: SubstrateBlock({})", + "tributary added distinct provided to delayed locally provided TX", + hex::encode(tributary.spec.genesis()), + block, + ); + sleep(Duration::from_secs(60)).await; + } + } panic!("provided an invalid transaction: {res:?}"); } } @@ -1019,8 +1031,20 @@ async fn handle_processor_messages( match tx.kind() { TransactionKind::Provided(_) => { log::trace!("providing transaction {}", hex::encode(tx.hash())); - let res = tributary.provide_transaction(tx).await; + let res = tributary.provide_transaction(tx.clone()).await; if !(res.is_ok() || (res == Err(ProvidedError::AlreadyProvided))) { + if res == Err(ProvidedError::LocalMismatchesOnChain) { + // Spin, since this is a crit for this Tributary + loop { + log::error!( + "{}. tributary: {}, provided: {:?}", + "tributary added distinct provided to delayed locally provided TX", + hex::encode(spec.genesis()), + &tx, + ); + sleep(Duration::from_secs(60)).await; + } + } panic!("provided an invalid transaction: {res:?}"); } } diff --git a/coordinator/src/tributary/scanner.rs b/coordinator/src/tributary/scanner.rs index 5d8f0016..ee254ff4 100644 --- a/coordinator/src/tributary/scanner.rs +++ b/coordinator/src/tributary/scanner.rs @@ -7,7 +7,7 @@ use ciphersuite::{Ciphersuite, Ristretto}; use serai_client::{validator_sets::primitives::ValidatorSet, subxt::utils::Encoded}; use tributary::{ - Transaction as TributaryTransaction, Block, TributaryReader, + TransactionKind, Transaction as TributaryTransaction, Block, TributaryReader, tendermint::{ tx::{TendermintTx, decode_evidence}, TendermintNetwork, @@ -120,6 +120,20 @@ pub(crate) async fn handle_new_blocks< let mut last_block = db.last_block(genesis); while let Some(next) = tributary.block_after(&last_block) { let block = tributary.block(&next).unwrap(); + + // Make sure we have all of the provided transactions for this block + for tx in &block.transactions { + // Provided TXs will appear first in the Block, so we can break after we hit a non-Provided + let TransactionKind::Provided(order) = tx.kind() else { + break; + }; + + // make sure we have all the provided txs in this block locally + if !tributary.locally_provided_txs_in_block(&block.hash(), order) { + return; + } + } + handle_block::<_, _, _, _, _, _, P>( db, key, diff --git a/coordinator/tributary/src/block.rs b/coordinator/tributary/src/block.rs index 0a7a2f25..06b3a5a3 100644 --- a/coordinator/tributary/src/block.rs +++ b/coordinator/tributary/src/block.rs @@ -33,7 +33,10 @@ pub enum BlockError { /// An unsigned transaction which was already added to the chain was present again. #[error("an unsigned transaction which was already added to the chain was present again")] UnsignedAlreadyIncluded, - /// Transactions weren't ordered as expected (Provided, followed by Unsigned, folowed by Signed). + /// A provided transaction which was already added to the chain was present again. + #[error("an provided transaction which was already added to the chain was present again")] + ProvidedAlreadyIncluded, + /// Transactions weren't ordered as expected (Provided, followed by Unsigned, followed by Signed). #[error("transactions weren't ordered as expected (Provided, Unsigned, Signed)")] WrongTransactionOrder, /// The block had a provided transaction this validator has yet to be provided. @@ -175,6 +178,8 @@ impl Block { schema: N::SignatureScheme, commit: impl Fn(u32) -> Option>, unsigned_in_chain: impl Fn([u8; 32]) -> bool, + provided_in_chain: impl Fn([u8; 32]) -> bool, // TODO: merge this with unsigned_on_chain? + allow_non_local_provided: bool, ) -> Result<(), BlockError> { #[derive(Clone, Copy, PartialEq, Eq, Debug)] enum Order { @@ -209,18 +214,22 @@ impl Block { let current_tx_order = match tx.kind() { TransactionKind::Provided(order) => { - let Some(local) = locally_provided.get_mut(order).and_then(|deque| deque.pop_front()) - else { - Err(BlockError::NonLocalProvided(txs.pop().unwrap()))? - }; - // Since this was a provided TX, it must be an application TX - let Transaction::Application(tx) = tx else { - Err(BlockError::NonLocalProvided(txs.pop().unwrap()))? - }; - if tx != &local { - Err(BlockError::DistinctProvided)?; + if provided_in_chain(tx_hash) { + Err(BlockError::ProvidedAlreadyIncluded)?; } + if let Some(local) = locally_provided.get_mut(order).and_then(|deque| deque.pop_front()) { + // Since this was a provided TX, it must be an application TX + let Transaction::Application(tx) = tx else { + Err(BlockError::NonLocalProvided(txs.pop().unwrap()))? + }; + if tx != &local { + Err(BlockError::DistinctProvided)?; + } + } else if !allow_non_local_provided { + Err(BlockError::NonLocalProvided(txs.pop().unwrap()))? + }; + Order::Provided } TransactionKind::Unsigned => { @@ -241,12 +250,6 @@ impl Block { } last_tx_order = current_tx_order; - if current_tx_order == Order::Provided { - // 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; - } - // TODO: should we modify the verify_transaction to take `Transaction` or // use this pattern of verifying tendermint Txs and app txs differently? match tx { diff --git a/coordinator/tributary/src/blockchain.rs b/coordinator/tributary/src/blockchain.rs index d21928ec..93d328a2 100644 --- a/coordinator/tributary/src/blockchain.rs +++ b/coordinator/tributary/src/blockchain.rs @@ -50,6 +50,9 @@ impl Blockchain { fn unsigned_included_key(genesis: &[u8], hash: &[u8; 32]) -> Vec { D::key(b"tributary_blockchain", b"unsigned_included", [genesis, hash].concat()) } + fn provided_included_key(genesis: &[u8], hash: &[u8; 32]) -> Vec { + D::key(b"tributary_blockchain", b"provided_included", [genesis, hash].concat()) + } fn next_nonce_key(&self, signer: &::G) -> Vec { D::key( b"tributary_blockchain", @@ -136,6 +139,23 @@ impl Blockchain { db.get(Self::block_after_key(&genesis, block)).map(|bytes| bytes.try_into().unwrap()) } + pub(crate) fn locally_provided_txs_in_block( + db: &D, + genesis: &[u8; 32], + block: &[u8; 32], + order: &str, + ) -> bool { + let local_key = ProvidedTransactions::::locally_provided_quantity_key(genesis, order); + let local = + db.get(local_key).map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())).unwrap_or(0); + let block_key = + ProvidedTransactions::::block_provided_quantity_key(genesis, block, order); + let block = + db.get(block_key).map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())).unwrap_or(0); + + local >= block + } + pub(crate) fn tip_from_db(db: &D, genesis: [u8; 32]) -> [u8; 32] { db.get(Self::tip_key(genesis)).map(|bytes| bytes.try_into().unwrap()).unwrap_or(genesis) } @@ -182,7 +202,7 @@ impl Blockchain { self.mempool.block(&self.next_nonces, unsigned_in_chain), ); // build_block should not return invalid blocks - self.verify_block::(&block, schema).unwrap(); + self.verify_block::(&block, schema, false).unwrap(); block } @@ -190,10 +210,13 @@ impl Blockchain { &self, block: &Block, schema: N::SignatureScheme, + allow_non_local_provided: bool, ) -> Result<(), BlockError> { let db = self.db.as_ref().unwrap(); let unsigned_in_chain = |hash: [u8; 32]| db.get(Self::unsigned_included_key(&self.genesis, &hash)).is_some(); + let provided_in_chain = + |hash: [u8; 32]| db.get(Self::provided_included_key(&self.genesis, &hash)).is_some(); let commit = |block: u32| -> Option> { let commit = self.commit_by_block_number(block)?; // commit has to be valid if it is coming from our db @@ -207,6 +230,8 @@ impl Blockchain { schema, &commit, unsigned_in_chain, + provided_in_chain, + allow_non_local_provided, ) } @@ -217,7 +242,7 @@ impl Blockchain { commit: Vec, schema: N::SignatureScheme, ) -> Result<(), BlockError> { - self.verify_block::(block, schema)?; + self.verify_block::(block, schema, true)?; log::info!( "adding block {} to tributary {} with {} TXs", @@ -249,7 +274,9 @@ impl Blockchain { for tx in &block.transactions { match tx.kind() { TransactionKind::Provided(order) => { - self.provided.complete(&mut txn, order, tx.hash()); + let hash = tx.hash(); + self.provided.complete(&mut txn, order, self.tip, hash); + txn.put(Self::provided_included_key(&self.genesis, &hash), []); } TransactionKind::Unsigned => { let hash = tx.hash(); diff --git a/coordinator/tributary/src/lib.rs b/coordinator/tributary/src/lib.rs index 9d8094f2..d9f50097 100644 --- a/coordinator/tributary/src/lib.rs +++ b/coordinator/tributary/src/lib.rs @@ -397,6 +397,10 @@ impl TributaryReader { .map(|commit| Commit::::decode(&mut commit.as_ref()).unwrap().end_time) } + pub fn locally_provided_txs_in_block(&self, hash: &[u8; 32], order: &str) -> bool { + Blockchain::::locally_provided_txs_in_block(&self.0, &self.1, hash, order) + } + // This isn't static, yet can be read with only minor discrepancy risks pub fn tip(&self) -> [u8; 32] { Blockchain::::tip_from_db(&self.0, self.1) diff --git a/coordinator/tributary/src/provided.rs b/coordinator/tributary/src/provided.rs index 0bf284d4..e4e8193c 100644 --- a/coordinator/tributary/src/provided.rs +++ b/coordinator/tributary/src/provided.rs @@ -11,12 +11,15 @@ pub enum ProvidedError { /// The provided transaction's kind wasn't Provided #[error("transaction wasn't a provided transaction")] NotProvided, - /// The provided transaction was invalid. + /// The provided transaction was invalid #[error("provided transaction was invalid")] InvalidProvided(TransactionError), /// Transaction was already provided #[error("transaction was already provided")] AlreadyProvided, + /// Local transaction mismatches the on-chain provided + #[error("local provides mismatches on-chain provided")] + LocalMismatchesOnChain, } #[derive(Clone, PartialEq, Eq, Debug)] @@ -34,6 +37,27 @@ impl ProvidedTransactions { fn current_provided_key(&self) -> Vec { D::key(b"tributary_provided", b"current", self.genesis) } + pub(crate) fn locally_provided_quantity_key(genesis: &[u8; 32], order: &str) -> Vec { + D::key(b"tributary_provided", b"local_quantity", [genesis, order.as_bytes()].concat()) + } + pub(crate) fn on_chain_provided_quantity_key(genesis: &[u8; 32], order: &str) -> Vec { + D::key(b"tributary_provided", b"on_chain_quantity", [genesis, order.as_bytes()].concat()) + } + pub(crate) fn block_provided_quantity_key( + genesis: &[u8; 32], + block: &[u8; 32], + order: &str, + ) -> Vec { + D::key(b"tributary_provided", b"block_quantity", [genesis, block, order.as_bytes()].concat()) + } + + pub(crate) fn on_chain_provided_key(genesis: &[u8; 32], order: &str, id: u32) -> Vec { + D::key( + b"tributary_provided", + b"on_chain_tx", + [genesis, order.as_bytes(), &id.to_le_bytes()].concat(), + ) + } pub(crate) fn new(db: D, genesis: [u8; 32]) -> Self { let mut res = ProvidedTransactions { db, genesis, transactions: HashMap::new() }; @@ -69,27 +93,61 @@ impl ProvidedTransactions { Ok(()) => {} Err(e) => Err(ProvidedError::InvalidProvided(e))?, } - let tx_hash = tx.hash(); + + // Check it wasn't already provided let provided_key = self.transaction_key(&tx_hash); if self.db.get(&provided_key).is_some() { Err(ProvidedError::AlreadyProvided)?; } - let current_provided_key = self.current_provided_key(); - #[allow(clippy::unwrap_or_default)] - let mut currently_provided = self.db.get(¤t_provided_key).unwrap_or(vec![]); + // get local and on-chain tx numbers + let local_key = Self::locally_provided_quantity_key(&self.genesis, order); + let mut local_quantity = self + .db + .get(&local_key) + .map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())) + .unwrap_or(0); + let on_chain_key = Self::on_chain_provided_quantity_key(&self.genesis, order); + let on_chain_quantity = self + .db + .get(on_chain_key) + .map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())) + .unwrap_or(0); + let current_provided_key = self.current_provided_key(); + + // This would have a race-condition with multiple calls to provide, though this takes &mut self + // peventing multiple calls at once let mut txn = self.db.txn(); txn.put(provided_key, tx.serialize()); - currently_provided.extend(tx_hash); - txn.put(current_provided_key, currently_provided); - txn.commit(); - if self.transactions.get(order).is_none() { - self.transactions.insert(order, VecDeque::new()); + let this_provided_id = local_quantity; + + local_quantity += 1; + txn.put(local_key, local_quantity.to_le_bytes()); + + if this_provided_id < on_chain_quantity { + // Verify against the on-chain version + if tx_hash.as_ref() != + txn.get(Self::on_chain_provided_key(&self.genesis, order, this_provided_id)).unwrap() + { + Err(ProvidedError::LocalMismatchesOnChain)?; + } + txn.commit(); + } else { + #[allow(clippy::unwrap_or_default)] + let mut currently_provided = txn.get(¤t_provided_key).unwrap_or(vec![]); + currently_provided.extend(tx_hash); + txn.put(current_provided_key, currently_provided); + txn.commit(); + + if self.transactions.get(order).is_none() { + self.transactions.insert(order, VecDeque::new()); + } + self.transactions.get_mut(order).unwrap().push_back(tx); } - self.transactions.get_mut(order).unwrap().push_back(tx); + Ok(()) } @@ -98,27 +156,46 @@ impl ProvidedTransactions { &mut self, txn: &mut D::Transaction<'_>, order: &'static str, + block: [u8; 32], tx: [u8; 32], ) { - assert_eq!(self.transactions.get_mut(order).unwrap().pop_front().unwrap().hash(), tx); + if let Some(next_tx) = self.transactions.get_mut(order).and_then(|queue| queue.pop_front()) { + assert_eq!(next_tx.hash(), tx); - let current_provided_key = self.current_provided_key(); - let mut currently_provided = txn.get(¤t_provided_key).unwrap(); + let current_provided_key = self.current_provided_key(); + let mut currently_provided = txn.get(¤t_provided_key).unwrap(); - // Find this TX's hash - let mut i = 0; - loop { - if currently_provided[i .. (i + 32)] == tx { - assert_eq!(¤tly_provided.drain(i .. (i + 32)).collect::>(), &tx); - break; + // Find this TX's hash + let mut i = 0; + loop { + if currently_provided[i .. (i + 32)] == tx { + assert_eq!(¤tly_provided.drain(i .. (i + 32)).collect::>(), &tx); + break; + } + + i += 32; + if i >= currently_provided.len() { + panic!("couldn't find completed TX in currently provided"); + } } - i += 32; - if i >= currently_provided.len() { - panic!("couldn't find completed TX in currently provided"); - } + txn.put(current_provided_key, currently_provided); } - txn.put(current_provided_key, currently_provided); + // bump the on-chain tx number. + let on_chain_key = Self::on_chain_provided_quantity_key(&self.genesis, order); + let block_order_key = Self::block_provided_quantity_key(&self.genesis, &block, order); + let mut on_chain_quantity = self + .db + .get(&on_chain_key) + .map(|bytes| u32::from_le_bytes(bytes.try_into().unwrap())) + .unwrap_or(0); + + let this_provided_id = on_chain_quantity; + txn.put(Self::on_chain_provided_key(&self.genesis, order, this_provided_id), tx); + + on_chain_quantity += 1; + txn.put(on_chain_key, on_chain_quantity.to_le_bytes()); + txn.put(block_order_key, on_chain_quantity.to_le_bytes()); } } diff --git a/coordinator/tributary/src/tendermint/mod.rs b/coordinator/tributary/src/tendermint/mod.rs index fad9b3e8..2861b259 100644 --- a/coordinator/tributary/src/tendermint/mod.rs +++ b/coordinator/tributary/src/tendermint/mod.rs @@ -35,10 +35,7 @@ use tendermint::{ SlashEvent, }; -use tokio::{ - sync::RwLock, - time::{Duration, sleep}, -}; +use tokio::sync::RwLock; use crate::{ TENDERMINT_MESSAGE, TRANSACTION_MESSAGE, BLOCK_MESSAGE, ReadWrite, @@ -360,12 +357,15 @@ impl Network for TendermintNetwork async fn validate(&mut self, block: &Self::Block) -> Result<(), TendermintBlockError> { let block = Block::read::<&[u8]>(&mut block.0.as_ref()).map_err(|_| TendermintBlockError::Fatal)?; - self.blockchain.read().await.verify_block::(&block, self.signature_scheme()).map_err( - |e| match e { + self + .blockchain + .read() + .await + .verify_block::(&block, self.signature_scheme(), false) + .map_err(|e| match e { BlockError::NonLocalProvided(_) => TendermintBlockError::Temporal, _ => TendermintBlockError::Fatal, - }, - ) + }) } async fn add_block( @@ -412,9 +412,6 @@ impl Network for TendermintNetwork hex::encode(hash), hex::encode(self.genesis) ); - // TODO: Use a notification system for when we have a new provided, in order to minimize - // latency - sleep(Duration::from_secs(Self::block_time().into())).await; } _ => return invalid_block(), } diff --git a/coordinator/tributary/src/tests/block.rs b/coordinator/tributary/src/tests/block.rs index 2bc4b823..67b2c27a 100644 --- a/coordinator/tributary/src/tests/block.rs +++ b/coordinator/tributary/src/tests/block.rs @@ -82,6 +82,7 @@ fn empty_block() { Some(Commit::> { end_time: 0, validators: vec![], signature: vec![] }) }; let unsigned_in_chain = |_: [u8; 32]| false; + let provided_in_chain = |_: [u8; 32]| false; Block::::new(LAST, vec![], vec![]) .verify::( GENESIS, @@ -91,6 +92,8 @@ fn empty_block() { validators, commit, unsigned_in_chain, + provided_in_chain, + false, ) .unwrap(); } @@ -114,6 +117,7 @@ fn duplicate_nonces() { Some(Commit::> { end_time: 0, validators: vec![], signature: vec![] }) }; let unsigned_in_chain = |_: [u8; 32]| false; + let provided_in_chain = |_: [u8; 32]| false; let res = Block::new(LAST, vec![], mempool).verify::( GENESIS, @@ -123,6 +127,8 @@ fn duplicate_nonces() { validators.clone(), commit, unsigned_in_chain, + provided_in_chain, + false, ); if i == 1 { res.unwrap(); diff --git a/coordinator/tributary/src/tests/blockchain.rs b/coordinator/tributary/src/tests/blockchain.rs index 21051095..43d7ad60 100644 --- a/coordinator/tributary/src/tests/blockchain.rs +++ b/coordinator/tributary/src/tests/blockchain.rs @@ -48,7 +48,7 @@ fn block_addition() { assert_eq!(block.header.parent, genesis); assert_eq!(block.header.transactions, [0; 32]); - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); assert!(blockchain.add_block::(&block, vec![], validators).is_ok()); assert_eq!(blockchain.tip(), block.hash()); assert_eq!(blockchain.block_number(), 1); @@ -71,14 +71,14 @@ fn invalid_block() { #[allow(clippy::redundant_clone)] // False positive let mut block = block.clone(); block.header.parent = Blake2s256::digest(block.header.parent).into(); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); } // Mutate tranactions merkle { let mut block = block; block.header.transactions = Blake2s256::digest(block.header.transactions).into(); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); } let key = Zeroizing::new(::F::random(&mut OsRng)); @@ -89,7 +89,7 @@ fn invalid_block() { // Manually create the block to bypass build_block's checks let block = Block::new(blockchain.tip(), vec![], vec![Transaction::Application(tx.clone())]); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); } // Run the rest of the tests with them as a participant @@ -99,7 +99,7 @@ fn invalid_block() { { let block = Block::new(blockchain.tip(), vec![], vec![Transaction::Application(tx.clone())]); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); } { @@ -112,11 +112,11 @@ fn invalid_block() { )); let mut block = blockchain.build_block::(validators.clone()); assert_eq!(block.header.transactions, merkle(&[tx.hash()])); - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); // And verify mutating the transactions merkle now causes a failure block.header.transactions = merkle(&[]); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); } { @@ -124,7 +124,7 @@ fn invalid_block() { 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(), vec![], vec![Transaction::Application(tx)]); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); } { @@ -136,14 +136,14 @@ fn invalid_block() { validators.clone() )); let mut block = blockchain.build_block::(validators.clone()); - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); match &mut block.transactions[0] { Transaction::Application(tx) => { tx.1.signature.s += ::F::ONE; } _ => panic!("non-signed tx found"), } - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); // Make sure this isn't because the merkle changed due to the transaction hash including the // signature (which it explicitly isn't allowed to anyways) @@ -191,7 +191,7 @@ fn signed_transaction() { ); // Verify and add the block - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); assert!(blockchain.add_block::(&block, vec![], validators.clone()).is_ok()); assert_eq!(blockchain.tip(), block.hash()); }; @@ -215,42 +215,134 @@ fn signed_transaction() { fn provided_transaction() { let genesis = new_genesis(); let validators = Arc::new(Validators::new(genesis, vec![]).unwrap()); - let (_, mut blockchain) = new_blockchain::(genesis, &[]); + let (db, mut blockchain) = new_blockchain::(genesis, &[]); - let tx = random_provided_transaction(&mut OsRng); + let tx = random_provided_transaction(&mut OsRng, "order1"); - // This should be provideable - let mut db = MemDb::new(); - let mut txs = ProvidedTransactions::<_, ProvidedTransaction>::new(db.clone(), genesis); + // This should be providable + let mut temp_db = MemDb::new(); + let mut txs = ProvidedTransactions::<_, ProvidedTransaction>::new(temp_db.clone(), genesis); txs.provide(tx.clone()).unwrap(); assert_eq!(txs.provide(tx.clone()), Err(ProvidedError::AlreadyProvided)); assert_eq!( - ProvidedTransactions::<_, ProvidedTransaction>::new(db.clone(), genesis).transactions, - HashMap::from([("provided", VecDeque::from([tx.clone()]))]), + ProvidedTransactions::<_, ProvidedTransaction>::new(temp_db.clone(), genesis).transactions, + HashMap::from([("order1", VecDeque::from([tx.clone()]))]), ); - let mut txn = db.txn(); - txs.complete(&mut txn, "provided", tx.hash()); + let mut txn = temp_db.txn(); + txs.complete(&mut txn, "order1", [0u8; 32], tx.hash()); txn.commit(); assert!(ProvidedTransactions::<_, ProvidedTransaction>::new(db.clone(), genesis) .transactions .is_empty()); - // Non-provided transactions should fail verification - let block = Block::new(blockchain.tip(), vec![tx.clone()], vec![]); - assert!(blockchain.verify_block::(&block, validators.clone()).is_err()); + // case we have the block's provided txs in our local as well + { + // Non-provided transactions should fail verification because we don't have them locally. + let block = Block::new(blockchain.tip(), vec![tx.clone()], vec![]); + assert!(blockchain.verify_block::(&block, validators.clone(), false).is_err()); - // Provided transactions should pass verification - blockchain.provide_transaction(tx.clone()).unwrap(); - blockchain.verify_block::(&block, validators.clone()).unwrap(); + // Provided transactions should pass verification + blockchain.provide_transaction(tx.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); - // add_block should work for verified blocks - assert!(blockchain.add_block::(&block, vec![], validators.clone()).is_ok()); + // add_block should work for verified blocks + assert!(blockchain.add_block::(&block, vec![], validators.clone()).is_ok()); - 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, validators.clone()).is_err()); - // add_block should fail for unverified provided transactions if told to add them - assert!(blockchain.add_block::(&block, vec![], validators.clone()).is_err()); + let block = Block::new(blockchain.tip(), vec![tx.clone()], vec![]); + + // The provided transaction should no longer considered provided but added to chain, + // causing this error + assert_eq!( + blockchain.verify_block::(&block, validators.clone(), false), + Err(BlockError::ProvidedAlreadyIncluded) + ); + } + + // case we don't have the block's provided txs in our local + { + let tx1 = random_provided_transaction(&mut OsRng, "order1"); + let tx2 = random_provided_transaction(&mut OsRng, "order1"); + let tx3 = random_provided_transaction(&mut OsRng, "order2"); + let tx4 = random_provided_transaction(&mut OsRng, "order2"); + + // add_block DOES NOT fail for unverified provided transactions if told to add them, + // since now we can have them later. + let block1 = Block::new(blockchain.tip(), vec![tx1.clone(), tx3.clone()], vec![]); + assert!(blockchain.add_block::(&block1, vec![], validators.clone()).is_ok()); + + // in fact, we can have many blocks that have provided txs that we don't have locally. + let block2 = Block::new(blockchain.tip(), vec![tx2.clone(), tx4.clone()], vec![]); + assert!(blockchain.add_block::(&block2, vec![], validators.clone()).is_ok()); + + // make sure we won't return ok for the block before we actually got the txs + let TransactionKind::Provided(order) = tx1.kind() else { panic!("tx wasn't provided") }; + assert!(!Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block1.hash(), + order + )); + // provide the first tx + blockchain.provide_transaction(tx1).unwrap(); + // it should be ok for this order now, since the second tx has different order. + assert!(Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block1.hash(), + order + )); + + // give the second tx + let TransactionKind::Provided(order) = tx3.kind() else { panic!("tx wasn't provided") }; + assert!(!Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block1.hash(), + order + )); + blockchain.provide_transaction(tx3).unwrap(); + // it should be ok now for the first block + assert!(Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block1.hash(), + order + )); + + // provide the second block txs + let TransactionKind::Provided(order) = tx4.kind() else { panic!("tx wasn't provided") }; + // not ok yet + assert!(!Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block2.hash(), + order + )); + blockchain.provide_transaction(tx4).unwrap(); + // ok now + assert!(Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block2.hash(), + order + )); + + // provide the second block txs + let TransactionKind::Provided(order) = tx2.kind() else { panic!("tx wasn't provided") }; + assert!(!Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block2.hash(), + order + )); + blockchain.provide_transaction(tx2).unwrap(); + assert!(Blockchain::::locally_provided_txs_in_block( + &db, + &genesis, + &block2.hash(), + order + )); + } } #[tokio::test] @@ -287,7 +379,7 @@ async fn tendermint_evidence_tx() { } // Verify and add the block - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); assert!(blockchain.add_block::(&block, vec![], validators.clone()).is_ok()); assert_eq!(blockchain.tip(), block.hash()); }; @@ -396,7 +488,8 @@ async fn block_tx_ordering() { assert!(blockchain.add_transaction::(true, unsigned_tx.clone(), validators.clone())); mempool.push(unsigned_tx); - let provided_tx = SignedTx::Provided(Box::new(random_provided_transaction(&mut OsRng))); + let provided_tx = + SignedTx::Provided(Box::new(random_provided_transaction(&mut OsRng, "order1"))); blockchain.provide_transaction(provided_tx.clone()).unwrap(); provided_txs.push(provided_tx); } @@ -424,7 +517,7 @@ async fn block_tx_ordering() { } // should be a valid block - blockchain.verify_block::(&block, validators.clone()).unwrap(); + blockchain.verify_block::(&block, validators.clone(), false).unwrap(); // Unsigned before Provided { @@ -433,7 +526,7 @@ async fn block_tx_ordering() { let unsigned = block.transactions.remove(128); block.transactions.insert(0, unsigned); assert_eq!( - blockchain.verify_block::(&block, validators.clone()).unwrap_err(), + blockchain.verify_block::(&block, validators.clone(), false).unwrap_err(), BlockError::WrongTransactionOrder ); } @@ -444,7 +537,7 @@ async fn block_tx_ordering() { let signed = block.transactions.remove(256); block.transactions.insert(0, signed); assert_eq!( - blockchain.verify_block::(&block, validators.clone()).unwrap_err(), + blockchain.verify_block::(&block, validators.clone(), false).unwrap_err(), BlockError::WrongTransactionOrder ); } @@ -454,7 +547,7 @@ async fn block_tx_ordering() { let mut block = block; block.transactions.swap(128, 256); assert_eq!( - blockchain.verify_block::(&block, validators.clone()).unwrap_err(), + blockchain.verify_block::(&block, validators.clone(), false).unwrap_err(), BlockError::WrongTransactionOrder ); } diff --git a/coordinator/tributary/src/tests/transaction/mod.rs b/coordinator/tributary/src/tests/transaction/mod.rs index a7d3abcb..266a1142 100644 --- a/coordinator/tributary/src/tests/transaction/mod.rs +++ b/coordinator/tributary/src/tests/transaction/mod.rs @@ -62,7 +62,11 @@ impl ReadWrite for ProvidedTransaction { impl Transaction for ProvidedTransaction { fn kind(&self) -> TransactionKind<'_> { - TransactionKind::Provided("provided") + match self.0[0] { + 1 => TransactionKind::Provided("order1"), + 2 => TransactionKind::Provided("order2"), + _ => panic!("unknown order"), + } } fn hash(&self) -> [u8; 32] { @@ -74,9 +78,17 @@ impl Transaction for ProvidedTransaction { } } -pub fn random_provided_transaction(rng: &mut R) -> ProvidedTransaction { +pub fn random_provided_transaction( + rng: &mut R, + order: &str, +) -> ProvidedTransaction { let mut data = vec![0; 512]; rng.fill_bytes(&mut data); + data[0] = match order { + "order1" => 1, + "order2" => 2, + _ => panic!("unknown order"), + }; ProvidedTransaction(data) } diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index da03591a..63271aea 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -78,7 +78,7 @@ 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, in an exact order. + /// This transaction should be provided by every validator, in an exact order. /// /// The contained static string names the orderer to use. This allows two distinct provided /// transaction kinds, without a synchronized order, to be ordered within their own kind without @@ -87,8 +87,9 @@ pub enum TransactionKind<'a> { /// 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. + /// If a supermajority of validators produce a commit for a block with a provided transaction + /// which isn't locally held, the block will be added to the local chain. When the transaction is + /// locally provided, it will be compared for correctness to the on-chain version Provided(&'static str), /// An unsigned transaction, only able to be included by the block producer. diff --git a/coordinator/tributary/tendermint/src/lib.rs b/coordinator/tributary/tendermint/src/lib.rs index a22672ce..487027b0 100644 --- a/coordinator/tributary/tendermint/src/lib.rs +++ b/coordinator/tributary/tendermint/src/lib.rs @@ -794,6 +794,9 @@ impl TendermintMachine { if self.block.log.has_consensus(self.block.round().number, Data::Prevote(Some(block.id()))) { match self.network.validate(block).await { Ok(_) => (), + // BlockError::Temporal is due to a temporal error we have, yet a supermajority of the + // network does not, Because we do not believe this block to be fatally invalid, and + // because a supermajority deems it valid, accept it. Err(BlockError::Temporal) => (), Err(BlockError::Fatal) => { log::warn!(target: "tendermint", "Validator proposed a fatally invalid block");