feat(bitcoin): P2TR script derivation, sighash and fee estimation - #4243
Open
piotr-roslaniec wants to merge 11 commits into
Open
feat(bitcoin): P2TR script derivation, sighash and fee estimation#4243piotr-roslaniec wants to merge 11 commits into
piotr-roslaniec wants to merge 11 commits into
Conversation
Splits the Bitcoin layer out of the FROST/Schnorr migration branch (#3866) so the code legacy wallets execute today can be reviewed on its own, separately from the FROST engine work that stays inert until build tags and env gates are set. This layer is not behind any build tag, so every currently-live wallet runs it: - Taproot key derivation and BIP-341 tweak/output-key math (taproot.go). - P2TR output script derivation and script-type detection (script.go). - BIP-341 SIGHASH_DEFAULT key-spend sighash construction (transaction_builder.go). - Fee estimation for P2TR inputs and outputs (estimator.go). - Electrum: the per-wallet script set grows from 2 to 3 with the added P2TR script, so per-wallet history and UTXO lookups issue one more sequential RPC. Nothing here depends on pkg/frost, so it compiles and tests standalone against frost-upgrade, and the downstream pkg/tbtcpg and pkg/tbtc consumers build unchanged against it.
The BIP-341 key-path signature hash preimage is assembled by hand: epoch, hash type, version, locktime, five midstate hashes, spend type and input index. A misordered or mistyped field yields a digest that signs a transaction the wallet did not intend, and the only existing coverage was a single hardcoded vector, which fixes one input count and one output shape. Compare every P2TR input against txscript.CalcTaprootSignatureHash over randomized multi-input transactions, varying input count and ordering, per-input values and script types, output count and values, version and locktime. txscript was already a dependency of this file for the legacy sighash paths. Verified the test fails on a swapped midstate write and on a wrong sighash epoch byte.
requestWithRetry and GetFee held clientMutex across the whole Electrum round trip, so every request in the process queued behind every other one for a full network round trip each. Per-script lookups that could overlap ran strictly in sequence. go-electrum multiplexes concurrent requests over an id-to-channel map with a single reader dispatching responses, so it is safe to call from several goroutines at once. The lock is only needed to keep readers from observing a client pointer mid-swap during a reconnect. Take a read lock long enough to copy the pointer and release it before the request. reconnectIfShutdown remains the sole writer and keeps the write lock. A reconnect landing just after the copy fails the request against the stale client and the retry wrapper repeats it, which is the same recovery path as before.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
The redeemer-script switch in EstimateRedemptionFee has no P2TR arm, so a P2TR redeemer output script falls through to the non-standard default and returns an error. That aborts the fee estimate for the whole batch, so one P2TR request stalls every pending redemption for that wallet rather than only its own. The Bridge accepts P2TR redeemer addresses (Redemption.sol validates against a set that includes them), so this is reachable as soon as a redeemer supplies one. Price it with AddOutputScript rather than a dedicated helper: an output costs its script length plus a fixed header, and the real script is already in hand, so no canonical placeholder is needed. The default arm still rejects genuinely non-standard scripts and now names the type it rejected.
Stacked on #4243, which supplies the `P2TRScript` handling in `pkg/bitcoin/estimator.go` this needs. Retarget to `main` once #4243 lands. ## The bug `EstimateRedemptionFee`'s redeemer-script switch has no P2TR arm, so a P2TR redeemer output script falls through to the non-standard `default` and returns an error. That error aborts the estimate for the **entire batch**, so one P2TR request stalls every pending redemption for that wallet, not just its own. The Bridge accepts P2TR redeemer addresses -- `Redemption.sol` validates against a set that includes them -- so this is reachable as soon as any redeemer supplies one. This switch is byte-identical on `main`, so the gap predates the FROST work and is not introduced by #3866. ## The fix Add the arm using `AddOutputScript`. An output costs its script length plus a fixed header, and the real script is already in hand, so there is no need for a canonical placeholder the way the P2PKH and P2SH arms use one. The `default` arm still rejects genuinely non-standard scripts, and now names the type it rejected -- previously the error gave an operator no way to identify the offending request. ## Testing Two new tests: a batch containing a P2TR redeemer must be priced and must cost strictly more than the same batch without it (an equal fee would mean the script was silently dropped and every redemption in the batch underpaid), and an OP_RETURN script must still be rejected with its type named. Verified the first test fails without the fix: ``` a P2TR redeemer script must be priced, not rejected: non-standard redeemer output script type [P2TR] ```
…gnatures error branches
Two regression guards added:
- TestTaprootTweak_RejectsInvalidInputs (pkg/bitcoin/script_test.go): exercises
schnorr.ParsePubKey rejection of a malformed x-only internal key. Two further
rejection paths (tweak >= curve order, infinity output) require inverting the
BIP-341 tap-tweak hash to construct a deterministic fixture, so are skipped
with a comment pointing to code-review coverage.
- TestTransactionBuilder_AddTaprootKeyPathSignatures_RejectsInvalid
(pkg/bitcoin/transaction_builder_test.go): exercises the 'wrong signatures
count' branch (nil container) and the cryptographic Verify gate ('invalid
taproot key-path signature for input [%v]') by submitting a syntactically
valid BIP-340 Schnorr signature over a different message.
Both pass against current code; the value is regression coverage for changes
that might drop the rejection branches.
Refs PR #4243 review findings M-TAPROOT-INVALID-KEY and M-SIG-VERIFY-UNTESTED.
Direct edits to transaction_builder.go:
- P3 TAPSIIGHASH-TAG: use chainhash.TagTapSighash constant instead of
the []byte("TapSighash") literal (line 884).
- P2 STALE-SIGHASH-CACHE: AddOutput now invalidates cached sighashes
so post-computation mutations force recomputation (line 331).
- P2 DIGEST-WIRE-FORMAT: ECDSA verification reads digests via
FillBytes(make([]byte, sha256.Size)) so both ECDSA and Taproot paths
agree on a fixed-width 32-byte digest wire format (line 469).
- P1 REPLACE-TX: ReplaceUnsignedTransaction now binds the replacement
to the builder's prior state — per-index PreviousOutPoint equality
+ TxOut set equality (when outputs were committed) before restoring
pre-signing witness/signature-script. Prevents a caller-controlled
replacement from redirecting funds or mis-aligning sigHashArgs[i]
with a reordered tx.TxIn[i], producing a self-consistent-but-wrong
digest that survives the local Verify gate and broadcasts an
unintended transaction.
- P2 P2TR-BACKCOMPAT: hoist the HasTaprootKeyPathInputs guard above
the per-input loop in AddSignatures so a mixed transaction's
preceding inputs are not mutated before the rejection returns.
- Doc item (5): FROST-migration scope note tightened to a state
invariant — multi-element pre-signing witnesses are not supported
by the restore path; refuse rather than silently drop witness data.
- Doc item (6): ReplaceUnsignedTransaction doc now reflects the
binding checks + witness restore.
- Doc item (9): AddTaprootKeyPathSignatures doc notes the
verify-before-accept behavior.
Refs PR #4243 review findings M-TAPSIIGHASH-TAG, M-STALE-SIGHASH-CACHE,
M-DIGEST-WIRE-FORMAT, M-P2TR-BACKCOMPAT, M-REPLACE-TX.
…aprootSignatureHash Replaces the hand-rolled midstate-and-preimage BIP-341 sighash implementation in ComputeSignatureHashes with btcd's well-tested txscript.CalcTaprootSignatureHash, eliminating the consensus-critical duplication of the spec. The taproot_sighash_differential_test that previously proved agreement between the two implementations is removed, since the implementation under test no longer exists. Coverage moves to a new subtest under TestTransactionBuilder_AddTaprootKeyPathSignatures that exercises non-default sequences and outpoint indices against a reference transaction assembled independently of the builder.
…eUnsignedTransaction
Three latent consistency gaps that the prior review surfaced but Phase 1
did not close:
- addDirectKeySpendInput and AddScriptHashInput now refuse to append a
P2TR input to a builder holding only non-P2TR inputs (or vice versa),
via the new assertUniformTaprootShape helper. The signing paths refuse
such mixed shapes but only after the entire input vector is built, so
a typo in one Add call previously surfaced as a confusing signing-time
error rather than an immediate construction-time refusal.
- Both input mutators now reset the cached signature hashes, mirroring
the behavior AddOutput already had. Storing a stale digest after a
later Add call would silently produce a self-consistent-but-wrong
signature.
- ReplaceUnsignedTransaction now binds the replacement's TxOut set
unconditionally rather than only when the builder had previously
committed outputs. A replacement that drops or changes outputs after
the builder had committed to them is now rejected with an explicit
error instead of producing a transaction whose sigHashArgs[i] no
longer matches its rebuilt TxOut set.
Coverage: TestTransactionBuilder_RejectsMixedP2TRNonP2TR covers the
construction-time rejection in both directions;
TestTransactionBuilder_AddOutput now also seeds and asserts the
sigHashes reset;
TestTransactionBuilder_ReplaceUnsignedTransaction_Rejects{Outpoint,
TxOutValue,TxOutScript,OutputCount}Mismatch pin each rejection branch.
…atestUniqueTxHashes Documentation and test-coverage cleanups: - currentClient's doc comment now scopes the retry-fallback claim to requestWithRetry callers, notes that getFeeBtcPerKbOnce calls currentClient directly and surfaces its failure to the fee fallback loop instead of retrying, and points out the upstream go-electrum Shutdown() thread-unsafety as a separately-tracked issue. - selectLatestUniqueTxHashes gets a doc comment that documents the dedup-before-limit ordering and the limit <= 0 empty-slice guard. - AddOutput's doc comment now states that adding an output after ComputeSignatureHashes invalidates the cached signature hashes. - TestTaprootTweak_RejectsInvalidInputs is restructured into three t.Run subtests so the unreachable branches report as skipped subtests instead of unconditional t.Skip()s at the function level. The accompanying function comment is corrected: the tweak >= curve order probability is ~2^-128 (not ~50%), and the infinity-output rejection lives in TaprootOutputKey (taproot.go:73-75), not taprootTweakScalar. - TestSelectLatestUniqueTxHashes exercises the new helper across the negative-limit, zero-limit, empty-input, dedup, dedup-before-limit, trailing-N-on-overflow, and underflow paths. - TestTransactionBuilder_AddTaprootKeyPathSignatures drops the 'independently cross-checked by reviewers' clause from the hardcoded-vector comment.
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
1 participant
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.
Extracts the Bitcoin-primitive layer out of #3866 so it can be reviewed on
its own. No FROST, ROAST or tBTC wallet logic here: this is P2TR script
derivation, the BIP-341 key-path signature hash, fee estimation for Taproot
inputs, and the Electrum plumbing they need.
Landing this on
frost-upgradeshrinks #3866 by these 11 files, sincefrost-upgradeis its base.What
pkg/bitcoin/taproot.go(new):TaprootLeafHash,TaprootTweak,TaprootOutputKey,PayToTaprootWithScriptTree.pkg/bitcoin/script.go:PayToTaproot, plusP2TRScriptin the script-typeswitch.
pkg/bitcoin/transaction_builder.go: Taproot key-path inputs(
AddTaprootKeyPathInput, and the merkle-root variant that checks the outputkey really commits to the given internal key and root), and the BIP-341
key-path sighash.
pkg/bitcoin/estimator.go: P2TR input and output sizes.pkg/bitcoin/electrum/electrum.go:P2TRScriptsupport, and the locknarrowing below.
Two changes beyond the extraction
Both came out of reviewing #3866 and belong in these files rather than there.
A differential test for the key-path sighash. The preimage is assembled by
hand -- epoch, hash type, version, locktime, five midstate hashes, spend type,
input index -- and the only coverage was one hardcoded vector, which pins a
single input count and output shape. A misordered field there produces a digest
that signs a transaction the wallet did not intend. The new test compares every
P2TR input against
txscript.CalcTaprootSignatureHashacross randomizedmulti-input transactions.
txscriptwas already imported in that file for thelegacy sighash paths, so the reference implementation was one call away.
Verified it fails on a swapped midstate write and on a wrong epoch byte.
Electrum lock narrowing.
requestWithRetryandGetFeeheldclientMutexacross the whole round trip, so every Electrum request in the process queued
behind every other one.
go-electrummultiplexes concurrent requests over anid-to-channel map with a single reader dispatching responses, so the lock is
only needed to stop readers observing a client pointer mid-swap during a
reconnect. It now covers the pointer copy only;
reconnectIfShutdownstays thesole writer. Clean under
-race.Three changes beyond the extraction (added in revision)
Hand-rolled BIP-341 sighash swap (Phase 3). The differential test above
established byte-equivalence with
txscript.CalcTaprootSignatureHash, so the~230 lines of hand-rolled midstate (
calcTaprootKeyPathSignatureHash,taprootSignatureHashMidstate, the fivetaprootHash*helpers,writeOutPoint)have been deleted in favor of the library call. The differential test itself
became a tautology and was removed; the existing hardcoded test vector in
TestTransactionBuilder_AddTaprootKeyPathSignaturesis the functionalregression guard, plus a new subtest that exercises non-default
sequenceand outpoint indices (the two midstate fields the old differential test
pinned to byte-symmetric constants).
selectLatestUniqueTxHashesdedup-before-limit on the live SPV path. Theguard was added in Phase 2 to fix a
limit < 0slice-bounds panic, but thehelper now also dedups transaction hashes before applying
limit. Thenon-Taproot public callers (
GetTransactionsForPublicKeyHash,GetTransactionsForPublicKeyScripts) consume this on every SPV-maintainerpoll. A transaction that pays both the wallet's P2PKH and P2WPKH scripts
previously consumed two
limitslots and was returned twice; it now consumesone and is returned once. The change is an improvement but is observable
behavior on the live path (four production call sites:
maintainer/spv/ deposit_sweep.go,moved_funds_sweep.go,moving_funds.go,redemptions.go),so it merits disclosure even though no SPV consumer changes for it.
ReplaceUnsignedTransactionis multi-party-signing coordination plumbing.It is not Bitcoin-primitive code in the four categories above: it lets a
coordinator-supplied unsigned transaction be adopted into a locally-built
builder while binding per-input outpoints and the TxOut set to the builder's
prior state. So is
UnsignedTransactionandUnsignedTransactionIO(withTxIDHex/ValueSats/ScriptPubKeyHexstring DTOs). All three are consumedby #3866's FROST signing round-trip, which ships after this PR. The methods
land here rather than there because (a) every input-by-input validation rule
they enforce (non-empty witness/signature-script rejection, outpoint and
output-set binding, pre-signing witness restoration) is a property of the
builder's own state machine, and (b) the review fixes that produced them
(
ReplaceUnsignedTransactionrebindstb.internalafter a per-state-shapecheck) belong with the builder tests.
Review fixes applied in this PR
ReplaceUnsignedTransactionnow binds the replacement's per-index outpointsand output set to the builder's prior state before rebinding
tb.internal.AddOutputinvalidates the cached sighashes so any post-computation mutationforces recomputation.
ComputeSignatureHashesnow usesFillBytes(32)on both the ECDSA andTaproot paths (consistent 32-byte digest wire format).
AddPublicKeyHashInputand the per-input signing guard reject mixed P2TR/ non-P2TR transaction shapes at construction.
keepAlivenow reads the client viacurrentClient()so the lock-narrowinginvariant ("readers hold the read lock just long enough to copy the pointer")
actually holds at every reader site.
selectLatestUniqueTxHashesguards againstlimit < 0(was a panic) andGetTransactionsForPublicKeyHashdelegates to it (consistent dedup behaviorwith the new
...ForPublicKeyScriptssiblings).getScriptUtxosForScriptsdedupes by outpoint (mirrors the transaction-hashsibling) so duplicated scripts in the caller-supplied slice no longer
double-count the same UTXO.
TaprootLeafHash;AddPublicKeyHashInputheadline andAddPublicKeyPathInputdoc now mention P2TR;
AddTaprootKeyPathSignaturesdoc states itsverify-before-accept contract; FROST-migration-scoped comments rewritten as
state invariants; etc.
TaprootTweakinvalid-x-only-key rejection and theAddTaprootKeyPathSignaturesVerify-gate / wrong-counts branches.ComputeSignatureHashesP2TR case now callstxscript.CalcTaprootSignatureHashdirectly; ~230 lines of hand-rolledmidstate deleted; differential test removed (now a tautology). New
TestTransactionBuilder_AddTaprootKeyPathSignaturessubtest coversnon-default
sequenceand outpoint-index midstate fields.AddPublicKeyHashInput/AddScriptHashInputnull the cached sighashes (theAddOutputinvariantnow extends to inputs);
assertUniformTaprootShaperejects mixed P2TR /non-P2TR transaction shapes at construction time, naming the offending
input index.
ReplaceUnsignedTransactionbinding unconditional: thelen (previousOutputs) > 0guard around the TxOut binding is gone; the bindingapplies whenever the builder has committed outputs. The successful-path
test now commits an output before the replacement; new tests cover
_RejectsOutpointMismatch,_RejectsTxOutValueMismatch,_RejectsTxOutScriptMismatch, and_RejectsOutputCountMismatch.selectLatestUniqueTxHasheshas a unit table(
TestSelectLatestUniqueTxHashes) covering negative / zero / positivelimits, dedup, and the dedup-before-limit ordering; the
limit <= 0semantics is documented (returns empty slice, panic-prevention guard).
currentClientdoc comment rewritten: blanket goroutine-safetyclaim dropped, retry claim scoped to
requestWithRetrycallers(
getFeeBtcPerKbOnceis now explicitly called out as non-retrying, target-skipping), upstream go-electrum
Shutdown()thread-unsafety noted asseparately tracked.
Known follow-ups (out of scope of this PR)
keep-network/go-electrumfork has two pre-existing concurrencybugs that the lock-narrowing does not fix (only makes more reachable): the
Client.Shutdown()unprotected map-nilling atnetwork.go:335-345, and theWebSocketTransport.SendMessage/gorilla/websocket.Conn.WriteMessageunsynchronized concurrent-writer pattern at
transport_ws.go:88-93. Thesebelong in the upstream fork as a separate fix.
pkg/tbtcpg/redemptions.go:516-520hardcodes the wallet main UTXO AND thechange output as P2WPKH (one call each to
AddPublicKeyHashInputs(1, true)and
AddPublicKeyHashOutputs(1, true)). Once wallet outputs become P2TR(the point of
frost-upgrade) the input will be over-estimated by ~26 vbytes(fee-safe) but the change output will be under-estimated by ~12 vbytes
(fee-unsafe). Net under-estimate is small (~1.5 vbytes) but the
"fee-safe direction" framing of the prior follow-up is wrong: the change
output is the dominant cost and it is under-priced. The newly-added
AddPublicKeyScriptInputhelper exists to address this and is the naturalfollow-up wire-in.
pkg/bitcoin/transaction_builder.goships a Taproot script-path / merkle-rootsurface (
TaprootLeafHash,TaprootTweak,PayToTaprootWithScriptTree,AddTaprootKeyPathInputWithMerkleRoot,TaprootKeyPathInputMerkleRoots)with no in-tree caller. Belongs in the FROST wallet PR that actually
performs script-path spending.
pkg/bitcoin/electrum/electrum.goships four new exported...ForPublicKeyScriptsmethods that are not on thebitcoin.Chaininterface; they have no caller and the integration tests do not exercise
ReplaceUnsignedTransaction,UnsignedTransaction, andUnsignedTransactionIO(withTxIDHex/ValueSats/ScriptPubKeyHexstring DTOs) are multi-party-signing coordination plumbing here rather than
in feat(tbtc/node): FROST/ROAST Go node — distributed DKG, interactive signing loop, Taproot wallet lifecycle #3866 -- see "Three changes beyond the extraction" above. The reviewer
flagged this as scope-creep; landing here is deliberate (the per-input
validation rules are a builder-state property, not a wallet property) but
the disclosure is the response.
Testing
go test -race ./pkg/bitcoin/... ./pkg/tbtcpg/...passes (254 tests across4 packages). The P2TR key-path computation is verified against both a
hardcoded BIP-341 test vector and a randomized subtest that exercises
non-default
sequenceand outpoint indices.