lnwallet: reject commitments missing aux HTLC signatures - #11068
lnwallet: reject commitments missing aux HTLC signatures#11068GeorgeTsagk wants to merge 3 commits into
Conversation
b601c4e to
8b7a73a
Compare
litbot-9000
left a comment
litbot-9000
left a comment
There was a problem hiding this comment.
Reviewed the three commits and ran the taproot-assets custom channel itests against this branch, as requested. I did not find a correctness or backwards-compatibility defect: the count check is gated correctly, it is equivalent to the BTC-level check that already exists two lines above it, and it is satisfied by every commitment an un-upgraded peer produces — verified empirically, not just by reading. The one thing blocking merge is the failing Lint code job. Everything below is what I actually ran.
Custom channel itest results
taproot-assets main @ a8c07ffd3e90b4c355ccf73303d1bedf57b9d94a.
How the dep was pointed at this branch. Local path replace against my checkout of this PR, rather than a pseudo-version, so that what ran is exactly the reviewed code:
git clone https://github.com/lightningnetwork/lnd.git /workspace/lnd-11068
cd /workspace/lnd-11068 && gh pr checkout 11068 # -> 8b7a73a5c86764e8eac5a9d85b7e06fb00846f45
cd /workspace/tapd-cc-11068
go mod edit -replace github.com/lightningnetwork/lnd=/workspace/lnd-11068 && go mod tidy
This PR only touches the lnd main module, so the fn/v2, cert, clock, ticker and tlv submodules stay on their released versions — no partial-replace skew.
Confirmation the binary under test really contained the change. The custom channel harness launches ../tapd-integrated-itest (itest/custom_channels/custom_channels_test.go:196), not lnd-itest, so that is the binary I checked:
$ go list -m github.com/lightningnetwork/lnd
github.com/lightningnetwork/lnd v0.21.0-beta.rc2.0.20260630214209-40c64f9db30d => /workspace/lnd-11068
$ strings -a itest/tapd-integrated-itest | grep -c "number of aux htlc sig mismatch"
1
$ strings -a itest/tapd-integrated-itest | grep -c "number of htlc sig mismatch" # pre-existing control
1
Full suite — make itest-cc (go test ./itest/custom_channels -tags="dev monitoring ... integration itest btcd" -test.run=TestCustomChannels -test.timeout=30m):
--- PASS: TestCustomChannels (1507.59s)
ok github.com/lightninglabs/taproot-assets/itest/custom_channels 1507.648s
27 subtests passed, 0 failed, 1 skipped, in this order: core (145.28s), large (94.44s), grouped_asset (144.86s), force_close (35.00s), group_tranches_force_close (65.17s), group_tranches_htlc_force_close (73.22s), htlc_force_close (47.73s), htlc_force_close_mpp (49.80s), liquidity_edge_cases (90.13s), liquidity_edge_cases_group (94.77s), balance_consistency (27.11s), single_asset_multi_input (18.77s), forward_bandwidth (58.82s), multi_channel_pathfinding (29.31s), strict_forwarding (59.06s), decode_asset_invoice (8.15s), self_payment (42.47s), multi_rfq (73.04s), oracle_pricing (86.14s), invoice_quote_expiry_mismatch (49.68s), limit_constraints (33.98s), fee (14.63s), breach (36.75s), forwarding_history (53.07s), passive_assets (18.05s), restart_coop_close (23.96s), list_invoices_and_payments (20.37s).
The one skip is v1_upgrade, which is skipped unconditionally in taproot-assets itself (itest/custom_channels/v1_upgrade_test.go:36, t.t.Skip("requires backward compatibility infrastructure")) — not an environment limitation on my side.
Backwards-compatibility suite — make itest-cc-compat. Since this PR makes a previously-tolerated omission fatal, I also ran the suite that puts an actual un-upgraded peer on the other end:
--- PASS: TestBackwardsCompatChannels (287.02s)
--- PASS: TestBackwardsCompatChannels/v0.8.0 (287.02s)
ok github.com/lightninglabs/taproot-assets/itest/custom_channels 287.071s
That harness builds tapd-integrated at tag v0.8.0 in a detached git worktree using v0.8.0's own go.mod, so the replace does not apply and that node runs an older tapd against its own older pinned lnd. I verified the asymmetry directly rather than assuming it:
$ strings -a ~/.tapd-compat-bins/tapd-integrated-v0.8.0 | grep -c "number of aux htlc sig mismatch"
0
$ strings -a ~/.tapd-compat-bins/tapd-integrated-v0.8.0 | grep -c "number of htlc sig mismatch"
1
So Charlie ran lnd without this change and Dave ran lnd with it, and Dave accepted Charlie's commitments. The scenario is not a bare open/close: it pushes two asset keysends old→new, one BTC-only keysend old→new (the non-asset-HTLC-on-an-overlay-channel case), two asset keysends new→old, then cooperatively closes with both sat and asset outputs (itest/custom_channels/compat_test.go:258-296).
No baseline run against unmodified tapd main was needed: a baseline only distinguishes a real break from a flake when something fails, and nothing failed.
What I verified
go test ./lnwallet/ -run 'TestAuxSig' -v -count=1→ PASS, all six new tests and the fourTestAuxSigCountsubtests, 0.392s.go test ./lnwallet/ -count=1→ok github.com/lightningnetwork/lnd/lnwallet 8.770s, no failures. Worth running because commit 1 changesrestartChannel, a helper shared by many tests in that package.- Traced the failure path: a count mismatch reaches
htlcswitch/link.go:4376→l.failf(LinkFailureError{code: ErrInvalidCommitment, FailureAction: LinkFailureForceClose, ...})athtlcswitch/link.go:4393. See the release-notes comment. - Checked the send side is in lockstep:
sigBatchandauxSigBatchare appended adjacently in both loops ofgenRemoteHtlcSigJobs(lnwallet/channel.go:3462-3467and3544-3549) and sorted by the sameOutputIndexkey (lnwallet/channel.go:4228-4233), so the two counts cannot diverge, dust or not. - Checked the gate's premise.
TapscriptRootBitis only ever set atlnwallet/reservation.go:486when the commit type isCommitmentTypeSimpleTaprootOverlay(and atchanrestore.go:172restoring the same), and lnd elsewhere treatsHasTapscriptRoot()as the definition of an overlay channel (rpcserver.go:4919,peer/brontide.go:4204). The rationale in the new comment is accurate. - Tried and failed to substantiate a retransmission break: a stored
CommitDiffkeeps its aux blob, becauseserializeCommitDiffwrites theCommitSigviaWriteElements→lnwire.WriteMessage/ReadMessage(channeldb/codec.go:386), which round-tripsCustomRecordsthroughExtraData. So a node restarting mid-flight does not retransmit a commit_sig stripped of its aux signatures. - Checked the wire round-trip preserves slot count on the taproot-assets side:
eHtlcPartialSigsRecordwrites an explicit varint count and the decoder reads it back (tapchannelmsg/records.go:1092-1155), andprocessAuxSigBatchanswers every job, including an empty response for a non-asset HTLC (tapchannel/aux_leaf_signer.go:278-289). One slot per non-dust HTLC, always. - Could not run: there is no container runtime in my environment (no docker/podman/psql), so anything behind
ory/dockertestor thetest_db_postgrestag did not run here. GitHub Actions covers those and they are green on this head.
Findings
Left inline, most severe first. Nothing I would block on, so this is a comment rather than a request for changes.
Posted by claudell (automated review agent) at georgetsagk's request. Findings are unverified by a human.
Three places modelled the aux signature blob less faithfully than production does, in ways that hid whether the blob was carried at all. MockAuxSigner.UnpackSigs returned a single slot no matter how many HTLCs were packed. A real aux signer emits exactly one slot per HTLC, and code that pairs the unpacked slots with HTLCs by index depends on that. The default mock now records the slot count in the packed blob and hands back that many slots. Since the blob is parsed into custom records, it has to stay a canonical TLV stream, so the count travels in the dummy record's value. Registering a function rather than a fixed value is what lets the result depend on the input. restartChannel rebuilt the channel with no channel options, so a restart silently dropped the aux signer and the restarted side stopped producing aux signatures while its peer still expected them. Production re-applies the aux components every time a channel is loaded. Two chan sync tests rebuilt CommitSigs from a retransmitted wire message without its aux signature blob, which the link carries across.
The aux signature blob is peer supplied and carries no count of its own. Verification jobs were only built for HTLCs an entry was received for, so a peer could withhold the aux signature for an HTLC simply by omitting it: no job was produced, nothing reported the omission, and the commitment became our state carrying a signature we never received. At force close the resolver then reads an empty signature out of that HTLC's custom records, and the second level HTLC cannot be swept. An aux signer emits exactly one entry per non-dust HTLC, in lockstep with the BTC level signatures, so the count is now checked the same way those already are, right beside them. The check is gated on the tapscript root bit rather than merely on an aux signer being configured. The signer is attached node wide, so it is also present on plain channels, whose peers correctly send no aux signatures at all, and requiring them there would force close those channels. That bit is set at funding time if and only if the negotiated commitment type is a taproot overlay, so it names exactly the channels whose peers owe us aux signatures.
8b7a73a to
b7c5058
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟢 Low (2 files)
AnalysisThe PR modifies To override, add a |
|
@litbot-9000: review reminder |
3 participants
Description
This adds a consistency check on the auxiliary HTLC signatures received on taproot overlay channels: their count must match the number of HTLCs on the commitment, mirroring the check that already exists for the BTC-level signatures. It also makes the aux signer test doubles model the one-signature-per-HTLC behaviour of a real aux signer, which the previous mocks did not, along with a few test helpers that were not carrying the aux signature blob the way production does.
Complementary to lightninglabs/taproot-assets#2234