Merge #2341: [Wallet][RPC] Add subtract-fee-from-amount option in Cre…

…ate[Fund]Transaction

a1bd590 [Doc] Document subtract-fee-from-amount RPC changes in the release notes (random-zebra)
6589429 wallet: first step generalizing CRecipient and ShieldedRecipient usage creating CRecipientBase class. (furszy)
1239506 [QA] Properly test change position in fundraw with sffa (random-zebra)
cc9ff09 [RPC] Add fSubtractFeeFromAmount to shieldsendmany (random-zebra)
65d3a80 [Refactoring] Subtract fee from destination when building shield tx (random-zebra)
cba898b [Refactoring] SendManyRecipient: use CRecipient for taddr + add sffa (random-zebra)
6fff9f5 [RPC] Add fSubtractFeeFromAmount to sendmany (transparent) (random-zebra)
7e30e3f [MOVE-ONLY] Move fundrawtransaction to rpcwallet (random-zebra)
499d62a [Tests] Add functional test for sffa in fundrawtransaction (random-zebra)
523b24d Subtract fee from amount (random-zebra)

Pull request description:

  Add a feature requested many times (#894, #1079, #2196, ...)

  Allow the user to deduct the fee from one or more of the transaction recipient amounts.
  The most common use-case is when sending the whole balance, or a selection of UTXOs, without getting any change back.
  This is already possible by selecting "after-fee-amount" in coin-control, but such metod is not user-friendly (e.g. in case of shield recipients, to get the correct value, the destinations must already be filled before opening coin-control and copying the "after-fee" value).
  Plus, this workaround is only available in the GUI.

  The subtract-fee option makes it easier, and enables it via the cli as well.

  Here it's exposed only to the RPC, both for transparent and shield recipients:
  - `sendtoaddress`
  - `sendmany`
  - `shieldsendmany`
  - `fundrawtransaction`

  GUI connection will be added in a successive PR (which will close those issues).

  Last commit is coming from bitcoin#10294

  Built on top of:
  - [x] #2337

ACKs for top commit:
  furszy:
    rebase + cherry-pick utACK a1bd590.
  Fuzzbawls:
    ACK a1bd590

Tree-SHA512: 7ed78f6ab8f1e9300d1468242e66016a4e1867f413256de23b7a60e103c2b628cdadcfc7998e8a7b252a7c2817d45261be108fad0eaa34d5ff892052c09b6e60

doc/release-notes.md

@@ -80,7 +80,6 @@ The `getwalletinfo` RPC method returns the name of the wallet used for the query
 Note that while multi-wallet is now fully supported, the RPC multi-wallet interface should be considered unstable for version 6.0.0, and there may backwards-incompatible changes in future versions.
 
 
-
 GUI changes
 -----------
 
@@ -110,6 +109,7 @@ Results without keys can be queried using an integer in brackets.
     example:    getblock(getblockhash(0),true)[tx][0]
 ```
 
+
 Support for JSON-RPC Named Arguments
 ------------------------------------
 
@@ -129,14 +129,11 @@ The order of arguments doesn't matter in this case. Named arguments are also use
 The RPC server remains fully backwards compatible with positional arguments.
 
 
-#### Allow to optional specify the directory for the blocks storage
-
-A new init option flag '-blocksdir' will allow one to keep the blockfiles external from the data directory.
-
-
 Low-level RPC changes
 ---------------------
 
+### Query options for listunspent RPC
+
 - The `listunspent` RPC now takes a `query_options` argument (see [PR #2317](https://github.com/PIVX-Project/PIVX/pull/2317)), which is a JSON object
   containing one or more of the following members:
   - `minimumAmount` - a number specifying the minimum value of each UTXO
@@ -151,12 +148,37 @@ Low-level RPC changes
 
 - the `stop` RPC no longer accepts the (already deprecated, ignored, and undocumented) optional boolean argument `detach`.
 
+### Subtract fee from recipient amount for RPC
+
+A new argument `subtract_fee_from` is added to `sendmany`/`shieldsendmany` RPC functions.
+It can be used to provide the list of recipent addresses paying the fee.
+```
+subtract_fee_from         (array, optional) 
+A json array with addresses.
+The fee will be equally deducted from the amount of each selected address.
+  [\"address\"          (string) Subtract fee from this address\n"
+   ,...
+  ]
+
+For `fundrawtransaction` a new key (`subtractFeeFromOutputs`) is added to the `options` argument.
+It can be used to specify a list of output indexes.
+```
+subtractFeeFromOutputs    (array, optional)  A json array of integers.
+The fee will be equally deducted from the amount of each specified output.
+The outputs are specified by their zero-based index, before any change output is added.
+  [vout_index,...]
+```
+
+For `sendtoaddress`, the new parameter is called `subtract_fee` and it is a simple boolean.
 
-#### Show wallet's auto-combine settings in getwalletinfo
+In all cases those recipients will receive less PIV than you enter in their corresponding amount field.
+If no outputs/addresses are specified, the sender pays the fee as usual.
+
+### Show wallet's auto-combine settings in getwalletinfo
 
 `getwalletinfo` now has two additional return fields. `autocombine_enabled` (boolean) and `autocombine_threshold` (numeric) that will show the auto-combine threshold and whether or not it is currently enabled.
 
-#### Deprecate the autocombine RPC command
+### Deprecate the autocombine RPC command
 
 The `autocombine` RPC command has been replaced with specific set/get commands (`setautocombinethreshold` and `getautocombinethreshold`, respectively) to bring this functionality further in-line with our RPC standards. Previously, the `autocombine` command gave no user-facing feedback when the setting was changed. This is now resolved with the introduction of the two new commands as detailed below:
 
@@ -191,26 +213,18 @@ The `autocombine` RPC command has been replaced with specific set/get commands (
     }
     ```
 
-#### Build system changes
+Build system changes
+--------------------
 
 The minimum supported miniUPnPc API version is set to 10. This keeps compatibility with Ubuntu 16.04 LTS and Debian 8 `libminiupnpc-dev` packages. Please note, on Debian this package is still vulnerable to [CVE-2017-8798](https://security-tracker.debian.org/tracker/CVE-2017-8798) (in jessie only) and [CVE-2017-1000494](https://security-tracker.debian.org/tracker/CVE-2017-1000494) (both in jessie and in stretch).
 
-
-#### Build System
-
 OpenSSL is no longer used by PIVX Core
 
 
-#### Disable PoW mining RPC Commands
-
-A new configure flag has been introduced to allow more granular control over weather or not the PoW mining RPC commands are compiled into the wallet. By default they are not. This behavior can be overridden by passing `--enable-mining-rpc` to the `configure` script.
-
-#### Removed startup options
-
-- `printstakemodifier`
+Configuration changes
+---------------------
 
-Configuration sections for testnet and regtest
-----------------------------------------------
+### Configuration sections for testnet and regtest
 
 It is now possible for a single configuration file to set different options for different networks. This is done by using sections or by prefixing the option with the network, such as:
 
@@ -226,16 +240,27 @@ It is now possible for a single configuration file to set different options for
 
 The `addnode=`, `connect=`, `port=`, `bind=`, `rpcport=`, `rpcbind=`, and `wallet=` options will only apply to mainnet when specified in the configuration file, unless a network is specified.
 
+### Allow to optional specify the directory for the blocks storage
 
-#### Logging
+A new init option flag '-blocksdir' will allow one to keep the blockfiles external from the data directory.
 
-The log timestamp format is now ISO 8601 (e.g. "2021-02-28T12:34:56Z").
+### Disable PoW mining RPC Commands
 
+A new configure flag has been introduced to allow more granular control over weather or not the PoW mining RPC commands are compiled into the wallet. By default they are not. This behavior can be overridden by passing `--enable-mining-rpc` to the `configure` script.
+
+### Removed startup options
+
+- `printstakemodifier`
 
-#### Automatic Backup File Naming
+### Logging
+
+The log timestamp format is now ISO 8601 (e.g. "2021-02-28T12:34:56Z").
+
+### Automatic Backup File Naming
 
 The file extension applied to automatic backups is now in ISO 8601 basic notation (e.g. "20210228T123456Z"). The basic notation is used to prevent illegal `:` characters from appearing in the filename.
 
+
 *version* Change log
 ==============
 

src/qt/walletmodel.cpp

@@ -598,16 +598,17 @@ OperationResult WalletModel::PrepareShieldedTransaction(WalletModelTransaction*
                                                         const CCoinControl* coinControl)
 {
     // Load shieldedAddrRecipients.
+    bool fSubtractFeeFromAmount{false};
     std::vector<SendManyRecipient> recipients;
     for (const auto& recipient : modelTransaction->getRecipients()) {
         if (recipient.isShieldedAddr) {
             auto pa = KeyIO::DecodeSaplingPaymentAddress(recipient.address.toStdString());
             if (!pa) return errorOut("Error, invalid shielded address");
-            recipients.emplace_back(*pa, recipient.amount, recipient.message.toStdString());
+            recipients.emplace_back(*pa, recipient.amount, recipient.message.toStdString(), fSubtractFeeFromAmount);
         } else {
             auto dest = DecodeDestination(recipient.address.toStdString());
             if (!IsValidDestination(dest)) return errorOut("Error, invalid transparent address");
-            recipients.emplace_back(dest, recipient.amount);
+            recipients.emplace_back(dest, recipient.amount, fSubtractFeeFromAmount);
         }
     }
 

src/rpc/client.cpp

@@ -134,8 +134,10 @@ static const CRPCConvertParam vRPCConvertParams[] = {
     { "rescanblockchain", 1, "stop_height"},
     { "sendmany", 1, "amounts" },
     { "sendmany", 2, "minconf" },
+    { "sendmany", 5, "subtract_fee_from" },
     { "sendrawtransaction", 1, "allowhighfees" },
     { "sendtoaddress", 1, "amount" },
+    { "sendtoaddress", 4, "subtract_fee" },
     { "setautocombinethreshold", 0, "enable" },
     { "setautocombinethreshold", 1, "threshold" },
     { "setban", 2, "bantime" },
@@ -149,6 +151,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {
     { "shieldsendmany", 1, "amounts" },
     { "shieldsendmany", 2, "minconf" },
     { "shieldsendmany", 3, "fee" },
+    { "shieldsendmany", 4, "subtract_fee_from" },
     { "signrawtransaction", 1, "prevtxs" },
     { "signrawtransaction", 2, "privkeys" },
     { "spork", 1, "value" },

src/rpc/rawtransaction.cpp

@@ -484,127 +484,6 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::
     vErrorsRet.push_back(entry);
 }
 
-UniValue fundrawtransaction(const JSONRPCRequest& request)
-{
-    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
-
-    if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
-        return NullUniValue;
-
-    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
-        throw std::runtime_error(
-            "fundrawtransaction \"hexstring\" ( options )\n"
-            "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n"
-            "This will not modify existing inputs, and will add one change output to the outputs.\n"
-            "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n"
-            "The inputs added will not be signed, use signrawtransaction for that.\n"
-            "Note that all existing inputs must have their previous output transaction be in the wallet.\n"
-            "Note that all inputs selected must be of standard form and P2SH scripts must be "
-            "in the wallet using importaddress or addmultisigaddress (to calculate fees).\n"
-            "You can see whether this is the case by checking the \"solvable\" field in the listunspent output.\n"
-            "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n"
-
-            "\nArguments:\n"
-            "1. \"hexstring\"    (string, required) The hex string of the raw transaction\n"
-            "2. options          (object, optional)\n"
-            "   {\n"
-            "     \"changeAddress\"     (string, optional, default pool address) The PIVX address to receive the change\n"
-            "     \"changePosition\"    (numeric, optional, default random) The index of the change output\n"
-            "     \"includeWatching\"   (boolean, optional, default false) Also select inputs which are watch only\n"
-            "     \"lockUnspents\"      (boolean, optional, default false) Lock selected unspent outputs\n"
-                " \"feeRate\"           (numeric, optional, default 0=estimate) Set a specific feerate (PIV per KB)\n"
-            "   }\n"
-            "\nResult:\n"
-            "{\n"
-            "  \"hex\":       \"value\", (string)  The resulting raw transaction (hex-encoded string)\n"
-            "  \"fee\":       n,         (numeric) The fee added to the transaction\n"
-            "  \"changepos\": n          (numeric) The position of the added change output, or -1\n"
-            "}\n"
-            "\"hex\"             \n"
-            "\nExamples:\n"
-            "\nCreate a transaction with no inputs\n"
-            + HelpExampleCli("createrawtransaction", "\"[]\" \"{\\\"myaddress\\\":0.01}\"") +
-            "\nAdd sufficient unsigned inputs to meet the output value\n"
-            + HelpExampleCli("fundrawtransaction", "\"rawtransactionhex\"") +
-            "\nSign the transaction\n"
-            + HelpExampleCli("signrawtransaction", "\"fundedtransactionhex\"") +
-            "\nSend the transaction\n"
-            + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"")
-            );
-
-    // 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
-    pwallet->BlockUntilSyncedToCurrentChain();
-
-    RPCTypeCheck(request.params, {UniValue::VSTR});
-
-    CTxDestination changeAddress = CNoDestination();
-    int changePosition = -1;
-    bool includeWatching = false;
-    bool lockUnspents = false;
-    CFeeRate feeRate = CFeeRate(0);
-    bool overrideEstimatedFeerate = false;
-
-    if (request.params.size() > 1) {
-        RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VOBJ});
-        UniValue options = request.params[1];
-        RPCTypeCheckObj(options,
-            {
-                {"changeAddress", UniValueType(UniValue::VSTR)},
-                {"changePosition", UniValueType(UniValue::VNUM)},
-                {"includeWatching", UniValueType(UniValue::VBOOL)},
-                {"lockUnspents", UniValueType(UniValue::VBOOL)},
-                {"feeRate", UniValueType()}, // will be checked below
-            },
-            true, true);
-
-        if (options.exists("changeAddress")) {
-            changeAddress = DecodeDestination(options["changeAddress"].get_str());
-
-            if (!IsValidDestination(changeAddress))
-                throw JSONRPCError(RPC_INVALID_PARAMETER, "changeAddress must be a valid PIVX address");
-        }
-
-        if (options.exists("changePosition"))
-            changePosition = options["changePosition"].get_int();
-
-        if (options.exists("includeWatching"))
-            includeWatching = options["includeWatching"].get_bool();
-
-        if (options.exists("lockUnspents"))
-            lockUnspents = options["lockUnspents"].get_bool();
-
-        if (options.exists("feeRate")) {
-            feeRate = CFeeRate(AmountFromValue(options["feeRate"]));
-            overrideEstimatedFeerate = true;
-        }
-    }
-
-    // parse hex string from parameter
-    CMutableTransaction origTx;
-    if (!DecodeHexTx(origTx, request.params[0].get_str()))
-        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
-
-    if (origTx.vout.size() == 0)
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output");
-
-    if (changePosition != -1 && (changePosition < 0 || (unsigned int) changePosition > origTx.vout.size()))
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds");
-
-    CMutableTransaction tx(origTx);
-    CAmount nFeeOut;
-    std::string strFailReason;
-    if(!pwallet->FundTransaction(tx, nFeeOut, overrideEstimatedFeerate, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress))
-        throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason);
-
-    UniValue result(UniValue::VOBJ);
-    result.pushKV("hex", EncodeHexTx(tx));
-    result.pushKV("changepos", changePosition);
-    result.pushKV("fee", ValueFromAmount(nFeeOut));
-
-    return result;
-}
-
 UniValue signrawtransaction(const JSONRPCRequest& request)
 {
     CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
@@ -1000,7 +879,6 @@ static const CRPCCommand commands[] =
     { "rawtransactions",    "createrawtransaction",   &createrawtransaction,   true,  {"inputs","outputs","locktime"} },
     { "rawtransactions",    "decoderawtransaction",   &decoderawtransaction,   true,  {"hexstring"}  },
     { "rawtransactions",    "decodescript",           &decodescript,           true,  {"hexstring"} },
-    { "rawtransactions",    "fundrawtransaction",     &fundrawtransaction,     false, {"hexstring","options"} },
     { "rawtransactions",    "getrawtransaction",      &getrawtransaction,      true,  {"txid","verbose","blockhash"} },
     { "rawtransactions",    "sendrawtransaction",     &sendrawtransaction,     false, {"hexstring","allowhighfees"} },
     { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     false, {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */

src/rpc/rpcevo.cpp

@@ -224,7 +224,7 @@ static void FundSpecialTx(CWallet* pwallet, CMutableTransaction& tx, SpecialTxPa
     int nChangePos = -1;
     std::string strFailReason;
     std::set<int> setSubtractFeeFromOutputs;
-    if (!pwallet->FundTransaction(tx, nFee, false, feeRate, nChangePos, strFailReason, false, false))
+    if (!pwallet->FundTransaction(tx, nFee, false, feeRate, nChangePos, strFailReason, false, false, {}))
         throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason);
 
     if (dummyTxOutAdded && tx.vout.size() > 1) {