bench: Fix bug in coin selection retry logic #13549

practicalswift commented at 4:54 PM on June 27, 2018: contributor

Fix bug in coin selection retry logic.

in src/bench/coin_selection.cpp:53 in cab56e69c9 outdated

  47 | @@ -48,8 +48,10 @@ static void CoinSelection(benchmark::State& state)
  48 |          bool bnb_used;
  49 |          CoinEligibilityFilter filter_standard(1, 6, 0);
  50 |          CoinSelectionParams coin_selection_params(false, 34, 148, CFeeRate(0), 0);
  51 | -        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)
  52 | -                       || wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
  53 | +        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
  54 | +        if (!success) {
  55 | +            success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);

Xekyo commented at 5:28 PM on June 27, 2018:

Perhaps it should also be clarified why the two are expected to have different results given that they have exactly the same input parameters. I assume that the first pass sets bnb_used to true, but @achow101 should be able to clarify.

achow101 commented at 6:17 PM on June 27, 2018:

This seems like a mistake. The first pass should be using BnB with its own CoinSelectionParams which signals for BnB to be used.

practicalswift commented at 7:49 PM on June 27, 2018:

@achow101 What is the correct CoinSelectionParams for the first pass? :-)

achow101 commented at 9:02 PM on June 27, 2018:

CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0);

practicalswift commented at 9:39 PM on June 27, 2018:

@achow101 Thanks! Please re-review updated version :-)

Xekyo commented at 5:29 PM on June 27, 2018: member

LGTM, I suggest that @achow101 also takes a look.

bench: Fix bug in coin selection retry logic 3131240670

practicalswift force-pushed on Jun 27, 2018

practicalswift renamed this:
~~bench: Clarify intended coin selection retry logic~~
bench: Fix bug in coin selection retry logic
on Jun 27, 2018

achow101 commented at 9:47 PM on June 27, 2018: member

utACK 313124067009e372a7d3726b6286d0001cd3ba78

fanquake added the label Tests on Jun 27, 2018

DrahtBot commented at 9:29 AM on June 28, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#13307 (Replace coin selection fallback strategy with Single Random Draw by achow101)
#12257 ([wallet] Use destination groups instead of coins in coin select by kallewoof)

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.

in src/bench/coin_selection.cpp:53 in 3131240670

  51 | -        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)
  52 | -                       || wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
  53 | +        CoinSelectionParams coin_selection_params_pass_1(true, 34, 148, CFeeRate(0), 0);
  54 | +        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_pass_1, bnb_used);
  55 | +        if (!success) {
  56 | +            CoinSelectionParams coin_selection_params_pass_2(false, 34, 148, CFeeRate(0), 0);

promag commented at 10:19 AM on June 28, 2018:

Looks like this is a 2 pass algorithm 😛

Why not just coin_selection_params.use_bnb = false; and keep the same params?

MarcoFalke commented at 10:31 AM on June 28, 2018:

sucess is never false, so this is dead code?

promag commented at 10:36 AM on June 28, 2018:

You mean it should never be false as there is enough coins?

MarcoFalke commented at 10:39 AM on June 28, 2018:

Yes, otherwise the benchmark would currently fail, no?

achow101 commented at 6:00 PM on June 28, 2018:

success can be false when BnB fails to find an exact match. The first pass should be using BnB (which is why it needs its own parameters with true set) which can fail. Then it falls back to not using BnB (which is the second params with false) which should never fail.

promag commented at 6:05 PM on June 28, 2018:

But given the current input it always succeeds. I agree that a different test exercising that case would be nice.

achow101 commented at 6:36 PM on June 28, 2018:

Ah, indeed. This should have different test cases to trigger both algorithms.

in src/bench/coin_selection.cpp:55 in 3131240670

  53 | +        CoinSelectionParams coin_selection_params_pass_1(true, 34, 148, CFeeRate(0), 0);
  54 | +        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_pass_1, bnb_used);
  55 | +        if (!success) {
  56 | +            CoinSelectionParams coin_selection_params_pass_2(false, 34, 148, CFeeRate(0), 0);
  57 | +            success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_pass_2, bnb_used);
  58 | +        }

promag commented at 10:21 AM on June 28, 2018:

nit, inside the retry block:

    assert(!bnb_used);
}

promag commented at 10:22 AM on June 28, 2018: member

utACK 3131240.

MarcoFalke commented at 10:35 AM on June 28, 2018: member

Everything other than SelectCoinsMinConf can be moved out of the hot loop, since we don't want to measure the performance of addCoin. (Looks like creating and deleting the wallet takes about 20% of the whole benchmark time)

MarcoFalke closed this on Jun 28, 2018

MarcoFalke referenced this in commit b330f3fdd5 on Jun 28, 2018

practicalswift deleted the branch on Apr 10, 2021

UdjinM6 referenced this in commit d46ef93f4b on Jun 20, 2021

UdjinM6 referenced this in commit 08345e5120 on Jun 24, 2021

UdjinM6 referenced this in commit e1bb644601 on Jun 26, 2021

UdjinM6 referenced this in commit d4a6c09f5c on Jun 26, 2021

UdjinM6 referenced this in commit aa2fbaf398 on Jun 26, 2021

UdjinM6 referenced this in commit 68e4e5b5fa on Jun 28, 2021

gades referenced this in commit 9d6fe3f902 on May 1, 2022

DrahtBot locked this on Aug 16, 2022