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

Add all sqlite coin chooser, enabled by setting coin_selection_strategy to sqlite #2959

Merged
merged 7 commits into from
Jun 8, 2020

Conversation

jackrobison
Copy link
Member

No description provided.

@kauffj kauffj marked this pull request as draft May 19, 2020 15:07
@jackrobison jackrobison force-pushed the sqlite-coin-chooser branch 6 times, most recently from c4736c4 to 2490cd8 Compare May 19, 2020 22:43
@jackrobison jackrobison marked this pull request as ready for review May 27, 2020 06:11
@kauffj kauffj requested a review from eukreign May 27, 2020 15:12
@lbry-bot lbry-bot assigned eukreign and unassigned jackrobison May 27, 2020
Copy link
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will take too long to review thoroughly: if the PR fixes a problem and the tests pass we can merge it. The long term solution is to implement a coin chooser in PL/SQL to match behavior of the existing branching selection algorithm; that will have to wait until after the postgresql refactor. This PR may be a great interim solution!

@lbry-bot lbry-bot assigned jackrobison and unassigned eukreign Jun 1, 2020
@tzarebczan
Copy link
Contributor

This also does not take into account unconfirmed utxos (even though the commit says so!)

@eukreign
Copy link
Member

eukreign commented Jun 1, 2020

If there are concerns about this selection algorithm not being as complete as the others then perhaps it should not be the default and it can then be explicitly configured in cases where performance is slow?

Copy link
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion: this should not be the default coin chooser.

@jackrobison jackrobison force-pushed the sqlite-coin-chooser branch 2 times, most recently from 28e57ee to 6ccdf52 Compare June 4, 2020 14:22
@eukreign eukreign merged commit e70bdd8 into master Jun 8, 2020
@eukreign eukreign deleted the sqlite-coin-chooser branch June 8, 2020 22:46
@jackrobison jackrobison added area: wallet type: new feature New functionality that does not exist yet labels Jun 8, 2020
@jackrobison jackrobison changed the title Add all sqlite coin chooser Add all sqlite coin chooser, enabled by setting coin_selection_strategy to sqlite Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: wallet type: new feature New functionality that does not exist yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants