rpc: Actually use sendmany::minconf #15595
Conversation
|
This can easily be verified by the test changes:
|
maflcko
force-pushed
the
1903-rpcWalletDummySendmany
branch
from
fa0a712 to
fa4fcf9
Compare
|
Concept ACK |
maflcko
force-pushed
the
1903-rpcWalletDummySendmany
branch
from
fa4fcf9 to
fad8952
Compare
maflcko
force-pushed
the
1903-rpcWalletDummySendmany
branch
from
fad8952 to
fae63c3
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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) || |
There was a problem hiding this comment.
How about pass m_min_conf_depth to a new CoinEligibilityFilter member and update OutputGroup::EligibleForSpending?
There was a problem hiding this comment.
That would be a very invasive change for a simple bugfix and would complicate the CoinEligibilityFilter constructor/interface unnecessarily
|
|
||
| // Check funds | ||
| if (totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth)) { | ||
| throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds"); |
There was a problem hiding this comment.
There was no test for this right?
There was a problem hiding this comment.
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
|
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. |
|
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. |
|
@gmaxwell I don't think that changes, because previously it would exit early due to the However, I am more than happy to drop this parameter and bring it in line with other RPCs such as |
|
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? |
|
Sorry you are right. It would even spend unconfirmed coins, so I guess #15596 is the way to go. |
5 participants
For the entirety of its history
sendmany::minconfhas 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.