From 9aa4e6a6c2cb9b4465d014c8efa672a919ff8f89 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 19 Dec 2016 09:04:05 +0100 Subject: [PATCH 1/3] [Wallet] Add an option to keep the change address key, true by default --- src/wallet/rpcwallet.cpp | 8 +++++++- src/wallet/wallet.cpp | 6 +++++- src/wallet/wallet.h | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2428ef04e..fa54197e7 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2496,6 +2496,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) " \"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" + " \"reserveChangeKey\" (boolean, optional, default true) Reserves the change output key from the keypool\n" " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n" " \"subtractFeeFromOutputs\" (array, optional) A json array of integers.\n" " The fee will be equally deducted from the amount of each specified output.\n" @@ -2528,6 +2529,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) int changePosition = -1; bool includeWatching = false; bool lockUnspents = false; + bool reserveChangeKey = true; CFeeRate feeRate = CFeeRate(0); bool overrideEstimatedFeerate = false; UniValue subtractFeeFromOutputs; @@ -2549,6 +2551,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) {"changePosition", UniValueType(UniValue::VNUM)}, {"includeWatching", UniValueType(UniValue::VBOOL)}, {"lockUnspents", UniValueType(UniValue::VBOOL)}, + {"reserveChangeKey", UniValueType(UniValue::VBOOL)}, {"feeRate", UniValueType()}, // will be checked below {"subtractFeeFromOutputs", UniValueType(UniValue::VARR)}, }, @@ -2572,6 +2575,9 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) if (options.exists("lockUnspents")) lockUnspents = options["lockUnspents"].get_bool(); + if (options.exists("reserveChangeKey")) + reserveChangeKey = options["reserveChangeKey"].get_bool(); + if (options.exists("feeRate")) { feeRate = CFeeRate(AmountFromValue(options["feeRate"])); @@ -2608,7 +2614,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) CAmount nFeeOut; string strFailReason; - if(!pwalletMain->FundTransaction(tx, nFeeOut, overrideEstimatedFeerate, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, setSubtractFeeFromOutputs, changeAddress)) + if(!pwalletMain->FundTransaction(tx, nFeeOut, overrideEstimatedFeerate, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, setSubtractFeeFromOutputs, reserveChangeKey, changeAddress)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0b18be876..06382af18 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2192,7 +2192,7 @@ bool CWallet::SelectCoins(const vector& vAvailableCoins, const CAmount& return res; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, const CTxDestination& destChange) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, bool keepReserveKey, const CTxDestination& destChange) { vector vecSend; @@ -2241,6 +2241,10 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov } } + // optionally keep the change output key + if (keepReserveKey) + reservekey.KeepKey(); + return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 92ad09848..fb20062b9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -780,7 +780,7 @@ public: * Insert additional inputs into the transaction by * calling CreateTransaction(); */ - bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, const CTxDestination& destChange = CNoDestination()); + bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, bool keepReserveKey = true, const CTxDestination& destChange = CNoDestination()); /** * Create a new transaction paying the recipients with a set of coins From 9eb325d079c7bbce749abe79d0bbac34d8ef5806 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 19 Dec 2016 09:10:33 +0100 Subject: [PATCH 2/3] [QA] Add test for fundrawtransactions new reserveChangeKey option --- qa/rpc-tests/fundrawtransaction.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/fundrawtransaction.py b/qa/rpc-tests/fundrawtransaction.py index b97e9aecd..b279c0b9d 100755 --- a/qa/rpc-tests/fundrawtransaction.py +++ b/qa/rpc-tests/fundrawtransaction.py @@ -651,7 +651,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(len(self.nodes[3].listunspent(1)), 1) inputs = [] - outputs = {self.nodes[2].getnewaddress() : 1} + outputs = {self.nodes[3].getnewaddress() : 1} rawtx = self.nodes[3].createrawtransaction(inputs, outputs) result = self.nodes[3].fundrawtransaction(rawtx) # uses min_relay_tx_fee (set by settxfee) result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee}) @@ -660,6 +660,32 @@ class RawTransactionsTest(BitcoinTestFramework): assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + ############################# + # Test address reuse option # + ############################# + + result3 = self.nodes[3].fundrawtransaction(rawtx, {"reserveChangeKey": False}) + res_dec = self.nodes[0].decoderawtransaction(result3["hex"]) + changeaddress = "" + for out in res_dec['vout']: + if out['value'] > 1.0: + changeaddress += out['scriptPubKey']['addresses'][0] + assert(changeaddress != "") + nextaddr = self.nodes[3].getnewaddress() + # frt should not have removed the key from the keypool + assert(changeaddress == nextaddr) + + result3 = self.nodes[3].fundrawtransaction(rawtx) + res_dec = self.nodes[0].decoderawtransaction(result3["hex"]) + changeaddress = "" + for out in res_dec['vout']: + if out['value'] > 1.0: + changeaddress += out['scriptPubKey']['addresses'][0] + assert(changeaddress != "") + nextaddr = self.nodes[3].getnewaddress() + # Now the change address key should be removed from the keypool + assert(changeaddress != nextaddr) + ###################################### # Test subtractFeeFromOutputs option # ###################################### From c9f3062d551823f006d400dddb54c12620cd29c4 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 19 Dec 2016 09:12:28 +0100 Subject: [PATCH 3/3] Add fundrawtransactions new reserveChangeKey option to the release notes --- doc/release-notes.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index bbda55854..0ea9e1949 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -128,6 +128,16 @@ Call "getmininginfo" loses the "testnet" field in favor of the more generic "cha ### Wallet +0.14.0 Fundrawtransaction change address reuse +============================================== + +Before 0.14, `fundrawtransaction` was by default wallet stateless. In almost all cases `fundrawtransaction` does add a change-output to the outputs of the funded transaction. Before 0.14, the used keypool key was never marked as change-address key and directly returned to the keypool (leading to address reuse). +Before 0.14, calling `getnewaddress` directly after `fundrawtransaction` did generate the same address as the change-output address. + +Since 0.14, fundrawtransaction does reserve the change-output-key from the keypool by default (optional by setting `reserveChangeKey`, default = `true`) + +Users should also consider using `getrawchangeaddress()` in conjunction with `fundrawtransaction`'s `changeAddress` option. + ### GUI ### Tests