Skip to content

Fix Invalid Pending Payments and Strengthen RBF Tests #647

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix Invalid Pending Payments and Strengthen RBF Tests #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc9f907
nit: introduce setup_node and rename setup_node to setup_node_from_co…
moisesPompilio Sep 9, 2025
fb1c52c
Add manual block generation and direct transaction insertion for RBF …
moisesPompilio Sep 22, 2025
cfabeb2
Add property-based reorg test for RBF transactions
moisesPompilio Sep 22, 2025
ab6990e
Fix pending payments not confirmed due to invalidation or mempool rem…
moisesPompilio Sep 23, 2025
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
30 changes: 29 additions & 1 deletion src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use persist::KVStoreWalletPersister;
use crate::config::Config;
use crate::fee_estimator::{ConfirmationTarget, FeeEstimator, OnchainFeeEstimator};
use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger};
use crate::payment::store::ConfirmationStatus;
use crate::payment::store::{ConfirmationStatus, PaymentDetailsUpdate};
use crate::payment::{PaymentDetails, PaymentDirection, PaymentStatus};
use crate::types::{Broadcaster, PaymentStore};
use crate::Error;
Expand Down Expand Up @@ -153,6 +153,7 @@ impl Wallet {
fn update_payment_store<'a>(
&self, locked_wallet: &'a mut PersistedWallet<KVStoreWalletPersister>,
) -> Result<(), Error> {
let mut unconfirmed_txid = Vec::new();
for wtx in locked_wallet.transactions() {
let id = PaymentId(wtx.tx_node.txid.to_byte_array());
let txid = wtx.tx_node.txid;
Expand All @@ -164,6 +165,7 @@ impl Wallet {
{
PaymentStatus::Succeeded
} else {
unconfirmed_txid.push(id);
PaymentStatus::Pending
};
let confirmation_status = ConfirmationStatus::Confirmed {
Expand All @@ -174,6 +176,7 @@ impl Wallet {
(payment_status, confirmation_status)
},
bdk_chain::ChainPosition::Unconfirmed { .. } => {
unconfirmed_txid.push(id);
(PaymentStatus::Pending, ConfirmationStatus::Unconfirmed)
},
};
Expand Down Expand Up @@ -221,6 +224,31 @@ impl Wallet {
self.payment_store.insert_or_update(payment)?;
}

self.update_transactions_unconfirmed(&unconfirmed_txid)?;

Ok(())
}

fn update_transactions_unconfirmed(
&self, canonical_payment: &[PaymentId],
) -> Result<(), Error> {
let failed_payment = self.payment_store.list_filter(|payment| {
payment.status == PaymentStatus::Pending && !canonical_payment.contains(&payment.id)
});
if failed_payment.is_empty() {
return Ok(());
}

for failed_payment in &failed_payment {
let id = failed_payment.id;
let payment_update = PaymentDetailsUpdate {
id,
status: Some(PaymentStatus::Failed),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, not convinced we should do this, as on a semantic level there is no such thing as a 'failed' onchain payment. Valid transactions are forever pending as they could always be rebroadcasted even after being evicted from the local mempool. That is why we currently only track them as Pending or Confirmed (once they are irrevocably confirmed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point. When a transaction leaves the mempool due to a low fee, it can still be valid and eventually confirmed. The issue arises with RBF, since only one of the conflicting transactions can be confirmed, making the others effectively double-spent and thus “failed”. The challenge is that BDK doesn’t currently provide a way to distinguish between these cases.

My main motivation is that keeping transactions evicted from the mempool marked as Pending can lead to UX confusion, especially with RBF. For example, if a user broadcasts a transaction and then later replaces it via RBF, they would see two onchain payments, one that gets confirmed and another that never will. Leaving the latter as Pending would incorrectly suggest it’s still valid. Maybe having an additional status like Dropped or Evicted could better represent transactions that are no longer canonical in BDK?

..PaymentDetailsUpdate::new(id)
};
self.payment_store.update(&payment_update)?;
}

Ok(())
}

Expand Down
161 changes: 151 additions & 10 deletions tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,22 @@ use std::env;
use std::future::Future;
use std::path::PathBuf;
use std::pin::Pin;
use std::str::FromStr;
use std::sync::{Arc, RwLock};
use std::time::Duration;

use bitcoin::hashes::hex::FromHex;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;
use bitcoin::absolute::LockTime;
use bitcoin::block::{Header, Version as BlockVersion};
use bitcoin::hashes::{hex::FromHex, sha256::Hash as Sha256, Hash};
use bitcoin::merkle_tree::calculate_root;
use bitcoin::opcodes::all::OP_RETURN;
use bitcoin::script::Builder as BuilderScriptBitcoin;
use bitcoin::transaction::Version;
use bitcoin::{
Address, Amount, Network, OutPoint, ScriptBuf, Sequence, Transaction, Txid, Witness,
Address, Amount, Block, BlockHash, CompactTarget, Network, OutPoint, ScriptBuf, Sequence,
Transaction, TxMerkleNode, Txid, Witness, Wtxid,
};
use electrsd::corepc_node::TemplateRules;
use electrsd::corepc_node::{Client as BitcoindClient, Node as BitcoinD};
use electrsd::{corepc_node, ElectrsD};
use electrum_client::ElectrumApi;
Expand Down Expand Up @@ -285,7 +292,7 @@ pub(crate) fn setup_two_nodes(
) -> (TestNode, TestNode) {
println!("== Node A ==");
let config_a = random_config(anchor_channels);
let node_a = setup_node(chain_source, config_a, None);
let node_a = setup_node_from_config(chain_source, config_a, None);

println!("\n== Node B ==");
let mut config_b = random_config(anchor_channels);
Expand All @@ -301,11 +308,16 @@ pub(crate) fn setup_two_nodes(
.trusted_peers_no_reserve
.push(node_a.node_id());
}
let node_b = setup_node(chain_source, config_b, None);
let node_b = setup_node_from_config(chain_source, config_b, None);
(node_a, node_b)
}

pub(crate) fn setup_node(
pub(crate) fn setup_node(chain_source: &TestChainSource, anchor_channels: bool) -> TestNode {
let config = random_config(anchor_channels);
setup_node_from_config(chain_source, config, None)
}

pub(crate) fn setup_node_from_config(
chain_source: &TestChainSource, config: TestConfig, seed_bytes: Option<Vec<u8>>,
) -> TestNode {
setup_node_for_async_payments(chain_source, config, seed_bytes, None)
Expand Down Expand Up @@ -389,6 +401,133 @@ pub(crate) fn setup_node_for_async_payments(
node
}

pub(crate) fn generate_block_and_insert_transactions<E: ElectrumApi>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would be simpler to

  1. Put the RBF into the mempool
  2. Sync the wallet so it registers
  3. Mine a block
  4. Call Bitcoin Core's invalidateblock to discard the tip and the RBF
  5. Put the original TX into the mempool
  6. Sync the wallet again

If I'm not mistaken that would save us all of this boilerplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That approach doesn’t seem to work. After the block is mined and then invalidated, the replaced transaction isn’t discarded. I tested this flow, and when the original transaction is reintroduced, it fails because the existing transaction has a higher fee.

bitcoind: &BitcoindClient, electrs: &E, txs: &[Transaction],
) {
let _ = bitcoind.create_wallet("ldk_node_test");
let _ = bitcoind.load_wallet("ldk_node_test");
let blockchain_info = bitcoind.get_blockchain_info().expect("failed to get blockchain info");
let cur_height = blockchain_info.blocks;
let address = bitcoind.new_address().expect("failed to get new address");

let request_block_template =
corepc_node::TemplateRequest { rules: vec![TemplateRules::Segwit] };
let bt =
bitcoind.get_block_template(&request_block_template).expect("failed to get block template");

// === BIP 141: Witness Commitment Calculation ===
let witness_root = if txs.is_empty() {
TxMerkleNode::all_zeros()
} else {
// BIP 141: Create wtxid list starting with all-zeros for coinbase
let wtxids: Vec<Wtxid> = std::iter::once(Wtxid::all_zeros())
.chain(txs.iter().map(|tx| tx.compute_wtxid()))
.collect();

calculate_root(wtxids.into_iter())
.map(|root| TxMerkleNode::from_byte_array(root.to_byte_array()))
.unwrap()
};

// BIP 141: Witness reserved value (32 zero bytes)
let witness_reserved_value = [0u8; 32];

// BIP 141: Calculate commitment hash = Double-SHA256(witness root || witness reserved value)
let mut commitment_preimage = Vec::new();
commitment_preimage.extend_from_slice(&witness_root.to_byte_array());
commitment_preimage.extend_from_slice(&witness_reserved_value);
let commitment_hash = Sha256::hash(&commitment_preimage);

// === Coinbase Transaction Construction ===
// BIP 141: Coinbase witness contains the witness reserved value
let mut coinbase_witness = Witness::new();
coinbase_witness.push(&witness_reserved_value);

// BIP 141: Witness commitment output script
let mut bip_141_data = [0u8; 36];
bip_141_data[0] = 0xaa;
bip_141_data[1] = 0x21;
bip_141_data[2] = 0xa9;
bip_141_data[3] = 0xed;
bip_141_data[4..].copy_from_slice(&commitment_hash.to_byte_array());

// Format: OP_RETURN + OP_PUSHBYTES_36 + 0xaa21a9ed + 32-byte commitment hash
let witness_commitment_script =
BuilderScriptBitcoin::new().push_opcode(OP_RETURN).push_slice(bip_141_data).into_script();

// BIP 34: Block height in coinbase input script
let block_height = bt.height;
let height_script = BuilderScriptBitcoin::new()
.push_int(block_height as i64) // BIP 34: block height as first item
.push_int(bdk_chain::bitcoin::secp256k1::rand::random()) // Random nonce for uniqueness
.into_script();

// Do not use the coinbase value from the block template.
// The template may include transactions not actually mined, so fees may be incorrect.
let coinbase_output_value = 1_250_000_000;

let coinbase_tx = Transaction {
version: Version::ONE,
lock_time: LockTime::from_height(0).unwrap(),
input: vec![bitcoin::TxIn {
previous_output: bdk_chain::bitcoin::OutPoint::default(), // Null outpoint for coinbase
script_sig: height_script, // BIP 34: height + random data
sequence: Sequence::default(),
witness: coinbase_witness, // BIP 141: witness reserved value
}],
output: vec![
// Coinbase reward output
bitcoin::TxOut {
value: Amount::from_sat(coinbase_output_value),
script_pubkey: address.script_pubkey(),
},
// BIP 141: Witness commitment output (must be last output)
bitcoin::TxOut { value: Amount::ZERO, script_pubkey: witness_commitment_script },
],
};

// === Block Construction ===
let bits_hex = bt.bits.clone();
let bits_vec = Vec::<u8>::from_hex(&bits_hex).expect("invalid hex for bits");
let bits: [u8; 4] = bits_vec.try_into().expect("bits must be 4 bytes");
let prev_hash_block = BlockHash::from_str(&bt.previous_block_hash).expect("invalid prev hash");

let txdata = std::iter::once(coinbase_tx).chain(txs.iter().cloned()).collect::<Vec<_>>();
let mut block = Block {
header: Header {
version: BlockVersion::default(),
prev_blockhash: prev_hash_block,
merkle_root: TxMerkleNode::all_zeros(), // Will be calculated below
time: Ord::max(
bt.min_time,
std::time::UNIX_EPOCH.elapsed().unwrap().as_secs().try_into().unwrap(),
) as u32,
bits: CompactTarget::from_consensus(u32::from_be_bytes(bits)),
nonce: 0,
},
txdata,
};

block.header.merkle_root = block.compute_merkle_root().expect("must compute");

for nonce in 0..=u32::MAX {
block.header.nonce = nonce;
if block.header.target().is_met_by(block.block_hash()) {
break;
}
}

match bitcoind.submit_block(&block) {
Ok(_) => println!("Generated block 1 manually with {} transactions", txs.len()),
Err(e) => panic!("Failed to submit block: {:?}", e),
}
wait_for_block(electrs, cur_height as usize + 1);

txs.iter().for_each(|tx| {
wait_for_tx(electrs, tx.compute_txid());
});
}

pub(crate) fn generate_blocks_and_wait<E: ElectrumApi>(
bitcoind: &BitcoindClient, electrs: &E, num: usize,
) {
Expand Down Expand Up @@ -570,11 +709,13 @@ pub(crate) fn bump_fee_and_broadcast<E: ElectrumApi>(
let tx_bytes = Vec::<u8>::from_hex(&signed_result.hex).unwrap();
tx = bitcoin::consensus::encode::deserialize::<Transaction>(&tx_bytes).unwrap();

if is_insert_block {
generate_block_and_insert_transactions(bitcoind, electrs, &[tx.clone()]);
return tx;
}

match bitcoind.send_raw_transaction(&tx) {
Ok(res) => {
if is_insert_block {
generate_blocks_and_wait(bitcoind, electrs, 1);
}
let new_txid: Txid = res.0.parse().unwrap();
wait_for_tx(electrs, new_txid);
return tx;
Expand Down
Loading
Loading