From e10e1e8db043e9b7c113e07faf408f337c1b732d Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Wed, 10 Jul 2019 11:37:45 -0400 Subject: Restrict lifetime of ReserveDestination to CWallet::CreateTransaction --- src/wallet/feebumper.cpp | 6 ++---- src/wallet/rpcwallet.cpp | 10 ++++------ src/wallet/test/wallet_tests.cpp | 5 ++--- src/wallet/wallet.cpp | 10 ++++------ src/wallet/wallet.h | 8 ++++---- 5 files changed, 16 insertions(+), 23 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index e6ed69bfd0..619197a57a 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -272,11 +272,10 @@ 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; } @@ -327,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 27fb009070..d432395d8b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -309,7 +309,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 vecSend; @@ -317,13 +316,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); } @@ -907,16 +906,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 d69e555e1d..61b1efafa1 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 75c3989a25..ad9dd18c35 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2666,9 +2666,8 @@ 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; } @@ -2784,10 +2783,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& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, +bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& 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) @@ -3147,7 +3147,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> orderForm, ReserveDestination& reservedest, CValidationState& state) +bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) { { auto locked_chain = chain().lock(); @@ -3161,8 +3161,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 14c373bddf..d6c2038fcb 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -256,8 +256,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 @@ -1084,9 +1084,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& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, int& nChangePosInOut, + bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& 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> orderForm, ReserveDestination& reservedest, CValidationState& state); + bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state); bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, bool use_max_sig = false) const { -- cgit v1.2.3