Skip to content

lnwallet/chancloser: use valid delivery scripts in RBF closer tests - #11042

Open
ziggie1984 wants to merge 2 commits into
lightningnetwork:masterfrom
ziggie1984:fix-rbf-coop-test-delivery-script-fixture
Open

lnwallet/chancloser: use valid delivery scripts in RBF closer tests#11042
ziggie1984 wants to merge 2 commits into
lightningnetwork:masterfrom
ziggie1984:fix-rbf-coop-test-delivery-script-fixture

Conversation

@ziggie1984

@ziggie1984 ziggie1984 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Change Description

TestRbfChannelActiveTransitions shadows the package-level localAddr and
remoteAddr with twenty raw bytes each:

localAddr := lnwire.DeliveryAddress(bytes.Repeat([]byte{0x01}, 20))
remoteAddr := lnwire.DeliveryAddress(bytes.Repeat([]byte{0x02}, 20))

Neither is a well-formed delivery script — not a witness program, and not any
other template a co-op close is willing to pay to — so both are rejected by
validateShutdownScript.

Why it goes unnoticed today

validateShutdown is a chain of guards with early returns:

order guard error
1 thaw height not reached ErrThawHeightNotReached
2 taproot shutdown nonce missing ErrTaprootShutdownNonceMissing
3 delivery script invalid ErrInvalidShutdownScript

Both subtests that feed the shadowed remoteAddr into a ShutdownReceived
remote_initiated_thaw_height_close_fail and
remote_initiated_taproot_no_nonce_fail — trip an earlier guard on purpose, so
guard 3 is never reached. The one subtest that drives the whole chain,
remote_initiated_close_ok, was extracted in #11019 into
testRemoteInitiatedCloseOkNonTap / testRemoteInitiatedCloseOkTaproot, which
sit outside the shadow and therefore pick up the valid package-level scripts.

To be clear: nothing currently passes for the wrong reason. The harness
asserts on specific sentinel errors via errors.Is. But that holds only by
accident of where the failures happen to land, not by design.

Why it is worth fixing anyway

The cost is paid by whoever touches that guard chain next, and it is not paid
in a readable failure. Hoisting the delivery-script check above the thaw-height
check — a reasonable thing to want, since it is the cheaper check — does not
produce an assertion diff. The unexpected ErrInvalidShutdownScript reaches the
mock error reporter as an unmatched call and panics the package's test
binary
:

panic: mock: Unexpected Method Call
  ReportError(*errors.errorString)
      0: &errors.errorString{s:"invalid shutdown script"}

The stack points into protofsm/state_machine.go and mock.go, not at the
fixture that is actually wrong.

Deleting the two shadowing locals with that same reorder still applied: the
whole package passes. So the shadow is the sole cause.

There is a second, quieter cost. local_initiated_close_ok passes the invalid
localAddr as localUpfrontAddr and asserts it lands in state unchallenged.
#11019 only added validation on the remote side; if anyone later makes that
symmetric, this test breaks for fixture reasons rather than subject reasons.

The fix

Drop the shadowing locals so those subtests use the valid package-level P2TR
scripts, and record the invariant where the scripts are declared. The general
principle: a negative test's fixture should be valid in every dimension except
the one under test, or it is not isolating what it claims to.

Where this came from

Found while backporting #11019 to v0.20.x. On that branch
remote_initiated_close_ok is still an inline subtest rather than an extracted
helper, so it sits inside the shadow and picks up the 20-byte blob. Once
#11019 makes delivery-script validation unconditional, that subtest fails with
invalid shutdown script. Master escapes this only because of the helper
extraction described above. The backport carries the same fixture fix; this PR
brings it to master, where it is latent rather than active.

Steps to Test

go test ./lnwallet/chancloser/... -race -count=1

To reproduce the latent failure this prevents, hoist the
validateRemoteDeliveryScript call to the top of validateShutdown in
lnwallet/chancloser/rbf_coop_transitions.go and run the package tests — on
master the binary panics; with this change it passes.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

Added under ## Testing in release-notes-0.22.0.md. This is a test-only
change with no user-visible effect, so no-changelog would also be reasonable
if maintainers prefer to keep that section for feature-level test work.

TestRbfChannelActiveTransitions shadowed the package-level localAddr and
remoteAddr with twenty raw bytes each:

    localAddr  := lnwire.DeliveryAddress(bytes.Repeat([]byte{0x01}, 20))
    remoteAddr := lnwire.DeliveryAddress(bytes.Repeat([]byte{0x02}, 20))

Neither is a well-formed delivery script -- not a witness program, and
not any other template a co-op close is willing to pay to -- so both are
rejected by validateShutdownScript.

That goes unnoticed today because validateShutdown is a chain of guards
with early returns: the thaw height is checked first, then the taproot
shutdown nonce, and only then the delivery script. Both subtests that
feed the shadowed remoteAddr into a ShutdownReceived, namely
remote_initiated_thaw_height_close_fail and
remote_initiated_taproot_no_nonce_fail, trip an earlier guard on
purpose, so the script check is never reached. The one subtest that does
drive the whole chain, remote_initiated_close_ok, was extracted into
testRemoteInitiatedCloseOkNonTap and testRemoteInitiatedCloseOkTaproot,
which sit outside the shadow and so pick up the valid package-level
scripts. Nothing currently passes for the wrong reason, since the
harness asserts on specific sentinel errors, but that holds only by
accident of where the failures happen to land.

The cost is paid by whoever touches that guard chain next. Hoisting the
delivery-script check above the thaw height check, a reasonable thing to
want, does not fail readably: the unexpected ErrInvalidShutdownScript
reaches the mock error reporter as an unmatched call and panics the
package's test binary, with a stack pointing into protofsm and the mock
plumbing rather than at the fixture that is actually wrong.

Drop the shadowing locals so those subtests use the valid package-level
P2TR scripts, and record the invariant where the scripts are declared. A
negative test's fixture should be valid in every dimension except the
one under test, or it is not isolating what it claims to.
@github-actions github-actions Bot added the severity-low Best-effort review label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

🟢 PR Severity: LOW

gh pr view | 2 files | 15 lines changed

🟢 Low (2 files)
  • docs/release-notes/release-notes-0.22.0.md - release notes documentation update
  • lnwallet/chancloser/rbf_coop_test.go - test-only change (*_test.go)

Analysis

Both changed files fall into the LOW tier: one is a release-notes markdown update, and the other is a test-only file (*_test.go) under lnwallet/chancloser/. Per the classification rules, test files are categorized by their test-only nature rather than the package they live in, and are excluded from the file/line counts used for severity bumps. With only 2 files and 15 lines changed overall, no bump conditions apply.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-itest severity-low Best-effort review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant