Fix conditions around dust thresholds.

qa/rpc-tests/wallet_shieldingcoinbase.py

@@ -61,10 +61,10 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         assert_equal(self.nodes[2].getbalance(), 0)
         assert_equal(self.nodes[3].getbalance(), 0)
 
-        check_value_pool(self.nodes[0], 'sprout', 0)
+        check_value_pool(self.nodes[0], 'sapling', 0)
-        check_value_pool(self.nodes[1], 'sprout', 0)
+        check_value_pool(self.nodes[1], 'sapling', 0)
-        check_value_pool(self.nodes[2], 'sprout', 0)
+        check_value_pool(self.nodes[2], 'sapling', 0)
-        check_value_pool(self.nodes[3], 'sprout', 0)
+        check_value_pool(self.nodes[3], 'sapling', 0)
 
         # Send will fail because we are enforcing the consensus rule that
         # coinbase utxos can only be sent to a zaddr.
@@ -77,7 +77,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
 
         # Prepare to send taddr->zaddr
         mytaddr = get_coinbase_address(self.nodes[0])
-        myzaddr = self.nodes[0].z_getnewaddress('sprout')
+        myzaddr = self.nodes[0].z_getnewaddress('sapling')
 
         # Node 3 will test that watch only address utxos are not selected
         self.nodes[3].importaddress(mytaddr)
@@ -86,7 +86,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
             myopid = self.nodes[3].z_sendmany(mytaddr, recipients)
         except JSONRPCException as e:
             errorString = e.error['message']
-        assert_equal("Invalid from address: does not belong to this node, spending key not found.", errorString);
+        assert_equal("Invalid from address, no spending key found for address", errorString);
 
         # This send will fail because our wallet does not allow any change when shielding a coinbase utxo,
         # as it's currently not possible to specify a change address in z_sendmany.
@@ -94,9 +94,11 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')})
 
         myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
-        error_result = wait_and_assert_operationid_status_result(self.nodes[0], myopid, "failed", ("Change 8.76542211 not allowed. "
+        error_result = wait_and_assert_operationid_status_result(
-            "When shielding coinbase funds, the wallet does not allow any change "
+                self.nodes[0], 
-            "as there is currently no way to specify a change address in z_sendmany."), 10)
+                myopid, "failed", 
+                "When shielding coinbase funds, the wallet does not allow any change. The proposed transaction would result in 8.76542211 in change.", 
+                10)
 
         # Test that the returned status object contains a params field with the operation's input parameters
         assert_equal(error_result["method"], "z_sendmany")
@@ -167,8 +169,8 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         assert_equal(Decimal(resp["total"]), Decimal('40.0') - DEFAULT_FEE)
 
         # The Sprout value pool should reflect the send
-        sproutvalue = shieldvalue
+        saplingvalue = shieldvalue
-        check_value_pool(self.nodes[0], 'sprout', sproutvalue)
+        check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
         # A custom fee of 0 is okay.  Here the node will send the note value back to itself.
         recipients = []
@@ -183,8 +185,8 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         assert_equal(Decimal(resp["private"]), Decimal('20.0') - DEFAULT_FEE)
         assert_equal(Decimal(resp["total"]), Decimal('40.0') - DEFAULT_FEE)
 
-        # The Sprout value pool should be unchanged
+        # The Sapling value pool should be unchanged
-        check_value_pool(self.nodes[0], 'sprout', sproutvalue)
+        check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
         # convert note to transparent funds
         unshieldvalue = Decimal('10.0')
@@ -203,12 +205,12 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         self.sync_all()
 
         # check balances
-        sproutvalue -= unshieldvalue + DEFAULT_FEE
+        saplingvalue -= unshieldvalue + DEFAULT_FEE
         resp = self.nodes[0].z_gettotalbalance()
         assert_equal(Decimal(resp["transparent"]), Decimal('30.0'))
         assert_equal(Decimal(resp["private"]), Decimal('10.0') - 2*DEFAULT_FEE)
         assert_equal(Decimal(resp["total"]), Decimal('40.0') - 2*DEFAULT_FEE)
-        check_value_pool(self.nodes[0], 'sprout', sproutvalue)
+        check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
         # z_sendmany will return an error if there is transparent change output considered dust.
         # UTXO selection in z_sendmany sorts in ascending order, so smallest utxos are consumed first.
@@ -217,7 +219,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         amount = Decimal('10.0') - DEFAULT_FEE - Decimal('0.00000001')    # this leaves change at 1 zatoshi less than dust threshold
         recipients.append({"address":self.nodes[0].getnewaddress(), "amount":amount })
         myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
-        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient transparent funds, have 10.00, need 0.00000053 more to avoid creating invalid change output 0.00000001 (dust threshold is 0.00000054)")
+        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 10.00, need 0.00000053 more to avoid creating invalid change output 0.00000001 (dust threshold is 0.00000054)")
 
         # Send will fail because send amount is too big, even when including coinbase utxos
         errorString = ""
@@ -231,9 +233,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         recipients = []
         recipients.append({"address":self.nodes[1].getnewaddress(), "amount":Decimal('10000.0')})
         myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
-        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient transparent funds, have 10.00, need 10000.00001")
+        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 10.00, need 10000.00001; if you are attempting to shield transparent coinbase funds, ensure that you have specified only a single recipient address.")
         myopid = self.nodes[0].z_sendmany(myzaddr, recipients)
-        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient shielded funds, have 9.99998, need 10000.00001")
+        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 9.99998, need 10000.00001; if you are attempting to shield transparent coinbase funds, ensure that you have specified only a single recipient address.")
 
         # Send will fail because of insufficient funds unless sender uses coinbase utxos
         try:
@@ -284,9 +286,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
 
         # check balance
         node2balance = amount_per_recipient * num_t_recipients
-        sproutvalue -= node2balance + DEFAULT_FEE
+        saplingvalue -= node2balance + DEFAULT_FEE
         assert_equal(self.nodes[2].getbalance(), node2balance)
-        check_value_pool(self.nodes[0], 'sprout', sproutvalue)
+        check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
         # Send will fail because fee is negative
         try:
@@ -332,7 +334,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         custom_fee = Decimal('0.00012345')
         zbalance = self.nodes[0].z_getbalance(myzaddr)
         for i in range(0,num_recipients):
-            newzaddr = self.nodes[2].z_getnewaddress('sprout')
+            newzaddr = self.nodes[2].z_getnewaddress('sapling')
             recipients.append({"address":newzaddr, "amount":amount_per_recipient})
         myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, custom_fee)
         wait_and_assert_operationid_status(self.nodes[0], myopid)
@@ -350,8 +352,8 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
 
         resp = self.nodes[0].z_getbalance(myzaddr)
         assert_equal(Decimal(resp), zbalance - custom_fee - send_amount)
-        sproutvalue -= custom_fee
+        saplingvalue -= custom_fee
-        check_value_pool(self.nodes[0], 'sprout', sproutvalue)
+        check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
         notes = self.nodes[0].z_listunspent(1, 99999, False, [myzaddr])
         sum_of_notes = sum([note["amount"] for note in notes])

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -161,12 +161,17 @@ void AsyncRPCOperation_sendmany::main() {
     LogPrintf("%s",s);
 }
 
-struct TxValues {
+class TxValues {
+public:
     CAmount t_inputs_total{0};
     CAmount z_inputs_total{0};
     CAmount t_outputs_total{0};
     CAmount z_outputs_total{0};
     CAmount targetAmount{0};
+
+    CAmount GetChangeAmount() {
+        return t_inputs_total + z_inputs_total - targetAmount;
+    }
 };
 
 bool IsFromAnyTaddr(const PaymentSource& paymentSource) {
@@ -231,17 +236,40 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
         !IsFromAnyTaddr(paymentSource_) && // allow coinbase inputs from at most a single t-addr
         transparentRecipients == 0; // cannot send transparent coinbase to transparent recipients
 
+    // Set the dust threshold so that we can select enough inputs to avoid
+    // creating dust change amounts.
+    CAmount dustThreshold{DefaultDustThreshold()};
+
     // Find spendable inputs, and select a minimal set of them that
     // can supply the required target amount.
     auto spendable = FindSpendableInputs(allowTransparentCoinbase);
-    if (!spendable.LimitToAmount(txValues.targetAmount)) {
+    if (!spendable.LimitToAmount(txValues.targetAmount, dustThreshold)) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
+        CAmount changeAmount{spendable.Total() - txValues.targetAmount};
-            strprintf("Insufficient funds: have %s, need %s",
+        if (changeAmount > 0 && changeAmount < dustThreshold) {
-            FormatMoney(spendable.Total()), FormatMoney(txValues.targetAmount))
+            // TODO: this condition is silly; we should provide the option for the caller to
-            + (allowTransparentCoinbase ? "" :
+            // forego change (definitionally amount below the dust amount) and send the
-                "; if you are attempting to shield transparent coinbase funds, "
+            // extra to the recipient or the miner fee to avoid creating dust change, rather
-                "ensure that you have specified only a single recipient address.")
+            // than prohibit them from sending entirely in this circumstance.
-            );
+            throw JSONRPCError(
+                    RPC_WALLET_INSUFFICIENT_FUNDS,
+                    strprintf(
+                        "Insufficient funds: have %s, need %s more to avoid creating invalid change output %s "
+                        "(dust threshold is %s)",
+                        FormatMoney(spendable.Total()),
+                        FormatMoney(dustThreshold - changeAmount),
+                        FormatMoney(changeAmount),
+                        FormatMoney(dustThreshold)));
+        } else {
+            throw JSONRPCError(
+                    RPC_WALLET_INSUFFICIENT_FUNDS,
+                    strprintf(
+                        "Insufficient funds: have %s, need %s",
+                        FormatMoney(spendable.Total()), FormatMoney(txValues.targetAmount))
+                        + (allowTransparentCoinbase ? "" :
+                            "; if you are attempting to shield transparent coinbase funds, "
+                            "ensure that you have specified only a single recipient address.")
+                    );
+        }
     }
 
     spendable.LogInputs(getId());
@@ -535,44 +563,59 @@ SpendableInputs AsyncRPCOperation_sendmany::FindSpendableInputs(bool allowTransp
     return unspent;
 }
 
-bool SpendableInputs::LimitToAmount(CAmount amount) {
+bool SpendableInputs::LimitToAmount(const CAmount amountRequired, const CAmount dustThreshold) {
+    assert(amountRequired >= 0 && dustThreshold > 0);
+
+    CAmount totalSelected{0};
+    auto haveSufficientFunds = [&]() {
+        // if the total would result in change below the dust threshold,
+        // we do not yet have sufficient funds
+        return totalSelected == amountRequired || totalSelected - amountRequired > dustThreshold;
+    };
+
     // select Sprout notes for spending first
-    std::sort(sproutNoteEntries.begin(), sproutNoteEntries.end(),
+    if (!haveSufficientFunds()) {
-        [](SproutNoteEntry i, SproutNoteEntry j) -> bool {
+        std::sort(sproutNoteEntries.begin(), sproutNoteEntries.end(),
-            return i.note.value() > j.note.value();
+            [](SproutNoteEntry i, SproutNoteEntry j) -> bool {
-        });
+                return i.note.value() > j.note.value();
-    auto sproutIt = sproutNoteEntries.begin();
+            });
-    while (sproutIt != sproutNoteEntries.end() && amount > 0) {
+        auto sproutIt = sproutNoteEntries.begin();
-        amount -= sproutIt->note.value();
+        while (sproutIt != sproutNoteEntries.end() && !haveSufficientFunds()) {
-        ++sproutIt;
+            totalSelected += sproutIt->note.value();
+            ++sproutIt;
+        }
+        sproutNoteEntries.erase(sproutIt, sproutNoteEntries.end());
     }
-    sproutNoteEntries.erase(sproutIt, sproutNoteEntries.end());
 
     // next select transparent utxos
-    std::sort(utxos.begin(), utxos.end(),
+    if (!haveSufficientFunds()) {
-        [](COutput i, COutput j) -> bool {
+        std::sort(utxos.begin(), utxos.end(),
-            return i.Value() > j.Value();
+            [](COutput i, COutput j) -> bool {
-        });
+                return i.Value() > j.Value();
-    auto utxoIt = utxos.begin();
+            });
-    while (utxoIt != utxos.end() && amount > 0) {
+        auto utxoIt = utxos.begin();
-        amount -= utxoIt->Value();
+        while (utxoIt != utxos.end() && !haveSufficientFunds()) {
-        ++utxoIt;
+            totalSelected += utxoIt->Value();
+            ++utxoIt;
+        }
+        utxos.erase(utxoIt, utxos.end());
     }
-    utxos.erase(utxoIt, utxos.end());
 
     // finally select Sapling outputs
-    std::sort(saplingNoteEntries.begin(), saplingNoteEntries.end(),
+    if (!haveSufficientFunds()) {
-        [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool {
+        std::sort(saplingNoteEntries.begin(), saplingNoteEntries.end(),
-            return i.note.value() > j.note.value();
+            [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool {
-        });
+                return i.note.value() > j.note.value();
-    auto saplingIt = saplingNoteEntries.begin();
+            });
-    while (saplingIt != saplingNoteEntries.end() && amount > 0) {
+        auto saplingIt = saplingNoteEntries.begin();
-        amount -= saplingIt->note.value();
+        while (saplingIt != saplingNoteEntries.end() && !haveSufficientFunds()) {
-        ++saplingIt;
+            totalSelected += saplingIt->note.value();
+            ++saplingIt;
+        }
+        saplingNoteEntries.erase(saplingIt, saplingNoteEntries.end());
     }
-    saplingNoteEntries.erase(saplingIt, saplingNoteEntries.end());
 
-    return (amount <= 0);
+    return haveSufficientFunds();
 }
 
 bool SpendableInputs::HasTransparentCoinbase() const {
@@ -609,6 +652,18 @@ void SpendableInputs::LogInputs(const AsyncRPCOperationId& id) const {
     }
 }
 
+/**
+ * Compute a dust threshold based upon a standard p2pkh txout.
+ */
+CAmount AsyncRPCOperation_sendmany::DefaultDustThreshold() {
+    CKey secret;
+    secret.MakeNewKey(true);
+    CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID());
+    CTxOut txout(CAmount(1), scriptPubKey);
+    // TODO: use a local for minRelayTxFee rather than a global
+    return txout.GetDustThreshold(minRelayTxFee);
+}
+
 std::array<unsigned char, ZC_MEMO_SIZE> AsyncRPCOperation_sendmany::get_memo_from_hex_string(std::string s) {
     // initialize to default memo (no_memo), see section 5.5 of the protocol spec
     std::array<unsigned char, ZC_MEMO_SIZE> memo = {{0xF6}};
@@ -622,7 +677,9 @@ std::array<unsigned char, ZC_MEMO_SIZE> AsyncRPCOperation_sendmany::get_memo_fro
     }
 
     if (rawMemo.size() > ZC_MEMO_SIZE) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Memo size of %d is too big, maximum allowed is %d", rawMemo.size(), ZC_MEMO_SIZE));
+        throw JSONRPCError(
+                RPC_INVALID_PARAMETER,
+                strprintf("Memo size of %d is too big, maximum allowed is %d", rawMemo.size(), ZC_MEMO_SIZE));
     }
 
     // copy vector into boost array

src/wallet/asyncrpcoperation_sendmany.h

@@ -53,7 +53,7 @@ public:
      * amount. Returns `false` if the available inputs do not add up to the
      * desired amount.
      */
-    bool LimitToAmount(CAmount amount);
+    bool LimitToAmount(CAmount amount, CAmount dustThreshold);
 
     /**
      * Compute the total ZEC amount of spendable inputs.
@@ -122,7 +122,9 @@ private:
 
     SpendableInputs FindSpendableInputs(bool fAcceptCoinbase);
 
-    std::array<unsigned char, ZC_MEMO_SIZE> get_memo_from_hex_string(std::string s);
+    static CAmount DefaultDustThreshold();
+
+    static std::array<unsigned char, ZC_MEMO_SIZE> get_memo_from_hex_string(std::string s);
 
     uint256 main_impl();
 };

src/wallet/test/rpc_wallet_tests.cpp

@@ -1268,7 +1268,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
         operation->main();
         BOOST_CHECK(operation->isFailed());
         std::string msg = operation->getErrorMessage();
-        BOOST_CHECK(msg.find("Insufficient funds, have 0.00") != string::npos);
+        BOOST_CHECK(msg.find("Insufficient funds: have 0.00") != string::npos);
     }
 
     // get_memo_from_hex_string())