fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT - #157
Open
jeremytsng wants to merge 1 commit into
Open
fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT#157jeremytsng wants to merge 1 commit into
jeremytsng wants to merge 1 commit into
Conversation
jeremytsng
force-pushed
the
fix/btc-output-reordering-fx-fail
branch
2 times, most recently
from
August 17, 2026 17:03
30f6166 to
54d49c5
Compare
4 tasks
jeremytsng
force-pushed
the
fix/btc-output-reordering-fx-fail
branch
2 times, most recently
from
August 19, 2026 13:17
cd9168d to
0712819
Compare
jeremytsng
marked this pull request as ready for review
August 19, 2026 18:38
Contributor
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.
|
|
||
| - Keep the template output order when filling a PSBT ([#157](https://github.com/MetaMask/internal-snaps/pull/157)) | ||
| - A template output belonging to the wallet is now only used as the drain output when it is the last output. Previously any such output was moved to the end of the transaction, silently reordering templates that place change before another output. | ||
| - Filling a PSBT now fails with a `ValidationError` when the built transaction does not reproduce every template output, at its original index, with its original value. Previously only the output count was compared, so a divergent transaction could be signed and broadcast. |
| } | ||
|
|
||
| const builtOutputs = builtPsbt.unsigned_tx.output; | ||
| const preserved = templateOutputs.every( |
Contributor
|
@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. |
… 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.
jeremytsng
force-pushed
the
fix/btc-output-reordering-fx-fail
branch
from
August 20, 2026 19:04
0712819 to
303e410
Compare
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
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.
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