Skip to content

lnwallet/test: wait for the expected confirmation status - #11508

Open
litbot-9000 wants to merge 2 commits into
masterfrom
yy-claudell-fix-neutrino-tx-conf
Open

litbot-9000 wants to merge 2 commits into
masterfrom
yy-claudell-fix-neutrino-tx-conf

Conversation

@litbot-9000

Copy link
Copy Markdown
Collaborator

This branch belongs to the litbot-9000 bot. The yy prefix is only the allowlisted branch prefix; the branch is not yyforyongyu's.

Fixes the unit-race tags="test_db_sqlite" failure reported in #11507.

Problem

The first nightly run (job 127023741626) failed with:

--- FAIL: TestLightningWallet/btcwallet/neutrino:get_transaction_details (1.66s)
    test_interface.go:1791: expected transaction fe2b4114… to be confirmed

sendCoins mines the transaction and then calls waitForWalletSync on both wallets. That waits until the wallet's best block equals the miner's tip and IsSynced() returns true. With neutrino, though, the wallet's sync height can advance before the matching block filter has been processed and the transaction's confirmation recorded, because that happens asynchronously. assertTxInWallet then reads the transaction list exactly once, and in that window it sees the transaction as still unconfirmed.

Fix

assertTxInWallet now polls with wait.NoError for up to 30 seconds until the transaction appears with the expected confirmation status, instead of failing on the first read. The final check is the same. A wallet that never records the confirmation, or that records one that's wrong, still fails, with the same message. Every caller benefits, including the assertions that expect a transaction to stay unconfirmed: polling can't make those pass spuriously, because they already require the transaction to be found in the unconfirmed state.

Verification

  • go vet ./lnwallet/test/... and gofmt are clean.
  • I ran the neutrino transaction_details, transaction_details_offset and get_transaction_details subtests locally against a btcd harness: pass.
  • I ran the full lnwallet/test/neutrino and lnwallet/test/btcd suites locally with -race: pass (neutrino 201s, btcd 108s).

The race itself is timing dependent, so it doesn't reproduce on demand; the nightly run is the real check.

Not covered

The other failure in #11507, Run windows itest (coop_close_exceeds_max_fee and wallet_import_pubkey/BIP-0049, both timeouts), is unrelated. Those are timeouts on slow Windows runners. The same job also failed in a merge-queue run on 2026-11-03. It's not a required check, and it isn't changed here.

A wallet can report itself as synced to the chain tip before it has
processed the transactions of the last block. With neutrino, the block's
filter is matched and the confirmation recorded asynchronously, so
assertTxInWallet, which read the transaction list exactly once, could
still see a just mined transaction as unconfirmed. The first nightly run
failed this way in get_transaction_details.

Poll until the transaction shows up with the expected confirmation
status instead, failing with the same error after 30 seconds.
@github-actions github-actions Bot added ci-hold CI is held; remove this label to run CI severity-low Best-effort review labels Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

🟢 PR Severity: LOW

gh pr view | 2 files | 65 lines changed

🟢 Low (2 files)
  • lnwallet/test/test_interface.go - test-only change: makes assertTxInWallet poll for the expected confirmation status instead of reading once, fixing a flaky unit-race failure; no production wallet logic touched
  • docs/release-notes/unreleased/11508.md - release notes entry

Analysis

Although lnwallet/* is normally CRITICAL, the only non-docs file here lives in lnwallet/test/, a dedicated test-harness package (analogous to lntest/) used exclusively by interface tests across wallet backends. The diff only changes polling/retry behavior in a test assertion helper to fix a timing-dependent flake reported in #‌11507 — it does not modify any production wallet, signing, or funding code. Combined with the release-notes entry, this PR is test-only and best-effort review is sufficient.


To override, add a severity-override-{critical,high,medium,low} label.

@litbot-9000 litbot-9000 added full-ci Run the merge queue CI tier on this PR and removed ci-hold CI is held; remove this label to run CI labels Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full-ci Run the merge queue CI tier on this PR severity-low Best-effort review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant