Merge #6033: backport: bitcoin#18202, #19202, #19501, #19725, #19770, #19877, #20043, partial #18878

34c80473a8 Merge #19877: [test] clarify rpc_net & p2p_disconnect_ban functional tests (Wladimir J. van der Laan)
e42412924f Merge #19770: RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (MarcoFalke)
f96966b7ea Merge #20043: doc: Add 19501 release notes (fanquake)
6a164eaea9 Merge #19501: send* RPCs in the wallet returns the "fee reason" (MarcoFalke)
b6c8d852e3 Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs (MarcoFalke)
f86263b180 Merge #18202: refactor: consolidate sendmany and sendtoaddress code (Samuel Dobson)
fab41fd3c5 partial Merge #18878: test: Add test for conflicted wallet tx notifications (Wladimir J. van der Laan)
db5bd34ee8 Merge #19202: log: remove deprecated `db` log category (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Regular backports from bitcoin v21

  ## What was done?
   - bitcoin/bitcoin#19202
   - partial bitcoin/bitcoin#18878
   - bitcoin/bitcoin#18202
   - bitcoin/bitcoin#19725
   - bitcoin/bitcoin#19501
   - bitcoin/bitcoin#20043
   - bitcoin/bitcoin#19770
   - bitcoin/bitcoin#19877

  ## How Has This Been Tested?
  Run unit/functional tests

  ## Breaking Changes
  - (RPC) The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
    field will be fully removed in the next major release.  It can be accessed
    with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
    it is recommended to instead use the `connection_type` field (it will return
    `manual` when addnode is true)
  - (Settings) The `sendtoaddress` and `sendmany` RPCs accept an optional `verbose=True`
    argument to also return the fee reason about the sent tx.
  - (Settings) The `-debug=db` logging category, which was deprecated in v0.18 and replaced by
    `-debug=walletdb` to distinguish it from `coindb`, has been removed.
  - (RPC)  To make RPC `sendtoaddress` more consistent with `sendmany` the following error
      `sendtoaddress` codes were changed from `-4` to `-6`:
    - Insufficient funds
    - Fee estimation failed
    - Transaction has too long of a mempool chain

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 34c80473a8

Tree-SHA512: 725a103e04c9c7d44a79da6f3f54e7745c7fb98ec906e7228ae16f7662d568e48c015c855902ff8485f2908f0f71815e769ca394cf6c3ca2e5fd920dd39cca74

doc/release-notes-18202.md

@@ -0,0 +1,8 @@
+Low-level RPC Changes
+---------------------
+
+- To make RPC `sendtoaddress` more consistent with `sendmany` the following error
+    `sendtoaddress` codes were changed from `-4` to `-6`:
+  - Insufficient funds
+  - Fee estimation failed
+  - Transaction has too long of a mempool chain

doc/release-notes-19202.md

@@ -0,0 +1,6 @@
+Updated settings
+----------------
+
+- The `-debug=db` logging category, which was deprecated in v0.18 and replaced by
+  `-debug=walletdb` to distinguish it from `coindb`, has been removed. (#6033)
+

doc/release-notes-19501.md

@@ -0,0 +1,10 @@
+
+New settings
+------------
+
+Wallet
+------
+
+- The `sendtoaddress` and `sendmany` RPCs accept an optional `verbose=True`
+  argument to also return the fee reason about the sent tx. (#6033)
+

doc/release-notes-19725.md

@@ -0,0 +1,10 @@
+
+Updated RPCs
+------------
+
+- The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
+  field will be fully removed in the next major release.  It can be accessed
+  with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
+  it is recommended to instead use the `connection_type` field (it will return
+  `manual` when addnode is true). (#6033)
+

src/coinjoin/util.cpp

@@ -274,7 +274,8 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult)
     CTransactionRef tx;
     {
         LOCK2(pwallet->cs_wallet, cs_main);
-        if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl)) {
+        FeeCalculation fee_calc_out;
+        if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl, fee_calc_out)) {
             return false;
         }
     }

src/logging.cpp

@@ -98,15 +98,7 @@ void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)
 bool BCLog::Logger::EnableCategory(const std::string& str)
 {
     BCLog::LogFlags flag;
-    if (!GetLogCategory(flag, str)) {
-        if (str == "db") {
-            // DEPRECATION: Added in 0.20, should start returning an error in 0.21
-            LogPrintf("Warning: logging category 'db' is deprecated, use 'walletdb' instead\n");
-            EnableCategory(BCLog::WALLETDB);
-            return true;
-        }
-        return false;
-    }
+    if (!GetLogCategory(flag, str)) return false;
     EnableCategory(flag);
     return true;
 }

src/rpc/client.cpp

@@ -44,6 +44,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "sendtoaddress", 6, "use_cj" },
     { "sendtoaddress", 7, "conf_target" },
     { "sendtoaddress", 9, "avoid_reuse" },
+    { "sendtoaddress", 10, "verbose"},
     { "settxfee", 0, "amount" },
     { "sethdseed", 0, "newkeypool" },
     { "getreceivedbyaddress", 1, "minconf" },
@@ -89,6 +90,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "sendmany", 6, "use_is" },
     { "sendmany", 7, "use_cj" },
     { "sendmany", 8, "conf_target" },
+    { "sendmany", 10, "verbose" },
     { "deriveaddresses", 1, "range" },
     { "scantxoutset", 1, "scanobjects" },
     { "addmultisigaddress", 0, "nrequired" },

src/rpc/evo.cpp

@@ -270,7 +270,8 @@ static void FundSpecialTx(CWallet* pwallet, CMutableTransaction& tx, const Speci
     int nChangePos = -1;
     bilingual_str strFailReason;
 
-    if (!pwallet->CreateTransaction(vecSend, newTx, nFee, nChangePos, strFailReason, coinControl, false, tx.vExtraPayload.size())) {
+    FeeCalculation fee_calc_out;
+    if (!pwallet->CreateTransaction(vecSend, newTx, nFee, nChangePos, strFailReason, coinControl, fee_calc_out, false, tx.vExtraPayload.size())) {
         throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason.original);
     }
 

src/rpc/net.cpp

@@ -145,7 +145,8 @@ static RPCHelpMan getpeerinfo()
                     {RPCResult::Type::NUM, "version", "The peer version, such as 70001"},
                     {RPCResult::Type::STR, "subver", "The string version"},
                     {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
-                    {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection"},
+                    {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection\n"
+                                                       "(DEPRECATED, returned only if the config option -deprecatedrpc=getpeerinfo_addnode is passed)"},
                     {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"},
                     {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"},
                     {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"},
@@ -158,7 +159,8 @@ static RPCHelpMan getpeerinfo()
                     {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"},
                     {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"},
                     {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"},
-                    {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"},
+                    {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions\n"
+                                                                                "(DEPRECATED, returned only if config option -deprecatedrpc=whitelisted is passed)"},
                     {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer",
                     {
                         {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
@@ -242,7 +244,10 @@ static RPCHelpMan getpeerinfo()
         // their ver message.
         obj.pushKV("subver", stats.cleanSubVer);
         obj.pushKV("inbound", stats.fInbound);
-        obj.pushKV("addnode", stats.m_manual_connection);
+        if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) {
+            // addnode is deprecated in v21 for removal in v22
+            obj.pushKV("addnode", stats.m_manual_connection);
+        }
         obj.pushKV("masternode", stats.m_masternode_connection);
         if (fStateStats) {
             if (IsDeprecatedRPCEnabled("banscore")) {
@@ -262,7 +267,10 @@ static RPCHelpMan getpeerinfo()
             obj.pushKV("addr_processed", statestats.m_addr_processed);
             obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited);
         }
-        obj.pushKV("whitelisted", stats.m_legacyWhitelisted);
+        if (IsDeprecatedRPCEnabled("whitelisted")) {
+            // whitelisted is deprecated in v0.21 for removal in v0.22
+            obj.pushKV("whitelisted", stats.m_legacyWhitelisted);
+        }
         UniValue permissions(UniValue::VARR);
         for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) {
             permissions.push_back(permission);

src/wallet/interfaces.cpp

@@ -271,8 +271,9 @@ public:
         LOCK(m_wallet->cs_wallet);
         ReserveDestination m_dest(m_wallet.get());
         CTransactionRef tx;
+        FeeCalculation fee_calc_out;
         if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos,
-                fail_reason, coin_control, sign)) {
+                fail_reason, coin_control, fee_calc_out, sign)) {
             return {};
         }
         return tx;

src/wallet/rpcwallet.cpp

@@ -352,40 +352,62 @@ static RPCHelpMan setlabel()
     };
 }
 
+void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient> &recipients) {
+    std::set<CTxDestination> destinations;
+    int i = 0;
+    for (const std::string& address: address_amounts.getKeys()) {
+        CTxDestination dest = DecodeDestination(address);
+        if (!IsValidDestination(dest)) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Dash address: ") + address);
+        }
 
-static CTransactionRef SendMoney(CWallet* const pwallet, const CTxDestination& address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue)
+        if (destinations.count(dest)) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address);
+        }
+        destinations.insert(dest);
+
+        CScript script_pub_key = GetScriptForDestination(dest);
+        CAmount amount = AmountFromValue(address_amounts[i++]);
+
+        bool subtract_fee = false;
+        for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
+            const UniValue& addr = subtract_fee_outputs[idx];
+            if (addr.get_str() == address) {
+                subtract_fee = true;
+            }
+        }
+
+        CRecipient recipient = {script_pub_key, amount, subtract_fee};
+        recipients.push_back(recipient);
+    }
+}
+
+UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std::vector<CRecipient> &recipients, mapValue_t map_value, bool verbose)
 {
-    CAmount curBalance = pwallet->GetBalance(0, coin_control.m_avoid_address_reuse).m_mine_trusted;
-
-    // Check amount
-    if (nValue <= 0)
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid amount");
-
-    if (nValue > curBalance)
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds");
+    EnsureWalletIsUnlocked(pwallet);
 
     if (coin_control.IsUsingCoinJoin()) {
-        mapValue["DS"] = "1";
+        map_value["DS"] = "1";
     }
 
-    // Parse Dash address
-    CScript scriptPubKey = GetScriptForDestination(address);
-
-    // Create and send the transaction
+    // Send
     CAmount nFeeRequired = 0;
-    bilingual_str error;
-    std::vector<CRecipient> vecSend;
     int nChangePosRet = -1;
-    CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
-    vecSend.push_back(recipient);
+    bilingual_str error;
     CTransactionRef tx;
-    if (!pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, error, coin_control)) {
-        if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
-            error = strprintf(Untranslated("Error: This transaction requires a transaction fee of at least %s"), FormatMoney(nFeeRequired));
-        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
+    FeeCalculation fee_calc_out;
+    bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
+    if (!fCreated) {
+        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
     }
-    pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */);
-    return tx;
+    pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
+    if (verbose) {
+        UniValue entry(UniValue::VOBJ);
+        entry.pushKV("txid", tx->GetHash().GetHex());
+        entry.pushKV("fee_reason", StringForFeeReason(fee_calc_out.reason));
+        return entry;
+    }
+    return tx->GetHash().GetHex();
 }
 
 static RPCHelpMan sendtoaddress()
@@ -397,12 +419,12 @@ static RPCHelpMan sendtoaddress()
             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Dash address to send to."},
             {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The amount in " + CURRENCY_UNIT + " to send. eg 0.1"},
             {"comment", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "A comment used to store what the transaction is for.\n"
-    "                             This is not part of the transaction, just kept in your wallet."},
+                "This is not part of the transaction, just kept in your wallet."},
             {"comment_to", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "A comment to store the name of the person or organization\n"
-    "                             to which you're sending the transaction. This is not part of the \n"
-    "                             transaction, just kept in your wallet."},
+                "to which you're sending the transaction. This is not part of the \n"
+                "transaction, just kept in your wallet."},
             {"subtractfeefromamount", RPCArg::Type::BOOL, /* default */ "false", "The fee will be deducted from the amount being sent.\n"
-    "                             The recipient will receive less amount of Dash than you enter in the amount field."},
+                "The recipient will receive less amount of Dash than you enter in the amount field."},
             {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"},
             {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"},
             {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
@@ -410,9 +432,19 @@ static RPCHelpMan sendtoaddress()
             "       \"" + FeeModes("\"\n\"") + "\""},
             {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
     "                             dirty if they have previously been used in a transaction."},
+            {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra information about the transaction."},
         },
-        RPCResult{
-            RPCResult::Type::STR_HEX, "txid", "The transaction id."
+                {
+                    RPCResult{"if verbose is not set or set to false",
+                        RPCResult::Type::STR_HEX, "txid", "The transaction id."
+                    },
+                    RPCResult{"if verbose is set to true",
+                        RPCResult::Type::OBJ, "", "",
+                        {
+                            {RPCResult::Type::STR_HEX, "txid", "The transaction id."},
+                            {RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
+                        },
+                    },
         },
         RPCExamples{
             HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1")
@@ -434,16 +466,6 @@ static RPCHelpMan sendtoaddress()
 
     LOCK(pwallet->cs_wallet);
 
-    CTxDestination dest = DecodeDestination(request.params[0].get_str());
-    if (!IsValidDestination(dest)) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
-    }
-
-    // Amount
-    CAmount nAmount = AmountFromValue(request.params[1]);
-    if (nAmount <= 0)
-        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
-
     // Wallet comments
     mapValue_t mapValue;
     if (!request.params[2].isNull() && !request.params[2].get_str().empty())
@@ -471,8 +493,19 @@ static RPCHelpMan sendtoaddress()
 
     EnsureWalletIsUnlocked(pwallet);
 
-    CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue));
-    return tx->GetHash().GetHex();
+    UniValue address_amounts(UniValue::VOBJ);
+    const std::string address = request.params[0].get_str();
+    address_amounts.pushKV(address, request.params[1]);
+    UniValue subtractFeeFromAmount(UniValue::VARR);
+    if (fSubtractFeeFromAmount) {
+        subtractFeeFromAmount.push_back(address);
+    }
+
+    std::vector<CRecipient> recipients;
+    ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
+    bool verbose = request.params[10].isNull() ? false: request.params[10].get_bool();
+
+    return SendMoney(pwallet, coin_control, recipients, mapValue, verbose);
 },
     };
 }
@@ -890,9 +923,9 @@ static RPCHelpMan sendmany()
                     {"addlocked", RPCArg::Type::BOOL, 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, "The addresses.\n"
-            "                           The fee will be equally deducted from the amount of each selected address.\n"
-            "                           Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
-            "                           If no addresses are specified here, the sender pays the fee.",
+                        "The fee will be equally deducted from the amount of each selected address.\n"
+                        "Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
+                        "If no addresses are specified here, the sender pays the fee.",
                         {
                             {"address", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Subtract fee from this address"},
                         },
@@ -902,11 +935,22 @@ static RPCHelpMan sendmany()
                     {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
                     {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
             "       \"" + FeeModes("\"\n\"") + "\""},
+                    {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."},
+                },
+                {
+                    RPCResult{"if verbose is not set or set to false",
+                        RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
+                "the number of addresses."
+                    },
+                    RPCResult{"if verbose is set to true",
+                        RPCResult::Type::OBJ, "", "",
+                        {
+                            {RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
+                "the number of addresses."},
+                            {RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
+                        },
+                    },
                 },
-                 RPCResult{
-                     RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
-            "the number of addresses."
-                 },
                 RPCExamples{
             "\nSend two amounts to two different addresses:\n"
             + HelpExampleCli("sendmany", "\"\" \"{\\\"" + EXAMPLE_ADDRESS[0] + "\\\":0.01,\\\"" + EXAMPLE_ADDRESS[1] + "\\\":0.02}\"") +
@@ -935,9 +979,9 @@ static RPCHelpMan sendmany()
     if (!request.params[4].isNull() && !request.params[4].get_str().empty())
         mapValue["comment"] = request.params[4].get_str();
 
-    UniValue subtractFeeFrom(UniValue::VARR);
+    UniValue subtractFeeFromAmount(UniValue::VARR);
     if (!request.params[5].isNull())
-        subtractFeeFrom = request.params[5].get_array();
+        subtractFeeFromAmount = request.params[5].get_array();
 
     // request.params[6] ("use_is") is deprecated and not used here
 
@@ -949,53 +993,11 @@ static RPCHelpMan sendmany()
 
     SetFeeEstimateMode(pwallet, coin_control, request.params[9], request.params[8]);
 
-    if (coin_control.IsUsingCoinJoin()) {
-        mapValue["DS"] = "1";
-    }
+    std::vector<CRecipient> recipients;
+    ParseRecipients(sendTo, subtractFeeFromAmount, recipients);
+    bool verbose = request.params[10].isNull() ? false : request.params[10].get_bool();
 
-    std::set<CTxDestination> destinations;
-    std::vector<CRecipient> vecSend;
-
-    std::vector<std::string> keys = sendTo.getKeys();
-    for (const std::string& name_ : keys) {
-        CTxDestination dest = DecodeDestination(name_);
-        if (!IsValidDestination(dest)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Dash address: ") + name_);
-        }
-
-        if (destinations.count(dest)) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_);
-        }
-        destinations.insert(dest);
-
-        CScript scriptPubKey = GetScriptForDestination(dest);
-        CAmount nAmount = AmountFromValue(sendTo[name_]);
-        if (nAmount <= 0)
-            throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
-
-        bool fSubtractFeeFromAmount = false;
-        for (unsigned int idx = 0; idx < subtractFeeFrom.size(); idx++) {
-            const UniValue& addr = subtractFeeFrom[idx];
-            if (addr.get_str() == name_)
-                fSubtractFeeFromAmount = true;
-        }
-
-        CRecipient recipient = {scriptPubKey, nAmount, fSubtractFeeFromAmount};
-        vecSend.push_back(recipient);
-    }
-
-    EnsureWalletIsUnlocked(pwallet);
-
-    // Send
-    CAmount nFeeRequired = 0;
-    int nChangePosRet = -1;
-    bilingual_str error;
-    CTransactionRef tx;
-    bool fCreated = pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, error, coin_control);
-    if (!fCreated)
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
-    pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */);
-    return tx->GetHash().GetHex();
+    return SendMoney(pwallet, coin_control, recipients, std::move(mapValue), verbose);
 },
     };
 }
@@ -1580,7 +1582,7 @@ static RPCHelpMan listsinceblock()
             {"target_confirmations", RPCArg::Type::NUM, /* default */ "1", "Return the nth block hash from the main chain. e.g. 1 would mean the best block hash. Note: this is not used as a filter, but only affects [lastblock] in the return value"},
             {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Include transactions to watch-only addresses (see 'importaddress')"},
             {"include_removed", RPCArg::Type::BOOL, /* default */ "true", "Show transactions that were removed due to a reorg in the \"removed\" array\n"
-    "                                                           (not guaranteed to work on pruned nodes)"},
+                "(not guaranteed to work on pruned nodes)"},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -3189,7 +3191,7 @@ static RPCHelpMan listunspent()
                 },
             },
             {"include_unsafe", RPCArg::Type::BOOL, /* default */ "true", "Include outputs that are not safe to spend\n"
-    "                  See description of \"safe\" attribute below."},
+                "See description of \"safe\" attribute below."},
             {"query_options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "JSON with query options",
                 {
                     {"minimumAmount", RPCArg::Type::AMOUNT, /* default */ "0", "Minimum value of each UTXO in " + CURRENCY_UNIT + ""},
@@ -3197,8 +3199,8 @@ static RPCHelpMan listunspent()
                     {"maximumCount", RPCArg::Type::NUM, /* default */ "unlimited", "Maximum number of UTXOs"},
                     {"minimumSumAmount", RPCArg::Type::AMOUNT, /* default */ "unlimited", "Minimum sum value of all UTXOs in " + CURRENCY_UNIT + ""},
                     {"coinType", RPCArg::Type::NUM, /* default */ "0", "Filter coinTypes as follows:\n"
-    "                         0=ALL_COINS, 1=ONLY_FULLY_MIXED, 2=ONLY_READY_TO_MIX, 3=ONLY_NONDENOMINATED,\n"
-    "                         4=ONLY_MASTERNODE_COLLATERAL, 5=ONLY_COINJOIN_COLLATERAL" },
+                        "0=ALL_COINS, 1=ONLY_FULLY_MIXED, 2=ONLY_READY_TO_MIX, 3=ONLY_NONDENOMINATED,\n"
+                        "4=ONLY_MASTERNODE_COLLATERAL, 5=ONLY_COINJOIN_COLLATERAL" },
                 },
                 "query_options"},
         },
@@ -3220,7 +3222,7 @@ static RPCHelpMan listunspent()
                     {RPCResult::Type::STR, "desc", "(only when solvable) A descriptor for spending this output"},
                     {RPCResult::Type::BOOL, "reused", /* optional*/ true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"},
                     {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions"
-                                                    "                             from outside keys and unconfirmed replacement transactions are considered unsafe\n"
+                                                    "from outside keys and unconfirmed replacement transactions are considered unsafe\n"
                                                     "and are not eligible for spending by fundrawtransaction and sendtoaddress."},
                     {RPCResult::Type::NUM, "coinjoin_rounds", "The number of CoinJoin rounds"},
                 }},
@@ -3536,9 +3538,9 @@ static RPCHelpMan fundrawtransaction()
                             {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
                             {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"},
                             {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "The integers.\n"
-                            "                              The fee will be equally deducted from the amount of each specified output.\n"
-                            "                              Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
-                            "                              If no outputs are specified here, the sender pays the fee.",
+                                "The fee will be equally deducted from the amount of each specified output.\n"
+                                "Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
+                                "If no outputs are specified here, the sender pays the fee.",
                                 {
                                     {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
                                 },
@@ -4175,7 +4177,7 @@ static RPCHelpMan send()
         "\nEXPERIMENTAL warning: this call may be changed in future releases.\n"
         "\nSend a transaction.\n",
         {
-            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "A JSON array with outputs (key-value pairs), where none of the keys are duplicated.\n"
+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n"
                     "That is, each address can only appear once and there can only be one 'data' object.\n"
                     "For convenience, a dictionary, which holds the key-value pairs directly, is also accepted.",
                 {
@@ -4513,7 +4515,7 @@ static RPCHelpMan walletcreatefundedpsbt()
             {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n"
                     "That is, each address can only appear once and there can only be one 'data' object.\n"
                     "For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also\n"
-                    "                             accepted as second parameter.",
+                    "accepted as second parameter.",
                 {
                     {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
                         {
@@ -4537,9 +4539,9 @@ static RPCHelpMan walletcreatefundedpsbt()
                     {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
                     {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"},
                     {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "The outputs to subtract the fee from.\n"
-                    "                              The fee will be equally deducted from the amount of each specified output.\n"
-                    "                              Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
-                    "                              If no outputs are specified here, the sender pays the fee.",
+                        "The fee will be equally deducted from the amount of each specified output.\n"
+                        "Those recipients will receive less Dash than you enter in their corresponding amount field.\n"
+                        "If no outputs are specified here, the sender pays the fee.",
                         {
                             {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
                         },
@@ -4750,8 +4752,8 @@ static const CRPCCommand commands[] =
     { "wallet",             "removeprunedfunds",                &removeprunedfunds,             {"txid"} },
     { "wallet",             "rescanblockchain",                 &rescanblockchain,              {"start_height", "stop_height"} },
     { "wallet",             "send",                             &send,                          {"outputs","conf_target","estimate_mode","options"} },
-    { "wallet",             "sendmany",                         &sendmany,                      {"dummy","amounts","minconf","addlocked","comment","subtractfeefrom","use_is","use_cj","conf_target","estimate_mode"} },
-    { "wallet",             "sendtoaddress",                    &sendtoaddress,                 {"address","amount","comment","comment_to","subtractfeefromamount","use_is","use_cj","conf_target","estimate_mode", "avoid_reuse"} },
+    { "wallet",             "sendmany",                         &sendmany,                      {"dummy","amounts","minconf","addlocked","comment","subtractfeefrom","use_is","use_cj","conf_target","estimate_mode","verbose"} },
+    { "wallet",             "sendtoaddress",                    &sendtoaddress,                 {"address","amount","comment","comment_to","subtractfeefromamount","use_is","use_cj","conf_target","estimate_mode", "avoid_reuse","verbose"} },
     { "wallet",             "sethdseed",                        &sethdseed,                     {"newkeypool","seed"} },
     { "wallet",             "setcoinjoinrounds",                &setcoinjoinrounds,             {"rounds"} },
     { "wallet",             "setcoinjoinamount",                &setcoinjoinamount,             {"amount"} },

src/wallet/test/coinjoin_tests.cpp

@@ -185,7 +185,8 @@ public:
         }
         for (CAmount nAmount : vecAmounts) {
             CTransactionRef tx;
-            BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl));
+            FeeCalculation fee_calc_out;
+            BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl, fee_calc_out));
             {
                 LOCK2(wallet->cs_wallet, cs_main);
                 wallet->CommitTransaction(tx, {}, {});

src/wallet/test/wallet_tests.cpp

@@ -602,7 +602,8 @@ public:
         int changePos = -1;
         bilingual_str error;
         CCoinControl dummy;
-        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy));
+        FeeCalculation fee_calc_out;
+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy, fee_calc_out));
         wallet->CommitTransaction(tx, {}, {});
         CMutableTransaction blocktx;
         {
@@ -756,7 +757,8 @@ public:
         int nChangePos = nChangePosRequest;
         bilingual_str strError;
 
-        bool fCreationSucceeded = wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePos, strError, coinControl);
+        FeeCalculation fee_calc_out;
+        bool fCreationSucceeded = wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePos, strError, coinControl, fee_calc_out);
         bool fHitMaxTries = strError.original == strExceededMaxTries;
         // This should never happen.
         if (fHitMaxTries) {
@@ -808,7 +810,8 @@ public:
         int nChangePosRet = -1;
         bilingual_str strError;
         CCoinControl coinControl;
-        BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePosRet, strError, coinControl));
+        FeeCalculation fee_calc_out;
+        BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePosRet, strError, coinControl, fee_calc_out));
         wallet->CommitTransaction(tx, {}, {});
         CMutableTransaction blocktx;
         {
@@ -1147,10 +1150,11 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
     int changePos = -1;
     bilingual_str error;
     CCoinControl dummy;
+    FeeCalculation fee_calc_out;
     BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 2 * COIN, true /* subtract fee */}},
-                                        tx1, fee, changePos, error, dummy));
+                                        tx1, fee, changePos, error, dummy, fee_calc_out));
     BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 1 * COIN, true /* subtract fee */}},
-                                        tx2, fee, changePos, error, dummy));
+                                        tx2, fee, changePos, error, dummy, fee_calc_out));
     wallet->CommitTransaction(tx1, {}, {});
     BOOST_CHECK_EQUAL(wallet->GetAvailableBalance(), 0);
     CreateAndProcessBlock({CMutableTransaction(*tx2)}, GetScriptForRawPubKey({}));

src/wallet/wallet.cpp

@@ -3090,7 +3090,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
     LOCK(cs_wallet);
 
     CTransactionRef tx_new;
-    if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, false, tx.vExtraPayload.size())) {
+    FeeCalculation fee_calc_out;
+    if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false, tx.vExtraPayload.size())) {
         return false;
     }
 
@@ -3397,7 +3398,8 @@ bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef& tx, uint256 hash, CAm
     if (!outpoint.IsNull()) {
         coinControl.Select(outpoint);
     }
-    bool success = CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, error, coinControl);
+    FeeCalculation fee_calc_out;
+    bool success = CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out);
     if(!success){
         WalletLogPrintf("CWallet::GetBudgetSystemCollateralTX -- Error: %s\n", error.original);
         return false;
@@ -3413,6 +3415,7 @@ bool CWallet::CreateTransactionInternal(
         int& nChangePosInOut,
         bilingual_str& error,
         const CCoinControl& coin_control,
+        FeeCalculation& fee_calc_out,
         bool sign,
         int nExtraPayloadSize)
 {
@@ -3771,6 +3774,7 @@ bool CWallet::CreateTransactionInternal(
     // Before we return success, we assume any change key will be used to prevent
     // accidental re-use.
     reservedest.KeepDestination();
+    fee_calc_out = feeCalc;
 
     WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
               nFeeRet, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
@@ -3790,12 +3794,13 @@ bool CWallet::CreateTransaction(
         int& nChangePosInOut,
         bilingual_str& error,
         const CCoinControl& coin_control,
+        FeeCalculation& fee_calc_out,
         bool sign,
         int nExtraPayloadSize)
 {
     int nChangePosIn = nChangePosInOut;
     Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
-    bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign, nExtraPayloadSize);
+    bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign, nExtraPayloadSize);
     // try with avoidpartialspends unless it's enabled already
     if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
         CCoinControl tmp_cc = coin_control;
@@ -3811,7 +3816,7 @@ bool CWallet::CreateTransaction(
         CTransactionRef tx2;
         int nChangePosInOut2 = nChangePosIn;
         bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
-        if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign, nExtraPayloadSize)) {
+        if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, fee_calc_out, sign, nExtraPayloadSize)) {
             // if fee of this alternative one is within the range of the max fee, we use this one
             const bool use_aps = nFeeRet2 <= nFeeRet + m_max_aps_fee;
             WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");

src/wallet/wallet.h

@@ -842,7 +842,7 @@ private:
     // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
     std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;
 
-    bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign, int nExtraPayloadSize);
+    bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out,  bool sign, int nExtraPayloadSize);
 
 public:
     /**
@@ -1133,7 +1133,7 @@ public:
      * selected by SelectCoins(); Also create the change output, when needed
      * @note passing nChangePosInOut as -1 will result in setting a random position
      */
-    bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign = true, int nExtraPayloadSize = 0);
+    bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0);
     /**
      * Submit the transaction to the node's mempool and then relay to peers.
      * Should be called after CreateTransaction unless you want to abort

test/functional/feature_notifications.py

@@ -7,6 +7,7 @@
 import os
 
 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
+
 from test_framework.test_framework import DashTestFramework
 from test_framework.util import (
     assert_equal,
@@ -95,6 +96,9 @@ class NotificationsTest(DashTestFramework):
             # directory content should equal the generated transaction hashes
             txids_rpc = list(map(lambda t: notify_outputname(self.wallet, t['txid']), self.nodes[1].listtransactions("*", block_count)))
             assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir)))
+            for tx_file in os.listdir(self.walletnotify_dir):
+                os.remove(os.path.join(self.walletnotify_dir, tx_file))
+
 
         self.log.info("test -chainlocknotify")
 
@@ -141,5 +145,6 @@ class NotificationsTest(DashTestFramework):
 
         # TODO: add test for `-alertnotify` large fork notifications
 
+
 if __name__ == '__main__':
     NotificationsTest().main()

test/functional/p2p_blocksonly.py

@@ -43,7 +43,7 @@ class P2PBlocksOnly(BitcoinTestFramework):
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
 
         self.log.info("Restarting node 0 with relay permission and blocksonly")
-        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"])
+        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted'])
         assert_equal(self.nodes[0].getrawmempool(), [])
         first_peer = self.nodes[0].add_p2p_connection(P2PInterface())
         second_peer = self.nodes[0].add_p2p_connection(P2PInterface())

test/functional/p2p_disconnect_ban.py

@@ -17,8 +17,11 @@ class DisconnectBanTest(BitcoinTestFramework):
 
     def run_test(self):
         self.log.info("Connect nodes both way")
+        # By default, the test framework sets up an addnode connection from
+        # node 1 --> node0. By connecting node0 --> node 1, we're left with
+        # the two nodes being connected both ways.
+        # Topology will look like: node0 <--> node1
         self.connect_nodes(0, 1)
-        self.connect_nodes(1, 0)
 
         self.log.info("Test setban and listbanned RPCs")
 

test/functional/p2p_permissions.py

@@ -39,6 +39,13 @@ class P2PPermissionsTests(BitcoinTestFramework):
             ["relay", "noban", "mempool", "download"],
             True)
 
+        self.checkpermission(
+            # check without deprecatedrpc=whitelisted
+            ["-whitelist=127.0.0.1"],
+            # Make sure the default values in the command line documentation match the ones here
+            ["relay", "noban", "mempool", "download"],
+            None)
+
         self.checkpermission(
             # no permission (even with forcerelay)
             ["-whitelist=@127.0.0.1", "-whitelistforcerelay=1"],
@@ -77,6 +84,12 @@ class P2PPermissionsTests(BitcoinTestFramework):
             ["noban", "mempool", "download"],
             False)
 
+        self.checkpermission(
+            # check without deprecatedrpc=whitelisted
+            ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"],
+            ["noban", "mempool", "download"],
+            None)
+
         self.checkpermission(
             # legacy whitelistforcerelay should be ignored
             ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"],
@@ -159,10 +172,15 @@ class P2PPermissionsTests(BitcoinTestFramework):
         )
 
     def checkpermission(self, args, expectedPermissions, whitelisted):
+        if whitelisted is not None:
+            args = [*args, '-deprecatedrpc=whitelisted']
         self.restart_node(1, args)
         self.connect_nodes(0, 1)
         peerinfo = self.nodes[1].getpeerinfo()[0]
-        assert_equal(peerinfo['whitelisted'], whitelisted)
+        if whitelisted is None:
+            assert 'whitelisted' not in peerinfo
+        else:
+            assert_equal(peerinfo['whitelisted'], whitelisted)
         assert_equal(len(expectedPermissions), len(peerinfo['permissions']))
         for p in expectedPermissions:
             if not p in peerinfo['permissions']:

test/functional/rpc_getpeerinfo_deprecation.py

@@ -2,23 +2,37 @@
 # Copyright (c) 2020 The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
-"""Test deprecation of getpeerinfo RPC banscore field."""
+"""Test deprecation of getpeerinfo RPC fields."""
 
 from test_framework.test_framework import BitcoinTestFramework
 
 
-class GetpeerinfoBanscoreDeprecationTest(BitcoinTestFramework):
+class GetpeerinfoDeprecationTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.extra_args = [[], ["-deprecatedrpc=banscore"]]
 
     def run_test(self):
+        self.test_banscore_deprecation()
+        self.test_addnode_deprecation()
+
+    def test_banscore_deprecation(self):
         self.log.info("Test getpeerinfo by default no longer returns a banscore field")
         assert "banscore" not in self.nodes[0].getpeerinfo()[0].keys()
 
         self.log.info("Test getpeerinfo returns banscore with -deprecatedrpc=banscore")
         assert "banscore" in self.nodes[1].getpeerinfo()[0].keys()
 
+    def test_addnode_deprecation(self):
+        self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"])
+        self.connect_nodes(0, 1)
+
+        self.log.info("Test getpeerinfo by default no longer returns an addnode field")
+        assert "addnode" not in self.nodes[0].getpeerinfo()[0].keys()
+
+        self.log.info("Test getpeerinfo returns addnode with -deprecatedrpc=addnode")
+        assert "addnode" in self.nodes[1].getpeerinfo()[0].keys()
+
 
 if __name__ == "__main__":
-    GetpeerinfoBanscoreDeprecationTest().main()
+    GetpeerinfoDeprecationTest().main()

test/functional/rpc_net.py

@@ -54,9 +54,11 @@ class NetTest(DashTestFramework):
         # Especially the exchange of messages like getheaders and friends causes test failures here
         self.nodes[0].ping()
         self.wait_until(lambda: all(['pingtime' in n for n in self.nodes[0].getpeerinfo()]))
-        self.log.info('Connect nodes both way')
+        # By default, the test framework sets up an addnode connection from
+        # node 1 --> node0. By connecting node0 --> node 1, we're left with
+        # the two nodes being connected both ways.
+        # Topology will look like: node0 <--> node1
         self.connect_nodes(0, 1)
-        self.connect_nodes(1, 0)
         self.sync_all()
 
         self.test_connection_count()
@@ -75,6 +77,41 @@ class NetTest(DashTestFramework):
         # during network setup
         assert_equal(self.nodes[0].getconnectioncount(), 3)
 
+    def test_getpeerinfo(self):
+        self.log.info("Test getpeerinfo")
+        # Create a few getpeerinfo last_block/last_transaction values.
+        self.wallet.send_self_transfer(from_node=self.nodes[0]) # Make a transaction so we can see it in the getpeerinfo results
+        self.nodes[1].generate(1)
+        self.sync_all()
+        time_now = self.mocktime
+        peer_info = [x.getpeerinfo() for x in self.nodes]
+        # Verify last_block and last_transaction keys/values.
+        for node, peer, field in product(range(self.num_nodes - self.mn_count), range(2), ['last_block', 'last_transaction']):
+            assert field in peer_info[node][peer].keys()
+            if peer_info[node][peer][field] != 0:
+                assert_approx(peer_info[node][peer][field], time_now, vspan=60)
+        # check both sides of bidirectional connection between nodes
+        # the address bound to on one side will be the source address for the other node
+        assert_equal(peer_info[0][0]['addrbind'], peer_info[1][0]['addr'])
+        assert_equal(peer_info[1][0]['addrbind'], peer_info[0][0]['addr'])
+        # check the `servicesnames` field
+        for info in peer_info:
+            assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"])
+
+        # Check dynamically generated networks list in getpeerinfo help output.
+        assert "(ipv4, ipv6, onion, i2p, not_publicly_routable)" in self.nodes[0].help("getpeerinfo")
+        # This part is slightly different comparing to the Bitcoin implementation. This is expected because we create connections on network setup a bit differently too.
+        # We also create more connection during the test itself to test mn specific stats
+        assert_equal(peer_info[0][0]['connection_type'], 'inbound')
+        assert_equal(peer_info[0][1]['connection_type'], 'inbound')
+        assert_equal(peer_info[0][2]['connection_type'], 'manual')
+
+        assert_equal(peer_info[1][0]['connection_type'], 'manual')
+        assert_equal(peer_info[1][1]['connection_type'], 'inbound')
+
+        assert_equal(peer_info[2][0]['connection_type'], 'manual')
+
+
     def test_getnettotals(self):
         self.log.info("Test getnettotals")
         # Test getnettotals and getpeerinfo by doing a ping. The bytes
@@ -113,15 +150,13 @@ class NetTest(DashTestFramework):
 
         with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: true\n']):
             self.nodes[0].setnetworkactive(state=True)
-        # Connect nodes both ways.
         self.connect_nodes(0, 1)
-        self.connect_nodes(1, 0)
 
         info = self.nodes[1].getnetworkinfo()
         assert_equal(info['networkactive'], True)
-        assert_equal(info['connections'], 2)
+        assert_equal(info['connections'], 1)
         assert_equal(info['connections_in'], 1)
-        assert_equal(info['connections_out'], 1)
+        assert_equal(info['connections_out'], 0)
         assert_equal(info['connections_mn'], 0)
         assert_equal(info['connections_mn_in'], 0)
         assert_equal(info['connections_mn_out'], 0)
@@ -135,15 +170,17 @@ class NetTest(DashTestFramework):
         assert "(ipv4, ipv6, onion, i2p)" in self.nodes[0].help("getnetworkinfo")
 
         self.log.info('Test extended connections info')
-        self.connect_nodes(1, 2)
+        # Connect nodes both ways.
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 0)
         self.nodes[1].ping()
         self.wait_until(lambda: all(['pingtime' in n for n in self.nodes[1].getpeerinfo()]))
-        assert_equal(self.nodes[1].getnetworkinfo()['connections'], 3)
+        assert_equal(self.nodes[1].getnetworkinfo()['connections'], 2)
         assert_equal(self.nodes[1].getnetworkinfo()['connections_in'], 1)
-        assert_equal(self.nodes[1].getnetworkinfo()['connections_out'], 2)
-        assert_equal(self.nodes[1].getnetworkinfo()['connections_mn'], 1)
+        assert_equal(self.nodes[1].getnetworkinfo()['connections_out'], 1)
+        assert_equal(self.nodes[1].getnetworkinfo()['connections_mn'], 0)
         assert_equal(self.nodes[1].getnetworkinfo()['connections_mn_in'], 0)
-        assert_equal(self.nodes[1].getnetworkinfo()['connections_mn_out'], 1)
+        assert_equal(self.nodes[1].getnetworkinfo()['connections_mn_out'], 0)
 
     def test_getaddednodeinfo(self):
         self.log.info("Test getaddednodeinfo")
@@ -165,40 +202,6 @@ class NetTest(DashTestFramework):
         # check that a non-existent node returns an error
         assert_raises_rpc_error(-24, "Node has not been added", self.nodes[0].getaddednodeinfo, '1.1.1.1')
 
-    def test_getpeerinfo(self):
-        self.log.info("Test getpeerinfo")
-        # Create a few getpeerinfo last_block/last_transaction values.
-        self.wallet.send_self_transfer(from_node=self.nodes[0]) # Make a transaction so we can see it in the getpeerinfo results
-        self.nodes[1].generate(1)
-        self.sync_all()
-        time_now = self.mocktime
-        peer_info = [x.getpeerinfo() for x in self.nodes]
-        # Verify last_block and last_transaction keys/values.
-        for node, peer, field in product(range(self.num_nodes - self.mn_count), range(2), ['last_block', 'last_transaction']):
-            assert field in peer_info[node][peer].keys()
-            if peer_info[node][peer][field] != 0:
-                assert_approx(peer_info[node][peer][field], time_now, vspan=60)
-        # check both sides of bidirectional connection between nodes
-        # the address bound to on one side will be the source address for the other node
-        assert_equal(peer_info[0][0]['addrbind'], peer_info[1][0]['addr'])
-        assert_equal(peer_info[1][0]['addrbind'], peer_info[0][0]['addr'])
-        # check the `servicesnames` field
-        for info in peer_info:
-            assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"])
-
-        # Check dynamically generated networks list in getpeerinfo help output.
-        assert "(ipv4, ipv6, onion, i2p, not_publicly_routable)" in self.nodes[0].help("getpeerinfo")
-        # This part is slightly different comparing to the Bitcoin implementation. This is expected because we create connections on network setup a bit differently too.
-        # We also create more connection during the test itself to test mn specific stats
-        assert_equal(peer_info[0][0]['connection_type'], 'inbound')
-        assert_equal(peer_info[0][1]['connection_type'], 'inbound')
-        assert_equal(peer_info[0][2]['connection_type'], 'manual')
-
-        assert_equal(peer_info[1][0]['connection_type'], 'manual')
-        assert_equal(peer_info[1][1]['connection_type'], 'inbound')
-
-        assert_equal(peer_info[2][0]['connection_type'], 'manual')
-
     def test_service_flags(self):
         self.log.info("Test service flags")
         self.nodes[0].add_p2p_connection(P2PInterface(), services=(1 << 4) | (1 << 63))

test/functional/test_runner.py

@@ -334,7 +334,7 @@ BASE_SCRIPTS = [
     'feature_config_args.py',
     'feature_settings.py',
     'rpc_getdescriptorinfo.py',
-    'rpc_getpeerinfo_banscore_deprecation.py',
+    'rpc_getpeerinfo_deprecation.py',
     'rpc_help.py',
     'feature_help.py',
     'feature_blockfilterindex_prune.py'

test/functional/wallet_basic.py

@@ -126,7 +126,7 @@ class WalletTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "Invalid parameter, expected locked output", self.nodes[2].lockunspent, True, [unspent_0])
         self.nodes[2].lockunspent(False, [unspent_0])
         assert_raises_rpc_error(-8, "Invalid parameter, output already locked", self.nodes[2].lockunspent, False, [unspent_0])
-        assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 200)
+        assert_raises_rpc_error(-6, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 200)
         assert_equal([unspent_0], self.nodes[2].listlockunspent())
         self.nodes[2].lockunspent(True, [unspent_0])
         assert_equal(len(self.nodes[2].listlockunspent()), 0)
@@ -380,6 +380,9 @@ class WalletTest(BitcoinTestFramework):
         assert_equal(tx_obj['amount'], Decimal('-0.0001'))
 
         # General checks for errors from incorrect inputs
+        # This will raise an exception because the amount is negative
+        assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "-1")
+
         # This will raise an exception because the amount type is wrong
         assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "1f-4")
 
@@ -608,7 +611,7 @@ class WalletTest(BitcoinTestFramework):
 
         node0_balance = self.nodes[0].getbalance()
         # With walletrejectlongchains we will not create the tx and store it in our wallet.
-        assert_raises_rpc_error(-4, "Transaction has too long of a mempool chain", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01'))
+        assert_raises_rpc_error(-6, "Transaction has too long of a mempool chain", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01'))
 
         # Verify nothing new in wallet
         assert_equal(total_txs, len(self.nodes[0].listtransactions("*", 99999)))
@@ -670,6 +673,18 @@ class WalletTest(BitcoinTestFramework):
         assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
         assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
 
+        self.log.info("Test send* RPCs with verbose=True")
+        address = self.nodes[0].getnewaddress("test")
+        txid_feeReason_one = self.nodes[2].sendtoaddress(address=address, amount=5, verbose=True)
+        assert_equal(txid_feeReason_one["fee_reason"], "Fallback fee")
+        txid_feeReason_two = self.nodes[2].sendmany(dummy='', amounts={address: 5}, verbose=True)
+        assert_equal(txid_feeReason_two["fee_reason"], "Fallback fee")
+        self.log.info("Test send* RPCs with verbose=False")
+        txid_feeReason_three = self.nodes[2].sendtoaddress(address=address, amount=5, verbose=False)
+        assert_equal(self.nodes[2].gettransaction(txid_feeReason_three)['txid'], txid_feeReason_three)
+        txid_feeReason_four = self.nodes[2].sendmany(dummy='', amounts={address: 5}, verbose=False)
+        assert_equal(self.nodes[2].gettransaction(txid_feeReason_four)['txid'], txid_feeReason_four)
+
 
 if __name__ == '__main__':
     WalletTest().main()

test/functional/wallet_fallbackfee.py

@@ -24,7 +24,7 @@ class WalletRBFTest(BitcoinTestFramework):
 
         # test sending a tx with disabled fallback fee (must fail)
         self.restart_node(0, extra_args=["-fallbackfee=0"])
-        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
+        assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
         assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
         assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))