Skip to content
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

Automated coin selection #51

Closed
darosior opened this issue Oct 7, 2022 · 15 comments
Closed

Automated coin selection #51

darosior opened this issue Oct 7, 2022 · 15 comments
Assignees
Labels
Daemon / Liana library This is about lianad or the liana library (not the GUI) Feature New feature or functionality. GUI gui related

Comments

@darosior
Copy link
Member

darosior commented Oct 7, 2022

The first release of the software won't come with any automated coin selection. Users will have to pick their coins on the interface when crafting a spending transaction ("coin control"). We might want to implement some automated coin selections in the future.

If we do it, what coin selections should we implement? How should people define which one to use? I can think of a few strategies we could have:

  • Regular BnB
  • Oldest-coin (or a BnB biased toward using old coins) first to prevent them to have to rotate their coins in a dedicated transaction.
  • Regular BnB + batch in coins that are about to "expire"

Also, i don't think it's worth re-implementing the wheel. Coin selection algorithm can be tricky to implement, and have been implemented many times already. Let's just re-use and contribute review to an existing implementation. For this purpose i've asked @danielabrozzoni if she thought it could be reasonable to make @bitcoindevkit's coinselection into its own crate.

Thoughts? Is there any interest from users to have this?

@darosior darosior added the RFC Needs conceptual discussion and/or feedback from users label Oct 7, 2022
@darosior darosior modified the milestones: Minisafe v0 release, Minisafe v0 feature freeze Oct 8, 2022
@danielabrozzoni
Copy link

@evanlinjin is currently working on our coin selection implementation as a part of the bdk_core update: https://github.com/LLFourn/bdk_core_staging/tree/master/bdk_core/src/coin_select

This means that it's going to take a while before we are able to decouple it in a different repository (finishing and integrating bdk with bdk_core is currently the priority)

Oldest-coin (or a BnB biased toward using old coins)
Regular BnB + batch in coins that are about to "expire"

The BNB in bdk_core now uses custom function for the score - while in Bitcoin Core the waste metric is hardcoded into the code, in bdk you'll be able to decide if you want to use the waste metric, or supply your own scoring function (which might be "waste metric with a bias towards expiring coins"), so it should be flexible enough for this! 😄

TL;DR: I'm hyped, the new CS should be pretty cool, we will eventually (/hopefully) move it to another crate, but it's going to take time (~3 months?)

@darosior
Copy link
Member Author

darosior commented Oct 10, 2022 via email

@evanlinjin
Copy link

@darosior @danielabrozzoni The bdk_core::coin_select module is very close to completion. @LLFourn will be putting in the final touches to the API in the next few days all so. After that, we will do even more testing.

If minisafe needs it soon, we can create a stable bdk_core repository and add slowly add in finished submodules from the staging repository.

@darosior
Copy link
Member Author

Good to hear!

If minisafe needs it soon, we can create a stable bdk_core repository and add slowly add in finished submodules from the staging repository.

It's not a short-term (~2 months) priority. It could become one depending on whether it's a feature desired by our future users. So no need to rush this just for us. :)
Note also that we don't use BDK (yet?) so we'll have to wait for the coin selection to be in its own crate anyways.

@evanlinjin
Copy link

evanlinjin commented Oct 10, 2022

@darosior Awesome, we will definitely get it done before then :)

Note also that we don't use BDK (yet?) so we'll have to wait for the coin selection to be in its own crate anyways.

My understanding is that bdk_core would be in it's own repository (and would be lightweight and std-lib would be optional).

@danielabrozzoni
Copy link

we don't use BDK (yet?)

😏😉

My understanding is that bdk_core would be in it's own repository (and would be lightweight and std-lib would be optional).

Revault would still need it in a different crate tho, so that they can pull the algorithm without SpkTrackers, SparseChains, and other non-cs-related-but-still-pretty-cool stuff

@LLFourn
Copy link

LLFourn commented Oct 17, 2022

I believe that CS API will be ready for experimentation by the end of the week but with breaking changes happening till the end of the year.

So it sounds like you would like coin selection to be a separate crate. I think this is worth doing if we make sure that it only depends on rust-bitcoin and not rust-miniscript.

@darosior
Copy link
Member Author

That makes sense, and a crate depending only on rust-bitcoin (or parts of it) would be ideal for us. By the way, what MSRV are you going for?

@darosior darosior removed this from Minisafe v0 Nov 4, 2022
@darosior darosior added this to Liana v1 Nov 4, 2022
@darosior darosior removed this from the Minisafe v0 feature freeze milestone Nov 4, 2022
@darosior darosior removed this from Liana v1 Jan 16, 2023
@darosior darosior added the Feature New feature or functionality. label Feb 10, 2023
@darosior
Copy link
Member Author

darosior commented Mar 7, 2023

Hey everyone, what's the status of the coin selection in BDK / BDK core? Do you think we could start using it in say a month?

@LLFourn
Copy link

LLFourn commented Mar 8, 2023

Yeah @darosior. I put CS on the backburner to focus on other things. It's ready for usage from an API point of view. It will be zero-dependency (doesn't even need rust-bitcoin). The thing that I'm not quite happy with is that the only real "algorithm" that has been implemented is the waste metric. I don't really think it's a good choice -- it has a tendency to over consolidate. It will spend everything in your wallet if the current feerate is lower than the long term feerate. I think the best one to recommend is to just try and minimize fee while also taking into account the cost of the change within that.

[EDIT] the code in the bdk core repo is not what I'm talking about -- it's the code in one of the PRs: https://github.com/LLFourn/bdk_core_staging/tree/0331b058d64ff3acc43a3584a94173605b594b48/bdk_coin_selection/src

@darosior
Copy link
Member Author

Nice. We could have a shot at integrating it here in a branch and provide a tested review of your PR this way.

Going for a suboptimal CS algorithm at first is fine for us. And the waste metric might actually not not be unreasonable as (combined with a bias toward using older coins) it could make rotating coins seamless.

Do you have a MSRV?

@LLFourn
Copy link

LLFourn commented Mar 17, 2023

I'll work on this next week and get something that can be released. I'll try and put the MSRV to 1.48.0. "spending older coins" I think should be possible but you'd need to roll it yourself (make a branch and bound metric that gives better scores to solutions that spend older coins ).

@darosior
Copy link
Member Author

Cool!

Yeah re applying a bias to BnB that's what i meant, we'd do it ourselves of course.

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Jun 14, 2023

Hi @darosior, I can work on this issue next before continuing with #43.

@darosior darosior moved this to In Progress in Liana v2 Jul 13, 2023
@darosior darosior added GUI gui related Daemon / Liana library This is about lianad or the liana library (not the GUI) and removed RFC Needs conceptual discussion and/or feedback from users labels Jul 17, 2023
@darosior darosior moved this from In Progress to Nice to have in Liana v2 Aug 7, 2023
@darosior darosior removed this from Liana v2 Aug 29, 2023
@darosior darosior moved this to Nice to have in Liana v3 Aug 29, 2023
@darosior
Copy link
Member Author

Just for facilitating reachability, the latest state of the implementation upstream is now at bitcoindevkit/bdk#1072.

@darosior darosior moved this from Nice to have to In Progress in Liana v3 Oct 18, 2023
@darosior darosior removed this from Liana v3 Oct 24, 2023
@darosior darosior moved this to In Progress in Liana v4 Oct 24, 2023
darosior added a commit that referenced this issue Nov 15, 2023
cfa0f91 commands: auto-select coins if none provided (jp1ac4)

Pull request description:

  These are some initial changes towards #51.

  I've added a `selectcoinsforspend` command that applies BnB coin selection using a waste metric.

  This coin selection is then used in `createspend` if no coins are specified.

  @darosior The changes are still in their early stages so I'm creating this draft PR to facilitate discussion about the approach in general and specific details.

ACKs for top commit:
  darosior:
    ACK cfa0f91

Tree-SHA512: 2b94a8f4d335366e477fff54fa51d478ef459e2e729bac00a5d4ac21d04667409cb685642f27fd1936456a05a8d76d23483e45a24f5d342f9a26de904bb6639c
@edouardparis edouardparis moved this from In Progress to Done in Liana v4 Nov 20, 2023
@darosior darosior closed this as completed Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daemon / Liana library This is about lianad or the liana library (not the GUI) Feature New feature or functionality. GUI gui related
Projects
No open projects
Status: Done
Development

No branches or pull requests

5 participants