diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 80fe1000bdb..8199344ab01 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -605,34 +605,6 @@ jobs: ENV: 'fueldevsway.env' DELETE_INFRA: true - # Deploy Latest Fuel-Core Release - # TODO: remove deploy steps after the old cluster is decommissioned - deploy: - if: github.ref == 'refs/heads/master' - needs: - - publish-docker-image - runs-on: buildjet-4vcpu-ubuntu-2204 - steps: - - name: Set Environment Variables - run: | - tag=(`echo $GITHUB_SHA | cut -c1-7`) - echo "IMAGE_TAG=`echo sha-$tag`" >> $GITHUB_ENV - echo "DEPLOYMENT_VERSION=$(echo $GITHUB_SHA)" >> $GITHUB_ENV - - - name: Deploy Fuel-Core Developer Environment - uses: benc-uk/workflow-dispatch@v1 - with: - workflow: Deploy Fuel-Core on k8s - repo: FuelLabs/fuel-deployment - ref: refs/heads/master - token: ${{ secrets.REPO_TOKEN }} - inputs: '{ "k8s-type": "${{ env.K8S }}", "config-directory": "${{ env.CONFIG }}", "config-env": "${{ env.ENV }}", "deployment-version": "${{ env.DEPLOYMENT_VERSION }}", "image-tag": "${{ env.IMAGE_TAG }}", "delete-infra": "${{ env.DELETE_INFRA }}" }' - env: - K8S: 'eks' - CONFIG: 'fuel-dev1' - ENV: 'fueldevsway.env' - DELETE_INFRA: true - cargo-audit: runs-on: ubuntu-latest continue-on-error: true diff --git a/CHANGELOG.md b/CHANGELOG.md index 55cca3ee19e..48706e23a15 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. - [#1640](https://github.com/FuelLabs/fuel-core/pull/1640): Upgrade to fuel-vm 0.45.0. - [#1635](https://github.com/FuelLabs/fuel-core/pull/1635): Move updating of the owned messages and coins to off-chain worker. diff --git a/crates/fuel-core/src/service/adapters/graphql_api.rs b/crates/fuel-core/src/service/adapters/graphql_api.rs index d90f8042ab4..1e41083f408 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 cdd8959899b..33ea6a06c2c 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, @@ -190,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 { @@ -224,23 +221,25 @@ 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 { - 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 +253,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; @@ -278,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 { @@ -290,24 +292,26 @@ 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. 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::NotInsertedInputUtxoIdNotDoesNotExist( + *utxo_id, + ), + )?; if !coin .matches_input(input) .expect("The input is coin above") { - return Err( - Error::NotInsertedIoCoinMismatch.into() - ) + return Err(Error::NotInsertedIoCoinMismatch) } } } else { @@ -327,15 +331,18 @@ 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::NotInsertedInputUtxoIdNotDoesNotExist( + *utxo_id, + ))?; if !coin .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); @@ -358,24 +365,26 @@ 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) .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)? { - return Err( - Error::NotInsertedInputMessageSpent(*nonce).into() - ) + if db + .is_message_spent(nonce) + .map_err(|e| Error::Database(format!("{:?}", e)))? + { + return Err(Error::NotInsertedInputMessageSpent(*nonce)) } } else { - return Err( - Error::NotInsertedInputMessageUnknown(*nonce).into() - ) + return Err(Error::NotInsertedInputMessageUnknown(*nonce)) } } @@ -385,8 +394,7 @@ impl Dependency { return Err(Error::NotInsertedCollisionMessageId( state.spent_by, *nonce, - ) - .into()) + )) } else { collided.push(state.spent_by); } @@ -406,20 +414,21 @@ 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.contract_exist(contract_id)? { - return Err(Error::NotInsertedInputContractNotExisting( + if !db + .contract_exist(contract_id) + .map_err(|e| Error::Database(format!("{:?}", e)))? + { + return Err(Error::NotInsertedInputContractDoesNotExist( *contract_id, - ) - .into()) + )) } // add depth max_depth = core::cmp::max(1, max_depth); @@ -447,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( @@ -478,7 +483,7 @@ impl Dependency { txs: &'a HashMap, db: &DB, tx: &'a ArcPoolTx, - ) -> anyhow::Result> + ) -> Result, Error> where DB: TxPoolDb, { @@ -532,10 +537,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 +726,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 +743,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 +760,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 +777,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 +787,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 +800,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 50e61fab098..a98407e28e3 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(_))) | Some(Err(Error::MintIsDisallowed)) => { GossipsubMessageAcceptance::Reject - } + }, _ => GossipsubMessageAcceptance::Ignore } } @@ -262,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 { @@ -355,7 +356,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/service/test_helpers.rs b/crates/services/txpool/src/service/test_helpers.rs index 633c6aa0da5..a1a44d7bedc 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 3a21e5156f1..614f6153d44 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" + ) +} diff --git a/crates/services/txpool/src/txpool.rs b/crates/services/txpool/src/txpool.rs index 63a84a803b5..61d140ef8d1 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,19 +274,17 @@ 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() { - return Err(Error::NoMetadata.into()) + return Err(Error::NoMetadata) } // verify max gas is less than block limit @@ -294,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; @@ -309,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 { @@ -361,7 +358,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 +399,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,9 +414,9 @@ 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()) + return Err(Error::NotSupportedTransactionType) } verify_tx_min_gas_price(&tx, config)?; @@ -428,24 +425,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 8e572c2abd5..e3d73e4ed1f 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::NotInsertedInputUtxoIdNotDoesNotExist(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] @@ -250,8 +247,8 @@ async fn try_to_insert_tx2_missing_utxo() { .insert_single(tx) .expect_err("Tx should be Err, got Ok"); assert!(matches!( - err.downcast_ref::(), - Some(Error::NotInsertedInputUtxoIdNotExisting(_)) + err, + Error::NotInsertedInputUtxoIdNotDoesNotExist(_) )); } @@ -332,8 +329,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 +375,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 +551,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 +598,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 +755,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 +799,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 +821,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 +869,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 f620ba70817..39d6bc248b2 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")] @@ -259,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}")] @@ -306,7 +307,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) + } +} diff --git a/tests/tests/tx_gossip.rs b/tests/tests/tx_gossip.rs index acebcf584ce..d24b289ec0f 100644 --- a/tests/tests/tx_gossip.rs +++ b/tests/tests/tx_gossip.rs @@ -8,7 +8,13 @@ use fuel_core::p2p_test_helpers::{ }; use fuel_core_client::client::FuelClient; use fuel_core_types::{ - fuel_tx::*, + fuel_tx::{ + input::{ + coin::CoinSigned, + Empty, + }, + *, + }, fuel_vm::*, services::{ block_importer::SharedImportResult, @@ -161,14 +167,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();