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

rpc: Actually use sendmany::minconf #15595

Closed

Conversation

Projects
None yet
5 participants
@MarcoFalke
Copy link
Member

MarcoFalke commented Mar 13, 2019

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 MarcoFalke added this to the 0.19.0 milestone Mar 13, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 13, 2019

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 MarcoFalke force-pushed the MarcoFalke:1903-rpcWalletDummySendmany branch from fa0a712 to fa4fcf9 Mar 13, 2019

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 13, 2019

Concept ACK

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1903-rpcWalletDummySendmany branch from fa4fcf9 to fad8952 Mar 13, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1903-rpcWalletDummySendmany branch from fad8952 to fae63c3 Mar 13, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Mar 13, 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:

  • #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.

(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)) ||
(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)) ||
(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));
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(conf(1), conf(6), 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||

This comment has been minimized.

Copy link
@promag

promag Mar 13, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 13, 2019

Author Member

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

@@ -929,11 +927,6 @@ static UniValue sendmany(const JSONRPCRequest& request)

EnsureWalletIsUnlocked(pwallet);

// Check funds
if (totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth)) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds");

This comment has been minimized.

Copy link
@promag

promag Mar 13, 2019

Member

There was no test for this right?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 13, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 13, 2019

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

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 13, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 13, 2019

@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

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 13, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 13, 2019

Sorry you are right.

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

@MarcoFalke MarcoFalke closed this Mar 13, 2019

@MarcoFalke MarcoFalke deleted the MarcoFalke:1903-rpcWalletDummySendmany branch Mar 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.