Remove Database type paramter from CoinSelectionAlgorithm

[LLFourn]

The rational for adding the database was iirc that the user may have some custom database and their coin selection algorithm may be coupled with particular data in their database.

This could easily be the case but it could also be that they need look up information from an internal HTTP API for some kind of business logic. Wherever they get their additional data from it can be done without implementing a coin selection algorithm for a particular database. Instead, the type implementing CoinSelectionAlgorithm can just store a reference to the database/api where it gets its additional data from.

To fix this issue I suggest:

• Remove the type parameter from CoinSelectionAlgorithm and just pass in a &dyn Database.
• Make TxBuilder take a & impl CoinSelectionAlgorithm to coin_selection (i.e. take a reference).

[csralvall]

In case someone lacks context of why was included the database field firstly go to the issue #121 where it was discussed.

I came up to the same conclusion, but for different reasons. When I was trying to include an associated function with a default implementation in the trait CoinSelectionAlgorithm I couldn't because the generic wasn't bounded to Sized. I used a workaround to avoid that limitation, but for me, it was something that added some unnecessary friction and for avoidable reasons. To begin with, the currently implemented algorithms don't use the database parameter. One of the first algorithms that could make use of the parameter is OldestFirst in #557. But if one use it and the others don't, I don't see much "generalization" in that parameter. The solution that came to my mind was to remove the database parameter from the coin_select method and include the generic type parameters as part of the struct that implements the CoinSelectionAlgorithm, including the database as a field in the struct implementing the trait. In that way, the coin_select implementation of that struct can make use of the database through its own field.

• The code should look like the following:

pub trait CoinSelectionAlgorithm: std::fmt::Debug {...}

#[derive(Debug, Default, Clone, Copy)]
pub struct OldestFirstCoinSelection<D: Database> {
  database: D,
  ...
}


impl<D: Database> CoinSelectionAlgorithm for OldestFirstCoinSelection<D> {...}

• You are proposing the following instead:

pub trait CoinSelectionAlgorithm: std::fmt::Debug {
 pub fn coin_select(&self, &dyn Database, ...) -> <...>;
 ...
}

• Combining my idea with yours the struct would look like this:

[derive(Debug, Default, Clone, Copy)]
 pub struct OldestFirstCoinSelection<D: Database> {
   database: &dyn D,
   ...
 }

• Another option, to avoid hidding the Database behind a struct field would be to include the parameter database as an Option<&dyn Database>. In the case of the current implementation of BnB of LargestCoin, that field would be None, but it would make clear the connection with a possible database. E. g.:

pub trait CoinSelectionAlgorithm: std::fmt::Debug {
 pub fn coin_select(&self, Option<&dyn Database>, ...) -> <...>;
 ...
}

---

About your second point I would like to know if the only improvement of it would be to make the TxBuild struct "lighter" or there are some other concerns about it.

---

I appreciate any feedback or opposite opinion.

[afilini]

I think I agree about passing a &dyn Database, I think most of the time the custom coin selection will just need access to the standard database interface, so that would be enough.

If you need access to your custom database to perform custom lookups, then you can take a reference and store it inside your structure.

I guess we need a couple of things first though:

• I don't think Database can be made into a trait-object, we may have to make some changes to the trait directly
• Having a thread-safe database would probably help keeping references around: we already have a getter on Wallet to get a reference to the database, but since it's wrapped in a RefCell you could easily cause a panic if you keep that around and then start a sync.

[LLFourn]

FWIW @csralvall I agree with removing Database entirely here.

My approach to coin selection in bdk_core is to not have the UTXO type be fixed. i.e. apps can make their own custom UTXO with whatever associated metadata they want as long as it confirms to some traits. I haven't implemented this yet but I think it will work out. I'm not sure how well the higher level TxBuilder API will be able to take advantage of this because it's kind of stuck with the UTXO type BDK gives you.

[notmandatory]

This is fixed in 1.0, no database parameter in CoinSelectionAlgorithm trait.