fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT - #157
fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT#157jeremytsng wants to merge 1 commit into
Conversation
30f6166 to
54d49c5
Compare
cd9168d to
0712819
Compare
There was a problem hiding this comment.
Pull request overview
Preserves PSBT template output ordering during wallet transaction rebuilding.
Changes:
- Restricts drain handling to trailing wallet-owned outputs.
- Validates rebuilt output scripts, positions, and values.
- Adds regression tests, changelog entry, and updated bundle hash.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
AccountUseCases.ts |
Updates PSBT rebuilding and validation. |
AccountUseCases.test.ts |
Adds output-order regression tests. |
snap.manifest.json |
Updates the bundle checksum. |
CHANGELOG.md |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@jeremytsng Could you please test with some swap providers in MM with a preview-build to secure nothing is breaking up on their side. outputs order is a high requirement to them. |
0712819 to
303e410
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@Battambang i tested with the preview build against mainnet providers, and output order was preserved in both swaps and didnt see any validation errors 
|
… a PSBT Only treat a wallet-owned template output as the drain output when it is the last output. BDK appends the drain output, so a wallet-owned output placed anywhere earlier was silently moved to the end of the transaction, reordering templates that put change before another output. Verify the built transaction against the template before returning it: every template output must appear at its original index with its original script and value, except the drain output, which takes the excess. Beyond the template, only a single appended output is tolerated, and it has to belong to the wallet. The previous check compared only the number of outputs, so a transaction whose outputs diverged from the template could still be signed and broadcast.
303e410 to
adc5a96
Compare
| (txout, index) => | ||
| builtOutputs[index]?.script_pubkey.to_hex_string() === | ||
| txout.script_pubkey.to_hex_string() && | ||
| (txout === drainOutput || |
There was a problem hiding this comment.
1. The new check is blind at the last index on the retry path (AccountUseCases.ts:849)
drainOutput is computed once at :785, but the fallback rebuild at :810-825 re-adds every template output via addRecipientByScript — no drain is configured on that attempt. The txout === drainOutput exemption still short-circuits the value comparison there, even though the value is now fully caller-specified.
Reproduced it:
template: [deposit 100000, change(mine) 5000]
1st finish(): [deposit] // sub-dust drain dropped -> triggers retry
2nd finish(): [deposit 100000, change 1] // fixed recipient, value diverged
=> fillPsbt RESOLVES (should throw)
That's the full-balance / dust-drain case, so it's exactly where the user's remaining balance sits. The symmetric non-last case does throw, so the gap is specific to the exempted index.
Suggested fix — track whether the drain was actually configured on the attempt that produced builtPsbt:
let drainConfigured = drainOutput !== undefined;
// ...inside the retry block, after switching to all-fixed recipients:
drainConfigured = false;then gate the exemption on drainConfigured && txout === drainOutput.
Explanation
The problem
When a bridge gives us a PSBT, the snap doesn't sign it as-is. It rebuilds the transaction with BDK and copies the outputs across. Protocols read those outputs by position — deposit first, memo second, change last — so the rebuild has to keep that order. Two things broke it.
First, any output belonging to the wallet was used as BDK's drain output, and BDK always puts the drain output last. So if one of our outputs sat anywhere but last, it got moved to the end and everything after it shifted up.
Second, nothing checked the result. The only test was whether the number of outputs had shrunk, so a transaction that no longer matched the template was signed and broadcast anyway.
The fix
Only a trailing output of ours becomes the drain output, since that's the only position BDK can keep. Any earlier one stays a normal recipient with its position and amount intact, and BDK adds its own change after it.
#fillPsbtnow also checks its own work before returning. Every template output has to appear at the same index, with the same script and amount. The drain output is exempt from the amount check, because it takes the excess by design, and an extra change output at the end is fine. Anything else throws instead of being signed.This covers everything that fills a PSBT:
fillPsbt,signPsbt({ fill: true }),computeFee, and thesignAndSendTransactioncall that bridging uses.Verification
5 new unit tests, and the package suite passes (595 tests).
Checked against the
@metamask/bitcoindevkitbinary the snap actually bundles. A template of[deposit, ours, OP_RETURN]used to come back as[deposit, OP_RETURN, ours]. It now comes back as[deposit, ours, OP_RETURN, change].#158 adds a regtest test that fails without this change.
References
Found while investigating a bridge deposit that was never credited: the transaction that confirmed didn't match the output order of the quote's PSBT.
Not covered here:
signPsbt({ fill: false })signs a PSBT the caller built, so there's no template to compare it against.Checklist