fix(tbtc): gate native BuildTaprootTx on the transaction being all-Taproot#4117
Merged
mswilkison merged 2 commits intoJun 27, 2026
Merged
Conversation
…proot signTransaction unconditionally ran the native (Rust cgo) BuildTaprootTx parity/substitution path at the top of the function -- for EVERY transaction, including legacy ECDSA redemptions/sweeps/moving-funds. The native builder is a Taproot builder, so running it for a legacy (non-Taproot) transaction is meaningless, and a hard error from it (other than ErrNativeCryptographyUnavailable) would fail the signing of that legacy transaction. In the default build the native builder is a no-op, so this only bit the frost_native+cgo build, but it is a latent regression on the legacy signing path. Gate the native-build/substitution path on unsignedTx.HasOnlyTaprootKeyPathInputs() -- the same predicate that already governs FROST signing later in the function -- so the native Taproot builder runs only for all-Taproot transactions. Extract the path into maybeSubstituteNativeBuildTaprootTx. The substitution LOGIC (observational logging, divergence rejection, matching-IO acceptance) remains covered directly by the TestEvaluateNativeUnsignedTransactionForSigning_* tests. Replace the four legacy-P2PKH substitution-through-signTransaction integration tests (which asserted the now-removed behaviour) with two gate tests -- SkipsNativeBuildForLegacyTransaction and InvokesNativeBuildForTaprootTransaction -- and retarget the two native-build error-propagation tests to a Taproot transaction (the only path on which the native build now runs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
✨ Finishing Touches🧪 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 |
…buildtx-gate-taproot
1d4e2fb
into
feat/frost-schnorr-migration-scaffold
17 checks passed
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
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.
Summary
Follow-up to #3866 (Go FROST/ROAST coordinator). Gates the native (Rust cgo)
BuildTaprootTxparity/substitution path so it runs only for Taproot transactions.walletTransactionExecutor.signTransactionran the nativeBuildTaprootTxpath unconditionally, at the very top, for every transaction a wallet signs — including legacy ECDSA redemptions / sweeps / moving-funds — before any scheme/Taproot check. The native builder is a Taproot builder, so running it for a legacy (non-Taproot) transaction is meaningless, and a hard error from it (other thanErrNativeCryptographyUnavailable) would fail the signing of that legacy transaction. In the default build the native builder is a no-op (("", nil)), so this only bit thefrost_native+cgobuild — but it's a latent regression on the legacy signing path.Fix
Gate the native-build/substitution path on
unsignedTx.HasOnlyTaprootKeyPathInputs()— the same predicate that already governs FROST signing later in the same function ("cannot apply FROST signatures to non-taproot transaction inputs"). The native Taproot builder now runs only for all-Taproot transactions; legacy transactions skip it. The path is extracted intomaybeSubstituteNativeBuildTaprootTx.Tests
The four legacy-P2PKH substitution-through-
signTransactionintegration tests asserted the now-removed behaviour (substitution for a legacy tx). They are replaced with:SkipsNativeBuildForLegacyTransaction— a legacy (P2PKH) tx: the native build is not invoked even with substitution enabled, and the tx signs via the Go path. (Verified to fail if the gate is removed.)SubstitutesNativeBuildForTaprootTransaction— an all-Taproot tx: the native build is invoked, and a matching native tx is substituted (ReplaceUnsignedTransaction+ the substitution info log) and signed with a Taproot witness.The two native-build error-propagation tests are retargeted to a Taproot tx (the only path on which the native build now runs), via a shared
buildTaprootKeyPathUnsignedTxForTesthelper. The substitution logic remains covered directly by theTestEvaluateNativeUnsignedTransactionForSigning_*tests.gofmt +
go vetclean; full untaggedpkg/tbtcsuite passes; builds clean under-tags 'frost_native frost_tbtc_signer cgo'.Found during review of #3866.
🤖 Generated with Claude Code