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

1 files changed, 7 insertions, 5 deletions

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<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;
 }