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.

tnull

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

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.

I don't think we'd want to do this, as we deliberately allow inbound connections as one of the last steps after we're positive we're ready-to-go.

Help me understand thank you, once we've passed all the can-fail calls, aren't we positive we are ready to go ?

Ah, I think I was misunderstanding what you're doing. Still, just moving code around so we don't start tasks before possibly emitting an error seems very brittle. We might easily add another error'ing case sometime soon, and then have the same issue again. We probably need to fix it 'properly' by signalling stop to the tasks etc., but doing this is probably out-of-scope for this PR as mentioned above.

Sounds good I kept the commit below, and we can add the proper signalling in the next PR.

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.

tnull

Feel free to squash fixups (or drop as discussed above).

tnull · 2026-07-27T09:25:24Z

 jobs:
  build-and-test:
-    timeout-minutes: 60
+    timeout-minutes: 75


Hmm, if this is the only issue, let's bump the timeout to to hours across the board to avoid other workflows hitting the same issue?

This was not the issue, I reverted this, and instead I now fetch feerates and validate zero-fee commitments support serially, see the commit below.

Hmm, if indeed the described (kinda well-known) Electrum issue might be the source, we should probably not just patch around this one instance in production code to make CI pass. Either we find a hotfix that is test-only for now or we need to look into if/how to tackle the electrum issue properly (in a separate PR though). As a first step, mind reverting the diff here and opening an issue that documents your findings?

Gave it some thought, I have some concerns about reverting the diff here. Yes we've only hit this issue in CI so far, but this means we could hit this issue in prod on some user's machine at some point ?

I would revert the diff after we've found and shipped a proper fix for the Electrum issue.

Let me know what you think.

tnull · 2026-07-27T09:45:25Z

 			return Err(Error::AlreadyRunning);
 		}

+		match self.start_inner(&mut is_running_lock) {


I don't think we'd want to do this, as we deliberately allow inbound connections as one of the last steps after we're positive we're ready-to-go.

tankyleo · 2026-07-28T05:09:44Z

The third run just passed, I believe CI for 0FC is now stable, will launch another run to check things again.

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.

This is particularly relevant for the electrum chain source; if we fail startup, 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. We also move code so as to complete all the fallible startup calls before spawning the other background tasks. We do not yet have a way to clean the tasks up if a startup error is returned after the tasks have been spawned.

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

Co-Authored-By: HAL 9000

In case of a timeout, these logs are very useful, and would otherwise fail to show.

When concurrent requests share an Electrum client connection, a clean EOF can cause the active reader to return an error without waking other requests waiting on the connection. Those requests remain blocked in spawn_blocking, causing Tokio runtime shutdown to wait indefinitely for them to finish. As a temporary fix, we run the startup fee update and zero-fee commitment validation in sequence. This avoids concurrent requests on the shared Electrum connection leaving a blocking task stranded after connection closure. 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 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 3 times, most recently from 114d9c9 to 35d0a28 Compare July 24, 2026 23:52

tnull reviewed Jul 27, 2026

View reviewed changes

tnull mentioned this pull request Jul 27, 2026

Electrum chain source stop() discards registered Filter entries #1005

Open

tankyleo force-pushed the 2026-07-0fc-followups branch 2 times, most recently from f7608d5 to 3136a05 Compare July 28, 2026 01:52

tankyleo requested a review from tnull July 28, 2026 05:09

tankyleo added 6 commits July 28, 2026 17:08

Increase the size of the broadcast package queue

0e76fe8

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.

Expose ChannelTypeFeatures in ChannelDetails

599e545

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

Add Eclair interop test for zero-fee-commitments

98c9088

Co-Authored-By: HAL 9000

Do not capture logs in 0FC channels CI

091a461

In case of a timeout, these logs are very useful, and would otherwise fail to show.

tankyleo force-pushed the 2026-07-0fc-followups branch from 3136a05 to fb5f3bc Compare July 28, 2026 17:11

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!

Uh oh!

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tankyleo commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

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