Skip to content

chanrestore: retain overlay tapscript root on restore - #11197

Open
Roasbeef wants to merge 1 commit into
lightningnetwork:masterfrom
Roasbeef:fix/restore-overlay-tapscript-root
Open

Roasbeef wants to merge 1 commit into
lightningnetwork:masterfrom
Roasbeef:fix/restore-overlay-tapscript-root

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

In this PR, we retain the tapscript root carried by the optional CloseTxInputs extension when reconstructing a channel shell for a simple Taproot overlay channel. openChannelShell rebuilt the TapscriptRootBit, but left OpenChannel.TapscriptRoot empty. The chain watcher would then derive a BIP-86 funding script, which can cause Neutrino to miss the peer's DLP force close.

Rootless backups continue through the existing recovery path. This preserves outpoint-based recovery with full-node backends while fixing backups that contain the authenticated root.

The regression test asserts that restore retains a nonzero root and that a backup without CloseTxInputs still restores with an empty root.

Test Plan

go test . ./chanbackup -count=1

In this commit, we carry the tapscript root stored in the optional CloseTxInputs extension into the reconstructed OpenChannel for TapscriptRootVersion backups. Without the root, the chain watcher derives a BIP-86 funding script, which can cause Neutrino to miss the DLP force close.

We preserve an empty root when the extension is absent so older backups retain their existing full-node recovery behavior.
@github-actions github-actions Bot added the severity-medium Focused review required label Sep 15, 2026
@github-actions

Copy link
Copy Markdown

🟡 PR Severity: MEDIUM

gh pr view | 2 files | 60 lines changed

🟡 Medium (1 file)
  • chanrestore.go - root-package (package lnd) file handling static channel backup (SCB) restore; not under any explicitly listed critical/high package path
🟢 Low (1 file)
  • chanrestore_test.go - test-only addition (regression test), excluded from severity-driving classification

Analysis

The only non-test file touched is chanrestore.go, which lives in the top-level lnd package rather than in one of the explicitly listed CRITICAL packages (lnwallet, htlcswitch, contractcourt, sweep, channeldb, etc.) or HIGH packages. Per the classification rules, files are categorized by their actual package path rather than by the sensitivity of the logic they touch or the packages they import, so this falls into the "other Go files not categorized above" MEDIUM bucket.

The change itself is small and scoped (11 added lines in the non-test file, no deletions) — well under the thresholds for a severity bump (>20 files or >500 lines changed). It fixes a bug in openChannelShell where the tapscript root from CloseTxInputs wasn't retained during channel-shell reconstruction for simple Taproot overlay channels, which could cause Neutrino backends to miss a peer's DLP force close. While the underlying issue touches recovery/breach-adjacent behavior, the file path itself does not qualify for an automatic HIGH/CRITICAL bump under the stated rules.


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

@Lrifton92 Lrifton92 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 6c283e3.

The fix is correct for the case it targets. Deserialize always populates TapscriptRoot for TapscriptRootVersion when the close-tx bit is set (chanbackup/single.go:731-744), and Serialize refuses to write that version without one (:462-467), so any version 6 backup that carries CloseTxInputs now yields a shell with a root. The root also survives persistence: RestoreChannelShells goes through syncNewChannel -> PutOpenChannel -> PutChanInfo, which encodes it as TLV type 6 (chanstate/kv_open_channel.go:362, :779). From there deriveFundingPkScript (contractcourt/chain_watcher.go:1850) derives the tweaked funding script instead of the BIP-86 one.

I ran go test -run TestOpenChannelShellTapscriptRoot . at this commit (passes), then dropped the TapscriptRoot: assignment from the shell: the top-level assertion fails with Option[chainhash.Hash] was None(), so the test does pin the regression. The subtest passes with or without the fix; it guards the rootless path against a panic or error, which is worth having, but it is not what catches the bug.

Two things.

Lint is red on this change. chanrestore.go:58 is 85 columns and fails the ll check in the "Lint code" job. See inline.

The rootless overlay case is still silent. A TapscriptRootVersion backup can lack CloseTxInputs when the local CommitTx was nil at backup time (chanbackup/backup.go:66-71). The shell still gets TapscriptRootBit (chanrestore.go:177-182) but no root, so the chain watcher registers the BIP-86 script, which is the exact Neutrino miss this PR describes. Recovering it needs the root and is out of scope here, but a warning at restore time would make that failure mode visible to a Neutrino user instead of leaving them waiting for a close that is never detected.

Comment thread chanrestore.go
if backup.Version.HasTapscriptRoot() {
// TapscriptRootVersion stores the root within the optional
// CloseTxInputs extension.
backup.CloseTxInputs.WhenSome(func(inputs chanbackup.CloseTxInputs) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is 85 columns and is what fails the ll linter in CI (chanrestore.go:58:1: the line is 85 characters long). Binding the option first keeps it under 80:

		closeInputs := backup.CloseTxInputs
		closeInputs.WhenSome(func(in chanbackup.CloseTxInputs) {
			tapscriptRoot = in.TapscriptRoot
		})

Comment thread chanrestore.go
*channeldb.ChannelShell, error) {

tapscriptRoot := fn.None[chainhash.Hash]()
if backup.Version.HasTapscriptRoot() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When HasTapscriptRoot() is true but CloseTxInputs is None (possible per chanbackup/backup.go:66-71), tapscriptRoot stays None while chanType still gets TapscriptRootBit below. On Neutrino that shell will never see the remote DLP close, since the spend is registered against the untweaked script. Would a ltndLog.Warnf naming the channel point be worth adding for that branch?

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

Labels

severity-medium Focused review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants