From cf82a9e704f56d245cf512d76ba9d0e6b178f3b0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Jul 2017 14:29:06 -0400 Subject: [PATCH] Do not allow users to get keys from keypool without reserving them fundrawtransaction allows users to add a change output and then not have it removed from keypool. While it would be nice to have users follow the normal CreateTransaction/CommitTransaction process we use internally, there isnt much benefit in exposing this option, especially with HD wallets, while there is ample room for users to misunderstand or misuse this option. This could be particularly nasty in some use-cases (especially pre-HD-split) - eg a user might fundrawtransaction, then call getnewaddress, hand out the address for someone to pay them, then sendrawtransaction. This may result in the user thinking they have received payment, even though it was really just their own change! This could obviously result in needless key-reuse. --- src/wallet/rpcwallet.cpp | 9 ++------- src/wallet/wallet.cpp | 12 +++++++----- src/wallet/wallet.h | 2 +- test/functional/fundrawtransaction.py | 17 +++-------------- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ee8c7548fc..26fcfea95c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2693,7 +2693,6 @@ 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" @@ -2732,7 +2731,6 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) CCoinControl coinControl; int changePosition = -1; bool lockUnspents = false; - bool reserveChangeKey = true; UniValue subtractFeeFromOutputs; std::set setSubtractFeeFromOutputs; @@ -2752,7 +2750,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) {"changePosition", UniValueType(UniValue::VNUM)}, {"includeWatching", UniValueType(UniValue::VBOOL)}, {"lockUnspents", UniValueType(UniValue::VBOOL)}, - {"reserveChangeKey", UniValueType(UniValue::VBOOL)}, + {"reserveChangeKey", UniValueType(UniValue::VBOOL)}, // DEPRECATED (and ignored), should be removed in 0.16 or so. {"feeRate", UniValueType()}, // will be checked below {"subtractFeeFromOutputs", UniValueType(UniValue::VARR)}, {"replaceable", UniValueType(UniValue::VBOOL)}, @@ -2779,9 +2777,6 @@ 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")) { coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); @@ -2830,7 +2825,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) CAmount nFeeOut; std::string strFailReason; - if (!pwallet->FundTransaction(tx, nFeeOut, changePosition, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl, reserveChangeKey)) { + if (!pwallet->FundTransaction(tx, nFeeOut, changePosition, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl)) { throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5317502589..c56fd05d7e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2471,7 +2471,7 @@ bool CWallet::SignTransaction(CMutableTransaction &tx) return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl, bool keepReserveKey) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) { std::vector vecSend; @@ -2493,8 +2493,13 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { return false; } - if (nChangePosInOut != -1) + + if (nChangePosInOut != -1) { tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]); + // we dont have the normal Create/Commit cycle, and dont want to risk reusing change, + // so just remove the key from the keypool here. + reservekey.KeepKey(); + } // Copy output sizes from new transaction; they may have had the fee subtracted from them for (unsigned int idx = 0; idx < tx.vout.size(); idx++) @@ -2515,9 +2520,6 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC } } - // 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 574fd8710d..bf09b040f6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -949,7 +949,7 @@ public: * Insert additional inputs into the transaction by * calling CreateTransaction(); */ - bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl, bool keepReserveKey = true); + bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); bool SignTransaction(CMutableTransaction& tx); /** diff --git a/test/functional/fundrawtransaction.py b/test/functional/fundrawtransaction.py index 0baab6d01c..e52e773918 100755 --- a/test/functional/fundrawtransaction.py +++ b/test/functional/fundrawtransaction.py @@ -636,20 +636,9 @@ 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].getrawchangeaddress() - # frt should not have removed the key from the keypool - assert(changeaddress == nextaddr) + ################################ + # Test no address reuse occurs # + ################################ result3 = self.nodes[3].fundrawtransaction(rawtx) res_dec = self.nodes[0].decoderawtransaction(result3["hex"])