Merge bitcoin#17219: wallet: allow transaction without change if keyp…

…ool is empty

92bcd70 [wallet] allow transaction without change if keypool is empty (Sjors Provoost)
709f868 [wallet] CreateTransaction: simplify change address check (Sjors Provoost)
5efc25f [wallet] translate "Keypool ran out" message (Sjors Provoost)

Pull request description:

  Extracted from bitcoin#16944

  First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check.

  Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error.

ACKs for top commit:
  fjahr:
    Code review ACK 92bcd70
  jonasschnelli:
    utACK 92bcd70
  achow101:
    ACK 92bcd70
  meshcollider:
    Code review ACK 92bcd70

Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006

src/wallet/scriptpubkeyman.cpp

@@ -23,7 +23,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string&
     // Generate a new key that is added to wallet
     CPubKey new_key;
     if (!GetKeyFromPool(new_key, false)) {
-        error = "Error: Keypool ran out, please call keypoolrefill first";
+        error = _("Error: Keypool ran out, please call keypoolrefill first").translated;
         return false;
     }
     //LearnRelatedScripts(new_key);

src/wallet/wallet.cpp

@@ -3467,20 +3467,14 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
                 //  rediscover unknown transactions that were written with keys of ours to recover
                 //  post-backup change.
 
-                // Reserve a new key pair from key pool
-                if (!CanGetAddresses(true)) {
-                    error = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys.");
-                    return false;
-                }
+                // Reserve a new key pair from key pool. If it fails, provide a dummy
+                // destination in case we don't need change.
                 CTxDestination dest;
-                bool ret = reservedest.GetReservedDestination(dest, true);
-                if (!ret)
-                {
-                    error = _("Keypool ran out, please call keypoolrefill first");
-                    return false;
+                if (!reservedest.GetReservedDestination(dest, true)) {
+                    error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
                 }
-
                 scriptChange = GetScriptForDestination(dest);
+                assert(!dest.empty() || scriptChange.empty());
             }
 
             nFeeRet = 0;
@@ -3730,6 +3724,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
                 error = _("Exceeded max tries.");
                 return false;
             }
+
+            // Give up if change keypool ran out and we failed to find a solution without change:
+            if (scriptChange.empty() && nChangePosInOut != -1) {
+                return false;
+            }
         }
 
         // Make sure change position was updated one way or another
@@ -4034,7 +4033,7 @@ bool CWallet::GetNewChangeDestination(CTxDestination& dest, std::string& error)
 
     ReserveDestination reservedest(this);
     if (!reservedest.GetReservedDestination(dest, true)) {
-        error = "Error: Keypool ran out, please call keypoolrefill first";
+        error = _("Error: Keypool ran out, please call keypoolrefill first").translated;
         return false;
     }
 

test/functional/rpc_fundrawtransaction.py

@@ -488,12 +488,20 @@ def test_locked_wallet(self):
 
         # Drain the keypool.
         self.nodes[1].getnewaddress()
-        inputs = []
-        outputs = {self.nodes[0].getnewaddress():1.1}
+        inputs = self.nodes[1].listunspent()
+        # Deduce fee to produce a changeless transaction
+        # bnb doesn't work same way as in bitcoin, so, `value` is also calculated by different way
+        value = sum(input_it["amount"] for input_it in inputs) - Decimal("0.00002200")
+        outputs = {self.nodes[0].getnewaddress():value}
         rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
+        # fund a transaction that does not require a new key for the change output
+        self.nodes[1].fundrawtransaction(rawtx)
+
         # fund a transaction that requires a new key for the change output
         # creating the key must be impossible because the wallet is locked
-        assert_raises_rpc_error(-4, "Keypool ran out, please call keypoolrefill first", self.nodes[1].fundrawtransaction, rawtx)
+        outputs = {self.nodes[0].getnewaddress():1.1}
+        rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
+        assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", self.nodes[1].fundrawtransaction, rawtx)
 
         # Refill the keypool.
         self.nodes[1].walletpassphrase("test", 100)

test/functional/wallet_keypool_hd.py

@@ -8,6 +8,7 @@
 # Add python-bitcoinrpc to module search path:
 
 import time
+from decimal import Decimal
 
 from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
@@ -16,7 +17,6 @@
 class KeyPoolTest(BitcoinTestFramework):
 
     def set_test_params(self):
-        self.setup_clean_chain = True
         self.num_nodes = 1
         self.extra_args = [['-usehd=1']]
 
@@ -101,5 +101,51 @@ def run_test(self):
         assert_equal(wi['keypoolsize_hd_internal'], 100)
         assert_equal(wi['keypoolsize'], 100)
 
+        # create a blank wallet
+        nodes[0].createwallet(wallet_name='w2', blank=True)
+        w2 = nodes[0].get_wallet_rpc('w2')
+
+        # refer to initial wallet as w1
+        w1 = nodes[0].get_wallet_rpc(self.default_wallet_name)
+
+        # import private key and fund it
+        address = addr.pop()
+        privkey = w1.dumpprivkey(address)
+        res = w2.importmulti([{'scriptPubKey': {'address': address}, 'keys': [privkey], 'timestamp': 'now'}])
+        assert_equal(res[0]['success'], True)
+        w1.walletpassphrase('test', 100)
+
+        res = w1.sendtoaddress(address=address, amount=0.00010000)
+        nodes[0].generate(1)
+        destination = addr.pop()
+
+        # Using a fee rate (10 sat / byte) well above the minimum relay rate
+        # creating a 5,000 sat transaction with change should not be possible
+        assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010})
+
+        # creating a 10,000 sat transaction without change, with a manual input, should still be possible
+        res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010})
+        assert_equal("psbt" in res, True)
+
+        # creating a 10,000 sat transaction without change should still be possible
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010})
+        assert_equal("psbt" in res, True)
+        # should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], options={"feeRate": 0.000010})
+        assert_equal("psbt" in res, True)
+
+        # dust change should be removed
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], options={"feeRate": 0.000010})
+        assert_equal("psbt" in res, True)
+
+        # create a transaction without change at the maximum fee rate, such that the output is still spendable:
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00008824})
+        assert_equal("psbt" in res, True)
+        assert_equal(res["fee"], Decimal("0.00001685"))
+
+        # creating a 10,000 sat transaction with a manual change address should be possible
+        res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010, "changeAddress": addr.pop()})
+        assert_equal("psbt" in res, True)
+
 if __name__ == '__main__':
     KeyPoolTest().main()