Enable BnB coin selection for preset inputs and subtract fee from outputs #17290

achow101 · 2019-10-28T22:18:46Z

Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases.

Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too.

DrahtBot · 2019-10-29T00:33:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17458 (Refactor OutputGroup effective value calculations and filtering to occur within the struct by achow101)
#17331 (Use effective values throughout coin selection by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

instagibbs · 2019-10-29T20:17:13Z

src/wallet/wallet.cpp

are these bnb_used lines really necessary? We aren't using knapsack solver either on SelectCoins failure

Yes. The CreateTransaction loop ends up infinitely looping without them

I find it a bit concerning that no test breaks when I remove this: Sjors@398877e

Since the same boolean variable is used throughout the tests, it carries over whatever value it had previously. We are expecting SelectCoins and SelectCoinsMinConf to modify it.

src/wallet/wallet.cpp

src/wallet/test/coinselector_tests.cpp

instagibbs · 2019-10-30T13:32:06Z

This change might have broken wallet_bumpfee.py somewhat randomly: https://travis-ci.org/bitcoin/bitcoin/jobs/604712627?utm_medium=notification&utm_source=github_status

N.B. bumpfee uses preselected inputs to create the transaction, so it might be the case that BnB ended up "hitting" where it previously hadn't and got rid of the change output earlier than expected in the test.

achow101 · 2019-10-30T16:19:13Z

This change might have broken wallet_bumpfee.py somewhat randomly:

I'm not sure why it would fail randomly since BnB should be deterministic. I also can't reproduce this at all..

instagibbs · 2019-10-30T17:23:32Z

Honestly I do not understand the error either. It seems either the original input was dropped somehow, or that it picked up additional inputs too early. I'll try to reproduce locally.

instagibbs · 2019-10-30T17:52:43Z

ok I'm able to reproduce consistently.... it's adding 15 inputs for some reason when it shouldn't be adding any. investigating

instagibbs · 2019-10-30T18:21:36Z

src/wallet/wallet.cpp

- value_to_select -= coin.effective_value; + if (coin_selection_params.use_bnb) { + value_to_select -= coin.effective_value; + } else { + value_to_select -= coin.txout.nValue; + }

instagibbs · 2019-10-31T14:23:09Z

code review ACK 7e6cf10

Sjors

Code review ACK 7e6cf10

Sjors · 2019-11-04T11:48:38Z

src/wallet/wallet.cpp

I find it a bit concerning that no test breaks when I remove this: Sjors@398877e

Sjors · 2019-11-04T12:37:32Z

src/wallet/wallet.cpp

+    CAmount value_to_select = nTargetValue;

    // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
    if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)


Would be nice to cover this case as well (used by the GUI and #16377), where coins are selected, but no extra inputs are allowed. Can wait for a followup though.

instagibbs · 2019-11-04T14:04:12Z

I find it a bit concerning that no test breaks when I remove this

That's a sanity check where the transaction in the wallet somehow doesn't match the coin requested. Maybe it warrants a log print statement, but that can be done later.

38516f9 Fix input size assertion in wallet_bumpfee.py (Gregory Sanders) Pull request description: I was investigating a curious error for #17290 and realized that this check should have caught that error earlier in the test. The loop is intended to ensure that only a single input exists the entire time until the change output disappears, a single additional bump occurs, then it leaves the loop. Top commit has no ACKs. Tree-SHA512: 1d2d6ef535ec2c55f516ee5de11352386ceac6bedaabc6842229a486d9f28d35310ad5f57bfcc1f1e654fc397ecff29ec33256f9b3da897500b7e1635004b63a

meshcollider

very light code review ACK 7e6cf10

Do not treat my review for much, I am not experienced with coin selection and am only providing limited experience when looking at this code. But it looks ok to me and I trust Sjors and instagibbs' reviews.

src/wallet/test/coinselector_tests.cpp

instagibbs · 2019-11-20T17:14:53Z

reACK b007efd

Sjors · 2019-11-22T09:12:56Z

re-ACK b007efd

…t fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df

…subtract fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df

…eeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: #17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally. Apparently this particular case doesn't have a test, so I've added one as well. ACKs for top commit: Sjors: ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test. fanquake: ACK eadd130 instagibbs: utACK eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

…btractFeeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: bitcoin#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally. Apparently this particular case doesn't have a test, so I've added one as well. ACKs for top commit: Sjors: ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test. fanquake: ACK eadd130 instagibbs: utACK bitcoin@eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

…puts Summary: * Use BnB when preset inputs are selected * Allow BnB when subtract fee from outputs This is a backport of Core [[bitcoin/bitcoin#17290 | PR17290]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7674

…subtract fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df

…btractFeeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: bitcoin#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally. Apparently this particular case doesn't have a test, so I've added one as well. ACKs for top commit: Sjors: ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test. fanquake: ACK eadd130 instagibbs: utACK bitcoin@eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

fanquake added the Wallet label Oct 28, 2019

instagibbs reviewed Oct 29, 2019

View reviewed changes

achow101 force-pushed the all-bnb branch from a27d38e to 34a31b2 Compare October 29, 2019 23:02

instagibbs mentioned this pull request Oct 30, 2019

Fix input size assertion in wallet_bumpfee.py #17322

Merged

instagibbs reviewed Oct 30, 2019

View reviewed changes

achow101 force-pushed the all-bnb branch from 34a31b2 to 7e6cf10 Compare October 30, 2019 18:32

achow101 mentioned this pull request Oct 31, 2019

Use effective values throughout coin selection #17331

Merged

Sjors mentioned this pull request Nov 4, 2019

wallet: avoid knapsack when there's no change #17246

Closed

Sjors approved these changes Nov 4, 2019

View reviewed changes

meshcollider reviewed Nov 20, 2019

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request Nov 20, 2019

High level road map for coin selection changes #12605

Closed

achow101 added 2 commits November 20, 2019 12:12

Use BnB when preset inputs are selected

db15e71

Allow BnB when subtract fee from outputs

b007efd

achow101 force-pushed the all-bnb branch from 7e6cf10 to b007efd Compare November 20, 2019 17:12

meshcollider merged commit b007efd into bitcoin:master Nov 22, 2019

achow101 mentioned this pull request Nov 23, 2019

wallet: fix when sufficient preset inputs and subtractFeeFromOutputs #17568

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Enable BnB coin selection for preset inputs and subtract fee from outputs #17290

Enable BnB coin selection for preset inputs and subtract fee from outputs #17290

Uh oh!

Conversation

achow101 commented Oct 28, 2019

Uh oh!

DrahtBot commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Oct 30, 2019

Uh oh!

achow101 commented Oct 30, 2019

Uh oh!

instagibbs commented Oct 30, 2019

Uh oh!

instagibbs commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Oct 31, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 4, 2019

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Nov 20, 2019

Uh oh!

Sjors commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Oct 29, 2019 •

edited

Loading