Skip to content

[v0.20.x-branch] Backport #10869: contractcourt: fix HTLC success resolver phantom-input bug (#10840) - #11088

Open
github-actions[bot] wants to merge 11 commits into
v0.20.x-branchfrom
backport-10869-to-v0.20.x-branch
Open

[v0.20.x-branch] Backport #10869: contractcourt: fix HTLC success resolver phantom-input bug (#10840)#11088
github-actions[bot] wants to merge 11 commits into
v0.20.x-branchfrom
backport-10869-to-v0.20.x-branch

Conversation

@github-actions

Copy link
Copy Markdown

Backport of #10869


Part of #10840.

Summary

  • validate that a commitment HTLC spend creates the exact second-level success output before offering it to the sweeper
  • classify legacy and taproot direct spends by witness path, including BIP341 annex-bearing success spends
  • checkpoint foreign spends as terminal timeout outcomes without leaving phantom inputs, balances, or notifications
  • reject malformed notifier or resolver spend details and verify terminal notifications use the expected circuit key

Ordered incoming HTLC expiry follow-up: #10990.

Delayed missing-input batch isolation is implemented in #10842.

Testing

  • go test ./contractcourt
  • go test -race ./contractcourt
  • go vet ./contractcourt
  • make lint-source
  • git diff d7d2aad11d612022c70c776b01a003f989b61c60..HEAD --check

@github-actions github-actions Bot added this to the v0.21.3 milestone Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Author

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-10869-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-10869-to-v0.20.x-branch backport-10869-to-v0.20.x-branch
cd .worktree/backport-10869-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x e9d88431e37ceb6d76889ff3de86f52621a525d6 5266bc0e26f0eda3565b3d65c08ff56dd46ebc1b 6267d18000566937968cf7a5a022e1bfc3e9e226 b9a3f844ea461f35fe1cff8e276ddd9de164a494 dd24c85c52598befbbbd0cd1ef1bccab04c434ca 96a123961b7088125f89667bc7b170eaff307d76 594581f441e1480a2d66576e9492ae793240cbee b8d2012357b059a8bc4fc47c69a07b45c8363893 5fd79d6a7027b1a9024ad1350167f87b7f87a627 eed8a05a3bd0dd78000c1f490ee07f5cf1220d13 faafbd091ea66ddf013c5e2c6901c2f198cb3f48 1f6a385f783120add6a67b32b671bd8e777efe4e
git push --force-with-lease

Spend notifications cross an internal boundary, so validate the selected
transaction input before callers inspect it.

Add focused coverage for missing metadata, invalid indices, nil inputs,
and unexpected outpoints.

(cherry picked from commit e9d8843)
Taproot leaf identity includes both the script and its leaf version. An
auxiliary leaf may reuse the success script under another version.

Normalize BIP341 annexes in the input package so success-spend
classification can share the exact predicate. Cover annex boundaries
and same-script auxiliary leaves.

(cherry picked from commit 5266bc0)
Taproot spenders can append a BIP341 annex without changing the
spend path.

Normalize timeout and revocation witness stacks before matching their
shapes. This keeps annexed key and script path spends classified the
same as their plain forms.

(cherry picked from commit 6267d18)
Timeout cleanup extracts a preimage from a success-shaped witness
before updating the cache and settling the incoming link.

Verify that the revealed preimage opens the resolver HTLC. Update
fixtures to use matching preimages and cover a mismatched
success-shaped witness.

(cherry picked from commit b9a3f84)
A re-signed success transaction can move its committed output to another
index while preserving the HTLC input signature.

Match the indexed output against the sweep descriptor and return the
confirmed outpoint. Keep malformed resolver and notifier state distinct
from complete non-matches.

(cherry picked from commit dd24c85)
A foreign transaction can consume the commitment HTLC without leaving a
second-level success output for this resolver.

Checkpoint a failed terminal outcome while the resolver is unresolved.
Roll back live report and incubation state on persistence errors. Retain
keyed notification coverage for a successful retry.

(cherry picked from commit 96a1239)
A direct commitment outpoint can confirm through the success, timeout,
or an auxiliary Taproot path after the resolver offers its own sweep.

Validate the notifier input and expected success leaf before reading the
preimage. Checkpoint other complete paths as failed. Treat a wrong
preimage on the expected leaf as malformed state.

(cherry picked from commit 594581f)
Sweeper aggregation can replace the pre-signed success transaction and
move its committed output to another index.

Report the confirmed claim outpoint and transaction in both resolver
stages. Use keyed notifier fixtures and distinct transaction IDs to
cover the terminal outcome directly.

(cherry picked from commit b8d2012)
Launch may replay a historical commitment spend before Resolve owns the
terminal transition.

Validate the selected input and match the confirmed second-level output
before offering it. Leave complete non-matches for Resolve instead of
creating a phantom sweep input.

(cherry picked from commit 5fd79d6)
Resolve owns terminal checkpointing for historical commitment spends.
A complete non-match is a failed outcome. Malformed spend or resolver
state must stop without persisting a terminal transition.

Derive the confirmed second-level outpoint from the validated
transaction. Cover fresh, restored, and malformed spend paths.

(cherry picked from commit eed8a05)
Add the foreign commitment spend fix to the 0.20.4 release notes.

Credit the contributor alongside the release entry.

(cherry picked from commit 1f6a385)
@ziggie1984
ziggie1984 force-pushed the backport-10869-to-v0.20.x-branch branch from e630f3f to e935c92 Compare August 17, 2026 20:34
@ziggie1984
ziggie1984 marked this pull request as ready for review August 17, 2026 20:34
@ziggie1984

Copy link
Copy Markdown
Collaborator

Backport notes for reviewers

This branch was rebuilt from v0.20.x-branch by cherry-picking #10869's commits
one at a time, rather than resolving the bot's conflict in place. The previous
tip ( BACKPORT-CONFLICT) had committed literal <<<<<<< HEAD markers into
contractcourt/htlc_success_resolver.go — the file did not parse — and carried
only the first of the twelve commits, so it is not part of this history.

Master's 12th commit (docs: note foreign success spend fix) is intentionally
omitted: it writes to release-notes-0.21.3.md, which belongs to the v0.21.x
series. The 0.20.4 note is included instead.

What differs from master, commit by commit

# Commit vs. master What differs
1 contractcourt: validate notifier spend inputs identical
2 multi: identify taproot success leaves 1 line txscript/v2txscript
3 multi: classify annexed HTLC spends identical
4 contractcourt: validate revealed preimages identical
5 contractcourt: match second-level outputs identical
6 contractcourt: checkpoint foreign spends 1 line drop chanType arg
7 contractcourt: classify direct success spends 1 line drop chanType arg
8 contractcourt: report re-signed success spends 1 line drop chanType arg
9 contractcourt: validate restored sweep offers 1 line drop chanType arg
10 contractcourt: validate restored success spends 2 lines drop chanType arg ×2
11 docs: add 0.20.4 release note identical

5 of 11 commits are byte-identical to their master counterparts. The other 6
differ by 7 lines in total, all for one of two reasons below.

Reason 1: btcd module paths (1 line)

Master has moved to the btcd v2 module split (txscript/v2, wire/v2,
btcutil/v2, chainhash/v2). v0.20.x pins btcd v0.24.3, where txscript and
wire are in the main module and chainhash is still under chaincfg/. Only
one added import line is affected; the other import lines that conflicted were
pre-existing v0.20.x lines, so they are not part of this change.

Reason 2: newSuccessResolver arity (6 lines)

Master's constructor takes an extra chanType channeldb.ChannelType parameter
for taproot-final channels. IsTaprootFinal appears in 33 files on master and
in zero files on v0.20.x — the feature does not exist on this branch. The
new tests pass 0 for that parameter, so each call site sheds one argument:

// master
return newSuccessResolver(resolution, 0, htlc, 0, cfg)
// v0.20.x
return newSuccessResolver(resolution, 0, htlc, cfg)

This adaptation is applied inside the commit that introduces each call site, so
every commit in the series builds on its own.

Two things worth a closer look

Master-only code was deliberately excluded from two conflict resolutions.
In commit 1, git presented isTaprootFinal() on the incoming side as though it
were part of the change. It is not — it is pre-existing master code, and it
calls the chanType field that does not exist here. Commit 2 had the same shape
in input/script_utils_test.go, where the incoming block spanned ~190 lines but
the commit only adds 65; the remainder was master's
TestTaprootHtlcScriptGeneration. Both were left out. Taking "theirs" in either
case would not have compiled.

All five chanType commits applied with zero git conflicts, and none of them
compiled.
The 5-argument call is a pure addition to a test file, so there was
no textual conflict for git to report — merge conflict detection is textual, not
semantic. Git flagged 2 commits, of which 1 needed no change at all; git stayed
silent on 5 commits that all needed changes.

Verification

  • Each of the 11 commits was checked out individually: go vet ./contractcourt/ ./input/ passes on every one.
  • go test ./contractcourt/ ./input/ passes on the tip.
  • Excluding release notes, the diff of this branch against v0.20.x-branch is line-for-line identical to contractcourt: fix HTLC success resolver phantom-input bug (#10840) #10869's diff against its own parent, apart from the 7 lines listed above.
  • Not yet run: make lint and the integration tests.

@ziggie1984
ziggie1984 requested a review from yyforyongyu August 17, 2026 20:42
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Author

🔴 PR Severity: CRITICAL

gh pr view | 9 files | 406 lines changed (excluding tests)

🔴 Critical (3 files)
  • contractcourt/htlc_success_resolver.go - on-chain HTLC success resolution logic (contractcourt/*)
  • contractcourt/htlc_timeout_resolver.go - on-chain HTLC timeout resolution logic (contractcourt/*)
  • input/script_utils.go - witness/script generation and spend classification (input/*)
🟢 Low (1 file, plus test-only changes not counted above)
  • docs/release-notes/release-notes-0.20.4.md - release notes
  • contractcourt/htlc_outgoing_contest_resolver_test.go, contractcourt/htlc_success_resolver_test.go, contractcourt/htlc_timeout_resolver_test.go, contractcourt/mock_htlcnotifier_test.go, input/script_utils_test.go - test-only, excluded from severity/line counting

Analysis

This is a backport of #‌10869 to v0.20.x-branch. The core changes touch contractcourt/* (breach/HTLC on-chain dispute resolution) and input/* (script/witness classification for spend validation) — both CRITICAL packages per policy, since correctness here directly affects fund safety during on-chain contract resolution. The PR spans two distinct critical packages, but severity is already at the ceiling so no further bump applies. File count (4 non-test files) and line count (406, excluding tests) are both under the bump thresholds anyway.


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-changelog severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants