diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 129af7011..c3a64b49c 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/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[1], 'sprout', 0) - check_value_pool(self.nodes[2], 'sprout', 0) - check_value_pool(self.nodes[3], 'sprout', 0) + check_value_pool(self.nodes[0], 'sapling', 0) + check_value_pool(self.nodes[1], 'sapling', 0) + check_value_pool(self.nodes[2], 'sapling', 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. " - "When shielding coinbase funds, the wallet does not allow any change " - "as there is currently no way to specify a change address in z_sendmany."), 10) + error_result = wait_and_assert_operationid_status_result( + self.nodes[0], + 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 - check_value_pool(self.nodes[0], 'sprout', sproutvalue) + saplingvalue = shieldvalue + 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 - check_value_pool(self.nodes[0], 'sprout', sproutvalue) + # The Sapling value pool should be unchanged + 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 - check_value_pool(self.nodes[0], 'sprout', sproutvalue) + saplingvalue -= custom_fee + 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]) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 0ce35a0f9..841636eb6 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/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)) { - 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.") - ); + if (!spendable.LimitToAmount(txValues.targetAmount, dustThreshold)) { + CAmount changeAmount{spendable.Total() - txValues.targetAmount}; + if (changeAmount > 0 && changeAmount < dustThreshold) { + // TODO: this condition is silly; we should provide the option for the caller to + // forego change (definitionally amount below the dust amount) and send the + // extra to the recipient or the miner fee to avoid creating dust change, rather + // 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(), - [](SproutNoteEntry i, SproutNoteEntry j) -> bool { - return i.note.value() > j.note.value(); - }); - auto sproutIt = sproutNoteEntries.begin(); - while (sproutIt != sproutNoteEntries.end() && amount > 0) { - amount -= sproutIt->note.value(); - ++sproutIt; + if (!haveSufficientFunds()) { + std::sort(sproutNoteEntries.begin(), sproutNoteEntries.end(), + [](SproutNoteEntry i, SproutNoteEntry j) -> bool { + return i.note.value() > j.note.value(); + }); + auto sproutIt = sproutNoteEntries.begin(); + while (sproutIt != sproutNoteEntries.end() && !haveSufficientFunds()) { + 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(), - [](COutput i, COutput j) -> bool { - return i.Value() > j.Value(); - }); - auto utxoIt = utxos.begin(); - while (utxoIt != utxos.end() && amount > 0) { - amount -= utxoIt->Value(); - ++utxoIt; + if (!haveSufficientFunds()) { + std::sort(utxos.begin(), utxos.end(), + [](COutput i, COutput j) -> bool { + return i.Value() > j.Value(); + }); + auto utxoIt = utxos.begin(); + while (utxoIt != utxos.end() && !haveSufficientFunds()) { + totalSelected += utxoIt->Value(); + ++utxoIt; + } + utxos.erase(utxoIt, utxos.end()); } - utxos.erase(utxoIt, utxos.end()); // finally select Sapling outputs - std::sort(saplingNoteEntries.begin(), saplingNoteEntries.end(), - [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool { - return i.note.value() > j.note.value(); - }); - auto saplingIt = saplingNoteEntries.begin(); - while (saplingIt != saplingNoteEntries.end() && amount > 0) { - amount -= saplingIt->note.value(); - ++saplingIt; + if (!haveSufficientFunds()) { + std::sort(saplingNoteEntries.begin(), saplingNoteEntries.end(), + [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool { + return i.note.value() > j.note.value(); + }); + auto saplingIt = saplingNoteEntries.begin(); + while (saplingIt != saplingNoteEntries.end() && !haveSufficientFunds()) { + 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 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 memo = {{0xF6}}; @@ -622,7 +677,9 @@ std::array 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 diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index d5b3ec532..a3dfccac9 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/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 get_memo_from_hex_string(std::string s); + static CAmount DefaultDustThreshold(); + + static std::array get_memo_from_hex_string(std::string s); uint256 main_impl(); }; diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index ca796c5b4..ad0d88959 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/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())