From 220e18c81cdca1ea3faa44cfb33375f07436e07f Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sat, 18 Apr 2020 21:37:20 +1200 Subject: [PATCH] Merge #17219: wallet: allow transaction without change if keypool is empty 92bcd70808b9cac56b184903aa6d37baf9641b37 [wallet] allow transaction without change if keypool is empty (Sjors Provoost) 709f8685ac37510aa145ac259753583c82280038 [wallet] CreateTransaction: simplify change address check (Sjors Provoost) 5efc25f9638866941028454cfa9bae27f1519cb4 [wallet] translate "Keypool ran out" message (Sjors Provoost) Pull request description: Extracted from #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 92bcd70808b9cac56b184903aa6d37baf9641b37 jonasschnelli: utACK 92bcd70808b9cac56b184903aa6d37baf9641b37 achow101: ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 meshcollider: Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006 --- src/wallet/scriptpubkeyman.cpp | 2 +- src/wallet/wallet.cpp | 23 ++++++----- test/functional/rpc_fundrawtransaction.py | 14 +++++-- test/functional/wallet_keypool_hd.py | 48 ++++++++++++++++++++++- 4 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 0a6f234e7bcad6..805eae9a4b3fec 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/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); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4f3fb4d6376d08..b6cd2de22a4d86 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3467,20 +3467,14 @@ bool CWallet::CreateTransaction(const std::vector& 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& 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; } diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index bf93b8e7909db6..9a74593b149b4d 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/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) diff --git a/test/functional/wallet_keypool_hd.py b/test/functional/wallet_keypool_hd.py index d22effdeafb6d8..0e45cdb52a09ec 100755 --- a/test/functional/wallet_keypool_hd.py +++ b/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()