wallet: TSS external signing + TSS key storage#1018
Open
marcopeereboom wants to merge 13 commits into
Open
Conversation
github-actions
Bot
added
area: docs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
marcopeereboom
force-pushed
the
marco_tss_external_sign
branch
4 times, most recently
from
ab0d6f3 to
0eb787f
Compare
marcopeereboom
force-pushed
the
marco_tss_external_sign
branch
from
0eb787f to
975df46
Compare
joshuasing
reviewed
May 28, 2026
| github.com/ethereum/go-ethereum v1.17.2 | ||
| github.com/go-test/deep v1.1.1 | ||
| github.com/golang-jwt/jwt/v5 v5.3.1 | ||
| github.com/hemilabs/x/tss-lib/v3 v3.0.0-20260507172513-c23bec7119b9 |
Contributor
There was a problem hiding this comment.
Must be changed to use github.com/hemilabs/x/tss/v3 once merged.
TSSNamedKey represents a key controlled by an external threshold signature scheme. No private material is held locally; the struct carries only the aggregated group public key and an opaque keyID that external signers use to identify which distributed key to sign with. The Zuul interface grows four symmetrical methods: PutTSSKey, GetTSSKey, PurgeTSSKey, LookupTSSKeyByAddr. These parallel the existing local-key methods but dispatch to a separate in-memory index so the two key types share address namespace without overlapping. TSS keys are indexed under P2PKH and P2WPKH only. Taproot P2TR key-path spends require schnorr signatures; ECDSA TSS (the variant this branch initially integrates) cannot satisfy a BIP-341 key-path spend, so exposing a TSS key under a P2TR address would be a footgun for callers trying to build a send from that address. Schnorr TSS, when added later, will get its own enrolment path that includes P2TR. Collisions are detected bidirectionally: enrolling a local key at an address already held by a TSS key (and vice versa) returns ErrKeyExists. PurgeTSSKey removes every indexed address form in a single call, mirroring the multi-address behaviour of the local PurgeKey. Tests cover: P2PKH+P2WPKH indexing with P2TR explicitly excluded; purge round-trip from any address form; field validation (nil struct, missing PublicKey, zero-length KeyID); collision detection in both directions between PutKey and PutTSSKey. Existing PoP and signing tests pass unchanged.
TransactionApplyECDSA wires a pre-computed DER-encoded ECDSA signature into a specific transaction input. The signature is produced out of band — by a hardware wallet, a PSBT flow, or a threshold signature committee — and the wallet only handles the witness or sigScript assembly. P2PKH inputs receive the standard two-push SignatureScript (<sig||hashType> <pubKey>). P2WPKH inputs receive a two-element witness stack ([sig||hashType, pubKey]). Other script classes are rejected with an explicit error: P2TR requires schnorr, and wrapped-segwit/P2WSH variants are not yet supported. Before applying, the function cross-checks the provided pubkey against the address embedded in the prev pkScript. A mismatch would produce a transaction the network rejects on broadcast; catching it at injection time surfaces the bug at the caller. ECDSASigFromRS assembles a DER signature from raw big-endian r and s scalar bytes, the shape commonly emitted by ECDSA TSS signing libraries. The helper rejects empty scalars, zero scalars, and scalars that overflow the secp256k1 group order. Low-S normalisation (BIP-146) is performed implicitly by Signature.Serialize, so high-S TSS output is accepted by the helper and emitted as low-S. Tests exercise: round-trip verify of assembled DER signatures; rejection of bad scalars; low-S normalisation of a high-S input; end-to-end P2PKH injection with script-engine verification; end-to-end P2WPKH injection with BIP-143 sighash and engine verification; wrong-pubkey rejection; and unsupported-script-class (P2TR) rejection. PoP and existing signing tests pass unchanged.
TransactionApplySchnorr wires a pre-computed 64-byte BIP-340 schnorr signature into a P2TR key-path input. The sibling of TransactionApplyECDSA, this is the injection path for schnorr threshold signature schemes (MuSig2, FROST, schnorr-TSS) that produce aggregated signatures the wallet cannot sign locally. pubKey is the untweaked internal key; the function applies the BIP-86 tweak via ComputeTaprootKeyNoScript and cross-checks the tweaked x-only output key against the witness program in the prev pkScript. A mismatch is reported before the transaction is mutated. For SigHashDefault (the common case) the witness stack is the bare 64-byte signature per BIP-341. Any other sighash type appends the single sighash byte. schnorr.ParseSignature is called on the input sig as a cheap structural check: it validates the 64-byte length and catches grossly malformed encodings. On-curve validity of R.x is only checked during actual Verify, so callers wanting a real cryptographic check before broadcast should use VerifySchnorr. Only BIP-86 key-path spends are supported. Script-path taproot spends require a committed leaf script and a control block; those inputs must be assembled by the caller. Tests cover: round-trip sign + inject + engine verification on a real P2TR input; structural rejection (wrong length, nil pubkey, empty signature); wrong-key rejection via tweak mismatch; and unsupported-script-class (P2PKH) rejection. PoP regression tests continue to pass.
VerifyECDSA and VerifySchnorr are pre-broadcast sanity helpers for externally-computed signatures. Callers producing a signature out of band (TSS committee, hardware wallet, PSBT flow) can run the result through these helpers before handing it to TransactionApplyECDSA or TransactionApplySchnorr to catch wrong-key or wrong-hash errors at injection time rather than on broadcast. VerifyECDSA parses a DER-encoded signature (no trailing sighash byte) and checks it against a 32-byte sighash under the provided public key. Structural guards reject nil pubkey, wrong-length sighash, empty signature, and malformed DER. VerifySchnorr is the BIP-340 counterpart. The caller supplies the tweaked x-only output key (not the internal key), the 32-byte BIP-341 sighash, and the 64-byte schnorr signature. Tests cover: happy-path verification for both algorithms; wrong-key rejection; wrong-hash rejection for ECDSA; structural rejection of nil/short/malformed inputs; and a taproot round-trip exercising the tweak flow a real TSS caller would follow. PoP regression tests continue to pass.
TestTSS_E2E_P2WPKH proves that an externally-produced ECDSA
threshold signature can be injected into a bitcoin transaction and
accepted by the same script engine a bitcoin node runs against
witnessed inputs.
The test runs a real 2-of-3 ECDSA TSS ceremony in-process using
github.com/hemilabs/x/tss-lib/v3: full Paillier pre-parameter
generation, 4-round distributed keygen, 9-round distributed
signing + finalize. The private key exists only as shares across
the committee at every moment of the test. No mocks, no
single-party shortcuts.
The group public key is turned into a P2WPKH testnet address, an
unsigned spend is built against a funding UTXO locked to that
address, the BIP-143 sighash is computed, and the committee signs
the sighash. The resulting raw (r, s) scalars flow through
ECDSASigFromRS, VerifyECDSA, and TransactionApplyECDSA, and the
final transaction is handed to txscript.NewEngine for consensus
validation.
Gated behind the tss_e2e build tag. Paillier safe-prime
generation takes roughly 50 seconds for a 3-party run, and the
full test finishes in about one minute. Regular make test does
not build this file, so the default test suite remains fast.
Run locally with:
go test -tags tss_e2e -timeout 15m \
-run TestTSS_E2E ./bitcoin/wallet/
Adds github.com/hemilabs/x/tss-lib/v3 as a direct test-only
dependency pinned to the max/tss_changes branch tip. Will follow
the upstream tss-lib v3 release once that repo tags a version.
Record the wallet changes introduced by this branch under the Unreleased section: external ECDSA/schnorr signature injection (TransactionApplyECDSA, TransactionApplySchnorr, ECDSASigFromRS, VerifyECDSA, VerifySchnorr), native P2WPKH and BIP-86 P2TR key-path signing in TransactionSign, and the TSSNamedKey storage type with its PutTSSKey/GetTSSKey/PurgeTSSKey/LookupTSSKeyByAddr interface on zuul. The prevOuts return-type change on TransactionCreate and PoPTransactionCreate is recorded under Changed as a minor breaking change for external consumers; internal callers (service/popm) treat prevOuts opaquely and are unaffected.
Codecov reported 77.88% patch coverage with 69 uncovered lines on the branch. The vast majority were error-return branches reachable from the public API but not exercised by the existing test suite. Add targeted tests to close the callable gaps: - UtxoPickerMultiple / UtxoPickerSingle no-match and skip-too-small paths. - TransactionSign error-wrap paths for unknown P2WPKH and P2TR keys, confirming the per-class dispatch propagates resolveInput- SigningKey failures with input index and class in the wrapping. - prevOutsFetcher defensive panic on a malformed outpoint key, asserted via recover(). Without this guard, a caller-crafted bad key would silently produce a corrupt sighash midstate. - TransactionApplyECDSA address-mismatch rejection on the P2WPKH branch of pubKeyMatchesAddress, the sibling to the existing P2PKH-branch coverage. - TransactionApplySchnorr witness assembly for non-SigHashDefault sighash types, verifying the trailing sighash byte is appended per BIP-341. Lifts statement coverage from 85.8% to 90.8% in bitcoin/wallet; patch coverage on the branch rises accordingly. The remaining uncovered branches are defensive wraps around btcd library calls that cannot fail on well-formed inputs (address derivation from valid pubkey bytes, SignatureScript building, etc.) and are not reachable without mocking dependencies Tobias forbids mocking.
Existing tests cover happy-path signing and verification for the
BIP-86 key-path P2TR signer, the external schnorr injection
helper, and VerifySchnorr. This adds the adversarial and
structural cases that prove the primitives actually enforce the
BIP-341 commitments they claim to:
- tampered-tx: signed tx survives mutation of output value,
output script, input sequence, or prev amount. Each
mutation must invalidate the witness. The prev-amount case
is distinctly taproot — BIP-341 commits to every input's
prev-amount, segwit v0 does not.
- wrong-key: spend a UTXO whose pkScript commits to key B
using only key A in zuul. Must fail with the
lookup-key-does-not-exist error rather than silently
signing.
- malformed pkScript at the resolve-input-signing-key layer:
truncated push opcodes and OP_RETURN. Both must error out
before any signing attempt.
- pubKeyMatchesTaprootAddress: rejects the wrong key, rejects
an untweaked internal key when the pkScript commits to a
raw pubkey (demonstrates the helper applies BIP-86 tweak
correctly), rejects malformed pkScripts.
- VerifySchnorr parse failures: a 64-byte signature whose r
is the field prime (out of range) forces
schnorr.ParseSignature to error; a 32-byte pubkey with
x = field prime forces schnorr.ParsePubKey to error. These
exercise the two parse branches missed by structural
(length-based) negative tests.
- schnorr external-sign roundtrip: compute BIP-341 sighash,
sign externally with the tweaked key, VerifySchnorr against
the tweaked x-only output key, TransactionApplySchnorr,
verify via script engine. Plus bit-flip negative control.
- cross-input replay: two txs spending different outpoints
under the same pkScript. Swapping the witness between them
must fail — sighash commits to the outpoint.
Coverage lift in bitcoin/wallet:
before: 91.2% after: 92.9%
VerifySchnorr 86.7% -> 100.0%
pubKeyMatchesTaprootAddress 75.0% -> 83.3%
resolveInputSigningKey 72.7% -> 81.8%
The remaining gap is unreachable without mocking btcd:
ExtractPkScriptAddrs never returns a non-nil error in
btcd v0.24.3 (every branch returns nil), and
btcutil.NewAddressTaproot only rejects non-32-byte input which
schnorr.SerializePubKey never produces. The defensive
error-returning code stays for forward compatibility if btcd
tightens its parsers.
PutKey now returns ErrTSSKeyOccupied when a TSS key blocks the address, and PutTSSKey returns ErrLocalKeyOccupied when a local key blocks it. Both wrap ErrKeyExists for backward compatibility. Update TestPutKeyVsPutTSSKeyCollision to assert the specific sentinel while also verifying the ErrKeyExists wrapping.
Improve error messages: use errors.New where no format verbs, clarify nil-argument and verify-failure wording. Remove underscore separators from number literals in tests. Replace append-copy idiom with preallocate+copy for sigWithHash and schnorr witness construction.
…re-encoding Round-trip ECDSA and schnorr signatures through parse+serialize to guarantee canonical encoding. Remove redundant length pre-checks and maxECDSASigDERLen constant since the parsers validate length internally.
Caught by golangci-lint --fix; missed in prior review cleanup.
marcopeereboom
force-pushed
the
marco_tss_external_sign
branch
from
975df46 to
f838e6e
Compare
Rename import paths from github.com/hemilabs/x/tss-lib/v3 to github.com/hemilabs/x/tss/v3, matching the module rename in the hemilabs/x repository.
marcopeereboom
force-pushed
the
marco_tss_external_sign
branch
from
f838e6e to
547b720
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add external ECDSA and Schnorr signature application for TSS-signed
Bitcoin transactions, TSS key storage in zuul, and signature
verification helpers.
Depends on #971 (segwit signing PR).
TSS key type
Add TSSNamedKey to zuul and PutTSSKey/LookupKeyByAddr to zuul/memory.
TSS keys store only the public key (the private key is distributed
across TSS participants). Multi-form address indexing (P2PKH, P2WPKH,
P2TR) is shared with local keys; cross-type collisions are rejected
with distinct sentinels (ErrTSSKeyOccupied, ErrLocalKeyOccupied)
wrapping ErrKeyExists.
External signature application
TransactionApplyECDSA signs P2PKH and P2WPKH inputs given a DER-encoded
ECDSA signature and the signer's public key. TransactionApplySchnorr
signs P2TR key-path inputs given a 64-byte Schnorr signature. Both
verify pubkey-to-address binding before applying.
Verification
VerifyECDSA and VerifySchnorr validate signatures against the
transaction's sighash without requiring the private key — intended for
coordinators that receive signatures from remote TSS participants.
Tests
engine for P2PKH, P2WPKH, and P2TR inputs
Blocked on hemilabs/x PR #9 (tss-lib v3 tag).