rpc: Actually use sendmany::minconf #15595

MarcoFalke commented at 8:44 PM on March 13, 2019: member

For the entirety of its history sendmany::minconf has always been ignored for the purpose of creating the transaction. It has only been used to exit early if "the balance confirmed at least this many times" is less than the total amount to send.

Fix that by actually passing it down via coincontrol.

This is a bugfix, but not a regression, so can go into 0.19.0.

MarcoFalke added the label Bug on Mar 13, 2019

MarcoFalke added the label Wallet on Mar 13, 2019

MarcoFalke added this to the milestone 0.19.0 on Mar 13, 2019

MarcoFalke commented at 8:48 PM on March 13, 2019: member

This can easily be verified by the test changes:

The test wallet_fallbackfee.py fails when minconf is not set to 0 (1 is the default)
The new test fails with a bitcoind compiled on master

MarcoFalke force-pushed on Mar 13, 2019

laanwj commented at 9:09 PM on March 13, 2019: member

Concept ACK

MarcoFalke force-pushed on Mar 13, 2019

rpc: Actually use sendmany::minconf fae63c3ef4

MarcoFalke force-pushed on Mar 13, 2019

DrahtBot commented at 9:49 PM on March 13, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13057 (refactor pre-selected coin code by instagibbs)

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/wallet/wallet.cpp:2604 in fae63c3ef4

2606 | -        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2607 | -        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2608 | -        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2609 | -        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2610 | -        (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
2611 | +               SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(conf(1), conf(6), 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||

promag commented at 10:00 PM on March 13, 2019:

How about pass m_min_conf_depth to a new CoinEligibilityFilter member and update OutputGroup::EligibleForSpending?

MarcoFalke commented at 10:02 PM on March 13, 2019:

That would be a very invasive change for a simple bugfix and would complicate the CoinEligibilityFilter constructor/interface unnecessarily

in src/wallet/rpcwallet.cpp:934 in fae63c3ef4

 926 | @@ -929,11 +927,6 @@ static UniValue sendmany(const JSONRPCRequest& request)
 927 |  
 928 |      EnsureWalletIsUnlocked(pwallet);
 929 |  
 930 | -    // Check funds
 931 | -    if (totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth)) {
 932 | -        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds");

promag commented at 10:02 PM on March 13, 2019:

There was no test for this right?

MarcoFalke commented at 10:06 PM on March 13, 2019:

No, we are far away from test coverage in rpc/wallet.

However, there was (and still is) test coverage for the check a few lines down RPC_WALLET_INSUFFICIENT_FUNDS

gmaxwell commented at 10:13 PM on March 13, 2019: contributor

Am I misreading this, or does it change the behaviour of send many to no longer use your own unconfirmed change, by default? That would be a pretty massive change.

gmaxwell commented at 10:16 PM on March 13, 2019: contributor

Aside, I am pretty confident that the reason for the minconf filtering in the first place was related to accounts. Imagine you are running a shared webwallet where each user was an account. When you send you specify the account and don't want to allow the user to spend more than their X-deep-confirmed balance or else you take a risk that they yank those coins back in a reorg after withdrawing different coins.

MarcoFalke commented at 10:21 PM on March 13, 2019: member

@gmaxwell I don't think that changes, because previously it would exit early due to the GetLegacyBalance(min_depth=1) check if you don't have the enough coins confirmed.

However, I am more than happy to drop this parameter and bring it in line with other RPCs such as sendtoaddress, which never take the option.

gmaxwell commented at 10:26 PM on March 13, 2019: contributor

So are you saying that currently, if you have a single 10 BTC txout then pay someone 1 BTC, you will be unable to make any further payments with sendmany until that first payment confirms? If so, when did that behaviour change?

MarcoFalke commented at 10:31 PM on March 13, 2019: member

Sorry you are right.

It would even spend unconfirmed coins, so I guess #15596 is the way to go.

MarcoFalke closed this on Mar 13, 2019

MarcoFalke deleted the branch on Mar 13, 2019

MarcoFalke locked this on Dec 16, 2021