diff options
-rw-r--r-- | src/interfaces/wallet.cpp | 7 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 9 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 10 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 5 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 19 | ||||
-rw-r--r-- | src/wallet/wallet.h | 8 |
6 files changed, 23 insertions, 35 deletions
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 8982c70fd2..deb1618ceb 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -36,7 +36,7 @@ namespace { class PendingWalletTxImpl : public PendingWalletTx { public: - explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_dest(&wallet) {} + explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet) {} const CTransaction& get() override { return *m_tx; } @@ -47,7 +47,7 @@ public: auto locked_chain = m_wallet.chain().lock(); LOCK(m_wallet.cs_wallet); CValidationState state; - if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_dest, state)) { + if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), state)) { reject_reason = state.GetRejectReason(); return false; } @@ -56,7 +56,6 @@ public: CTransactionRef m_tx; CWallet& m_wallet; - ReserveDestination m_dest; }; //! Construct wallet tx struct. @@ -238,7 +237,7 @@ public: auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); auto pending = MakeUnique<PendingWalletTxImpl>(*m_wallet); - if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_dest, fee, change_pos, + if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, fee, change_pos, fail_reason, coin_control, sign)) { return {}; } diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 0d07ae860c..619197a57a 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -272,18 +272,14 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo new_coin_control.m_min_depth = 1; CTransactionRef tx_new = MakeTransactionRef(); - ReserveDestination reservedest(wallet); CAmount fee_ret; int change_pos_in_out = -1; // No requested location for change std::string fail_reason; - if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, reservedest, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { + if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { errors.push_back("Unable to create transaction: " + fail_reason); return Result::WALLET_ERROR; } - // If change key hasn't been ReturnKey'ed by this point, we take it out of keypool - reservedest.KeepDestination(); - // Write back new fee if successful new_fee = fee_ret; @@ -330,9 +326,8 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti mapValue_t mapValue = oldWtx.mapValue; mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); - ReserveDestination reservedest(wallet); CValidationState state; - if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, reservedest, state)) { + if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) { // NOTE: CommitTransaction never returns false, so this should never happen. errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state))); return Result::WALLET_ERROR; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 19becf93c7..ab732dc0d8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -317,7 +317,6 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet CScript scriptPubKey = GetScriptForDestination(address); // Create and send the transaction - ReserveDestination reservedest(pwallet); CAmount nFeeRequired; std::string strError; std::vector<CRecipient> vecSend; @@ -325,13 +324,13 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; vecSend.push_back(recipient); CTransactionRef tx; - if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, reservedest, nFeeRequired, nChangePosRet, strError, coin_control)) { + if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strError, coin_control)) { if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance) strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, reservedest, state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state)) { strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } @@ -915,16 +914,15 @@ static UniValue sendmany(const JSONRPCRequest& request) std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext()); // Send - ReserveDestination changedest(pwallet); CAmount nFeeRequired = 0; int nChangePosRet = -1; std::string strFailReason; CTransactionRef tx; - bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, changedest, nFeeRequired, nChangePosRet, strFailReason, coin_control); + bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, changedest, state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state)) { strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index a2fa94a2de..8af05dea45 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -361,17 +361,16 @@ public: CWalletTx& AddTx(CRecipient recipient) { CTransactionRef tx; - ReserveDestination reservedest(wallet.get()); CAmount fee; int changePos = -1; std::string error; CCoinControl dummy; { auto locked_chain = m_chain->lock(); - BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservedest, fee, changePos, error, dummy)); + BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, fee, changePos, error, dummy)); } CValidationState state; - BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservedest, state)); + BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, state)); CMutableTransaction blocktx; { LOCK(wallet->cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 07fc8c35d5..452d4f7a6a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2779,17 +2779,13 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC auto locked_chain = chain().lock(); LOCK(cs_wallet); - ReserveDestination reservedest(this); CTransactionRef tx_new; - if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservedest, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { + if (!CreateTransaction(*locked_chain, vecSend, tx_new, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { return false; } if (nChangePosInOut != -1) { tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); - // We don't have the normal Create/Commit cycle, and don't want to risk - // reusing change, so just remove the key from the keypool here. - reservedest.KeepDestination(); } // Copy output sizes from new transaction; they may have had the fee @@ -2900,10 +2896,11 @@ OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vec return m_default_address_type; } -bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, +bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) { CAmount nValue = 0; + ReserveDestination reservedest(this); int nChangePosRequest = nChangePosInOut; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) @@ -3183,8 +3180,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std } } - if (nChangePosInOut == -1) reservedest.ReturnDestination(); // Return any reserved address if we don't have change - // Shuffle selected coins and fill in final vin txNew.vin.clear(); std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end()); @@ -3247,6 +3242,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std } } + // Before we return success, we assume any change key will be used to prevent + // accidental re-use. + reservedest.KeepDestination(); + WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, feeCalc.est.pass.start, feeCalc.est.pass.end, @@ -3261,7 +3260,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std /** * Call after CreateTransaction unless you want to abort */ -bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, ReserveDestination& reservedest, CValidationState& state) +bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state) { { auto locked_chain = chain().lock(); @@ -3275,8 +3274,6 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ { - // Take key pair from key pool so it won't be used again - reservedest.KeepDestination(); // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5f226cf5c0..6a7097bf44 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -266,8 +266,8 @@ public: /** A wrapper to reserve an address from a wallet * - * ReserveDestination is used to reserve an address. It is passed around - * during the CreateTransaction/CommitTransaction procedure. + * ReserveDestination is used to reserve an address. + * It is currently only used inside of CreateTransaction. * * Instantiating a ReserveDestination does not reserve an address. To do so, * GetReservedDestination() needs to be called on the object. Once an address has been @@ -1137,9 +1137,9 @@ public: * selected by SelectCoins(); Also create the change output, when needed * @note passing nChangePosInOut as -1 will result in setting a random position */ - bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, int& nChangePosInOut, + bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); - bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, ReserveDestination& reservedest, CValidationState& state); + bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state); bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, bool use_max_sig = false) const { |