fix(bitcoin): support sending to taproot (bc1p) addresses - #811
Open
KTibow wants to merge 1 commit into
Open
Conversation
bitcoinjs-lib requires initEccLib() before it can build a P2TR output script, and nothing in the extension ever called it. address.toOutputScript() therefore threw "No ECC Library provided" for every bc1p... address, isAddress() swallowed that in its catch and returned false, and taproot recipients were rejected as invalid with no setting able to change it. Register @bitcoinerlab/secp256k1 once from a shared module and import it from the address validation and PSBT paths. That package is already in the lockfile at the same version by way of ledger-bitcoin, so this adds no new package to the bundle. Also size the recipient output from its own address type. Fee rates are quoted per vByte and floored at 1 sat/vB, so charging for a 43 vB taproot output as though it were a 31 vB P2WPKH one leaves the effective rate under the minimum relay fee and the broadcast fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughAdds secp256k1 initialization, recognizes Taproot and other Bitcoin address formats, classifies transaction outputs by address type, and propagates output addresses through send and swap fee calculations. ChangesBitcoin Taproot sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SendTransaction
participant TxSize
participant FeeCalculator
SendTransaction->>TxSize: calculate size using addressTo
TxSize->>TxSize: classify Taproot and SegWit outputs
TxSize-->>SendTransaction: return transaction size
SendTransaction->>FeeCalculator: request fee values
FeeCalculator-->>SendTransaction: return updated fees
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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.
Problem
Taproot addresses are rejected as invalid in the Bitcoin send screen. Pasting any
bc1p...address turns the input red and leaves the Send button disabled, and there is no setting that changes it.Root cause
isAddressinproviders/bitcoin/libs/utils.tsvalidates by round-tripping throughbitcoinjs-lib:bitcoinjs-libv6 does not bundle an elliptic curve implementation. It requiresinitEccLib()before it can build a P2TR output script, and nothing in the repo ever calls it:So
toOutputScriptthrows, thecatchswallows the error, and the address is reported invalid. Againstbitcoinjs-lib@6.1.7, the version in the lockfile:This is a missing initialisation rather than a validation bug: the version of
bitcoinjs-libin use supports taproot fine.Fix
Register an ECC implementation once, from a small side-effect module imported by the address validation and PSBT paths.
On the dependency:
@bitcoinerlab/secp256k1@^1.2.0is already resolved inyarn.lockat exactly that version, pulled in byledger-bitcoinvia@enkryptcom/hw-wallets:Adding it as a direct dependency of the extension is a one line change to
yarn.lockand adds no package to the bundle. It is pure JavaScript with a single dependency,@noble/curves ^1.7.0, which the lockfile already resolves. I avoidedtiny-secp256k1because v2 ships asecp256k1.wasmbinary and browser wasm loaders, which is a real integration cost in the Vite build for identical functionality here.Fee sizing. Registering the curve alone is not enough to make a taproot send succeed.
calculateSizeBasedOnTypesized every output as the account's own payment type, so a P2TR output was charged as 31 vB when it is 43 vB. Fee rates come back fromBTCFeeHandlerasMath.ceil(sat_per_vbyte)with an effective floor of 1 sat/vB, so on a 1-in/2-out transaction that 12 vB shortfall drops the real rate to roughly 0.92 sat/vB — under the minimum relay fee, and the broadcast fails. Outputs are now sized from their address type where it is known.Only bech32/bech32m outputs are classified, since those are the ones whose size differs enough from the account's own type to matter. Anything else keeps the previous behaviour and is at most 3 vB out.
The change output and a partially typed recipient have no entry and are sized as before, so nothing changes for existing send flows.
send-transaction/index.vuenow recomputes the fee whenaddressTochanges, which it previously did not do at all.This PR deliberately does not add
P2TRtoPaymentTypeor derive taproot accounts. It only makes taproot valid as a recipient.Testing
yarn testinpackages/extension: 18 passing, up from 11. New cases inproviders/bitcoin/tests/bitcoin.address.validation.test.tscover taproot, the other standard address types, malformed and wrong-network addresses, output classification, and the 12 vB size delta.initEccLibimport fails the new taproot case withexpected false to equal true, so it is a genuine regression test.vue-tsc --build --forceproduces exactly the same 309 pre-existing errors asdevelop; none added.eslintandprettier --checkclean on the touched files.On the test environment. The new test file carries
// @vitest-environment node.bitcoinjs-libverifies an ECC library by feeding itBuffer.from(...)vectors and@noble/curvesrejects anything that is not aUint8Array; under the default jsdom environment those two come from different realms, soinstanceoffails andinitEccLibthrows even though the library is fine.To confirm that is only a harness artifact and not something users would hit, I built a bundle with the production
nodePolyfillsconfig fromvite.config.tsand ran it in a page with no NodeBufferpresent at all:In a browser there is one realm and the bundled
bufferpolyfill extends that realm'sUint8Array, so the check passes.Notes
I could not find an existing issue or PR covering this. Happy to adjust the approach, split out the fee sizing, or swap the ECC dependency if you would rather go a different way.
Written by Claude Opus 5 with human oversight. Every claim above was checked against the source and the failure modes reproduced.
Summary by CodeRabbit
New Features
Bug Fixes
Tests