Skip to content

Commit 569cebe

Browse files
moisesPompiliomoisesPompilio
committed
Fix pending payments not confirmed due to invalidation or mempool removal
Pending payments that become invalid or are removed from the mempool are now marked as failed. RBF tests were updated to check that only confirmed transactions succeed and all others are failed.only the actually confirmed transactions have the correct
1 parent 69959f7 commit 569cebe

File tree

3 files changed

+92
-14
lines changed

3 files changed

+92
-14
lines changed

‎src/wallet/mod.rs‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::config::Config;
1111
use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger};
1212

1313
use crate::fee_estimator::{ConfirmationTarget, FeeEstimator};
14-
use crate::payment::store::ConfirmationStatus;
14+
use crate::payment::store::{ConfirmationStatus, PaymentDetailsUpdate};
1515
use crate::payment::{PaymentDetails, PaymentDirection, PaymentStatus};
1616
use crate::types::PaymentStore;
1717
use crate::Error;
@@ -165,6 +165,7 @@ where
165165
fn update_payment_store<'a>(
166166
&self, locked_wallet: &'a mut PersistedWallet<KVStoreWalletPersister>,
167167
) -> Result<(), Error> {
168+
let mut unconfirmed_txid = Vec::new();
168169
for wtx in locked_wallet.transactions() {
169170
let id = PaymentId(wtx.tx_node.txid.to_byte_array());
170171
let txid = wtx.tx_node.txid;
@@ -176,6 +177,7 @@ where
176177
{
177178
PaymentStatus::Succeeded
178179
} else {
180+
unconfirmed_txid.push(id);
179181
PaymentStatus::Pending
180182
};
181183
let confirmation_status = ConfirmationStatus::Confirmed {
@@ -186,6 +188,7 @@ where
186188
(payment_status, confirmation_status)
187189
},
188190
bdk_chain::ChainPosition::Unconfirmed { .. } => {
191+
unconfirmed_txid.push(id);
189192
(PaymentStatus::Pending, ConfirmationStatus::Unconfirmed)
190193
},
191194
};
@@ -233,6 +236,31 @@ where
233236
self.payment_store.insert_or_update(payment)?;
234237
}
235238

239+
self.update_transactions_unconfirmed(&unconfirmed_txid)?;
240+
241+
Ok(())
242+
}
243+
244+
fn update_transactions_unconfirmed(
245+
&self, canonical_payment: &[PaymentId],
246+
) -> Result<(), Error> {
247+
let failed_payment = self.payment_store.list_filter(|payment| {
248+
payment.status == PaymentStatus::Pending && !canonical_payment.contains(&payment.id)
249+
});
250+
if failed_payment.is_empty() {
251+
return Ok(());
252+
}
253+
254+
for failed_payment in &failed_payment {
255+
let id = failed_payment.id;
256+
let payment_update = PaymentDetailsUpdate {
257+
id,
258+
status: Some(PaymentStatus::Failed),
259+
..PaymentDetailsUpdate::new(id)
260+
};
261+
self.payment_store.update(&payment_update)?;
262+
}
263+
236264
Ok(())
237265
}
238266

‎tests/integration_tests_rust.rs‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,9 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
750750
let mut final_amount_sat = 0;
751751
let mut original_tx;
752752

753+
let mut array_original_txid = Vec::new();
754+
let mut array_rbf_txid = Vec::new();
755+
753756
// Step 1: Bump fee and change output address
754757
distribute_funds_all_nodes!();
755758
validate_balances!(amount_sat, false);
@@ -769,18 +772,21 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
769772
final_amount_sat += amount_sat;
770773
}
771774
validate_balances!(final_amount_sat, is_insert_block || is_insertion_original_tx);
775+
array_original_txid.push(original_tx.compute_txid());
772776

773777
// Step 2: Bump fee only
774778
distribute_funds_all_nodes!();
775779
validate_total_onchain_balance!(amount_sat + final_amount_sat);
776780
(tx, fee_output_index) = prepare_rbf(electrs, txid, &scripts_buf);
777781
original_tx = tx.clone();
778-
bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
782+
let rbf_tx = bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
779783
if is_insertion_original_tx {
780784
generate_block_and_insert_transactions(bitcoind, electrs, &[original_tx.clone()]);
781785
}
782786
final_amount_sat += amount_sat;
783787
validate_balances!(final_amount_sat, is_insert_block || is_insertion_original_tx);
788+
array_original_txid.push(original_tx.compute_txid());
789+
array_rbf_txid.push(rbf_tx.compute_txid());
784790

785791
// Step 3: Increase output value
786792
let value_sat = 21_000;
@@ -794,7 +800,7 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
794800
}
795801
});
796802
tx.output[fee_output_index].value -= Amount::from_sat(scripts_buf.len() as u64 * value_sat);
797-
bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
803+
let rbf_tx = bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
798804
if is_insertion_original_tx {
799805
generate_block_and_insert_transactions(bitcoind, electrs, &[original_tx.clone()]);
800806
}
@@ -803,6 +809,8 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
803809
final_amount_sat += value_sat;
804810
}
805811
validate_balances!(final_amount_sat, is_insert_block || is_insertion_original_tx);
812+
array_original_txid.push(original_tx.compute_txid());
813+
array_rbf_txid.push(rbf_tx.compute_txid());
806814

807815
// Step 4: Decrease output value
808816
distribute_funds_all_nodes!();
@@ -815,7 +823,7 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
815823
}
816824
});
817825
tx.output[fee_output_index].value += Amount::from_sat(scripts_buf.len() as u64 * value_sat);
818-
bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
826+
let rbf_tx = bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_insert_block);
819827
if is_insertion_original_tx {
820828
generate_block_and_insert_transactions(bitcoind, electrs, &[original_tx.clone()]);
821829
}
@@ -824,10 +832,31 @@ fn run_rbf_test(is_insert_block: bool, is_insertion_original_tx: bool) {
824832
final_amount_sat -= value_sat;
825833
}
826834
validate_balances!(final_amount_sat, is_insert_block || is_insertion_original_tx);
835+
array_original_txid.push(original_tx.compute_txid());
836+
array_rbf_txid.push(rbf_tx.compute_txid());
827837

828-
if !is_insert_block {
829-
generate_blocks_and_wait(bitcoind, electrs, 1);
830-
validate_balances!(final_amount_sat, true);
838+
// Confirm transaction
839+
generate_blocks_and_wait(bitcoind, electrs, 6);
840+
validate_balances!(final_amount_sat, true);
841+
842+
// Validate the list of payments: all must be succeeded and match the confirmed on-chain txids.
843+
let confirmed_onchain_txids =
844+
if is_insertion_original_tx { array_original_txid } else { array_rbf_txid };
845+
for node in &nodes {
846+
let all_payments: Vec<PaymentDetails> = node.list_payments();
847+
let pending: Vec<_> =
848+
all_payments.iter().filter(|p| p.status == PaymentStatus::Pending).collect();
849+
850+
assert!(pending.is_empty());
851+
852+
let succeeded: Vec<_> =
853+
all_payments.iter().filter(|p| p.status == PaymentStatus::Succeeded).collect();
854+
assert_eq!(succeeded.len(), confirmed_onchain_txids.len());
855+
for p in succeeded {
856+
assert_eq!(p.direction, PaymentDirection::Inbound);
857+
assert!(matches!(p.kind, PaymentKind::Onchain { txid: _, .. }));
858+
assert!(confirmed_onchain_txids.contains(&bitcoin::Txid::from_slice(&p.id.0).unwrap()));
859+
}
831860
}
832861

833862
// Check if it is possible to send all funds from the node

‎tests/reorg_test.rs‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
mod common;
2+
use bitcoin::hashes::Hash;
23
use bitcoin::{Amount, ScriptBuf};
3-
use ldk_node::payment::{PaymentDirection, PaymentKind};
4+
use ldk_node::payment::{PaymentDirection, PaymentKind, PaymentStatus};
45
use ldk_node::{Event, LightningBalance, PendingSweepBalance};
6+
use lightning::ln::channelmanager::PaymentId;
57
use proptest::strategy::Strategy;
68
use proptest::strategy::ValueTree;
79
use proptest::{prelude::prop, proptest};
@@ -220,11 +222,26 @@ proptest! {
220222
let txid = distribute_funds_unconfirmed(bitcoind, electrs, all_addrs, Amount::from_sat(amount_sat));
221223

222224
let mut is_spendable = false;
225+
let mut expect_payment_status = PaymentStatus::Pending;
223226
macro_rules! verify_wallet_balances_and_transactions {
224-
($expected_balance_sat: expr, $expected_size_list_payments: expr) => {
227+
($expected_balance_sat: expr, $expected_size_list_payments: expr, $expect_payment_id: expr) => {
225228
let spend_balance = if is_spendable { $expected_balance_sat } else { 0 };
226229
for node in &nodes {
227230
node.sync_wallets().unwrap();
231+
let all_payments = node.list_payments();
232+
assert_eq!(all_payments.len(), $expected_size_list_payments);
233+
234+
let payment: Vec<_> =
235+
all_payments.iter().filter(|p| p.status == expect_payment_status).collect();
236+
assert_eq!(payment.len(), 1);
237+
assert_eq!(payment[0].id, $expect_payment_id);
238+
239+
// There should be one payment with the expected status (Succeeded or Pending),
240+
// and all other payments must be marked as Failed.
241+
let failed: Vec<_> =
242+
all_payments.iter().filter(|p| p.status == PaymentStatus::Failed).collect();
243+
assert_eq!(failed.len() + payment.len(), all_payments.len());
244+
228245
let balances = node.list_balances();
229246
assert_eq!(balances.total_onchain_balance_sats, $expected_balance_sat);
230247
assert_eq!(balances.spendable_onchain_balance_sats, spend_balance);
@@ -235,8 +252,9 @@ proptest! {
235252
let mut tx_to_amount = HashMap::new();
236253
let (mut tx, fee_output_index) = prepare_rbf(electrs, txid, &scripts_buf);
237254
tx_to_amount.insert(tx.clone(), amount_sat);
255+
let mut expected_size_list_payments = 1;
238256
generate_block_and_insert_transactions(bitcoind, electrs, &[]);
239-
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments);
257+
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments, PaymentId(tx.compute_txid().to_byte_array()));
240258
for _ in 0..quantity_rbf {
241259
let is_alterable_value = prop::bool::ANY.new_tree(&mut runner).unwrap().current();
242260
if is_alterable_value {
@@ -259,8 +277,10 @@ proptest! {
259277

260278
tx = bump_fee_and_broadcast(bitcoind, electrs, tx, fee_output_index, is_spendable);
261279
tx_to_amount.insert(tx.clone(), amount_sat);
262-
263-
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments);
280+
expected_size_list_payments += 1;
281+
// Bitcoind needs a block to insert the transactions in list payments
282+
generate_block_and_insert_transactions(bitcoind, electrs, &[]);
283+
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments, PaymentId(tx.compute_txid().to_byte_array()));
264284
}
265285

266286
is_spendable = true;
@@ -269,7 +289,7 @@ proptest! {
269289
generate_block_and_insert_transactions(bitcoind, electrs, &[tx_to_confirm.0.clone()]);
270290
generate_blocks_and_wait(bitcoind, electrs, reorg_depth - 1);
271291
amount_sat = *tx_to_confirm.1;
272-
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments);
292+
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments, PaymentId(tx_to_confirm.0.compute_txid().to_byte_array()));
273293

274294
invalidate_blocks(bitcoind, reorg_depth);
275295
generate_block_and_insert_transactions(bitcoind, electrs, &[]);
@@ -278,7 +298,8 @@ proptest! {
278298
let tx_to_confirm = tx_to_amount.iter().nth(index_tx_confirm).unwrap();
279299
generate_block_and_insert_transactions(bitcoind, electrs, &[tx_to_confirm.0.clone()]);
280300
amount_sat = *tx_to_confirm.1;
301+
expect_payment_status = PaymentStatus::Succeeded;
281302
generate_blocks_and_wait(bitcoind, electrs, 5);
282-
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments);
303+
verify_wallet_balances_and_transactions!(amount_sat, expected_size_list_payments, PaymentId(tx_to_confirm.0.compute_txid().to_byte_array()));
283304
}
284305
}

0 commit comments

Comments
 (0)