Return Iterator instead of Vector for SRD

[yancyribbens]

Return Iterator instead of Vector for SRD.

The caller may prefer to work with an Iterator instead of a vec. If the caller prefers a vec, they can use collect(). This is the first commit to close #31

[Kixunil]

What? Why is this not used?

[yancyribbens]

It's only used by bnb, not srd and frankly the current version of bnb that's in master is broken so I just commented it out. I'm still waiting to get some review on #30, especially those that created issues.

[Kixunil]

Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument?

[yancyribbens]

I should have marked this draft since it's not really intended to merge, at least not until the bnb pr merges.

Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument?

As in bitcoin core, bnb uses cost of change to determine if an exact match can be found where the match is not greater than the target + cost of creating a change output (cost_of_change). However, if bnb can't find an exact match the fallback strategy is to use SRD (single random draw) to choose any selection which will surely create a change output, so the cost_of_change isn't needed in the fallback scenario.

…

On Thu, Feb 15, 2024 at 03:28:40PM -0800, Martin Habovštiak wrote: @Kixunil commented on this pull request. > @@ -59,15 +59,11 @@ pub struct WeightedUtxo { #[cfg_attr(docsrs, doc(cfg(feature = "rand")))] pub fn select_coins<T: Utxo>( target: Amount, - cost_of_change: u64, + _cost_of_change: u64, Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument? -- Reply to this email directly or view it on GitHub: #33 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[yancyribbens]

It looks like I'm remembering wrong actually. SRD does use cost_of_change since it should always need to create a change output, so the target should include the cost_of_change. Looks like I'm using the crate const CHANGE_LOWER. oops should probably be consistent and either use CHANGE_LOWER in all algos or pass cost_of_change as an arg.

…

On Thu, Feb 15, 2024 at 03:28:40PM -0800, Martin Habovštiak wrote: @Kixunil commented on this pull request. > @@ -59,15 +59,11 @@ pub struct WeightedUtxo { #[cfg_attr(docsrs, doc(cfg(feature = "rand")))] pub fn select_coins<T: Utxo>( target: Amount, - cost_of_change: u64, + _cost_of_change: u64, Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument? -- Reply to this email directly or view it on GitHub: #33 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[yancyribbens]

Nope, looks like I was right the first time. CHANGE_LOWER is used to avoid excessively small amounts of change, so the target is increased by that amount. Although, it looks like a bug was found in core since I last looked where if the cost_of_change is greater than CHANGE_LOWER then there's not enough to pay for creating the change output and so none is created. That needs to be added in this crate as well.

…

On Thu, Feb 15, 2024 at 03:28:40PM -0800, Martin Habovštiak wrote: @Kixunil commented on this pull request. > @@ -59,15 +59,11 @@ pub struct WeightedUtxo { #[cfg_attr(docsrs, doc(cfg(feature = "rand")))] pub fn select_coins<T: Utxo>( target: Amount, - cost_of_change: u64, + _cost_of_change: u64, Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument? -- Reply to this email directly or view it on GitHub: #33 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[yancyribbens]

Anyway, I'm primarily trying to decide on an interface to make "stable". It's either: 1) ```rust pub fn select_coins_srd<'a, R: rand::Rng + ?Sized>( target: Amount, fee_rate: FeeRate, weighted_utxos: &'a mut [WeightedUtxo], rng: &mut R, ) -> Option<impl Iterator<Item = &'a WeightedUtxo>> { ``` or 2) ```rust pub fn select_coins_srd<'a, R: rand::Rng + ?Sized>( target: Amount, fee_rate: FeeRate, weighted_utxos: &'a mut [&'a WeightedUtxo], rng: &mut R, ) -> Option<impl Iterator<Item = &'a WeightedUtxo>> { ``` I think the second actually is the most flexible for the consumer although I'm not sure. Either would probably be fine, just don't want to need to change it in the future.

…

On Thu, Feb 15, 2024 at 03:28:40PM -0800, Martin Habovštiak wrote: @Kixunil commented on this pull request. > @@ -59,15 +59,11 @@ pub struct WeightedUtxo { #[cfg_attr(docsrs, doc(cfg(feature = "rand")))] pub fn select_coins<T: Utxo>( target: Amount, - cost_of_change: u64, + _cost_of_change: u64, Weird that it doesn't take into account the cost of change (why would anyone want such algo then?) anyway, just remove the argument? -- Reply to this email directly or view it on GitHub: #33 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[yancyribbens]

moved changes to #30