Add Single Random Draw as an additional coin selection algorithm #17526

achow101 · 2019-11-19T22:47:59Z

To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared.

achow101 · 2019-11-19T22:49:38Z

Some simulations results: https://gist.github.com/achow101/edf6b5e308035a489fbb1f28d12e2109

I think the important thing to see here is that this is the same or better in every metric except for "mean UTXOs". But I think the thing to note here is that it's about the same mean UTXOs as the positive only effective value simulation. So I think the different there is primarily due to dust outputs that aren't being consumed.

DrahtBot · 2019-11-20T02:17:12Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

murchandamus · 2021-09-06T16:18:28Z

Any simulations/results/intuitions on how often each of the algorithms are likely to be picked?

Not currently. Perhaps @xekyo has something?

That depends heavily on the amount and value diversity of the UTXOs that are available. I've seen an exchange wallet do 93% BnB transactions across a timeframe of four weeks just after enabling BnB while they were chewing through a large UTXO pool. The same wallet later settled around 55-65%, IIRC. I'd guess most enterprises with a send-and-receive hot wallet would probably fall into the range of 30-60%, send-only enterprise wallets probably more like 5-15%. On the higher end of that range if they're batching payments since BnB solutions are easier to find for larger amounts.

On an end-user wallet I would expect 0-15% BnB solutions, especially on the lower end if it often sends at extremely low feerates since that shrinks the exact match window (as the cost of change is less) and tends to consolidate the available UTXO pool further.

For some of the tests within rpc_fundrawtx, there is the expectation that two independent calls to coin selection RPCs will use the same type of UTXO. This is not necessarily guaranteed, so to make sure it is, use lockunspent prior to those tests.

For the test that checks that there is no error when change is unavailable but change is also not needed, use specific UTXOs so that SRD does not cause this to fail when it chooses random inputs.

murchandamus · 2021-09-09T19:50:52Z

since I believe the argument is change-less solutions are simply only preferred after this in an economic sense, not privacy/ending the chaining of change way.

Given that the change output has a big negative impact on the waste score, the BnB solution should always be preferred, if it has the same count of inputs (given the same type). Due to usually needing to combine multiple inputs in order to arrive at a specific amount and the deterministic search path going from highest effective value to lowest, BnB solutions tend to have a small count of 2 or more inputs.

Especially in a cluttered wallet, SRD or Knapsack often find a larger solution that may be preferred at low feerates even when it produces a change output. At higher feerates, especially SRD can find the occasional lucky single input solutions that would might have a lower waste score. This may result in slightly fewer BnB solutions than before, but only in cases where the BnB solution was less cost effective.

Given the high estimated cost of change outputs, I'd say that we already have a decent handicap on solutions producing change. It would be possible to further dissuade change, but I'd consider it an open question how to pick the "correct" handicap.

achow101 · 2021-09-13T18:33:41Z

I've ran simulations (using the same data) on a recent commit of master as well as on this PR. I've uploaded the results to a gist for master and for this PR.

The main result from this particular simulation is that with SRD, less inputs are being consumed overall (mean # of utxos is higher), but fees are slightly lower. SRD ended up being used ~1/3 of the time, BnB ~1/5, and Knapsack the rest.

murchandamus · 2021-09-15T16:33:41Z

I've ran simulations (using the same data) on a recent commit of master as well as on this PR. I've uploaded the results to a gist for master and for this PR.

The main result from this particular simulation is that with SRD, less inputs are being consumed overall (mean # of utxos is higher), but fees are slightly lower. SRD ended up being used ~1/3 of the time, BnB ~1/5, and Knapsack the rest.

Let me try to cast the results slightly differently: the variant with SRD reduces fees by 6% and while that variant spends 296 fewer inputs those seem to be largely accounted for by it creating 255 fewer change outputs in the first place. So yes, the mean UTXO pool is a bit larger, but in effect it also creates 4.4% fewer change outputs in the first place. Not creating so many UTXOs in the first place seems great to me!

achow101 · 2021-09-15T22:20:52Z

I redid the simulations with some fixes to how data is gathered and gathered a lot more data. The original simulation was accidentally incorrectly counting usage due to the double selection that occurs for the optimistic avoidpartialspends. I am now also measuing how many uneconomical UTXOs are being spent, and how many changeless txs of each algorithm produces. The aforementioned gist has been updated with the new measurements.

With the new measuring, we see that SRD produces a lot more BnB solutions and ends up spending almost no uneconomical outputs (which would explain the significant increase in UTXOs in the wallet).

glozow

code review ACK 981b9d1

I wanted to see if there were any other tests that could be broken due to relying on deterministic coin selection behavior, so I edited AttemptSelection to always return the SRD solution; all the functional tests passed.

src/wallet/coinselection.h

test/functional/rpc_fundrawtransaction.py

Don't assume that specific inputs are going to be used when they aren't specified explicitly. Also fixes a bug in the include_unsafe test where after the inputs confirm, include_unsafe should be set to False rather than True.

Instead of relying on coin selection to deterministically choose the correct inputs to use, just specify them explicitly and use the raw transaction RPCs.

To avoid accidentally spending UTXOs that are needed later in the test, lock those UTXOs after they're creation.

Try to find a solution with SelectCoinsSRD. If we do have one, add it to the list of solutions from which we choose the one with the least waste as the solution to use.

glozow · 2021-09-24T09:29:17Z

reACK 3633b66 via git range-diff 981b9d1...3633b66, thanks for taking the suggestions

meshcollider

Code review ACK 8bf789b

I like that this coin selection algorithm is only 20 lines 😄

meshcollider · 2021-09-26T02:50:29Z

src/wallet/coinselection.cpp

+    std::set<CInputCoin> out_set;
+    CAmount value_ret = 0;
+
+    std::vector<size_t> indexes;


nit: indices > indexes :)

laanwj · 2021-09-27T16:22:04Z

Concept and code review ACK 3633b66

maflcko · 2021-09-28T14:15:55Z

Does this cause the intermittent wallet_basic issues?

 test  2021-09-28T12:06:48.167000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_basic.py", line 500, in run_test
                                       assert_fee_amount(fee, tx_size, Decimal(fee_rate_btc_kvb))
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 41, in assert_fee_amount
                                       raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
                                   AssertionError: Fee of 0.00000255 BTC too low! (Should be 0.00000256 BTC)

https://cirrus-ci.com/task/6110688373374976?logs=ci#L3915

achow101 · 2021-09-28T16:10:18Z

Does this cause the intermittent wallet_basic issues?

Potentially. The main thing is that this change makes coin selection non-deterministic which means that tests which rely on assuming certain UTXOs are being selected are no longer correct. I thought I got all of the tests that did that, but it is possible that a few were missed.

S3RK · 2021-09-30T07:43:00Z

Post merge ACK 3633b66

Reviewed code and independently reproduced simulation results. Confirmed that adding SRD increases amount of changeless solutions.

maflcko · 2022-03-22T08:36:59Z

I think this picked up #13307 (comment), so I removed the "up for grabs" label there.

fanquake added the Wallet label Nov 19, 2019

Sjors mentioned this pull request Nov 20, 2019

High level road map for coin selection changes #12605

Closed

DrahtBot added the Needs rebase label Nov 22, 2019

achow101 force-pushed the srd-fallback-2 branch from 23fcc1b to c11d14b Compare November 22, 2019 21:50

DrahtBot removed the Needs rebase label Nov 22, 2019

achow101 force-pushed the srd-fallback-2 branch from c11d14b to 72f58ae Compare November 25, 2019 21:12

DrahtBot added the Needs rebase label Dec 1, 2019

achow101 force-pushed the srd-fallback-2 branch from 72f58ae to e417551 Compare December 2, 2019 15:15

DrahtBot removed the Needs rebase label Dec 2, 2019

DrahtBot mentioned this pull request Feb 20, 2020

Replace current benchmarking framework with nanobench #18011

Merged

This was referenced Mar 6, 2020

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

Merged

rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure #18274

Merged

DrahtBot mentioned this pull request Mar 13, 2020

Use effective values throughout coin selection #17331

Merged

DrahtBot mentioned this pull request Mar 25, 2020

Use shared pointers only in validation interface #18354

Closed

laanwj added the Feature label Mar 27, 2020

DrahtBot mentioned this pull request Mar 28, 2020

WIP NOMERGE [bench] gitian builds for OP_IF bench #18352

Closed

DrahtBot added the Needs rebase label Apr 17, 2020

achow101 force-pushed the srd-fallback-2 branch from e417551 to 89060d2 Compare April 19, 2020 21:26

DrahtBot removed the Needs rebase label Apr 19, 2020

DrahtBot mentioned this pull request Apr 19, 2020

wallet: Avoid translating RPC errors #18699

Merged

bitschmidty mentioned this pull request Sep 7, 2021

Newsletters: add 165 (2021-09-08) bitcoinops/bitcoinops.github.io#639

Merged

5 tasks

DrahtBot added the Needs rebase label Sep 9, 2021

achow101 added 2 commits September 9, 2021 15:07

tests: rpc_fundrawtx use specific inputs for unavailable change test

a165bfb

For the test that checks that there is no error when change is unavailable but change is also not needed, use specific UTXOs so that SRD does not cause this to fail when it chooses random inputs.

achow101 force-pushed the srd-fallback-2 branch from 397eccc to 981b9d1 Compare September 9, 2021 19:50

DrahtBot removed the Needs rebase label Sep 9, 2021

DrahtBot mentioned this pull request Sep 10, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

glozow reviewed Sep 23, 2021

View reviewed changes

src/wallet/coinselection.h Outdated Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

achow101 added 5 commits September 23, 2021 13:32

tests: wallet_txn explicilty specify inputs

b77885f

Instead of relying on coin selection to deterministically choose the correct inputs to use, just specify them explicitly and use the raw transaction RPCs.

tests: wallet_basic lock needed unspents

2ad3b5d

To avoid accidentally spending UTXOs that are needed later in the test, lock those UTXOs after they're creation.

Add SelectCoinsSRD function

8bf789b

Use SelectCoinsSRD if it has less waste

3633b66

Try to find a solution with SelectCoinsSRD. If we do have one, add it to the list of solutions from which we choose the one with the least waste as the solution to use.

achow101 force-pushed the srd-fallback-2 branch from 981b9d1 to 3633b66 Compare September 23, 2021 17:33

meshcollider reviewed Sep 26, 2021

View reviewed changes

fanquake requested a review from instagibbs September 27, 2021 12:56

meshcollider merged commit d5d0a5c into bitcoin:master Sep 28, 2021

maflcko mentioned this pull request Sep 28, 2021

Intermittent CI failure: "AssertionError: Fee of 0.00011000 BTC too low!" when running wallet_basic.py #19418

Closed

bitcoin locked and limited conversation to collaborators Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Single Random Draw as an additional coin selection algorithm #17526

Add Single Random Draw as an additional coin selection algorithm #17526

achow101 commented Nov 19, 2019 •

edited

Loading

achow101 commented Nov 19, 2019

DrahtBot commented Nov 20, 2019 •

edited

Loading

murchandamus commented Sep 6, 2021 •

edited

Loading

murchandamus commented Sep 9, 2021

achow101 commented Sep 13, 2021

murchandamus commented Sep 15, 2021

achow101 commented Sep 15, 2021

glozow left a comment

glozow commented Sep 24, 2021

meshcollider left a comment

meshcollider Sep 26, 2021

laanwj commented Sep 27, 2021

maflcko commented Sep 28, 2021

achow101 commented Sep 28, 2021

S3RK commented Sep 30, 2021

maflcko commented Mar 22, 2022

Add Single Random Draw as an additional coin selection algorithm #17526

Add Single Random Draw as an additional coin selection algorithm #17526

Conversation

achow101 commented Nov 19, 2019 • edited Loading

achow101 commented Nov 19, 2019

DrahtBot commented Nov 20, 2019 • edited Loading

Conflicts

murchandamus commented Sep 6, 2021 • edited Loading

murchandamus commented Sep 9, 2021

achow101 commented Sep 13, 2021

murchandamus commented Sep 15, 2021

achow101 commented Sep 15, 2021

glozow left a comment

Choose a reason for hiding this comment

glozow commented Sep 24, 2021

meshcollider left a comment

Choose a reason for hiding this comment

meshcollider Sep 26, 2021

Choose a reason for hiding this comment

laanwj commented Sep 27, 2021

maflcko commented Sep 28, 2021

achow101 commented Sep 28, 2021

S3RK commented Sep 30, 2021

maflcko commented Mar 22, 2022

achow101 commented Nov 19, 2019 •

edited

Loading

DrahtBot commented Nov 20, 2019 •

edited

Loading

murchandamus commented Sep 6, 2021 •

edited

Loading