TxBuilder should not change order of insertion#261
Closed
nymius wants to merge 2 commits intobitcoindevkit:masterfrom
Closed
TxBuilder should not change order of insertion#261nymius wants to merge 2 commits intobitcoindevkit:masterfrom
nymius wants to merge 2 commits intobitcoindevkit:masterfrom
Conversation
When TxBuilder::ordering is TxOrdering::Untouched, the insertion order of recipients and manually selected UTxOs should be preserved in transaction's output and input vectors respectively. Fixes bitcoindevkit#244
Pull Request Test Coverage Report for Build 15537285874Details
💛 - Coveralls |
This was referenced Jun 9, 2025
Contributor
Author
|
Overrode by #262 |
oleonardolima
added a commit
that referenced
this pull request
Jul 1, 2025
…xOrdering::Untouched` 0522114 test(tx_builder): update precedence check of local UTXOs over add_foreign_utxos (valued mammal) 73bef28 doc(tx_builder): add info about manually selected UTxOs priority (nymius) 3316236 fix(tx_builder): preserve insertion order with TxOrdering::Untouched (nymius) Pull request description: ### Description On my attempt to fix bitcoindevkit/bdk#1794 in bitcoindevkit/bdk#1798, I broke the assumption that insertion order is preserved when `TxBuilder::ordering` is `TxOrdering::Untouched`. Some users are relying in this assumption, so here I'm trying to restore it back, without adding a new dependency for this single use case like #252, or creating a new struct just to track this. In this fourth alternative solution I'm going back to use `Vec` to store the manually selected UTxOs. I was reluctant to do it in this way because `HashMap` seems a better solution giving its property of avoiding duplicates, but as we also want to keep the secuential nature of the insertion of UTxOs in `TxBuilder`, here is an alternative aligned with that principle. May replace #252 May replace #261 . Fixes #244 ### Notes to the reviewers Also, as I was working on this, I came back to the following tests: - `test_prexisting_foreign_utxo_have_no_precedence_over_local_utxo_with_same_outpoint` - `test_prexisting_local_utxo_have_precedence_over_foreign_utxo_with_same_outpoint` Motivated during the implementation and review of bitcoindevkit/bdk#1798. Which required the underlying structure to also hold the properties of no duplication for manually selected UTxOs, as the structures were accessed directly for these cases. The test tries to cover the case when there are two wallets using the same descriptor, one tracks transactions and the other does not. The first passes UTxOs belonging to the second one and this one creates transactions using the `add_foreign_utxo` interface. In this case it was expected for any `LocalUtxo` of the offline wallet to supersede any conflicting foreign UTxO. But, the simulation of this case went against the borrowing constraints of rust. By how costly was to reproduce this behavior for me in the tests, I would like to have second opinions in the feasibility of the test case. ### Changelog notice No public APIs are changed by these commits. ### Checklists > [!IMPORTANT] > This pull request **DOES NOT** break the existing API * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo +nightly fmt` and `cargo clippy` before committing * [x] I've added tests for the new code * [x] I've expanded docs addressing new code * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: ValuedMammal: reACK 0522114 oleonardolima: ACK 0522114 Tree-SHA512: f2726d75eab83e28cc748ac5cd6bd0c7f3dddb409ac61baf7d0a7030ddf81c11b10dbd5b18e8ac3d29a6afb4b8f29ee9a88f83094aebec771fdb4da2cd718326
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
2 participants
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.
Description
On my attempt to fix bitcoindevkit/bdk#1794 in bitcoindevkit/bdk#1798, I broke the assumption that insertion order is preserved when
TxBuilder::orderingisTxOrdering::Untouched. Some users are relying in this assumption so here I'm trying to restore it back, without adding a new dependency for this single use case like #252 , and trying to not redesign theTxBuilder::utxosinterface again.However, I'm open to do that if there are strong reasons for it. Opening as Draft to gather opinions.
Fixes #244
cc: @eauxxs
Notes to the reviewers
Changelog notice
No public APIs are changed by these commits.
Checklists
Important
This pull request DOES NOT break the existing API
cargo +nightly fmtandcargo clippybefore committing