Skip to content

Prefer Utxo::Local over Utxo::Foreign in OldestFirstCoinSelection #265

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nymius
Copy link
Contributor

@nymius nymius commented Jun 11, 2025

Description

The comments in the OldestFirstCoinSelection implementation stated the following:

// For utxo that doesn't exist in DB, they will have lowest priority to be selected

But this was not honored in the code.

This PR enforces this and ensures the expected behaviour through the two following new tests:

  • test_oldest_first_coin_selection_uses_all_optional_with_foreign_utxo_locals_sorted_first
  • test_oldest_first_coin_selection_uses_only_all_optional_local_utxos_not_a_single_foreign

Fixes #264

Changelog notice

No public APIs are changed by these commits.

Checklists

Important

This pull request DOES NOT break the existing API

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing
  • I've added tests for the new code
  • I've expanded docs addressing new code
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@coveralls
Copy link

coveralls commented Jun 11, 2025

Pull Request Test Coverage Report for Build 15622468119

Details

  • 74 of 74 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 85.678%

Totals Coverage Status
Change from base Build 15476130196: 0.1%
Covered Lines: 7448
Relevant Lines: 8693

💛 - Coveralls

@MaxFangX
Copy link
Contributor

Thanks for the quick fix!

@nymius nymius force-pushed the fix/issue-264-oldest-first-selects-local-last branch from c286a82 to ef5cf30 Compare June 12, 2025 22:55
@nymius nymius force-pushed the fix/issue-264-oldest-first-selects-local-last branch from ef5cf30 to d23d07b Compare June 12, 2025 22:57
@nymius
Copy link
Contributor Author

nymius commented Jun 12, 2025

@MaxFangX thanks for looking at the code so close!
Just for clarification, if you're using TxBuilder to create transactions, the only way to have foreign UTxOs in your selection is through the add_foreign_utxo method, which adds UTxOs to the manual selected list. That list would be passed to the coin selection implementation as the required_utxos. So, there is no way through that API to stumble upon this issue.
If you are not using TxBuilder::create_tx, then you could create your optional_utxos with as many foreign UTxOs as you want, and in that case the outcome would be affected.

@nymius nymius added this to the Wallet 2.1.0 milestone Jun 17, 2025
@nymius nymius requested a review from evanlinjin June 18, 2025 02:12
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d23d07b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

OldestFirstCoinSelection selects Utxo::Foreigns first instead of last
4 participants