From fae5f874d51c770322fb18cc4050b3f14697de66 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 13 Mar 2019 14:45:53 -0400 Subject: [PATCH 1/4] rpc: Document that minconf is an ignored dummy value --- src/wallet/rpcwallet.cpp | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 31a3209a4..a35659e37 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -807,9 +807,7 @@ static UniValue sendmany(const JSONRPCRequest& request) return NullUniValue; } - if (request.fHelp || request.params.size() < 2 || request.params.size() > 8) - throw std::runtime_error( - RPCHelpMan{"sendmany", + const RPCHelpMan help{"sendmany", "\nSend multiple times. Amounts are double-precision floating point numbers." + HelpRequiringPassphrase(pwallet) + "\n", { @@ -819,7 +817,7 @@ static UniValue sendmany(const JSONRPCRequest& request) {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The bitcoin address is the key, the numeric amount (can be string) in " + CURRENCY_UNIT + " is the value"}, }, }, - {"minconf", RPCArg::Type::NUM, /* default */ "1", "Only use the balance confirmed at least this many times."}, + {"minconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED_NAMED_ARG, "Ignored dummy value"}, {"comment", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "A comment"}, {"subtractfeefrom", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "A json array with addresses.\n" " The fee will be equally deducted from the amount of each selected address.\n" @@ -850,7 +848,11 @@ static UniValue sendmany(const JSONRPCRequest& request) "\nAs a JSON-RPC call\n" + HelpExampleRpc("sendmany", "\"\", {\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\":0.01,\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\":0.02}, 6, \"testing\"") }, - }.ToString()); + }; + + if (request.fHelp || !help.IsValidNumArgs(request.params.size())) { + throw std::runtime_error(help.ToString()); + } // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now @@ -867,9 +869,6 @@ static UniValue sendmany(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_PARAMETER, "Dummy value must be set to \"\""); } UniValue sendTo = request.params[1].get_obj(); - int nMinDepth = 1; - if (!request.params[2].isNull()) - nMinDepth = request.params[2].get_int(); mapValue_t mapValue; if (!request.params[3].isNull() && !request.params[3].get_str().empty()) @@ -897,7 +896,6 @@ static UniValue sendmany(const JSONRPCRequest& request) std::set destinations; std::vector vecSend; - CAmount totalAmount = 0; std::vector keys = sendTo.getKeys(); for (const std::string& name_ : keys) { CTxDestination dest = DecodeDestination(name_); @@ -914,7 +912,6 @@ static UniValue sendmany(const JSONRPCRequest& request) CAmount nAmount = AmountFromValue(sendTo[name_]); if (nAmount <= 0) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); - totalAmount += nAmount; bool fSubtractFeeFromAmount = false; for (unsigned int idx = 0; idx < subtractFeeFromAmount.size(); idx++) { @@ -929,11 +926,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"); - } - // Shuffle recipient list std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext()); From faa3a246e8809b4954a4a6d202fe8c7f7f776b6e Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 10 Mar 2019 17:29:54 -0400 Subject: [PATCH 2/4] scripted-diff: wallet: Rename pcoin to wtx -BEGIN VERIFY SCRIPT- sed -i --regexp-extended -e 's/const CWalletTx ?\* ?pcoin = &/const CWalletTx\& wtx = /g' src/wallet/wallet.cpp sed -i -e 's/\/wtx./g' src/wallet/wallet.cpp sed -i -e 's/\/\&wtx/g' src/wallet/wallet.cpp -END VERIFY SCRIPT- --- src/wallet/wallet.cpp | 90 +++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e47f5fa95..cd1a37e3e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2180,9 +2180,9 @@ CAmount CWallet::GetBalance(const isminefilter& filter, const int min_depth) con LOCK(cs_wallet); for (const auto& entry : mapWallet) { - const CWalletTx* pcoin = &entry.second; - if (pcoin->IsTrusted(*locked_chain) && pcoin->GetDepthInMainChain(*locked_chain) >= min_depth) { - nTotal += pcoin->GetAvailableCredit(*locked_chain, true, filter); + const CWalletTx& wtx = entry.second; + if (wtx.IsTrusted(*locked_chain) && wtx.GetDepthInMainChain(*locked_chain) >= min_depth) { + nTotal += wtx.GetAvailableCredit(*locked_chain, true, filter); } } } @@ -2198,9 +2198,9 @@ CAmount CWallet::GetUnconfirmedBalance() const LOCK(cs_wallet); for (const auto& entry : mapWallet) { - const CWalletTx* pcoin = &entry.second; - if (!pcoin->IsTrusted(*locked_chain) && pcoin->GetDepthInMainChain(*locked_chain) == 0 && pcoin->InMempool()) - nTotal += pcoin->GetAvailableCredit(*locked_chain); + const CWalletTx& wtx = entry.second; + if (!wtx.IsTrusted(*locked_chain) && wtx.GetDepthInMainChain(*locked_chain) == 0 && wtx.InMempool()) + nTotal += wtx.GetAvailableCredit(*locked_chain); } } return nTotal; @@ -2214,8 +2214,8 @@ CAmount CWallet::GetImmatureBalance() const LOCK(cs_wallet); for (const auto& entry : mapWallet) { - const CWalletTx* pcoin = &entry.second; - nTotal += pcoin->GetImmatureCredit(*locked_chain); + const CWalletTx& wtx = entry.second; + nTotal += wtx.GetImmatureCredit(*locked_chain); } } return nTotal; @@ -2229,9 +2229,9 @@ CAmount CWallet::GetUnconfirmedWatchOnlyBalance() const LOCK(cs_wallet); for (const auto& entry : mapWallet) { - const CWalletTx* pcoin = &entry.second; - if (!pcoin->IsTrusted(*locked_chain) && pcoin->GetDepthInMainChain(*locked_chain) == 0 && pcoin->InMempool()) - nTotal += pcoin->GetAvailableCredit(*locked_chain, true, ISMINE_WATCH_ONLY); + const CWalletTx& wtx = entry.second; + if (!wtx.IsTrusted(*locked_chain) && wtx.GetDepthInMainChain(*locked_chain) == 0 && wtx.InMempool()) + nTotal += wtx.GetAvailableCredit(*locked_chain, true, ISMINE_WATCH_ONLY); } } return nTotal; @@ -2245,8 +2245,8 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const LOCK(cs_wallet); for (const auto& entry : mapWallet) { - const CWalletTx* pcoin = &entry.second; - nTotal += pcoin->GetImmatureWatchOnlyCredit(*locked_chain); + const CWalletTx& wtx = entry.second; + nTotal += wtx.GetImmatureWatchOnlyCredit(*locked_chain); } } return nTotal; @@ -2319,25 +2319,25 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< for (const auto& entry : mapWallet) { const uint256& wtxid = entry.first; - const CWalletTx* pcoin = &entry.second; + const CWalletTx& wtx = entry.second; - if (!locked_chain.checkFinalTx(*pcoin->tx)) { + if (!locked_chain.checkFinalTx(*wtx.tx)) { continue; } - if (pcoin->IsImmatureCoinBase(locked_chain)) + if (wtx.IsImmatureCoinBase(locked_chain)) continue; - int nDepth = pcoin->GetDepthInMainChain(locked_chain); + int nDepth = wtx.GetDepthInMainChain(locked_chain); if (nDepth < 0) continue; // We should not consider coins which aren't at least in our mempool // It's possible for these to be conflicted via ancestors which we may never be able to detect - if (nDepth == 0 && !pcoin->InMempool()) + if (nDepth == 0 && !wtx.InMempool()) continue; - bool safeTx = pcoin->IsTrusted(locked_chain); + bool safeTx = wtx.IsTrusted(locked_chain); // We should not consider coins from transactions that are replacing // other transactions. @@ -2354,7 +2354,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< // be a 1-block reorg away from the chain where transactions A and C // were accepted to another chain where B, B', and C were all // accepted. - if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) { + if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) { safeTx = false; } @@ -2366,7 +2366,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< // intending to replace A', but potentially resulting in a scenario // where A, A', and D could all be accepted (instead of just B and // D, or just A and A' like the user would want). - if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid")) { + if (nDepth == 0 && wtx.mapValue.count("replaced_by_txid")) { safeTx = false; } @@ -2377,8 +2377,8 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< if (nDepth < nMinDepth || nDepth > nMaxDepth) continue; - for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) { - if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount) + for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { + if (wtx.tx->vout[i].nValue < nMinimumAmount || wtx.tx->vout[i].nValue > nMaximumAmount) continue; if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i))) @@ -2390,20 +2390,20 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< if (IsSpent(locked_chain, wtxid, i)) continue; - isminetype mine = IsMine(pcoin->tx->vout[i]); + isminetype mine = IsMine(wtx.tx->vout[i]); if (mine == ISMINE_NO) { continue; } - bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey); + bool solvable = IsSolvable(*this, wtx.tx->vout[i].scriptPubKey); bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - vCoins.push_back(COutput(pcoin, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); + vCoins.push_back(COutput(&wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { - nTotal += pcoin->tx->vout[i].nValue; + nTotal += wtx.tx->vout[i].nValue; if (nTotal >= nMinimumSumAmount) { return; @@ -2562,13 +2562,13 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm std::map::const_iterator it = mapWallet.find(outpoint.hash); if (it != mapWallet.end()) { - const CWalletTx* pcoin = &it->second; + const CWalletTx& wtx = it->second; // Clearly invalid input, fail - if (pcoin->tx->vout.size() <= outpoint.n) + if (wtx.tx->vout.size() <= outpoint.n) return false; // Just to calculate the marginal byte size - nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue; - setPresetCoins.insert(CInputCoin(pcoin->tx, outpoint.n)); + nValueFromPresetInputs += wtx.tx->vout[outpoint.n].nValue; + setPresetCoins.insert(CInputCoin(wtx.tx, outpoint.n)); } else return false; // TODO: Allow non-wallet inputs } @@ -3606,27 +3606,27 @@ std::map CWallet::GetAddressBalances(interfaces::Chain: LOCK(cs_wallet); for (const auto& walletEntry : mapWallet) { - const CWalletTx *pcoin = &walletEntry.second; + const CWalletTx& wtx = walletEntry.second; - if (!pcoin->IsTrusted(locked_chain)) + if (!wtx.IsTrusted(locked_chain)) continue; - if (pcoin->IsImmatureCoinBase(locked_chain)) + if (wtx.IsImmatureCoinBase(locked_chain)) continue; - int nDepth = pcoin->GetDepthInMainChain(locked_chain); - if (nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? 0 : 1)) + int nDepth = wtx.GetDepthInMainChain(locked_chain); + if (nDepth < (wtx.IsFromMe(ISMINE_ALL) ? 0 : 1)) continue; - for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) + for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { CTxDestination addr; - if (!IsMine(pcoin->tx->vout[i])) + if (!IsMine(wtx.tx->vout[i])) continue; - if(!ExtractDestination(pcoin->tx->vout[i].scriptPubKey, addr)) + if(!ExtractDestination(wtx.tx->vout[i].scriptPubKey, addr)) continue; - CAmount n = IsSpent(locked_chain, walletEntry.first, i) ? 0 : pcoin->tx->vout[i].nValue; + CAmount n = IsSpent(locked_chain, walletEntry.first, i) ? 0 : wtx.tx->vout[i].nValue; if (!balances.count(addr)) balances[addr] = 0; @@ -3646,13 +3646,13 @@ std::set< std::set > CWallet::GetAddressGroupings() for (const auto& walletEntry : mapWallet) { - const CWalletTx *pcoin = &walletEntry.second; + const CWalletTx& wtx = walletEntry.second; - if (pcoin->tx->vin.size() > 0) + if (wtx.tx->vin.size() > 0) { bool any_mine = false; // group all input addresses with each other - for (const CTxIn& txin : pcoin->tx->vin) + for (const CTxIn& txin : wtx.tx->vin) { CTxDestination address; if(!IsMine(txin)) /* If this input isn't mine, ignore it */ @@ -3666,7 +3666,7 @@ std::set< std::set > CWallet::GetAddressGroupings() // group change with input addresses if (any_mine) { - for (const CTxOut& txout : pcoin->tx->vout) + for (const CTxOut& txout : wtx.tx->vout) if (IsChange(txout)) { CTxDestination txoutAddr; @@ -3683,7 +3683,7 @@ std::set< std::set > CWallet::GetAddressGroupings() } // group lone addrs by themselves - for (const auto& txout : pcoin->tx->vout) + for (const auto& txout : wtx.tx->vout) if (IsMine(txout)) { CTxDestination address; From fac1a0fe54287d819cd0967ad6c75bbcb49b332b Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 12 Mar 2019 23:16:42 -0400 Subject: [PATCH 3/4] wallet: Remove unused GetLegacyBalance --- src/wallet/wallet.cpp | 40 ---------------------------------------- src/wallet/wallet.h | 1 - 2 files changed, 41 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cd1a37e3e..c0a650f87 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2252,46 +2252,6 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const return nTotal; } -// Calculate total balance in a different way from GetBalance. The biggest -// difference is that GetBalance sums up all unspent TxOuts paying to the -// wallet, while this sums up both spent and unspent TxOuts paying to the -// wallet, and then subtracts the values of TxIns spending from the wallet. This -// also has fewer restrictions on which unconfirmed transactions are considered -// trusted. -CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth) const -{ - auto locked_chain = chain().lock(); - LOCK(cs_wallet); - - CAmount balance = 0; - for (const auto& entry : mapWallet) { - const CWalletTx& wtx = entry.second; - const int depth = wtx.GetDepthInMainChain(*locked_chain); - if (depth < 0 || !locked_chain->checkFinalTx(*wtx.tx) || wtx.IsImmatureCoinBase(*locked_chain)) { - continue; - } - - // Loop through tx outputs and add incoming payments. For outgoing txs, - // treat change outputs specially, as part of the amount debited. - CAmount debit = wtx.GetDebit(filter); - const bool outgoing = debit > 0; - for (const CTxOut& out : wtx.tx->vout) { - if (outgoing && IsChange(out)) { - debit -= out.nValue; - } else if (IsMine(out) & filter && depth >= minDepth) { - balance += out.nValue; - } - } - - // For outgoing txs, subtract amount debited. - if (outgoing) { - balance -= debit; - } - } - - return balance; -} - CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const { auto locked_chain = chain().lock(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ea196c879..4eedf37da 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -950,7 +950,6 @@ public: CAmount GetImmatureBalance() const; CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; - CAmount GetLegacyBalance(const isminefilter& filter, int minDepth) const; CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const; OutputType TransactionChangeType(OutputType change_type, const std::vector& vecSend); From fabfb79673d6bf9bff5258cd709d8294e77c1764 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 20 Mar 2019 12:59:18 -0400 Subject: [PATCH 4/4] doc: Add release notes for 15596 --- doc/release-notes.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index ebcdcda30..a876b70ba 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -66,9 +66,21 @@ platform. Notable changes =============== -Example item +Updated RPCs ------------ +Note: some low-level RPC changes mainly useful for testing are described in the +Low-level Changes section below. + +* The `sendmany` RPC had an argument `minconf` that was not well specified and + would lead to RPC errors even when the wallet's coin selection would succeed. + The `sendtoaddress` RPC never had this check, so to normalize the behavior, + `minconf` is now ignored in `sendmany`. If the coin selection does not + succeed due to missing coins, it will still throw an RPC error. Be reminded + that coin selection is influenced by the `-spendzeroconfchange`, + `-limitancestorcount`, `-limitdescendantcount` and `-walletrejectlongchains` + command line arguments. + Low-level changes =================