From 25824a201b07ab12d1c2a13ae628a859eeb41fb5 Mon Sep 17 00:00:00 2001 From: Brandon Kite Date: Mon, 12 Feb 2024 20:24:50 -0800 Subject: [PATCH 1/6] Only punish peers for submitting transactions with invalid data --- .../src/service/adapters/graphql_api.rs | 7 +- .../txpool/src/containers/dependency.rs | 99 +++++++++++-------- crates/services/txpool/src/service.rs | 8 +- crates/services/txpool/src/txpool.rs | 26 ++--- crates/services/txpool/src/txpool/tests.rs | 55 ++++------- crates/types/src/services/txpool.rs | 19 +++- 6 files changed, 115 insertions(+), 99 deletions(-) diff --git a/crates/fuel-core/src/service/adapters/graphql_api.rs b/crates/fuel-core/src/service/adapters/graphql_api.rs index d90f8042ab..1e41083f40 100644 --- a/crates/fuel-core/src/service/adapters/graphql_api.rs +++ b/crates/fuel-core/src/service/adapters/graphql_api.rs @@ -61,7 +61,12 @@ impl TxPoolPort for TxPoolAdapter { &self, txs: Vec>, ) -> Vec> { - self.service.insert(txs).await + self.service + .insert(txs) + .await + .into_iter() + .map(|res| res.map_err(anyhow::Error::from)) + .collect() } fn tx_update_subscribe( diff --git a/crates/services/txpool/src/containers/dependency.rs b/crates/services/txpool/src/containers/dependency.rs index cdd8959899..8c202474ed 100644 --- a/crates/services/txpool/src/containers/dependency.rs +++ b/crates/services/txpool/src/containers/dependency.rs @@ -4,7 +4,6 @@ use crate::{ Error, TxInfo, }; -use anyhow::anyhow; use fuel_core_types::{ fuel_tx::{ input::{ @@ -166,7 +165,7 @@ impl Dependency { output: &Output, input: &Input, is_output_filled: bool, - ) -> anyhow::Result<()> { + ) -> Result<(), Error> { if let Input::CoinSigned(CoinSigned { owner, amount, @@ -240,7 +239,9 @@ impl Dependency { } }; } else { - return Err(anyhow!("Use it only for coin output check")) + return Err(Error::Other( + "Use it only for coin output check".to_string(), + )) } Ok(()) } @@ -254,13 +255,16 @@ impl Dependency { txs: &'a HashMap, db: &dyn TxPoolDb, tx: &'a ArcPoolTx, - ) -> anyhow::Result<( - usize, - HashMap, - HashMap, - HashMap, - Vec, - )> { + ) -> Result< + ( + usize, + HashMap, + HashMap, + HashMap, + Vec, + ), + Error, + > { let mut collided: Vec = Vec::new(); // iterate over all inputs and check for collision let mut max_depth = 0; @@ -296,11 +300,16 @@ impl Dependency { if state.is_in_database() { // this means it is loaded from db. Get tx to compare output. if self.utxo_validation { - let coin = db.utxo(utxo_id)?.ok_or( - Error::NotInsertedInputUtxoIdNotExisting( - *utxo_id, - ), - )?; + let coin = db + .utxo(utxo_id) + .map_err(|e| { + Error::Database(format!("{:?}", e)) + })? + .ok_or( + Error::NotInsertedInputUtxoIdNotExisting( + *utxo_id, + ), + )?; if !coin .matches_input(input) .expect("The input is coin above") @@ -327,9 +336,12 @@ impl Dependency { } else { if self.utxo_validation { // fetch from db and check if tx exist. - let coin = db.utxo(utxo_id)?.ok_or( - Error::NotInsertedInputUtxoIdNotExisting(*utxo_id), - )?; + let coin = db + .utxo(utxo_id) + .map_err(|e| Error::Database(format!("{:?}", e)))? + .ok_or(Error::NotInsertedInputUtxoIdNotExisting( + *utxo_id, + ))?; if !coin .matches_input(input) @@ -358,7 +370,10 @@ impl Dependency { | Input::MessageDataPredicate(MessageDataPredicate { nonce, .. }) => { // since message id is derived, we don't need to double check all the fields if self.utxo_validation { - if let Some(db_message) = db.message(nonce)? { + if let Some(db_message) = db + .message(nonce) + .map_err(|e| Error::Database(format!("{:?}", e)))? + { // verify message id integrity if !db_message .matches_input(input) @@ -367,7 +382,10 @@ impl Dependency { return Err(Error::NotInsertedIoMessageMismatch.into()) } // return an error if spent block is set - if db.is_message_spent(nonce)? { + if db + .is_message_spent(nonce) + .map_err(|e| Error::Database(format!("{:?}", e)))? + { return Err( Error::NotInsertedInputMessageSpent(*nonce).into() ) @@ -415,7 +433,10 @@ impl Dependency { return Err(Error::NotInsertedMaxDepth.into()) } } else { - if !db.contract_exist(contract_id)? { + if !db + .contract_exist(contract_id) + .map_err(|e| Error::Database(format!("{:?}", e)))? + { return Err(Error::NotInsertedInputContractNotExisting( *contract_id, ) @@ -478,7 +499,7 @@ impl Dependency { txs: &'a HashMap, db: &DB, tx: &'a ArcPoolTx, - ) -> anyhow::Result> + ) -> Result, Error> where DB: TxPoolDb, { @@ -532,10 +553,10 @@ impl Dependency { // iterate over all outputs and insert them, marking them as available. for (index, output) in tx.outputs().iter().enumerate() { let index = u8::try_from(index).map_err(|_| { - anyhow::anyhow!( + Error::Other(format!( "The number of outputs in `{}` is more than `u8::max`", tx.id() - ) + )) })?; let utxo_id = UtxoId::new(tx.id(), index); match output { @@ -721,9 +742,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test2 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoWrongAmount), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoWrongAmount), "test2" ); @@ -739,9 +759,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test3 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoWrongOwner), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoWrongOwner), "test3" ); @@ -757,9 +776,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test4 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoWrongAssetId), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoWrongAssetId), "test4" ); @@ -775,9 +793,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test5 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoWrongAssetId), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoWrongAssetId), "test5" ); @@ -786,9 +803,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test6 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoContractOutput), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoContractOutput), "test6" ); @@ -800,9 +816,8 @@ mod tests { let out = Dependency::check_if_coin_input_can_spend_output(&output, &input, false); assert!(out.is_err(), "test8 There should be error"); - assert_eq!( - out.err().unwrap().downcast_ref(), - Some(&Error::NotInsertedIoContractOutput), + assert!( + matches!(out.err().unwrap(), Error::NotInsertedIoContractOutput), "test7" ); } diff --git a/crates/services/txpool/src/service.rs b/crates/services/txpool/src/service.rs index 50e61fab09..8024f7d6c6 100644 --- a/crates/services/txpool/src/service.rs +++ b/crates/services/txpool/src/service.rs @@ -251,9 +251,11 @@ where Some(Ok(_)) => { GossipsubMessageAcceptance::Accept }, - Some(Err(_)) => { + // Use similar p2p punishment rules as bitcoin + // https://github.com/bitcoin/bitcoin/blob/6ff0aa089c01ff3e610ecb47814ed739d685a14c/src/net_processing.cpp#L1856 + Some(Err(Error::ConsensusValidity(_))) => { GossipsubMessageAcceptance::Reject - } + }, _ => GossipsubMessageAcceptance::Ignore } } @@ -355,7 +357,7 @@ where pub async fn insert( &self, txs: Vec>, - ) -> Vec> { + ) -> Vec> { // verify txs let current_height = *self.current_height.lock(); diff --git a/crates/services/txpool/src/txpool.rs b/crates/services/txpool/src/txpool.rs index 63a84a803b..512b3a3c09 100644 --- a/crates/services/txpool/src/txpool.rs +++ b/crates/services/txpool/src/txpool.rs @@ -263,7 +263,7 @@ where fn insert_single( &mut self, tx: Checked, - ) -> anyhow::Result { + ) -> Result { let view = self.database.latest_view(); self.insert_inner(tx, &view) } @@ -274,15 +274,13 @@ where &mut self, tx: Checked, view: &View, - ) -> anyhow::Result { + ) -> Result { let tx: CheckedTransaction = tx.into(); let tx = Arc::new(match tx { CheckedTransaction::Script(script) => PoolTransaction::Script(script), CheckedTransaction::Create(create) => PoolTransaction::Create(create), - CheckedTransaction::Mint(_) => { - return Err(anyhow::anyhow!("Mint transactions is not supported")) - } + CheckedTransaction::Mint(_) => return Err(Error::MintIsDisallowed), }); if !tx.is_computed() { @@ -361,7 +359,7 @@ where &mut self, tx_status_sender: &TxStatusChange, txs: Vec>, - ) -> Vec> { + ) -> Vec> { // Check if that data is okay (witness match input/output, and if recovered signatures ara valid). // should be done before transaction comes to txpool, or before it enters RwLocked region. let mut res = Vec::new(); @@ -402,7 +400,7 @@ pub async fn check_transactions( txs: &[Arc], current_height: BlockHeight, config: &Config, -) -> Vec>> { +) -> Vec, Error>> { let mut checked_txs = Vec::with_capacity(txs.len()); for tx in txs.iter() { @@ -417,7 +415,7 @@ pub async fn check_single_tx( tx: Transaction, current_height: BlockHeight, config: &Config, -) -> anyhow::Result> { +) -> Result, Error> { if tx.is_mint() { return Err(Error::NotSupportedTransactionType.into()) } @@ -428,24 +426,20 @@ pub async fn check_single_tx( let consensus_params = &config.chain_config.consensus_parameters; let tx = tx - .into_checked_basic(current_height, consensus_params) - .map_err(|e| anyhow::anyhow!("{e:?}"))? - .check_signatures(&consensus_params.chain_id) - .map_err(|e| anyhow::anyhow!("{e:?}"))?; + .into_checked_basic(current_height, consensus_params)? + .check_signatures(&consensus_params.chain_id)?; let tx = tx .check_predicates_async::(&CheckPredicateParams::from( consensus_params, )) - .await - .map_err(|e| anyhow::anyhow!("{e:?}"))?; + .await?; debug_assert!(tx.checks().contains(Checks::all())); tx } else { - tx.into_checked_basic(current_height, &config.chain_config.consensus_parameters) - .map_err(|e| anyhow::anyhow!("{e:?}"))? + tx.into_checked_basic(current_height, &config.chain_config.consensus_parameters)? }; Ok(tx) diff --git a/crates/services/txpool/src/txpool/tests.rs b/crates/services/txpool/src/txpool/tests.rs index 8e572c2abd..8768130d9a 100644 --- a/crates/services/txpool/src/txpool/tests.rs +++ b/crates/services/txpool/src/txpool/tests.rs @@ -53,7 +53,7 @@ async fn check_unwrap_tx(tx: Transaction, config: &Config) -> Checked anyhow::Result> { +) -> Result, Error> { check_single_tx(tx, Default::default(), config).await } @@ -156,8 +156,8 @@ async fn faulty_t2_collided_on_contract_id_from_tx1() { .insert_single(tx_faulty) .expect_err("Tx2 should be Err, got Ok"); assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedCollisionContractId(id)) if id == &contract_id + err, + Error::NotInsertedCollisionContractId(id) if id == contract_id )); } @@ -202,8 +202,8 @@ async fn fail_to_insert_tx_with_dependency_on_invalid_utxo_type() { .insert_single(tx) .expect_err("Tx2 should be Err, got Ok"); assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedInputUtxoIdNotExisting(id)) if id == &UtxoId::new(tx_faulty_id, 0) + err, + Error::NotInsertedInputUtxoIdNotExisting(id) if id == UtxoId::new(tx_faulty_id, 0) )); } @@ -226,10 +226,7 @@ async fn not_inserted_known_tx() { let err = txpool .insert_single(tx) .expect_err("Second insertion of Tx1 should be Err, got Ok"); - assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedTxKnown) - )); + assert!(matches!(err, Error::NotInsertedTxKnown)); } #[tokio::test] @@ -249,10 +246,7 @@ async fn try_to_insert_tx2_missing_utxo() { let err = txpool .insert_single(tx) .expect_err("Tx should be Err, got Ok"); - assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedInputUtxoIdNotExisting(_)) - )); + assert!(matches!(err, Error::NotInsertedInputUtxoIdNotExisting(_))); } #[tokio::test] @@ -332,8 +326,8 @@ async fn underpriced_tx1_not_included_coin_collision() { .insert_single(tx3_checked) .expect_err("Tx3 should be Err, got Ok"); assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedCollision(id, utxo_id)) if id == &tx2.id(&Default::default()) && utxo_id == &UtxoId::new(tx1.id(&Default::default()), 0) + err, + Error::NotInsertedCollision(id, utxo_id) if id == tx2.id(&Default::default()) && utxo_id == UtxoId::new(tx1.id(&Default::default()), 0) )); } @@ -378,8 +372,8 @@ async fn overpriced_tx_contract_input_not_inserted() { .expect_err("Tx2 should be Err, got Ok"); assert!( matches!( - err.downcast_ref::(), - Some(Error::NotInsertedContractPricedLower(id)) if id == &contract_id + err, + Error::NotInsertedContractPricedLower(id) if id == contract_id ), "wrong err {err:?}" ); @@ -554,10 +548,7 @@ async fn tx_limit_hit() { let err = txpool .insert_single(tx2) .expect_err("Tx2 should be Err, got Ok"); - assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedLimitHit) - )); + assert!(matches!(err, Error::NotInsertedLimitHit)); } #[tokio::test] @@ -604,10 +595,7 @@ async fn tx_depth_hit() { let err = txpool .insert_single(tx3) .expect_err("Tx3 should be Err, got Ok"); - assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedMaxDepth) - )); + assert!(matches!(err, Error::NotInsertedMaxDepth)); } #[tokio::test] @@ -764,10 +752,7 @@ async fn tx_below_min_gas_price_is_not_insertable() { .await .expect_err("expected insertion failure"); - assert!(matches!( - err.root_cause().downcast_ref::().unwrap(), - Error::NotInsertedGasPriceTooLow - )); + assert!(matches!(err, Error::NotInsertedGasPriceTooLow)); } #[tokio::test] @@ -811,8 +796,8 @@ async fn tx_rejected_when_input_message_id_is_spent() { // check error assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedInputMessageSpent(msg_id)) if msg_id == message.id() + err, + Error::NotInsertedInputMessageSpent(msg_id) if msg_id == *message.id() )); } @@ -833,8 +818,8 @@ async fn tx_rejected_from_pool_when_input_message_id_does_not_exist_in_db() { // check error assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedInputMessageUnknown(msg_id)) if msg_id == message.id() + err, + Error::NotInsertedInputMessageUnknown(msg_id) if msg_id == *message.id() )); } @@ -881,8 +866,8 @@ async fn tx_rejected_from_pool_when_gas_price_is_lower_than_another_tx_with_same // check error assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedCollisionMessageId(tx_id, msg_id)) if tx_id == &tx_high_id && msg_id == message.id() + err, + Error::NotInsertedCollisionMessageId(tx_id, msg_id) if tx_id == tx_high_id && msg_id == *message.id() )); } diff --git a/crates/types/src/services/txpool.rs b/crates/types/src/services/txpool.rs index f620ba7081..286fe1f1dc 100644 --- a/crates/types/src/services/txpool.rs +++ b/crates/types/src/services/txpool.rs @@ -33,7 +33,10 @@ use crate::{ }, services::executor::TransactionExecutionResult, }; -use fuel_vm_private::checked_transaction::CheckedTransaction; +use fuel_vm_private::checked_transaction::{ + CheckError, + CheckedTransaction, +}; use std::{ sync::Arc, time::Duration, @@ -234,7 +237,7 @@ pub fn from_executor_to_status( } #[allow(missing_docs)] -#[derive(thiserror::Error, Debug, PartialEq, Eq, Clone)] +#[derive(thiserror::Error, Debug, Clone)] #[non_exhaustive] pub enum Error { #[error("TxPool required that transaction contains metadata")] @@ -306,7 +309,19 @@ pub enum Error { TTLReason, #[error("Transaction squeezed out because {0}")] SqueezedOut(String), + #[error("Invalid transaction data: {0:?}")] + ConsensusValidity(CheckError), + #[error("Mint transactions are disallowed from the txpool")] + MintIsDisallowed, + #[error("Database error: {0}")] + Database(String), // TODO: We need it for now until channels are removed from TxPool. #[error("Got some unexpected error: {0}")] Other(String), } + +impl From for Error { + fn from(e: CheckError) -> Self { + Error::ConsensusValidity(e) + } +} From 3711d389e9a30e7670545f10481f89f5a4fe60f0 Mon Sep 17 00:00:00 2001 From: Brandon Kite Date: Mon, 12 Feb 2024 20:55:32 -0800 Subject: [PATCH 2/6] Fix p2p integration test --- tests/tests/tx_gossip.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/tests/tx_gossip.rs b/tests/tests/tx_gossip.rs index 5e9f72f476..9c308a6ad0 100644 --- a/tests/tests/tx_gossip.rs +++ b/tests/tests/tx_gossip.rs @@ -12,7 +12,13 @@ use fuel_core_client::client::{ }; use fuel_core_poa::ports::BlockImporter; use fuel_core_types::{ - fuel_tx::*, + fuel_tx::{ + input::{ + coin::CoinSigned, + Empty, + }, + *, + }, fuel_vm::*, }; use futures::StreamExt; @@ -162,14 +168,19 @@ async fn test_tx_gossiping_invalid_txs( for _ in 0..NUMBER_OF_INVALID_TXS { let invalid_tx = TransactionBuilder::script(vec![], vec![]) - .add_unsigned_coin_input( - SecretKey::random(&mut rng), - rng.gen(), - rng.gen(), - rng.gen(), - Default::default(), - Default::default(), - ) + .add_input(Input::CoinSigned(CoinSigned { + utxo_id: rng.gen(), + owner: rng.gen(), + amount: 0, + asset_id: rng.gen(), + tx_pointer: Default::default(), + witness_index: 0, + maturity: Default::default(), + predicate_gas_used: Empty::new(), + predicate: Empty::new(), + predicate_data: Empty::new(), + })) + .add_witness(Witness::default()) .finalize() .into(); From 91dacbf55990c287d48eff54852daa90513c10bd Mon Sep 17 00:00:00 2001 From: Brandon Kite Date: Mon, 12 Feb 2024 21:01:21 -0800 Subject: [PATCH 3/6] clippy --- .../txpool/src/containers/dependency.rs | 64 +++++++------------ crates/services/txpool/src/txpool.rs | 11 ++-- 2 files changed, 29 insertions(+), 46 deletions(-) diff --git a/crates/services/txpool/src/containers/dependency.rs b/crates/services/txpool/src/containers/dependency.rs index 8c202474ed..de875341c3 100644 --- a/crates/services/txpool/src/containers/dependency.rs +++ b/crates/services/txpool/src/containers/dependency.rs @@ -189,31 +189,29 @@ impl Dependency { asset_id, } => { if to != i_owner { - return Err(Error::NotInsertedIoWrongOwner.into()) + return Err(Error::NotInsertedIoWrongOwner) } if amount != i_amount { - return Err(Error::NotInsertedIoWrongAmount.into()) + return Err(Error::NotInsertedIoWrongAmount) } if asset_id != i_asset_id { - return Err(Error::NotInsertedIoWrongAssetId.into()) + return Err(Error::NotInsertedIoWrongAssetId) } } - Output::Contract(_) => { - return Err(Error::NotInsertedIoContractOutput.into()) - } + Output::Contract(_) => return Err(Error::NotInsertedIoContractOutput), Output::Change { to, asset_id, amount, } => { if to != i_owner { - return Err(Error::NotInsertedIoWrongOwner.into()) + return Err(Error::NotInsertedIoWrongOwner) } if asset_id != i_asset_id { - return Err(Error::NotInsertedIoWrongAssetId.into()) + return Err(Error::NotInsertedIoWrongAssetId) } if is_output_filled && amount != i_amount { - return Err(Error::NotInsertedIoWrongAmount.into()) + return Err(Error::NotInsertedIoWrongAmount) } } Output::Variable { @@ -223,19 +221,19 @@ impl Dependency { } => { if is_output_filled { if to != i_owner { - return Err(Error::NotInsertedIoWrongOwner.into()) + return Err(Error::NotInsertedIoWrongOwner) } if amount != i_amount { - return Err(Error::NotInsertedIoWrongAmount.into()) + return Err(Error::NotInsertedIoWrongAmount) } if asset_id != i_asset_id { - return Err(Error::NotInsertedIoWrongAssetId.into()) + return Err(Error::NotInsertedIoWrongAssetId) } } // else do nothing, everything is variable and can be only check on execution } Output::ContractCreated { .. } => { - return Err(Error::NotInsertedIoContractOutput.into()) + return Err(Error::NotInsertedIoContractOutput) } }; } else { @@ -282,7 +280,7 @@ impl Dependency { max_depth = core::cmp::max(state.depth.saturating_add(1), max_depth); if max_depth > self.max_depth { - return Err(Error::NotInsertedMaxDepth.into()) + return Err(Error::NotInsertedMaxDepth) } // output is present but is it spend by other tx? if let Some(ref spend_by) = state.is_spend_by { @@ -294,8 +292,7 @@ impl Dependency { if txpool_tx.price() > tx.price() { return Err(Error::NotInsertedCollision( *spend_by, *utxo_id, - ) - .into()) + )) } else { if state.is_in_database() { // this means it is loaded from db. Get tx to compare output. @@ -314,9 +311,7 @@ impl Dependency { .matches_input(input) .expect("The input is coin above") { - return Err( - Error::NotInsertedIoCoinMismatch.into() - ) + return Err(Error::NotInsertedIoCoinMismatch) } } } else { @@ -347,7 +342,7 @@ impl Dependency { .matches_input(input) .expect("The input is coin above") { - return Err(Error::NotInsertedIoCoinMismatch.into()) + return Err(Error::NotInsertedIoCoinMismatch) } } max_depth = core::cmp::max(1, max_depth); @@ -379,21 +374,17 @@ impl Dependency { .matches_input(input) .expect("Input is a message above") { - return Err(Error::NotInsertedIoMessageMismatch.into()) + return Err(Error::NotInsertedIoMessageMismatch) } // return an error if spent block is set if db .is_message_spent(nonce) .map_err(|e| Error::Database(format!("{:?}", e)))? { - return Err( - Error::NotInsertedInputMessageSpent(*nonce).into() - ) + return Err(Error::NotInsertedInputMessageSpent(*nonce)) } } else { - return Err( - Error::NotInsertedInputMessageUnknown(*nonce).into() - ) + return Err(Error::NotInsertedInputMessageUnknown(*nonce)) } } @@ -403,8 +394,7 @@ impl Dependency { return Err(Error::NotInsertedCollisionMessageId( state.spent_by, *nonce, - ) - .into()) + )) } else { collided.push(state.spent_by); } @@ -424,13 +414,12 @@ impl Dependency { if tx.price() > state.gas_price { return Err(Error::NotInsertedContractPricedLower( *contract_id, - ) - .into()) + )) } // check depth. max_depth = core::cmp::max(state.depth, max_depth); if max_depth > self.max_depth { - return Err(Error::NotInsertedMaxDepth.into()) + return Err(Error::NotInsertedMaxDepth) } } else { if !db @@ -439,8 +428,7 @@ impl Dependency { { return Err(Error::NotInsertedInputContractNotExisting( *contract_id, - ) - .into()) + )) } // add depth max_depth = core::cmp::max(1, max_depth); @@ -468,16 +456,12 @@ impl Dependency { if let Some(contract) = self.contracts.get(contract_id) { // we have a collision :( if contract.is_in_database() { - return Err( - Error::NotInsertedContractIdAlreadyTaken(*contract_id).into() - ) + return Err(Error::NotInsertedContractIdAlreadyTaken(*contract_id)) } // check who is priced more if contract.gas_price > tx.price() { // new tx is priced less then current tx - return Err( - Error::NotInsertedCollisionContractId(*contract_id).into() - ) + return Err(Error::NotInsertedCollisionContractId(*contract_id)) } // if we are prices more, mark current contract origin for removal. let origin = contract.origin.expect( diff --git a/crates/services/txpool/src/txpool.rs b/crates/services/txpool/src/txpool.rs index 512b3a3c09..61d140ef8d 100644 --- a/crates/services/txpool/src/txpool.rs +++ b/crates/services/txpool/src/txpool.rs @@ -284,7 +284,7 @@ where }); if !tx.is_computed() { - return Err(Error::NoMetadata.into()) + return Err(Error::NoMetadata) } // verify max gas is less than block limit @@ -292,12 +292,11 @@ where return Err(Error::NotInsertedMaxGasLimit { tx_gas: tx.max_gas(), block_limit: self.config.chain_config.block_gas_limit, - } - .into()) + }) } if self.by_hash.contains_key(&tx.id()) { - return Err(Error::NotInsertedTxKnown.into()) + return Err(Error::NotInsertedTxKnown) } let mut max_limit_hit = false; @@ -307,7 +306,7 @@ where // limit is hit, check if we can push out lowest priced tx let lowest_price = self.by_gas_price.lowest_value().unwrap_or_default(); if lowest_price >= tx.price() { - return Err(Error::NotInsertedLimitHit.into()) + return Err(Error::NotInsertedLimitHit) } } if self.config.metrics { @@ -417,7 +416,7 @@ pub async fn check_single_tx( config: &Config, ) -> Result, Error> { if tx.is_mint() { - return Err(Error::NotSupportedTransactionType.into()) + return Err(Error::NotSupportedTransactionType) } verify_tx_min_gas_price(&tx, config)?; From 5a63beb84629e94ed06e12c24366b2c551fa192b Mon Sep 17 00:00:00 2001 From: Brandon Kite Date: Mon, 12 Feb 2024 21:05:42 -0800 Subject: [PATCH 4/6] correct spelling on error messages --- .../services/txpool/src/containers/dependency.rs | 6 +++--- crates/services/txpool/src/txpool/tests.rs | 7 +++++-- crates/types/src/services/txpool.rs | 14 ++++++-------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/services/txpool/src/containers/dependency.rs b/crates/services/txpool/src/containers/dependency.rs index de875341c3..33ea6a06c2 100644 --- a/crates/services/txpool/src/containers/dependency.rs +++ b/crates/services/txpool/src/containers/dependency.rs @@ -303,7 +303,7 @@ impl Dependency { Error::Database(format!("{:?}", e)) })? .ok_or( - Error::NotInsertedInputUtxoIdNotExisting( + Error::NotInsertedInputUtxoIdNotDoesNotExist( *utxo_id, ), )?; @@ -334,7 +334,7 @@ impl Dependency { let coin = db .utxo(utxo_id) .map_err(|e| Error::Database(format!("{:?}", e)))? - .ok_or(Error::NotInsertedInputUtxoIdNotExisting( + .ok_or(Error::NotInsertedInputUtxoIdNotDoesNotExist( *utxo_id, ))?; @@ -426,7 +426,7 @@ impl Dependency { .contract_exist(contract_id) .map_err(|e| Error::Database(format!("{:?}", e)))? { - return Err(Error::NotInsertedInputContractNotExisting( + return Err(Error::NotInsertedInputContractDoesNotExist( *contract_id, )) } diff --git a/crates/services/txpool/src/txpool/tests.rs b/crates/services/txpool/src/txpool/tests.rs index 8768130d9a..e3d73e4ed1 100644 --- a/crates/services/txpool/src/txpool/tests.rs +++ b/crates/services/txpool/src/txpool/tests.rs @@ -203,7 +203,7 @@ async fn fail_to_insert_tx_with_dependency_on_invalid_utxo_type() { .expect_err("Tx2 should be Err, got Ok"); assert!(matches!( err, - Error::NotInsertedInputUtxoIdNotExisting(id) if id == UtxoId::new(tx_faulty_id, 0) + Error::NotInsertedInputUtxoIdNotDoesNotExist(id) if id == UtxoId::new(tx_faulty_id, 0) )); } @@ -246,7 +246,10 @@ async fn try_to_insert_tx2_missing_utxo() { let err = txpool .insert_single(tx) .expect_err("Tx should be Err, got Ok"); - assert!(matches!(err, Error::NotInsertedInputUtxoIdNotExisting(_))); + assert!(matches!( + err, + Error::NotInsertedInputUtxoIdNotDoesNotExist(_) + )); } #[tokio::test] diff --git a/crates/types/src/services/txpool.rs b/crates/types/src/services/txpool.rs index 286fe1f1dc..39d6bc248b 100644 --- a/crates/types/src/services/txpool.rs +++ b/crates/types/src/services/txpool.rs @@ -262,16 +262,14 @@ pub enum Error { "Transaction is not inserted. A higher priced tx {0:#x} is already spending this message: {1:#x}" )] NotInsertedCollisionMessageId(TxId, Nonce), - #[error( - "Transaction is not inserted. Dependent UTXO output is not existing: {0:#x}" - )] - NotInsertedOutputNotExisting(UtxoId), - #[error("Transaction is not inserted. UTXO input contract is not existing: {0:#x}")] - NotInsertedInputContractNotExisting(ContractId), + #[error("Transaction is not inserted. UTXO input does not exist: {0:#x}")] + NotInsertedOutputDoesNotExist(UtxoId), + #[error("Transaction is not inserted. UTXO input contract does not exist or was already spent: {0:#x}")] + NotInsertedInputContractDoesNotExist(ContractId), #[error("Transaction is not inserted. ContractId is already taken {0:#x}")] NotInsertedContractIdAlreadyTaken(ContractId), - #[error("Transaction is not inserted. UTXO is not existing: {0:#x}")] - NotInsertedInputUtxoIdNotExisting(UtxoId), + #[error("Transaction is not inserted. UTXO does not exist: {0:#x}")] + NotInsertedInputUtxoIdNotDoesNotExist(UtxoId), #[error("Transaction is not inserted. UTXO is spent: {0:#x}")] NotInsertedInputUtxoIdSpent(UtxoId), #[error("Transaction is not inserted. Message is spent: {0:#x}")] From e013c36bdb32bf7c65d5b558167e14c5584e4ba6 Mon Sep 17 00:00:00 2001 From: Brandon Kite Date: Tue, 13 Feb 2024 19:43:00 -0800 Subject: [PATCH 5/6] CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index efed575378..3a942d6df9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Description of the upcoming release here. ### Changed +- [#1663](https://github.com/FuelLabs/fuel-core/pull/1663): Reduce the punishment criteria for mempool gossipping. - [#1658](https://github.com/FuelLabs/fuel-core/pull/1658): Removed `Receipts` table. Instead, receipts are part of the `TransactionStatuses` table. - [#1650](https://github.com/FuelLabs/fuel-core/pull/1650): Add api endpoint for getting estimates for future gas prices - [#1649](https://github.com/FuelLabs/fuel-core/pull/1649): Add api endpoint for getting latest gas price From 8108aea07da372f2d6dbbd4b87493b2922f2d190 Mon Sep 17 00:00:00 2001 From: Voxelot Date: Mon, 19 Feb 2024 18:01:09 -0800 Subject: [PATCH 6/6] reject mint txs. properly notify ignored messages. add tests to validate message acceptance. --- crates/services/txpool/src/service.rs | 15 +- .../txpool/src/service/test_helpers.rs | 12 +- .../services/txpool/src/service/tests_p2p.rs | 139 +++++++++++++++++- 3 files changed, 152 insertions(+), 14 deletions(-) diff --git a/crates/services/txpool/src/service.rs b/crates/services/txpool/src/service.rs index 8024f7d6c6..a98407e28e 100644 --- a/crates/services/txpool/src/service.rs +++ b/crates/services/txpool/src/service.rs @@ -253,7 +253,7 @@ where }, // Use similar p2p punishment rules as bitcoin // https://github.com/bitcoin/bitcoin/blob/6ff0aa089c01ff3e610ecb47814ed739d685a14c/src/net_processing.cpp#L1856 - Some(Err(Error::ConsensusValidity(_))) => { + Some(Err(Error::ConsensusValidity(_))) | Some(Err(Error::MintIsDisallowed)) => { GossipsubMessageAcceptance::Reject }, _ => GossipsubMessageAcceptance::Ignore @@ -264,14 +264,13 @@ where } }; - if acceptance != GossipsubMessageAcceptance::Ignore { - let message_info = GossipsubMessageInfo { - message_id, - peer_id, - }; + // notify p2p layer about whether this tx was accepted + let message_info = GossipsubMessageInfo { + message_id, + peer_id, + }; - let _ = self.shared.p2p.notify_gossip_transaction_validity(message_info, acceptance); - } + let _ = self.shared.p2p.notify_gossip_transaction_validity(message_info, acceptance); should_continue = true; } else { diff --git a/crates/services/txpool/src/service/test_helpers.rs b/crates/services/txpool/src/service/test_helpers.rs index 633c6aa0da..a1a44d7bed 100644 --- a/crates/services/txpool/src/service/test_helpers.rs +++ b/crates/services/txpool/src/service/test_helpers.rs @@ -97,8 +97,7 @@ impl MockP2P { }); Box::pin(stream) }); - p2p.expect_broadcast_transaction() - .returning(move |_| Ok(())); + p2p } } @@ -190,7 +189,14 @@ impl TestContextBuilder { let config = self.config.unwrap_or_default(); let mock_db = self.mock_db; - let p2p = self.p2p.unwrap_or_else(|| MockP2P::new_with_txs(vec![])); + let mut p2p = self.p2p.unwrap_or_else(|| MockP2P::new_with_txs(vec![])); + // set default handlers for p2p methods after test is set up, so they will be last on the FIFO + // ordering of methods handlers: https://docs.rs/mockall/0.12.1/mockall/index.html#matching-multiple-calls + p2p.expect_notify_gossip_transaction_validity() + .returning(move |_, _| Ok(())); + p2p.expect_broadcast_transaction() + .returning(move |_| Ok(())); + let importer = self .importer .unwrap_or_else(|| MockImporter::with_blocks(vec![])); diff --git a/crates/services/txpool/src/service/tests_p2p.rs b/crates/services/txpool/src/service/tests_p2p.rs index 3a21e5156f..614f6153d4 100644 --- a/crates/services/txpool/src/service/tests_p2p.rs +++ b/crates/services/txpool/src/service/tests_p2p.rs @@ -1,11 +1,21 @@ use super::*; -use crate::service::test_helpers::{ - MockP2P, - TestContextBuilder, +use crate::{ + service::test_helpers::{ + MockP2P, + TestContextBuilder, + }, + test_helpers::TEST_COIN_AMOUNT, }; use fuel_core_services::Service; +use fuel_core_storage::rand::{ + prelude::StdRng, + SeedableRng, +}; use fuel_core_types::fuel_tx::{ + field::Inputs, + AssetId, Transaction, + TransactionBuilder, UniqueIdentifier, }; use std::{ @@ -133,3 +143,126 @@ async fn test_insert_from_p2p_does_not_broadcast_to_p2p() { "expected a timeout because no broadcast should have occurred" ) } + +#[tokio::test] +async fn test_gossipped_transaction_with_check_error_rejected() { + // verify that gossipped transactions which fail basic sanity checks are rejected (punished) + + let mut ctx_builder = TestContextBuilder::new(); + // add coin to builder db and generate a valid tx + let mut tx1 = ctx_builder.setup_script_tx(10); + // now intentionally muck up the tx such that it will return a CheckError, + // by duplicating an input + let script = tx1.as_script_mut().unwrap(); + let input = script.inputs()[0].clone(); + script.inputs_mut().push(input); + // setup p2p mock - with tx incoming from p2p + let txs = vec![tx1.clone()]; + let mut p2p = MockP2P::new_with_txs(txs); + let (send, mut receive) = broadcast::channel::<()>(1); + p2p.expect_notify_gossip_transaction_validity() + .returning(move |_, validity| { + // Expect the transaction to be rejected + assert_eq!(validity, GossipsubMessageAcceptance::Reject); + // Notify test that the gossipsub acceptance was set + send.send(()).unwrap(); + Ok(()) + }); + ctx_builder.with_p2p(p2p); + + // build and start the txpool service + let ctx = ctx_builder.build(); + let service = ctx.service(); + service.start_and_await().await.unwrap(); + // verify p2p was notified about the transaction validity + let gossip_validity_notified = + tokio::time::timeout(Duration::from_millis(100), receive.recv()).await; + assert!( + gossip_validity_notified.is_ok(), + "expected to receive gossip validity notification" + ) +} + +#[tokio::test] +async fn test_gossipped_mint_rejected() { + // verify that gossipped mint transactions are rejected (punished) + let tx1 = TransactionBuilder::mint( + 0u32.into(), + 0, + Default::default(), + Default::default(), + 1, + AssetId::BASE, + ) + .finalize_as_transaction(); + // setup p2p mock - with tx incoming from p2p + let txs = vec![tx1.clone()]; + let mut p2p = MockP2P::new_with_txs(txs); + let (send, mut receive) = broadcast::channel::<()>(1); + p2p.expect_notify_gossip_transaction_validity() + .returning(move |_, validity| { + // Expect the transaction to be rejected + assert_eq!(validity, GossipsubMessageAcceptance::Reject); + // Notify test that the gossipsub acceptance was set + send.send(()).unwrap(); + Ok(()) + }); + // setup test context + let mut ctx_builder = TestContextBuilder::new(); + ctx_builder.with_p2p(p2p); + + // build and start the txpool service + let ctx = ctx_builder.build(); + let service = ctx.service(); + service.start_and_await().await.unwrap(); + // verify p2p was notified about the transaction validity + let gossip_validity_notified = + tokio::time::timeout(Duration::from_millis(100), receive.recv()).await; + assert!( + gossip_validity_notified.is_ok(), + "expected to receive gossip validity notification" + ) +} + +#[tokio::test] +async fn test_gossipped_transaction_with_transient_error_ignored() { + // verify that gossipped transactions that fails stateful checks are ignored (but not punished) + let mut rng = StdRng::seed_from_u64(100); + let mut ctx_builder = TestContextBuilder::new(); + // add coin to builder db and generate a valid tx + let mut tx1 = ctx_builder.setup_script_tx(10); + // now intentionally muck up the tx such that it will return a coin not found error + // by replacing the default coin with one that is not in the database + let script = tx1.as_script_mut().unwrap(); + script.inputs_mut()[0] = crate::test_helpers::random_predicate( + &mut rng, + AssetId::BASE, + TEST_COIN_AMOUNT, + None, + ); + // setup p2p mock - with tx incoming from p2p + let txs = vec![tx1.clone()]; + let mut p2p = MockP2P::new_with_txs(txs); + let (send, mut receive) = broadcast::channel::<()>(1); + p2p.expect_notify_gossip_transaction_validity() + .returning(move |_, validity| { + // Expect the transaction to be rejected + assert_eq!(validity, GossipsubMessageAcceptance::Ignore); + // Notify test that the gossipsub acceptance was set + send.send(()).unwrap(); + Ok(()) + }); + ctx_builder.with_p2p(p2p); + + // build and start the txpool service + let ctx = ctx_builder.build(); + let service = ctx.service(); + service.start_and_await().await.unwrap(); + // verify p2p was notified about the transaction validity + let gossip_validity_notified = + tokio::time::timeout(Duration::from_millis(100), receive.recv()).await; + assert!( + gossip_validity_notified.is_ok(), + "expected to receive gossip validity notification" + ) +}