Skip to content

Make the test harness's chain waits tolerate slow CI runners - #1171

Open
jkczyz wants to merge 8 commits into
lightningdevkit:mainfrom
jkczyz:2026-10-interop-harness-waits
Open

Make the test harness's chain waits tolerate slow CI runners#1171
jkczyz wants to merge 8 commits into
lightningdevkit:mainfrom
jkczyz:2026-10-interop-harness-waits

Conversation

@jkczyz

@jkczyz jkczyz commented Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Several recent CI failures on #1079 and #1080 came from the test harness's own waits and sync handling rather than from the code under test; the same commits passed on re-run. CLN polls bitcoind every 30 seconds by default, the harness gave up polling for a block after a fixed number of tries, a cooperative close was attempted before the channel was reestablished, and a manual sync_wallets could return while the node was still behind the tip, or return a background sync's error without having synced itself.

This PR makes those waits budgeted by time rather than by try count, waits for the external node and for the channel state the next step actually needs, and makes sync_wallets always run its own sync to the current tip. For the one failure whose cause is still unknown, the eclair job's WalletOperationFailed panic, the interop jobs now upload the node's storage directory and the electrs and external node logs when they fail.

Measured locally against the CLN v25.12.1 image: the CLN suite takes 70 seconds with the 1 second poll versus 309 seconds with the default. reorg_test and channel_full_cycle pass with the new poll budget.

🤖 Generated with Claude Code

jkczyz and others added 3 commits October 15, 2026 13:45
@jkczyz @claude
Poll bitcoind every second in the CLN interop tests
e18c6cd 
lightningd polls bitcoind every 30 seconds by default, so after the
tests mine the blocks a channel needs it can take CLN half a minute to
notice them, plus the time to walk them, before it sends
`channel_ready`. The tests wait 60 seconds for the resulting
`ChannelReady` event, which left a few seconds of margin on a busy CI
runner and timed out in `test_basic_channel_cycle`. The same scenario
takes 8 seconds against LND and 12 against Eclair, which get notified of
blocks over ZMQ.

The poll interval is a developer option, and developer mode turns
deprecated APIs off, which drops the `bech32` address from `newaddr` and
the `tx`/`txid` fields from `close` that the tests read, so deprecated
APIs are explicitly allowed again.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Wait for the external node's chain view before expecting ChannelReady
6cbd370 
`channel_ready` only arrives once the external node has seen the blocks
the test mined, but the scenarios started the 60-second event wait right
after mining, so an external node that lags its chain backend ate into
that budget and a timeout there read as a channel problem. The setup
already waits for the peer to reach Bitcoin Core's height before
funding; do the same after mining the blocks a channel open or a splice
lock needs, so the event wait measures the channel exchange alone and a
lagging peer fails with a message that says so.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Give the test harness's polls a time budget instead of a try count
653c335 
`exponential_backoff_poll` gave up after 20 tries, about nine seconds in
all once the backoff had reached its cap. That is not enough for electrs
to index a hundred freshly generated blocks on a busy CI runner:
`postgres_node_restart` died in the premine wait and `reorg_test` after
generating 139 blocks, both with "Reached max tries". Poll for up to 60
seconds instead; a condition that already holds still returns at once,
so passing runs are unaffected.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@ldk-reviews-bot

ldk-reviews-bot commented Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown

I've assigned @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.

@ldk-reviews-bot
ldk-reviews-bot requested a review from tnull October 15, 2026 19:24
@jkczyz jkczyz mentioned this pull request Oct 15, 2026
Open
jkczyz and others added 5 commits October 15, 2026 15:03
@jkczyz @claude
Wait for the channel to be reestablished before closing it
7e67c5e 
a_close_is_classified_for_a_channel_whose_facts_were_never_recorded
reconnects a restarted node to its peer and immediately asks for a
cooperative close. Connecting returns once the peer handshake is done,
but the close is refused until channel_reestablish has been exchanged
over the new connection, which takes another message round trip. On a
slow runner the close came first and the test failed with
ChannelClosingFailed.

Poll the channel until it reports usable, which includes having been
reestablished, before closing it. Another test already does this after
a reconnect.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Finish every TestSyncStore operation once it has started
9d22ba3 
TestSyncStore mirrors each write and remove to a filesystem store, a
SQLite store and an in-memory store, and asserts on every access that
they agree. The mirrors were applied one after another with awaits in
between, so a caller that dropped the future partway left the stores
disagreeing: the filesystem and SQLite writes had already been handed
to blocking threads and landed, while the in-memory one never ran.

The node does exactly that when it stops: its cancellable background
tasks, several of which persist the node metrics, are dropped wherever
they are suspended. The next node built on the same store then failed
its first read with a consistency assertion in read_node_metrics, which
is how
a_sweep_confirmed_while_down_is_classified_for_a_channel_whose_facts_were_never_recorded
failed in about one run out of six locally.

Run each operation on a runtime the store owns and only await its
completion from the caller, so dropping the caller's future no longer
interrupts it.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Sync to the current tip even when a sync is already running
5c59043 
`Node::sync_wallets` returned as soon as a sync that was already in
progress finished, taking that sync's result as its own. That sync took
its view of the chain when it started, so the caller could get back a
node that was still behind the tip.

With a Bitcoin Core chain source this is easy to hit: blocks are
processed one at a time, with the channel monitors, the sweeper and the
on-chain wallet persisted for each, so a background poll that started
while a batch of blocks was being mined keeps running long after mining
is done. A manual sync arriving in the meantime piggybacked on it and
returned with the rest of the batch still unprocessed. The reorg test
then timed out on CI: the sweep it waited for could only be broadcast
once the remaining blocks had been processed, which now happened inside
the wait's time budget rather than in the sync call before it.

A sync now waits for the one in progress and then runs its own, so it
reflects the tip as of when it was called. A failed or cancelled sync in
progress still fails the waiting caller as before.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Sync anyway when the sync in progress failed
5f3dc00 
Since a caller runs its own sync after waiting for the one in progress,
that sync's result no longer stands in for the caller's. Returning its
error still failed the caller without it having synced at all. With
background syncing on, a manual `sync_wallets` call that arrived while
a background sync was failing returned that failure as its own.

The caller now logs the failure and syncs anyway, so what it returns is
the result of its own sync.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jkczyz @claude
Upload the LDK node's logs when an interop job fails
2cab327 
The eclair job failed with `sync_wallets failed: WalletOperationFailed`
on three unrelated branches on the same day. The node logs which part
of the sync failed, but only to its log file under a random directory
in the runner's temp dir, which the job discards. So each occurrence
leaves nothing to tell a transient electrs error from a failed
background sync the manual call piggybacked on.

Point the harness's temp dir at a directory under the runner's temp
and upload it as an artifact when the tests fail, together with the
electrs and external node container logs for the same window. The
same for the CLN and LND jobs, which run on the same harness.

Written with the help of an AI tool.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnull tnull Awaiting requested review from tnull

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jkczyz @ldk-reviews-bot