Skip to content

fix: make sure tx_build does not change the order of add_utxos when u… #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fix: make sure tx_build does not change the order of add_utxos when u… #252

wants to merge 2 commits into from

Conversation

@eauxxs
Copy link

@eauxxs eauxxs commented Jun 2, 2025

fixes #244

@nymius nymius mentioned this pull request Jun 9, 2025
Closed
7 tasks
@nymius
Copy link
Contributor

nymius commented Jun 9, 2025

Thanks for working on this! I'd prefer to not add the indexmap dependency just to cover this case. I've proposed an alternative solution in #261

@eauxxs
Copy link
Author

eauxxs commented Jun 10, 2025

@nymius Indeed. Adding a dependency for a small case is not a good idea. How about this idea? I still think it is better to replace the code in place as much as possible, rather than invading the code to do index calculations.

@nymius
Copy link
Contributor

nymius commented Jun 10, 2025 •
edited
Loading

Thanks for this! I still think is a workaround for the dependency issue. You are moving the problem of maintaining compatibility with the dependency to maintaining the OrderUtxos structure ourselves. Is a solution, but I still think we can do better.

@nymius nymius mentioned this pull request Jun 10, 2025
Merged
7 tasks
@coveralls
Copy link

coveralls commented Jun 19, 2025

Pull Request Test Coverage Report for Build 15555027734

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 39 of 39 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 85.61%
Totals Coverage Status
Change from base Build 15476130196: 0.06%
Covered Lines: 7413
Relevant Lines: 8659
💛 - Coveralls

oleonardolima added a commit that referenced this pull request Jul 1, 2025
@oleonardolima
Merge #262: Preserve insertion order of manually selected utxos if `T…
b925402 
…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
@oleonardolima
Copy link
Contributor

oleonardolima commented Jul 2, 2025

This can be closed, superseded by #262.

@eauxxs eauxxs closed this Jul 2, 2025
@eauxxs eauxxs deleted the fix_txordering branch July 2, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

BDK Wallet
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TxOrdering::Untouched no longer ensures the order of tx input

4 participants

@eauxxs @nymius @coveralls @oleonardolima