bitcoindevkit / coin-select

Tool to help you select inputs for making bitcoin transactions.
Other
12 stars 6 forks source link

Simplify `DrainWeights`/`Target` API #1

Closed evanlinjin closed 5 months ago

evanlinjin commented 9 months ago

Original discussion: https://github.com/bitcoindevkit/bdk/pull/1072#issuecomment-1802704053

@jp1ac4 @darosior I'm working on some breaking changes to the API.

After a discussion with @LLFourn, we've decided to include DrainWeights in Target and keep Target in the CoinSelector. This simplifies the API and gives us an easy pathway to take into account the output count varint weight changes when including the drain output.

How this simplifies the API:

How we can take into account the output varint weight changes when including drain:

pub struct Target {
    /*other fields*/

    pub base_weight: u32,
    pub drain_weights: DrainWeights,
}

impl Target {
    pub fn new(
        feerate: FeeRate,
        recipient_outputs: impl IntoIterator<Item = (u32 /*weight*/, u64 /*value*/)>
        drain_outputs: impl IntoIterator<Item = DrainWeights>,
    ) -> Self {
        todo!()
    }
}

This calculates the base_weight and the drain_weight (which includes output varint changes) that are both included in Target.

evanlinjin commented 9 months ago

@LLFourn's thoughts (https://github.com/bitcoindevkit/bdk/pull/1072#issuecomment-1804957404):

So in my mind Target and DrainWeights would still be separate things just both exposed and settable publicly. Target would include the number of outputs and DrainWeights would include the number of outputs in the draining output. I also think you need to include the "base weight" in Target as it is coupled to the number of outputs.

Now I thought further -- if we create this invariant I think this also allows us to reduce the change_policy to a single integer -- i.e. the threshold of excess coin which should trigger the inclusion of the drain output. Wdyt @evanlinjin?

LLFourn commented 8 months ago

Note I did the change policy as a single value in #14