fix: make boost detection more accurate (#5369)

* fix: make boost detection more accurate * fix: remove unnecessary TaintedTransactionStatus::Boosted * test: Added more tests - Proof that we can report between pre and witness still rejecting a tx if the channel was not bossted - We can not change the status of a report if its already prewitnessed * chore: fix cargo fmt * chore: remove dupe test --------- Co-authored-by: Jan Börner <jan@chainflip.io>
chainflip-io · Oct 31, 2024 · 0c70f5f · 0c70f5f
1 parent bfd14ea
commit 0c70f5f
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 24 deletions.
diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs
@@ -82,11 +82,9 @@ pub enum BoostStatus<ChainAmount> {
 
 #[derive(Clone, Debug, PartialEq, Eq, Encode, Decode, TypeInfo, Default)]
 pub enum TaintedTransactionStatus {
-	/// Transaction was boosted, can't be rejected.
-	Boosted,
 	/// Transaction was prewitnessed but not boosted due to being reported.
 	Prewitnessed,
-	/// Transaction has been reported but not neither prewitnessed nor boosted.
+	/// Transaction has been reported but was not prewitnessed.
 	#[default]
 	Unseen,
 }
@@ -849,7 +847,7 @@ pub mod pallet {
 							Ok(())
 						},
 						_ => {
-							// Don't apply the mutation. We expect the pre-witnessed/boosted
+							// Don't apply the mutation. We expect the pre-witnessed
 							// transaction to eventually be fully witnessed.
 							Err(())
 						},
@@ -1725,16 +1723,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 							*status = TaintedTransactionStatus::Prewitnessed;
 							true
 						},
+						// Pre-witnessing twice is unlikely but possible. Either way we don't want
+						// to change the status and we don't want to allow boosting.
+						Some(TaintedTransactionStatus::Prewitnessed) => true,
 						// Transaction has not been reported, mark it as boosted to prevent further
 						// reports.
-						None => {
-							let _ = opt.insert(TaintedTransactionStatus::Boosted);
-							false
-						},
-						// Pre-witnessing twice or pre-witnessing after boosting is unlikely but
-						// possible. Either way we don't want to change the status.
-						Some(TaintedTransactionStatus::Prewitnessed) |
-						Some(TaintedTransactionStatus::Boosted) => true,
+						None => false,
 					}
 				}) {
 					continue;
@@ -1963,8 +1957,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		let channel_owner = deposit_channel_details.owner.clone();
 
 		if let Some(tx_id) = deposit_details.deposit_id() {
-			if matches!(TaintedTransactions::<T, I>::take(&channel_owner, &tx_id),
-			Some(status) if status != TaintedTransactionStatus::Boosted)
+			if TaintedTransactions::<T, I>::take(&channel_owner, &tx_id).is_some() &&
+				!matches!(deposit_channel_details.boost_status, BoostStatus::Boosted { .. })
 			{
 				let refund_address = match deposit_channel_details.action.clone() {
 					ChannelAction::Swap { refund_params, .. } => refund_params

diff --git a/state-chain/pallets/cf-ingress-egress/src/tests/screening.rs b/state-chain/pallets/cf-ingress-egress/src/tests/screening.rs
@@ -168,10 +168,8 @@ fn finalize_boosted_tx_if_tainted_after_prewitness() {
 			10,
 		);
 
-		assert_noop!(
-			IngressEgress::mark_transaction_as_tainted(OriginTrait::signed(BROKER), tx_id,),
-			crate::Error::<Test, ()>::TransactionAlreadyPrewitnessed
-		);
+		// It's possible to report the tx, but reporting will have no effect.
+		assert_ok!(IngressEgress::mark_transaction_as_tainted(OriginTrait::signed(BROKER), tx_id,),);
 
 		assert_ok!(IngressEgress::process_single_deposit(
 			address,
@@ -319,15 +317,13 @@ fn can_not_report_transaction_after_witnessing() {
 		let unreported = Hash::random();
 		let unseen = Hash::random();
 		let prewitnessed = Hash::random();
-		let boosted = Hash::random();
 
 		TaintedTransactions::<Test, ()>::insert(BROKER, unseen, TaintedTransactionStatus::Unseen);
 		TaintedTransactions::<Test, ()>::insert(
 			BROKER,
 			prewitnessed,
 			TaintedTransactionStatus::Prewitnessed,
 		);
-		TaintedTransactions::<Test, ()>::insert(BROKER, boosted, TaintedTransactionStatus::Boosted);
 
 		assert_ok!(IngressEgress::mark_transaction_as_tainted(
 			OriginTrait::signed(BROKER),
@@ -340,10 +336,6 @@ fn can_not_report_transaction_after_witnessing() {
 			IngressEgress::mark_transaction_as_tainted(OriginTrait::signed(BROKER), prewitnessed,),
 			crate::Error::<Test, ()>::TransactionAlreadyPrewitnessed
 		);
-		assert_noop!(
-			IngressEgress::mark_transaction_as_tainted(OriginTrait::signed(BROKER), boosted,),
-			crate::Error::<Test, ()>::TransactionAlreadyPrewitnessed
-		);
 	});
 }
 
@@ -373,3 +365,50 @@ fn send_funds_back_after_they_have_been_rejected() {
 		);
 	});
 }
+
+#[test]
+fn can_report_between_prewitness_and_witness_if_tx_was_not_boosted() {
+	new_test_ext().execute_with(|| {
+		let tx_id = Hash::random();
+		let deposit_details = helpers::generate_btc_deposit(tx_id);
+
+		assert_ok!(<MockAccountRoleRegistry as AccountRoleRegistry<Test>>::register_as_broker(
+			&BROKER,
+		));
+
+		let (_id, address, ..) = IngressEgress::request_liquidity_deposit_address(
+			BROKER,
+			btc::Asset::Btc,
+			0,
+			ForeignChainAddress::Btc(ScriptPubkey::P2SH(DEFAULT_BTC_ADDRESS)),
+		)
+		.unwrap();
+
+		let deposit_address = match address {
+			ForeignChainAddress::Btc(script_pubkey) => script_pubkey,
+			_ => unreachable!(),
+		};
+
+		let deposit_witnesses = vec![DepositWitness {
+			deposit_address,
+			asset: btc::Asset::Btc,
+			amount: DEFAULT_DEPOSIT_AMOUNT,
+			deposit_details,
+		}];
+
+		assert_ok!(IngressEgress::add_prewitnessed_deposits(deposit_witnesses.clone(), 10,));
+		assert_ok!(IngressEgress::mark_transaction_as_tainted(OriginTrait::signed(BROKER), tx_id,));
+		assert_ok!(IngressEgress::process_deposit_witnesses(deposit_witnesses, 10,));
+
+		assert_has_matching_event!(
+			Test,
+			RuntimeEvent::IngressEgress(crate::Event::DepositIgnored {
+				deposit_address: _,
+				asset: btc::Asset::Btc,
+				amount: DEFAULT_DEPOSIT_AMOUNT,
+				deposit_details: _,
+				reason: DepositIgnoredReason::TransactionTainted,
+			})
+		);
+	});
+}