Skip to content

Minor follow-ups to #628 #802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tnull merged 1 commit into from
Mar 2, 2026
+41 −43
Merged

Minor follow-ups to #628 #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,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)?;
},
Expand Down Expand Up @@ -1044,7 +1044,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);
Expand Down Expand Up @@ -1125,13 +1128,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| {
Expand All @@ -1156,6 +1159,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
Expand Down
58 changes: 21 additions & 37 deletions tests/integration_tests_rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2466,7 +2466,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
Expand All @@ -2490,6 +2490,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;
Copy link
Collaborator

@tnull tnull Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a second should be enough for this?

node_a.sync_wallets().unwrap();
node_b.sync_wallets().unwrap();

Expand All @@ -2515,10 +2519,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.
Copy link
Contributor Author

@Camillarhi Camillarhi Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sleep is needed here because of test-environment-specific reasons, wait_for_tx uses the Electrum API to confirm the tx is in the mempool, but when Esplora is the chain source, Esplora may not have indexed it yet. If sync_wallets() runs in that window, BDK misses the tx and bump_fee_rbf can't find it via tx_details.

In practice, the fee of a transaction will not be bumped the instant they send. By the time a user decides to bump, the wallet has gone through multiple sync cycles, and the tx is long indexed. The sleep simulates that gap.

Copy link
Collaborator

@tnull tnull Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I guess that makes sense (interesting we never ran into this edge case so far). Though, a second should be more than enough for Esplora to catch up to Electrum I assume?

Copy link
Contributor Author

@Camillarhi Camillarhi Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes a second is enough but not always, locally it worked fine, but CI failed, so bumped it to 5 seconds be safe

// 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();

Expand All @@ -2540,26 +2544,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();

Expand All @@ -2579,19 +2566,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();
Expand All @@ -2613,10 +2587,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"),
}
Comment on lines +2595 to +2603
Copy link
Contributor Author

@Camillarhi Camillarhi Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to sync timing, the assertion verifying that node a's inbound payment txid has been updated to the replacement_txid is now done after the transaction is confirmed rather than immediately after each bump. This still fully validates the behaviour. Once confirmed, we check that the txid reflects the final bumped transaction, confirming that node A's sync correctly processed the replacement.

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);
}
Loading