rpc: Actually use sendmany::minconf #15595

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1903-rpcWalletDummySendmany changing 7 files +73 −22
  1. 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.

  2. MarcoFalke added the label Bug on Mar 13, 2019
  3. MarcoFalke added the label Wallet on Mar 13, 2019
  4. MarcoFalke added this to the milestone 0.19.0 on Mar 13, 2019
  5. 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
  6. MarcoFalke force-pushed on Mar 13, 2019
  7. laanwj commented at 9:09 pm on March 13, 2019: member
    Concept ACK
  8. MarcoFalke force-pushed on Mar 13, 2019
  9. rpc: Actually use sendmany::minconf fae63c3ef4
  10. MarcoFalke force-pushed on Mar 13, 2019
  11. 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.

  12. 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
  13. 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

  14. 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.
  15. 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.
  16. 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.

  17. 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?
  18. 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.

  19. MarcoFalke closed this on Mar 13, 2019

  20. MarcoFalke deleted the branch on Mar 13, 2019
  21. MarcoFalke locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-25 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me