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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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>

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

+
+// 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

+	///
+	/// 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?

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

 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?)?

Also a good point. I think the current approach worked fine when we still iterated on the wallet txs, but now with the events we should consistently update first to ensure events are replayed. Now added a fixup and another commit fixing it for apply_update.

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

 &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).

Hmm, good point, we should only skip if there's nothing to update. Added a fixup

TheBlueMatt

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

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.

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>

Previously, we'd update the payment store after persisting the wallet in some cases. This was fine as long as we iterated all wallet transactions anyways (hence idempotent). However, now that we use the event-based flow we should persist the payment store(s) first, so that wallet events get replayed if there was a crash in-between some of the persistence operations.

tnull · 2026-02-16T15:02:09Z

Really don't fully grok all the BDK parts, but from what I can tell this looks reasonable. Feel free to squash + land.

Squashed fixups without further changes.

joostjager · 2026-02-20T08:57:03Z

For the record, as I've also noted in other PRs that randomize tests: I don't think this is a good approach. A more extensive test suite that runs on main or as a nightly job, iterating through all backends, would be a far superior solution in my opinion.

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 ef6895dJanuary 23, 2026 15:24

tnull force-pushed the 2026-01-test-setup branch from 96b6f29 to b8179caCompare 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
Closed

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

tnull force-pushed the 2026-01-test-setup branch from b8179ca to 282605bCompare 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 d0946d1Compare 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 force-pushed the 2026-01-test-setup branch from d0946d1 to 7c08a0bCompare 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 c4ca6d3Compare February 13, 2026 09:54

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

TheBlueMatt reviewed Feb 13, 2026
View reviewed changes

tnull force-pushed the 2026-01-test-setup branch from c4ca6d3 to ce5955aCompare February 16, 2026 09:10

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

This was referenced Feb 16, 2026
Enhance onchain transaction management #628
Merged
Add ability to open channel / splice with all on-chain funds #788
Merged

TheBlueMatt approved these changes Feb 16, 2026
View reviewed changes

tnull added 4 commits February 16, 2026 16:01

tnull force-pushed the 2026-01-test-setup branch from ce5955a to 594e1fbCompare February 16, 2026 15:01

tnull requested a review from TheBlueMatt February 16, 2026 15:02

TheBlueMatt approved these changes Feb 16, 2026
View reviewed changes

tnull merged commit b55de44 into lightningdevkit:mainFeb 16, 2026
23 of 28 checks passed

github-project-automationBot moved this from Goal: Merge to Done in Weekly Goals Feb 16, 2026

tnull mentioned this pull request Mar 6, 2026
Wallet birthday height for seed recovery on pruned nodes #818
Closed

FreeOnlineUser mentioned this pull request Mar 10, 2026
Add wallet birthday height for seed recovery on pruned nodes #822
Closed