feat(bitcoin): P2TR script derivation, sighash and fee estimation - #4243
Open
piotr-roslaniec wants to merge 3 commits into
Open
feat(bitcoin): P2TR script derivation, sighash and fee estimation#4243piotr-roslaniec wants to merge 3 commits into
piotr-roslaniec wants to merge 3 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 |
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.Follow-up
pkg/tbtcpg/redemptions.gohas nocase bitcoin.P2TRScriptin itsredeemer-script switch, so a redemption batch containing a P2TR redeemer output
script fails its whole fee estimate. That switch is identical on
main, so thegap predates this work and goes live when tbtc-v2 #971 ships. It needs
estimator.go's P2TR support from this PR, so it follows in a separate PRrather than riding along here.
Testing
go test ./pkg/bitcoin/... ./pkg/tbtcpg/...passes, including under-race.