Merge pull request #802 from Camillarhi/feat/tx-rebroadcast-fee-bumping-follow-up

Minor follow-ups to #628

Author: tnull

src/wallet/mod.rs

@@ -388,8 +388,8 @@ impl Wallet {
 					);
 					let payment =
 						self.payment_store.get(&payment_id).ok_or(Error::InvalidPaymentId)?;
-					let pending_payment_details = self
-						.create_pending_payment_from_tx(payment.clone(), conflict_txids.clone());
+					let pending_payment_details =
+						self.create_pending_payment_from_tx(payment, conflict_txids.clone());
 
 					self.pending_payment_store.insert_or_update(pending_payment_details)?;
 				},
@@ -1200,7 +1200,10 @@ impl Wallet {
 
 		if let Some(replaced_details) = self
 			.pending_payment_store
-			.list_filter(|p| p.conflicting_txids.contains(&target_txid))
+			.list_filter(|p| {
+				matches!(p.details.kind, PaymentKind::Onchain { txid, .. } if txid == target_txid)
+					|| p.conflicting_txids.contains(&target_txid)
+			})
 			.first()
 		{
 			return Some(replaced_details.details.id);
@@ -1281,13 +1284,13 @@ impl Wallet {
 			old_fee_rate.to_sat_per_kwu() + INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT as u64;
 
 		let confirmation_target = ConfirmationTarget::OnchainPayment;
-		let estimated_fee_rate =
-			fee_rate.unwrap_or_else(|| self.fee_estimator.estimate_fee_rate(confirmation_target));
+		let estimated_fee_rate = self.fee_estimator.estimate_fee_rate(confirmation_target);
 
 		// Use the higher of minimum RBF requirement or current network estimate
 		let final_fee_rate_sat_per_kwu =
 			min_required_fee_rate_sat_per_kwu.max(estimated_fee_rate.to_sat_per_kwu());
-		let final_fee_rate = FeeRate::from_sat_per_kwu(final_fee_rate_sat_per_kwu);
+		let final_fee_rate =
+			fee_rate.unwrap_or_else(|| FeeRate::from_sat_per_kwu(final_fee_rate_sat_per_kwu));
 
 		let mut psbt = {
 			let mut builder = locked_wallet.build_fee_bump(txid).map_err(|e| {
@@ -1312,6 +1315,17 @@ impl Wallet {
 			match builder.finish() {
 				Ok(psbt) => Ok(psbt),
 				Err(CreateTxError::FeeRateTooLow { required: required_fee_rate }) => {
+					if fee_rate.is_some() {
+						log_error!(
+							self.logger,
+							"Provided fee rate {} is too low for RBF fee bump of txid {}, required minimum fee rate: {}",
+							fee_rate.unwrap(),
+							txid,
+							required_fee_rate
+						);
+						return Err(Error::InvalidFeeRate);
+					}
+
 					log_info!(self.logger, "BDK requires higher fee rate: {}", required_fee_rate);
 
 					// BDK may require a higher fee rate than our estimate due to

tests/integration_tests_rust.rs

@@ -2473,7 +2473,7 @@ async fn persistence_backwards_compatibility() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
 async fn onchain_fee_bump_rbf() {
 	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
-	let chain_source = TestChainSource::Esplora(&electrsd);
+	let chain_source = random_chain_source(&bitcoind, &electrsd);
 	let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false);
 
 	// Fund both nodes
@@ -2497,6 +2497,10 @@ async fn onchain_fee_bump_rbf() {
 	let txid =
 		node_b.onchain_payment().send_to_address(&addr_a, amount_to_send_sats, None).unwrap();
 	wait_for_tx(&electrsd.client, txid).await;
+	// Give the chain source time to index the unconfirmed transaction before syncing.
+	// Without this, Esplora may not yet have the tx, causing sync to miss it and
+	// leaving the BDK wallet graph empty.
+	tokio::time::sleep(std::time::Duration::from_secs(5)).await;
 	node_a.sync_wallets().unwrap();
 	node_b.sync_wallets().unwrap();
 
@@ -2522,10 +2526,10 @@ async fn onchain_fee_bump_rbf() {
 	// Successful fee bump
 	let new_txid = node_b.onchain_payment().bump_fee_rbf(payment_id, None).unwrap();
 	wait_for_tx(&electrsd.client, new_txid).await;
-
-	// Sleep to allow for transaction propagation
+	// Give the chain source time to index the unconfirmed transaction before syncing.
+	// Without this, Esplora may not yet have the tx, causing sync to miss it and
+	// leaving the BDK wallet graph empty.
 	tokio::time::sleep(std::time::Duration::from_secs(5)).await;
-
 	node_a.sync_wallets().unwrap();
 	node_b.sync_wallets().unwrap();
 
@@ -2547,26 +2551,9 @@ async fn onchain_fee_bump_rbf() {
 		_ => panic!("Unexpected payment kind"),
 	}
 
-	// Verify node_a has the inbound payment txid updated to the replacement txid
-	let node_a_inbound_payment = node_a.payment(&payment_id).unwrap();
-	assert_eq!(node_a_inbound_payment.direction, PaymentDirection::Inbound);
-	match &node_a_inbound_payment.kind {
-		PaymentKind::Onchain { txid: inbound_txid, .. } => {
-			assert_eq!(
-				*inbound_txid, new_txid,
-				"node_a inbound payment txid should be updated to the replacement txid"
-			);
-		},
-		_ => panic!("Unexpected payment kind"),
-	}
-
 	// Multiple consecutive bumps
 	let second_bump_txid = node_b.onchain_payment().bump_fee_rbf(payment_id, None).unwrap();
 	wait_for_tx(&electrsd.client, second_bump_txid).await;
-
-	// Sleep to allow for transaction propagation
-	tokio::time::sleep(std::time::Duration::from_secs(5)).await;
-
 	node_a.sync_wallets().unwrap();
 	node_b.sync_wallets().unwrap();
 
@@ -2586,19 +2573,6 @@ async fn onchain_fee_bump_rbf() {
 		_ => panic!("Unexpected payment kind"),
 	}
 
-	// Verify node_a has the inbound payment txid updated to the second replacement txid
-	let node_a_second_inbound_payment = node_a.payment(&payment_id).unwrap();
-	assert_eq!(node_a_second_inbound_payment.direction, PaymentDirection::Inbound);
-	match &node_a_second_inbound_payment.kind {
-		PaymentKind::Onchain { txid: inbound_txid, .. } => {
-			assert_eq!(
-				*inbound_txid, second_bump_txid,
-				"node_a inbound payment txid should be updated to the second replacement txid"
-			);
-		},
-		_ => panic!("Unexpected payment kind"),
-	}
-
 	// Confirm the transaction and try to bump again (should fail)
 	generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await;
 	node_a.sync_wallets().unwrap();
@@ -2620,10 +2594,20 @@ async fn onchain_fee_bump_rbf() {
 	}
 
 	// Verify node A received the funds correctly
-	let node_a_received_payment = node_a.list_payments_with_filter(
-		|p| matches!(p.kind, PaymentKind::Onchain { txid, .. } if txid == second_bump_txid),
-	);
+	let node_a_received_payment = node_a.list_payments_with_filter(|p| {
+		p.id == payment_id && matches!(p.kind, PaymentKind::Onchain { .. })
+	});
+
 	assert_eq!(node_a_received_payment.len(), 1);
+	match &node_a_received_payment[0].kind {
+		PaymentKind::Onchain { txid: inbound_txid, .. } => {
+			assert_eq!(
+				*inbound_txid, second_bump_txid,
+				"node_a inbound payment txid should be updated to the second replacement txid"
+			);
+		},
+		_ => panic!("Unexpected payment kind"),
+	}
 	assert_eq!(node_a_received_payment[0].amount_msat, Some(amount_to_send_sats * 1000));
 	assert_eq!(node_a_received_payment[0].status, PaymentStatus::Succeeded);
 }