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

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

Merged
merged 2 commits into from Nov 22, 2019

Conversation

@achow101
Copy link
Member

achow101 commented Oct 28, 2019

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.

@fanquake fanquake added the Wallet label Oct 28, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 29, 2019

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.

std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(outpoint.hash);
if (it != mapWallet.end())
{
const CWalletTx& wtx = it->second;
// Clearly invalid input, fail
if (wtx.tx->vout.size() <= outpoint.n)
if (wtx.tx->vout.size() <= outpoint.n) {
bnb_used = false;

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 29, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Oct 29, 2019

Author Member

Yes. The CreateTransaction loop ends up infinitely looping without them

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 4, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Nov 4, 2019

Author Member

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 Outdated Show resolved Hide resolved
src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:all-bnb branch from a27d38e to 34a31b2 Oct 29, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member Author

achow101 commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 30, 2019

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

return false; // Not solvable, can't estimate size for fee
}
coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
value_to_select -= coin.effective_value;

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 30, 2019

Member
-            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;
+            }

This comment has been minimized.

Copy link
@achow101

achow101 Oct 30, 2019

Author Member

Changed.

@achow101 achow101 force-pushed the achow101:all-bnb branch from 34a31b2 to 7e6cf10 Oct 30, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 31, 2019

code review ACK 7e6cf10

@Sjors
Sjors approved these changes Nov 4, 2019
Copy link
Member

Sjors left a comment

Code review ACK 7e6cf10

std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(outpoint.hash);
if (it != mapWallet.end())
{
const CWalletTx& wtx = it->second;
// Clearly invalid input, fail
if (wtx.tx->vout.size() <= outpoint.n)
if (wtx.tx->vout.size() <= outpoint.n) {
bnb_used = false;

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 4, 2019

Member

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

@@ -2674,6 +2678,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const
{
std::vector<COutput> vCoins(vAvailableCoins);
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)

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 4, 2019

Member

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

This comment has been minimized.

Copy link
Member

instagibbs commented Nov 4, 2019

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.

MarcoFalke added a commit that referenced this pull request Nov 15, 2019
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
Copy link
Member

meshcollider left a comment

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 Outdated Show resolved Hide resolved
@achow101 achow101 force-pushed the achow101:all-bnb branch from 7e6cf10 to b007efd Nov 20, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Nov 20, 2019

reACK b007efd

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 22, 2019

re-ACK b007efd

meshcollider added a commit that referenced this pull request Nov 22, 2019
…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
@meshcollider meshcollider merged commit b007efd into bitcoin:master Nov 22, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 22, 2019
…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
fanquake added a commit that referenced this pull request Dec 1, 2019
…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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 1, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.