/bitcoin

Do not allow users to get keys from keypool without reserving them #10784

Merged
merged 1 commit into from Jul 18, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+13 −27
Split
View
9 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<int> 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);
}
View
12 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<int>& setSubtractFeeFromOutputs, CCoinControl coinControl, bool keepReserveKey)
+bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
{
std::vector<CRecipient> 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;
}
View
2 src/wallet/wallet.h
@@ -949,7 +949,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
* 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<int>& setSubtractFeeFromOutputs, CCoinControl, bool keepReserveKey = true);
+ bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl);
bool SignTransaction(CMutableTransaction& tx);
/**
View
17 test/functional/fundrawtransaction.py
@@ -636,20 +636,9 @@ def run_test(self):
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"])