Follow-ups to #660 by tankyleo · Pull Request #992 · lightningdevkit/ldk-node

tankyleo · 2026-07-17T05:18:04Z

Fixes #989

ldk-reviews-bot · 2026-07-17T05:18:06Z

👋 Thanks for assigning @tnull 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.

tankyleo · 2026-07-17T05:19:53Z

 use crate::Error;

-const BCAST_PACKAGE_QUEUE_SIZE: usize = 50;
+const BCAST_PACKAGE_QUEUE_SIZE: usize = 256;


I went back and forth with codex on this one, for now I lean against drawing transactions at random order: we do not do this now, but in the future we might want to make sure we broadcast parents before children no ?

but in the future we might want to make sure we broadcast parents before children no ?

Well, in short, I'm not sure we could even begin to guarantee this? Broadcast is inherently fallible, we never know when the network connection could drop, when the backend won't accept anything into the mempool, and when it will just decide to drop any transaction again. So without mempool introspection I'd always lean on treating broadcast as an entirely opaque operation: we submit and retry, and only stop once we see what we expect confirmed in a block.

sounds good added the random draw below

tankyleo · 2026-07-17T05:21:32Z

 }

+fn esplora_submitpackage_error_implies_unsupported(e: &esplora_client::Error) -> bool {
+	matches!(e, esplora_client::Error::HttpResponse { status: 400 | 404, .. })


We map 400 here to ChainSourceNotSupported because Bitcoin Core v26 returns a RPC error when submitting the dummy package, which maps to error code 400.

Grrr, this is all very brittle. I can't wait to drop all of this logic again once we can just assume submitpackage is available if we have any chain source at all.

tnull · 2026-07-17T08:10:52Z

-		})?;
+		});
+		if let Err(e) = startup_chain_check_res {
+			self.chain_source.stop();


Actually, it seems there are a bunch of (pre-existing) potential cases below where we'd error. To solve this once and for all, let's rename start to start_inner and add a new pub fn start that wraps that start_inner (probably also takes the running lock) and stops the chain source for any error returned.

tnull · 2026-07-17T08:12:31Z

      - name: Test with 0FC enabled
        run: |
          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1
+  eclair-interop-test:


Hmm, any chance we could make this an interop test for 0fc channels, not a 0fc test that does interop? I.e., can this be (a cfg-gated) part of the regular eclair tests, where we already have the docker setup etc.?

tnull

This needs a rebase post #996.

Btw, it seems the 0fc integration test is flaky, i.e., timing out in CI very often. Would be good to investigate why and include a fix as part of this PR.

tnull · 2026-07-21T08:23:58Z

+		match self.start_inner(&mut is_running_lock) {
+			Ok(()) => Ok(()),
+			Err(e) => {
+				self.chain_source.stop();


Codex:

[P2] Roll back background tasks after startup failure. /home/tnull/worktrees/ldk-node/pr-992-latest-20260721/src/lib.rs:294 now stops only the chain source when start_inner fails. However, wallet sync, RGS, and pathfinding tasks are spawned before listener resolution/binding can fail at
lines 427–473. The node remains “not running,” so stop() cannot clean them up, while another start() creates duplicate loops. Move fallible listener setup before task spawning or perform a complete task rollback.

In 2024749, we started taking one slot in the package queue for each transaction broadcasted by the wallet upon `WalletEvent::ChainTipChanged`, so we increase the number of slots available in the queue.

Co-Authored-By: HAL 9000

This is particularly relevant for the electrum chain source; if we fail to fetch feerates, or zero fee commitments validation fails, and we do not stop the electrum chain source before returning an error, then the user will hit a debug assertion on the next restart.

Co-Authored-By: HAL 9000

tnull

Thanks! Seems the 0fc integration tests are still timing out in CI here?

tnull · 2026-07-24T10:52:32Z

 								})
 								.collect()
 						};
+						for i in (1..txs_to_broadcast.len()).rev() {


Hmm, shouldn't this be a feature of the broadcaster queue in general rather than just randomizing this one callsite? Maybe we could just have it drop random entries when the queue is full?

The broadcast queue itself doesn't currently allow us to drop random entries when it is full; this would require a new data structure inside the queue that allows for this random removal.

If we want this, I think this should be a separate PR no?

tnull · 2026-07-24T10:58:31Z

 /// We use this parent-child TRUC package to make sure the configured chain source supports
 /// broadcasting packages via the `submitpackage` Bitcoin Core RPC.
-const PARENT_TXID: &str = "9a015f93fac6cb203c2b994e18b85176eb0354a22a468255516f3c6002d3f696";
+const DUMMY_PACKAGE_EXPECTED_ERROR: &str = "bad-txns-inputs-missingorspent";


Hmm, matching on the exact string seems not very robust. Seems some minor change in how Bitcoin Core handles errors could lead to LDK Node not starting anymore.

As mentioned on #660 (comment), maybe we just need to accept that we currently don't have a good way of checking all backends support v29+? Should we drop this commit?

I could totally see us dropping this commit. Here's my argument against dropping it:

We'd only abort startup in case enable_zero_fee_commitments is true, and we default to false at the moment. So given this default, I think we can be more aggressive in how we check for 0FC support once the flag is turned on. If someone turns this flag on and fails to start, they can easily turn this flag back off.

With codex, I've tested this validation across 26, 27, 28, 29, 30, and 31, and across mempool, blockstream, romanz, Fulcrum, and ElectrumX. With codex I think we can easily test this logic against upcoming releases as they come out, since Core only ships two releases a year, and we can adjust this as the reality on the ground changes.

tnull · 2026-07-24T10:59:38Z

      - name: Test with 0FC enabled
        run: |
-          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1
+          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test


Seems that should be a fixup commit?

I was just testing things out to see if removing this flag helped. It doesn't look like it I am going to revisit this.

tnull · 2026-07-24T11:05:27Z

 			return Err(Error::AlreadyRunning);
 		}

+		match self.start_inner(&mut is_running_lock) {


Codex:

[P1] Fully unwind late startup failures — /home/tnull/worktrees/ldk-node/pr-992-latest-20260724/src/lib.rs:295

A listener resolution/bind failure can occur after wallet sync and other background tasks have already spawned. The new error handler only stops the chain source. Since is_running remains false, callers cannot invoke stop(), and retrying start() leaves duplicate tasks running. Fallible
listener setup should occur before spawning tasks, or the error path must fully unwind them.

Seems that might be worth an additional PR though. Let me know if you prefer I pick that up.

Let me know what you think of the commit below, we move the fallible operations before starting the tasks.

tnull · 2026-07-24T11:06:06Z

+}
+
+#[uniffi::export]
+impl ChannelTypeFeatures {


Codex:

[P2] Expose all channel-type feature flags to bindings — /home/tnull/worktrees/ldk-node/pr-992-latest-20260724/src/ffi/types.rs:1830

ChannelTypeFeatures omits typed accessors for option_scid_alias and option_zeroconf, although both are valid ChannelTypeContext features supported by the underlying type. Rust callers retain those methods, but UniFFI users must manually decode to_bytes().

Done below.

Resolve and bind configured listening addresses before starting wallet sync, gossip sync, pathfinding score sync, and the remaining background loops. If listener setup now fails, startup returns while only the chain source needs cleanup, so a retry cannot leave duplicate loops behind. AI-assisted-by: OpenAI Codex

Require Electrum and Esplora zero-fee commitments validation to observe the Bitcoin Core v29+ failure shape for the dummy TRUC package, instead of accepting any structured submitpackage response. Set the locktime field of the transaction to zero so that the test works at any chain-height, which is particularly helpful when starting ldk-node against test networks. Map HTTP 400 errors returned to `ChainSourceNotSupported` as this error code is returned by blockstream-electrs and mempool-electrs when running against Bitcoin Core v26. We previously would map this error to a general `ConnectionFailed` error, which is not correct for Bitcoin Core v26. Co-Authored-By: HAL 9000

Add a UniFFI wrapper so bindings can inspect channel type flags. Update anchor accounting tests to use channel type features instead of inferring zero-fee commitments from the commitment feerate. Co-Authored-By: HAL 9000

Expose typed UniFFI accessors for option_scid_alias and option_zeroconf on ChannelTypeFeatures, matching the underlying LDK feature API. Add coverage for optional and required flag forms. Co-Authored-By: HAL 9000

Co-Authored-By: HAL 9000

tankyleo requested a review from tnull July 17, 2026 05:18

tankyleo commented Jul 17, 2026

View reviewed changes

tnull reviewed Jul 17, 2026

View reviewed changes

tankyleo force-pushed the 2026-07-0fc-followups branch 2 times, most recently from edf8512 to bda78cc Compare July 17, 2026 17:46

tankyleo requested a review from tnull July 17, 2026 18:38

tnull reviewed Jul 21, 2026

View reviewed changes

tankyleo added 4 commits July 24, 2026 04:47

Increase the size of the broadcast package queue

4e7516b

In 2024749, we started taking one slot in the package queue for each transaction broadcasted by the wallet upon `WalletEvent::ChainTipChanged`, so we increase the number of slots available in the queue.

f: Draw the rebroadcast tx batch at random

3da69d8

Co-Authored-By: HAL 9000

f: Stop chain source after *all* startup failures

670d952

Co-Authored-By: HAL 9000

tankyleo force-pushed the 2026-07-0fc-followups branch from bda78cc to 30616f5 Compare July 24, 2026 04:53

tnull added this to the 0.8 milestone Jul 24, 2026

tnull reviewed Jul 24, 2026

View reviewed changes

tankyleo force-pushed the 2026-07-0fc-followups branch from 30616f5 to 3985da7 Compare July 24, 2026 22:25

tankyleo added 6 commits July 24, 2026 23:31

Expose ChannelTypeFeatures in ChannelDetails

00826a1

Add a UniFFI wrapper so bindings can inspect channel type flags. Update anchor accounting tests to use channel type features instead of inferring zero-fee commitments from the commitment feerate. Co-Authored-By: HAL 9000

f: expose channel type feature flags

10b3030

Expose typed UniFFI accessors for option_scid_alias and option_zeroconf on ChannelTypeFeatures, matching the underlying LDK feature API. Add coverage for optional and required flag forms. Co-Authored-By: HAL 9000

Add Eclair interop test for zero-fee-commitments

62bf391

Co-Authored-By: HAL 9000

f: move 0FC eclair interop test to eclair workflow

86bfd98

tankyleo force-pushed the 2026-07-0fc-followups branch from 3985da7 to 114d9c9 Compare July 24, 2026 23:50

f: give 0fc integration more time to complete

35d0a28

tankyleo force-pushed the 2026-07-0fc-followups branch from 114d9c9 to 35d0a28 Compare July 24, 2026 23:52

Uh oh!

Conversation

tankyleo commented Jul 17, 2026 • edited by tnull Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tankyleo Jul 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tankyleo commented Jul 17, 2026 •

edited by tnull

Loading

ldk-reviews-bot commented Jul 17, 2026 •

edited

Loading

tankyleo Jul 17, 2026 •

edited

Loading

tankyleo Jul 17, 2026 •

edited

Loading

tankyleo Jul 24, 2026 •

edited

Loading