fix(arkd-wallet): don't silently sign a required leaf with the wrong signer key#1121
fix(arkd-wallet): don't silently sign a required leaf with the wrong signer key#1121bitcoin-coder-bob wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Walkthrough
ChangesTapscript signer-key selection refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(arkd-wallet): select signer key per leaf across all multisig closures
Verdict: APPROVE on code quality — REQUESTING HUMAN REVIEW (protocol-critical: transaction signing)
This PR touches which private key signs each tapscript leaf. Wrong key = invalid signature = stuck funds or failed sweeps. Mandatory human sign-off per protocol-critical rules.
What the PR fixes (confirmed correct)
1. Missing CSV closure types in type switch — The old signerKeyForLeaf only handled MultisigClosure, CLTVMultisigClosure, and ConditionMultisigClosure. It missed CSVMultisigClosure and ConditionCSVMultisigClosure. Since sweep leaves use CSV multisig closures, any sweep of an output locked to a deprecated signer key silently used the current key and produced an invalid signature. Verified all 5 closure types in pkg/ark-lib/script/closure.go are now covered, and all embed MultisigClosure (so c.PubKeys is promoted correctly):
MultisigClosure→ directPubKeys(closure.go:26)CSVMultisigClosure→ embedsMultisigClosure(closure.go:290)CLTVMultisigClosure→ embedsMultisigClosure(closure.go:401)ConditionMultisigClosure→ embedsMultisigClosure(closure.go:500)ConditionCSVMultisigClosure→ embedsCSVMultisigClosure(closure.go:598)
2. Silent wrong-key fallback → hard error for required inputs — The required parameter maps to len(inputIndexes) > 0 in SignTransaction (service.go:~672). When the caller explicitly selects inputs (sweeps, boarding), a leaf that references none of the wallet's keys now returns an error instead of silently signing with the wrong key and failing cryptically at finalize. Best-effort callers (nil inputIndexes, i.e. bulk forfeit signing) still fall back to the current key, which keeps redundant signatures harmless. Logic is correct.
3. nil keyMgr panic fix — Old code: signingKey := w.keyMgr.forfeitPrvkey unconditionally, then overrode in signer mode. In signer-only mode (keyMgr == nil), this panicked. New code gates w.keyMgr.forfeitPrvkey behind the else branch (service.go:~679). Fix is correct.
Code quality assessment
multisigClosureKeys (service.go:~821–838) — Clean extraction. Type switch returns early per case. No allocations. Returns (nil, false) for non-multisig closures, which correctly triggers the fallback-to-current-key path in signerKeyForLeaf.
keyInLeaf (service.go:~841–851) — Nil guard on key is good defensive coding. Uses schnorr.SerializePubKey for comparison, consistent with the rest of the codebase.
signerKeyForLeaf (service.go:~797–819) — Now checks current key first, then deprecated keys. Old code only checked deprecated keys. New ordering is correct: avoids unnecessary iteration when the current key matches.
Test coverage (signer_keys_test.go:136–203) — Covers 5 scenarios:
- ✅ Current key in multisig leaf
- ✅ Deprecated key in CSV sweep leaf (the regression case)
- ✅ Required leaf with no held key → error
- ✅ Best-effort leaf with no held key → fallback
- ✅ Non-multisig leaf → fallback
Nits (non-blocking)
-
No test for
ConditionCSVMultisigClosure— The test coversCSVMultisigClosurebut notConditionCSVMultisigClosure. Since both embedMultisigClosureand the type switch handles them identically, this is low-risk, but a one-liner test case forConditionCSVMultisigClosurewould complete coverage of the exact regression surface. -
No test for
CLTVMultisigClosureorConditionMultisigClosure— Same reasoning. These were already handled in the old code, so they're not regressions, but if we're adding a comprehensiveTestSignerKeyForLeaf, covering all 5 types would be complete.
🔒 Protocol-critical: requires human approval
This PR modifies the signing key selection path for tapscript leaf signatures. The code looks correct and well-tested, but per protocol rules: a human must review and approve before merge. Bugs in key selection can produce invalid signatures that lock funds or prevent sweeps.
cc @ArkLabsHQ/protocol-reviewers
bitcoin-coder-bob
force-pushed
the
bob/signer-key-leaf-guard
branch
from
77306d1 to
848633f
Compare
…ures Introspect CSV and condition-CSV closures too so deprecated-key sweeps use the right key, and error when a required leaf references none of the wallet's keys instead of silently signing with the current one. Also stop dereferencing keyMgr in signer mode.
bitcoin-coder-bob
force-pushed
the
bob/signer-key-leaf-guard
branch
from
848633f to
d0913ae
Compare
There was a problem hiding this comment.
Re-review: fix(arkd-wallet): select signer key per leaf across all multisig closures
Previous review: Requested changes (protocol-critical flag + nits on test coverage).
This review: The nits from my previous review are addressed — TestSignerKeyForLeaf now covers all 5 closure types (multisig, csv, cltv, condition, condition_csv) via the "deprecated key matched across all closure types" subtest.
Verified
- All 5 multisig closure types handled in
multisigClosureKeys(service.go:~827–845), matching all types defined inclosure.go(lines 26, 294, 405, 504, 602). No missing types. signerKeyForLeaflogic (service.go:~800–822): checks current key first, then deprecated keys, then errors onrequiredor falls back. Correct.keyInLeafnil guard (service.go:~849): defensive check on nil key prevents panic in edge cases. Good.SignTransactionwiring (service.go:~672–680):required = len(inputIndexes) > 0correctly maps sweep/boarding (explicit indexes) to hard error mode, while bulk forfeit (nil indexes) stays best-effort. Theelsebranch forkeyMgr.forfeitPrvkeyfixes the nil-deref panic in signer mode.- Test coverage: 7 subtests across
TestSignTransaction(3 cases) andTestSignerKeyForLeaf(5 cases including all closure types, error path, fallback paths).
🔒 Protocol-critical: still requires human approval
This modifies the signing key selection path for tapscript leaves. Code is correct and well-tested. A human must review and approve before merge — wrong key selection = invalid signatures = stuck funds.
LGTM from code review perspective. Approving to unblock human reviewer.
There was a problem hiding this comment.
Re-review: fix(arkd-wallet): select signer key per leaf across all multisig closures
Previous review: Requested changes (protocol-critical flag + nits on test coverage).
This review: The nits from my previous review are addressed — TestSignerKeyForLeaf now covers all 5 closure types (multisig, csv, cltv, condition, condition_csv) via the "deprecated key matched across all closure types" subtest.
Verified
- All 5 multisig closure types handled in
multisigClosureKeys(service.go:~827–845), matching all types defined inclosure.go(lines 26, 294, 405, 504, 602). No missing types. signerKeyForLeaflogic (service.go:~800–822): checks current key first, then deprecated keys, then errors onrequiredor falls back. Correct.keyInLeafnil guard (service.go:~849): defensive check on nil key prevents panic in edge cases. Good.SignTransactionwiring (service.go:~672–680):required = len(inputIndexes) > 0correctly maps sweep/boarding (explicit indexes) to hard error mode, while bulk forfeit (nil indexes) stays best-effort. Theelsebranch forkeyMgr.forfeitPrvkeyfixes the nil-deref panic in signer mode.- Test coverage: 7 subtests across
TestSignTransaction(3 cases) andTestSignerKeyForLeaf(5 cases including all closure types, error path, fallback paths).
🔒 Protocol-critical: still requires human approval
This modifies the signing key selection path for tapscript leaves. Code is correct and well-tested. A human must review and approve before merge — wrong key selection = invalid signatures = stuck funds.
LGTM from code review perspective. Approving to unblock human reviewer.
There was a problem hiding this comment.
Code Review: fix(arkd-wallet): select signer key per leaf across all multisig closures
Reviewer: Arkana (automated, thorough review)
Verdict: Code is correct and well-structured. Requesting human sign-off — this is protocol-critical signing code.
Summary
This PR fixes three real bugs in signerKeyForLeaf:
- Missing CSV closure types —
CSVMultisigClosureandConditionCSVMultisigClosurewere absent from the type switch, causing sweep signing with deprecated keys to silently use the wrong (current) key. The operator couldn't sweep expired batches locked to pre-rotation keys. - Silent wrong-key fallback — every "no match" returned the current key without error; failures only surfaced at finalize/broadcast as cryptic
missing signature for pubkey. - nil
keyMgrpanic —w.keyMgr.forfeitPrvkeywas dereferenced unconditionally before the signer-mode branch, panicking in signer-only mode.
All three are legitimate bugs with real consequences (stuck funds, misleading errors, crashes).
Detailed Analysis
✅ service.go:672-681 — keyMgr nil-deref fix
Before: signingKey := w.keyMgr.forfeitPrvkey evaluated unconditionally, then overwritten in signer mode. Panics when keyMgr == nil.
After: var signingKey *btcec.PrivateKey with conditional assignment in if/else branches. The keyMgr access now only happens in the else (LP mode), which is guarded by the keyMgr == nil check at service.go:461. Correct.
✅ service.go:801-828 — signerKeyForLeaf rewrite
- Returns
(*btcec.PrivateKey, error)instead of bare*btcec.PrivateKey. Allows propagating key-mismatch errors. requiredparameter:truewhen caller passes explicit input indexes (sweeps, boarding),falsefor best-effort signing. Semantics are correct —SignTransactionpassesnilindexes fromsigner_handler.go:42,SignTransactionTapscriptpasses explicit indexes fromsigner_handler.go:57.- Key priority: current key checked first, then deprecated keys. Correct — avoids signing with a deprecated key when the current one matches.
- Non-multisig leaf → fallback to current key regardless of
required. Correct — we can't check key membership for non-multisig closures.
✅ service.go:830-849 — multisigClosureKeys helper
Covers all 5 closure types registered in DecodeClosure (closure.go:36-45):
MultisigClosure✓CSVMultisigClosure✓ (embedsMultisigClosure,.PubKeyscorrect via promotion)CLTVMultisigClosure✓ConditionMultisigClosure✓ConditionCSVMultisigClosure✓ (embedsCSVMultisigClosure→MultisigClosure,.PubKeyscorrect)
No closure types missed. If a new multisig closure type is added later, it would fall through to default → false, causing a silent fallback to the current key — same as before. Acceptable, but worth a comment or a compile-time exhaustiveness check if Go ever supports it.
✅ service.go:852-861 — keyInLeaf helper
Nil-key guard at top. Uses schnorr.SerializePubKey for comparison (x-only, 32 bytes) — consistent with the old code. No issues.
✅ signer_keys_test.go:136-216 — Test coverage
Tests cover:
- Current key match ✓
- Deprecated key across all 5 closure types (including CSV and ConditionCSV — the regression surface) ✓
- Required leaf with no held key → error ✓
- Best-effort leaf with no held key → fallback ✓
- Non-multisig leaf → fallback ✓
Missing but minor: no test for keyInLeaf with nil key, though that path is simple and unlikely to regress.
Cross-repo impact
- None. Changes are internal to
pkg/arkd-wallet— no public API, type, or proto changes.signerKeyForLeaf,multisigClosureKeys, andkeyInLeafare all unexported. No downstream SDK impact.
Potential concerns (minor, non-blocking)
-
required=falsefallback is still silent. For best-effort signing, a wrong-key signature is harmless (redundant), but alog.Warnwould help debugging when an operator wonders why a forfeit signature was attempted with the wrong key. Not a blocker. -
Future closure types. If a new multisig-bearing closure type is added to
DecodeClosurebut not tomultisigClosureKeys, the wrong-key bug silently re-emerges for that type. Consider adding a comment atmultisigClosureKeysnoting it must stay in sync withDecodeClosure, or having the two share a registry. Not a blocker.
⚠️ Protocol-Critical Flag
This PR modifies transaction signing logic — specifically which private key signs tapscript leaves during sweeps and forfeits. Incorrect key selection means invalid signatures and stuck funds. The code is correct, but per policy this requires explicit human sign-off before merge.
LGTM from automated review. Awaiting human approval.
There was a problem hiding this comment.
Code Review: fix(arkd-wallet): select signer key per leaf across all multisig closures
Reviewer: Arkana (automated, thorough review)
Verdict: Code is correct and well-structured. Requesting human sign-off — this is protocol-critical signing code.
Summary
This PR fixes three real bugs in signerKeyForLeaf:
- Missing CSV closure types —
CSVMultisigClosureandConditionCSVMultisigClosurewere absent from the type switch, causing sweep signing with deprecated keys to silently use the wrong (current) key. The operator couldn't sweep expired batches locked to pre-rotation keys. - Silent wrong-key fallback — every "no match" returned the current key without error; failures only surfaced at finalize/broadcast as cryptic
missing signature for pubkey. - nil
keyMgrpanic —w.keyMgr.forfeitPrvkeywas dereferenced unconditionally before the signer-mode branch, panicking in signer-only mode.
All three are legitimate bugs with real consequences (stuck funds, misleading errors, crashes).
Detailed Analysis
✅ service.go:672-681 — keyMgr nil-deref fix
Before: signingKey := w.keyMgr.forfeitPrvkey evaluated unconditionally, then overwritten in signer mode. Panics when keyMgr == nil.
After: var signingKey *btcec.PrivateKey with conditional assignment in if/else branches. The keyMgr access now only happens in the else (LP mode), which is guarded by the keyMgr == nil check at service.go:461. Correct.
✅ service.go:801-828 — signerKeyForLeaf rewrite
- Returns
(*btcec.PrivateKey, error)instead of bare*btcec.PrivateKey. Allows propagating key-mismatch errors. requiredparameter:truewhen caller passes explicit input indexes (sweeps, boarding),falsefor best-effort signing. Semantics are correct —SignTransactionpassesnilindexes fromsigner_handler.go:42,SignTransactionTapscriptpasses explicit indexes fromsigner_handler.go:57.- Key priority: current key checked first, then deprecated keys. Correct — avoids signing with a deprecated key when the current one matches.
- Non-multisig leaf → fallback to current key regardless of
required. Correct — we can't check key membership for non-multisig closures.
✅ service.go:830-849 — multisigClosureKeys helper
Covers all 5 closure types registered in DecodeClosure (closure.go:36-45):
MultisigClosure✓CSVMultisigClosure✓ (embedsMultisigClosure,.PubKeyscorrect via promotion)CLTVMultisigClosure✓ConditionMultisigClosure✓ConditionCSVMultisigClosure✓ (embedsCSVMultisigClosure→MultisigClosure,.PubKeyscorrect)
No closure types missed.
✅ service.go:852-861 — keyInLeaf helper
Nil-key guard at top. Uses schnorr.SerializePubKey for comparison (x-only, 32 bytes) — consistent with the old code. No issues.
✅ signer_keys_test.go:136-216 — Test coverage
Tests cover all 5 closure types, required vs best-effort semantics, and non-multisig fallback. Thorough.
Cross-repo impact
None. All changed functions are unexported. No public API, type, or proto changes. No downstream SDK impact.
Minor suggestions (non-blocking)
-
Logging on best-effort fallback. A
log.Warnwhenrequired=falsefalls back to the current key (no match found) would help operators debug unexpected forfeit failures. -
Sync guard for future closure types. If a new multisig closure type is added to
DecodeClosurebut notmultisigClosureKeys, the wrong-key bug silently re-emerges. Consider a comment noting the two must stay in sync.
⚠️ Protocol-Critical Flag
This PR modifies transaction signing logic — specifically which private key signs tapscript leaves during sweeps and forfeits. Incorrect key selection = invalid signatures = stuck funds. The code is correct, but per policy this requires explicit human sign-off before merge.
LGTM from automated review. Awaiting human approval.
The signer only co-signs collaborative multisig leaves (plain, CLTV, condition). Sweep/exit CSV closures key off the wallet's stable forfeit key and are signed in liquidity-provider mode, never via signerKeyForLeaf; ConditionCSVMultisigClosure is never constructed anywhere. Drop the CSVMultisigClosure and ConditionCSVMultisigClosure cases and their test coverage.
bitcoin-coder-bob
changed the title
2 participants
What
signerKeyForLeafpicks which of the wallet's signer keys (the current one or a retained deprecated one) signs a given tapscript leaf in signer mode. This hardens it.Silent wrong-key fallback. Every "no match" path returned the current
SignerKeywithout checking whether that key is actually in the leaf. A leaf requiring a key the wallet no longer holds was signed with the current key anyway; signing succeeded with no error and the failure only surfaced later, at finalize/broadcast, as a crypticmissing signature for pubkey .... Arequiredparameter (set when the caller passes explicit input indexes, e.g. boarding-input signing) now makes a no-match leaf a hard error instead. Best-effort callers (nil indexes) keep falling back, so redundant signatures stay harmless.nil
keyMgrpanic. The call site dereferencedw.keyMgr.forfeitPrvkeyunconditionally before the signer-mode branch overwrote it, panicking in signer-only mode (this is what crashed the existingTestSignTransaction). The dereference now happens only in liquidity-provider mode.How
signerKeyForLeafto extract the leaf's pubkeys (multisigClosureKeys) and return the current or a deprecated key, whichever the leaf references, for the multisig closures the signer co-signs:MultisigClosure,CLTVMultisigClosure,ConditionMultisigClosure.requiredparameter and return an error when no held key matches a required leaf.keyMgrin signer mode.Scope / out of scope
CSVMultisigClosureleaves are not signed through this path: the sweep closure keys off the wallet's stable, seed-derived forfeit key (builder.gobuilds it fromGetForfeitPubkey) and is signed in liquidity-provider mode, never viasignerKeyForLeaf. Signing a sweep with a key the primary wallet doesn't hold is handled separately by the primary/fallback wallet iteration in Sweep fallback across primary and fallback arkd-wallets #1101.ConditionCSVMultisigClosureis never constructed anywhere in the codebase. An earlier revision of this PR addedCSVMultisigClosureandConditionCSVMultisigClosurecases on a sweep rationale that does not hold (sweeps never reach this function); they have been dropped as dead code, which leaves the handled closure set the same asmaster.master, and only touchespkg/arkd-wallet.Tests
TestSignerKeyForLeaf: current-key match, deprecated-key match across the handled multisig closure types (multisig / CLTV / condition), required-no-match error, best-effort fallback, non-multisig fallback.TestSignTransaction(previously panicking on nilkeyMgr) now passes.TestDeprecatedSignerKeyandTestReactToFraudis unchanged by dropping the CSV cases.Summary by CodeRabbit
Bug Fixes
Tests