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

[nymius]

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]

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
Change from base Build 15476130196:	0.1%
Covered Lines:	7448
Relevant Lines:	8693

---

💛 - Coveralls

[MaxFangX]

Thanks for the quick fix!

[evanlinjin]

Why do you think about using .sort_unstable_by or just using .sort_unstable_by_key with a different key?

I think it's better to sort everything in one loop than in two goes.

            optional_utxos.sort_unstable_by_key(|wu| {
                Reverse(match &wu.utxo {
                    Utxo::Local(local) => Some(Reverse(local.chain_position)),
                    Utxo::Foreign { .. } => None,
                })
            });

[nymius]

I like it, however, by reading the Reverse docs I arrived to the following solution:

optional_utxos.sort_unstable_by_key(|wu| {
    match &wu.utxo {
        Utxo::Local(local) => (false, Some(local.chain_position)),
        Utxo::Foreign { .. } => (true, None),
    }
});

false < true, so all LocalUtxos are ordered at the beginning of the Vec. WDYT?

[evanlinjin]

That works too!

[nymius]

@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.