Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions doc/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 27 additions & 1 deletion qa/rpc-tests/fundrawtransaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ def run_test(self):
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})
Expand All @@ -660,6 +660,32 @@ 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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why += instead of a regular assignment? What if there's more outputs than one with value > 1?

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 #
######################################
Expand Down
8 changes: 7 additions & 1 deletion src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand All @@ -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)},
},
Expand All @@ -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"]));
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 5 additions & 1 deletion src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2192,7 +2192,7 @@ bool CWallet::SelectCoins(const vector<COutput>& 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<int>& 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<int>& setSubtractFeeFromOutputs, bool keepReserveKey, const CTxDestination& destChange)
{
vector<CRecipient> vecSend;

Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
* 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<int>& 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<int>& setSubtractFeeFromOutputs, bool keepReserveKey = true, const CTxDestination& destChange = CNoDestination());

/**
* Create a new transaction paying the recipients with a set of coins
Expand Down