Browse Source

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

cf82a9e Do not allow users to get keys from keypool without reserving them (Matt Corallo)

Pull request description:

  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 partially reverts #9377. Would be nice to get this for 15 since its kinda crazy we have this option to begin with IMO, will need release notes as an RPC option is now ignored.

Tree-SHA512: 72b5ee9c4a229b84d799dfb00c56fe80d8bba914ce81a433c3f5ab325bf9bf2b839ee658c261734f0ee183ab19435039481014d09c41dbe155e6323e63beb01d
0.15
Wladimir J. van der Laan 7 years ago
parent
commit
9e8d6a3fb4
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
  1. 9
      src/wallet/rpcwallet.cpp
  2. 12
      src/wallet/wallet.cpp
  3. 2
      src/wallet/wallet.h
  4. 17
      test/functional/fundrawtransaction.py

9
src/wallet/rpcwallet.cpp

@ -2705,7 +2705,6 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
" \"changePosition\" (numeric, optional, default random) The index of the change output\n" " \"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" " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n"
" \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\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" " \"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" " \"subtractFeeFromOutputs\" (array, optional) A json array of integers.\n"
" The fee will be equally deducted from the amount of each specified output.\n" " The fee will be equally deducted from the amount of each specified output.\n"
@ -2744,7 +2743,6 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
CCoinControl coinControl; CCoinControl coinControl;
int changePosition = -1; int changePosition = -1;
bool lockUnspents = false; bool lockUnspents = false;
bool reserveChangeKey = true;
UniValue subtractFeeFromOutputs; UniValue subtractFeeFromOutputs;
std::set<int> setSubtractFeeFromOutputs; std::set<int> setSubtractFeeFromOutputs;
@ -2764,7 +2762,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
{"changePosition", UniValueType(UniValue::VNUM)}, {"changePosition", UniValueType(UniValue::VNUM)},
{"includeWatching", UniValueType(UniValue::VBOOL)}, {"includeWatching", UniValueType(UniValue::VBOOL)},
{"lockUnspents", 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 {"feeRate", UniValueType()}, // will be checked below
{"subtractFeeFromOutputs", UniValueType(UniValue::VARR)}, {"subtractFeeFromOutputs", UniValueType(UniValue::VARR)},
{"replaceable", UniValueType(UniValue::VBOOL)}, {"replaceable", UniValueType(UniValue::VBOOL)},
@ -2791,9 +2789,6 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
if (options.exists("lockUnspents")) if (options.exists("lockUnspents"))
lockUnspents = options["lockUnspents"].get_bool(); lockUnspents = options["lockUnspents"].get_bool();
if (options.exists("reserveChangeKey"))
reserveChangeKey = options["reserveChangeKey"].get_bool();
if (options.exists("feeRate")) if (options.exists("feeRate"))
{ {
coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"]));
@ -2842,7 +2837,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
CAmount nFeeOut; CAmount nFeeOut;
std::string strFailReason; 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); throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
} }

12
src/wallet/wallet.cpp

@ -2471,7 +2471,7 @@ bool CWallet::SignTransaction(CMutableTransaction &tx)
return true; 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; 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)) { if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
return false; return false;
} }
if (nChangePosInOut != -1)
if (nChangePosInOut != -1) {
tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]); 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 // 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++) 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; return true;
} }

2
src/wallet/wallet.h

@ -949,7 +949,7 @@ public:
* Insert additional inputs into the transaction by * Insert additional inputs into the transaction by
* calling CreateTransaction(); * 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); bool SignTransaction(CMutableTransaction& tx);
/** /**

17
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(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate)
assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate)
############################# ################################
# Test address reuse option # # Test no address reuse occurs #
############################# ################################
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)
result3 = self.nodes[3].fundrawtransaction(rawtx) result3 = self.nodes[3].fundrawtransaction(rawtx)
res_dec = self.nodes[0].decoderawtransaction(result3["hex"]) res_dec = self.nodes[0].decoderawtransaction(result3["hex"])

Loading…
Cancel
Save