Randomize chain source selection in tests by tnull · Pull Request #769 · lightningdevkit/ldk-node

tnull · 2026-01-23T14:00:54Z

.. all of our tests should be robust against switching chain sources. We
here opt to pick a random one each time to considerably extend our test
coverage, instead of just running some cases against non-Esplora chain
sources.

ldk-reviews-bot · 2026-01-23T14:00:57Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2026-01-23T14:02:49Z

This will be flaky, at the very least until lightningdevkit/rust-lightning#4341 gets release, which would have been caught if we'd ever had run the 0conf test case with an Electrum chain source.

TheBlueMatt

makes sense to me. Hopefully CI fails until lightningdevkit/rust-lightning#4341

tnull · 2026-01-23T14:36:20Z

makes sense to me. Hopefully CI fails until lightningdevkit/rust-lightning#4341

I think I'll whack-a-mole a few more flakes/bugs before landing this.

tnull · 2026-01-23T16:16:51Z

Previously

    simple_bolt12_send_receive (bitcoind RPC)
    test_node_announcement_propagation (bitcoind RPC)

had failed, I want to double-check them once more before moving forward here.

tnull · 2026-01-29T19:12:37Z

Pushed an update, but currently onchain_send_receive is still (or newly?) broken when syncing with bitcoind. Will look into that.

It's weird to have a special intermediary `setup_node` method if we have `TestConfig` for exactly that reason by now. So we move `async_payment_role` over.

tnull · 2026-02-12T10:10:57Z

Pushed an update, but currently onchain_send_receive is still (or newly?) broken when syncing with bitcoind. Will look into that.

Alright, fixed that one, too and rebased. Should be ready for review now.

.. all of our tests should be robust against switching chain sources. We here opt to pick a random one each time to considerably extend our test coverage, instead of just running some cases against non-Esplora chain sources. Signed-off-by: Elias Rohrer <dev@tnull.de>

When we intially implemented `bitcoind` syncing polling the mempool was very frequent and rather inefficient so we made a choice not to unnecessarily update the payment store for mempool changes, especially since we only consider transactions `Succeeded` after `ANTI_REORG_DELAY` anyways. However, since then we made quite a few peformance improvements to the mempool syncing, and by now we should just update they payment store as not doing so will lead to rather unexpected behavior, making some tests fail for `TestChainSource::Bitcoind`, e.g., `channel_full_cycle_0conf`, which we fix here. As we recently switched to updating the payment store based on BDK's `WalletEvent`, but they currently don't offer an API returning such events when applying mempool transactions, we copy over the respective method for generating events from `bdk_wallet`, with the intention of dropping it again once they do. Signed-off-by: Elias Rohrer <dev@tnull.de>

Previously, we fixed than a fresh node syncing via `bitcoind` RPC would resync all chain data back to genesis. However, while introducing a wallet birthday is great, it disallowed discovery of historical funds if a wallet would be imported from seed. Here, we add a recovery mode flag to the builder that explictly allows to re-enable resyncing from genesis in such a scenario. Going forward, we intend to reuse that API for an upcoming Lightning recoery flow, too.

TheBlueMatt

I don't really know BDK well enough to feel like I can meaningfully review this, but a few questions.

TheBlueMatt · 2026-02-12T13:57:17Z

src/wallet/mod.rs

+
+// FIXME/TODO: This is copied-over from bdk_wallet and only used to generate `WalletEvent`s after
+// applying mempool transactions. We should drop this when BDK offers to generate events for
+// mempool transactions natively.


Hmm, if BDK doesn't support this natively, do they have logic to correctly handle conflicts and build balance knowledge appropriately? ie if there's an RBF that conflicts do we remove the 0conf tx from our balance?

Yes, they support all of the canonical transaction handling, it's just when they recently introduced the WalletEvent, they didn't update all APIs we require to return them. Arguably, they should just expose the helper we here copied over which would avoid them having to provide _event variants for all API endpoints individually. See also the discussion on bitcoindevkit/bdk_wallet#374

TheBlueMatt · 2026-02-12T13:58:31Z

src/builder.rs

+	///
+	/// This should only be set on first startup when importing an older wallet from a previously
+	/// used [`NodeEntropy`].
+	pub fn set_wallet_recovery_mode(&mut self) -> &mut Self {


Rather than a coarse "recovery mode" can we just enable setting the "wallet birthday"? In some setups that might be available and would avoid a lot of effort.

Hmm, wallet birthday support is still inflight on the BDK side (see bitcoindevkit/bdk_wallet#368) and I'd like to punt on it until we can actually make use of that and remove our hacky birthday logic.

That said, if you prefer we could of course rename the current "recovery_mode" API to "disable_wallet_birthday" for now which ~does the same thing?

Previously, we'd selectively insert the funding outputs into the onchain wallet to later allow calculating `fees_paid` when creating payment store entries (only for splicing mostly). However, this didn't always work, and we might for example end up with a missing funding output (and hence would fall back to `fees_paid: Some(0)`) if it was a counterparty-initiated channel and we synced via `bitcoind` RPC. Here, we fix this by tracking all LDK-registered `txids` in `ChainSource` and then in the `Wallet`'s `Listen` implementation insert all outputs of all registered transactions into the `Wallet`, ensuring we'd always have sufficient data for `calculate_fee` available. Thereby we also fix the `onchain_send_receive` test which previously failed when using `TestChainSource::Bitcoind`. Signed-off-by: Elias Rohrer <dev@tnull.de>

tnull · 2026-02-13T09:55:37Z

Amended the last commit to include a comment explaining what we do/why we do it:

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index 835e1b31..2decf981 100644
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -1031,4 +1031,12 @@ impl Listen for Wallet {
                }

+               // In order to be able to reliably calculate fees the `Wallet` needs access to the previous
+               // ouput data. To this end, we here insert any ouputs of transactions that LDK is intersted
+               // in (e.g., funding transaction ouputs) into the wallet's transaction graph when we see
+               // them, so it is reliably able to calculate fees for subsequent spends.
+               //
+               // FIXME: technically, we should also do this for mempool transactions. However, at the
+               // current time fixing the edge case doesn't seem worth the additional conplexity /
+               // additional overhead..
                let registered_txids = self.chain_source.registered_txids();
                for tx in &block.txdata {

TheBlueMatt · 2026-02-13T14:21:16Z

src/wallet/mod.rs

 			Error::PersistenceFailed
 		})?;

+		self.update_payment_store(&mut *locked_wallet, events).map_err(|e| {


Should this happen before locked_wallet.persist()? What happens if we crash between the wallet persist and the payment store update (presumably the second is idempotent so we should do it first to reply on restart if needed?)?

TheBlueMatt · 2026-02-13T14:23:24Z

src/wallet/mod.rs

 		&self, unconfirmed_txs: Vec<(Transaction, u64)>, evicted_txids: Vec<(Txid, u64)>,
 	) -> Result<(), Error> {
+		if unconfirmed_txs.is_empty() {
+			return Ok(());


This doesn't appear to be tested. Also not clear to me why we'd want to skip this if we don't have any unconfirmed txs in the mempool but do have some txs that got evicted (iiuc).

tnull requested a review from TheBlueMatt January 23, 2026 14:00

TheBlueMatt previously approved these changes Jan 23, 2026

View reviewed changes

tnull dismissed TheBlueMatt’s stale review via ef6895d January 23, 2026 15:24

tnull force-pushed the 2026-01-test-setup branch from 96b6f29 to b8179ca Compare January 23, 2026 16:36

m1sterc001guy mentioned this pull request Jan 27, 2026

chore: Update Lightning dependencies fedimint/fedimint#8198

Draft

tnull mentioned this pull request Jan 29, 2026

Drop copied-over wallet_events when possible #776

Open

tnull marked this pull request as draft January 29, 2026 19:12

tnull force-pushed the 2026-01-test-setup branch from b8179ca to 282605b Compare January 29, 2026 19:12

Move async_payment_role to TestConfig

b476e05

It's weird to have a special intermediary `setup_node` method if we have `TestConfig` for exactly that reason by now. So we move `async_payment_role` over.

tnull force-pushed the 2026-01-test-setup branch from 7796735 to d0946d1 Compare February 12, 2026 10:10

tnull marked this pull request as ready for review February 12, 2026 10:10

tnull requested a review from TheBlueMatt February 12, 2026 10:11

tnull added 3 commits February 12, 2026 11:21

tnull force-pushed the 2026-01-test-setup branch from d0946d1 to 7c08a0b Compare February 12, 2026 10:22

TheBlueMatt reviewed Feb 12, 2026

View reviewed changes

tnull added this to LDK Priorities and Weekly Goals Feb 12, 2026

tnull removed this from LDK Priorities Feb 12, 2026

tnull moved this to Goal: Merge in Weekly Goals Feb 12, 2026

tnull self-assigned this Feb 12, 2026

tnull force-pushed the 2026-01-test-setup branch from 7c08a0b to c4ca6d3 Compare February 13, 2026 09:54

tnull requested a review from TheBlueMatt February 13, 2026 09:56

TheBlueMatt reviewed Feb 13, 2026

View reviewed changes

Conversation

tnull commented Jan 23, 2026

Uh oh!

ldk-reviews-bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Jan 23, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jan 23, 2026

Uh oh!

tnull commented Jan 23, 2026

Uh oh!

tnull commented Jan 29, 2026

Uh oh!

tnull commented Feb 12, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Feb 13, 2026

Uh oh!

TheBlueMatt Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Jan 23, 2026 •

edited

Loading

tnull Feb 12, 2026 •

edited

Loading