foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-05 14:06:27 -05:00
Code Issues Activity

Restrict lifetime of ReserveDestination to CWallet::CreateTransaction

Browse source
This commit is contained in:
Gregory Sanders 2019-07-10 11:37:45 -04:00
parent d9ff862f2d
commit e10e1e8db0
6 changed files with 19 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/interfaces/wallet.cpp
View file

@ -36,7 +36,7 @@ namespace {
class PendingWalletTxImpl : public PendingWalletTx class PendingWalletTxImpl : public PendingWalletTx
{ {
public: 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; } const CTransaction& get() override { return *m_tx; }
@ -47,7 +47,7 @@ public:
auto locked_chain = m_wallet.chain().lock(); auto locked_chain = m_wallet.chain().lock();
LOCK(m_wallet.cs_wallet); LOCK(m_wallet.cs_wallet);
CValidationState state; 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(); reject_reason = state.GetRejectReason();
return false; return false;
} }
@ -56,7 +56,6 @@ public:
CTransactionRef m_tx; CTransactionRef m_tx;
CWallet& m_wallet; CWallet& m_wallet;
ReserveDestination m_dest;
}; };
//! Construct wallet tx struct. //! Construct wallet tx struct.
@ -238,7 +237,7 @@ public:
auto locked_chain = m_wallet->chain().lock(); auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet); LOCK(m_wallet->cs_wallet);
auto pending = MakeUnique<PendingWalletTxImpl>(*m_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)) { fail_reason, coin_control, sign)) {
return {}; return {};
} }

6
src/wallet/feebumper.cpp
View file

@ -272,11 +272,10 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
new_coin_control.m_min_depth = 1; new_coin_control.m_min_depth = 1;
CTransactionRef tx_new = MakeTransactionRef(); CTransactionRef tx_new = MakeTransactionRef();
ReserveDestination reservedest(wallet);
CAmount fee_ret; CAmount fee_ret;
int change_pos_in_out = -1; // No requested location for change int change_pos_in_out = -1; // No requested location for change
std::string fail_reason; 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); errors.push_back("Unable to create transaction: " + fail_reason);
return Result::WALLET_ERROR; return Result::WALLET_ERROR;
} }
@ -327,9 +326,8 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
mapValue_t mapValue = oldWtx.mapValue; mapValue_t mapValue = oldWtx.mapValue;
mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
ReserveDestination reservedest(wallet);
CValidationState state; 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. // NOTE: CommitTransaction never returns false, so this should never happen.
errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state))); errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
return Result::WALLET_ERROR; return Result::WALLET_ERROR;

10
src/wallet/rpcwallet.cpp
View file

@ -309,7 +309,6 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet
CScript scriptPubKey = GetScriptForDestination(address); CScript scriptPubKey = GetScriptForDestination(address);
// Create and send the transaction // Create and send the transaction
ReserveDestination reservedest(pwallet);
CAmount nFeeRequired; CAmount nFeeRequired;
std::string strError; std::string strError;
std::vector<CRecipient> vecSend; std::vector<CRecipient> vecSend;
@ -317,13 +316,13 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet
CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
vecSend.push_back(recipient); vecSend.push_back(recipient);
CTransactionRef tx; 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) if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired)); strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));
throw JSONRPCError(RPC_WALLET_ERROR, strError); throw JSONRPCError(RPC_WALLET_ERROR, strError);
} }
CValidationState state; 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)); strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state));
throw JSONRPCError(RPC_WALLET_ERROR, strError); throw JSONRPCError(RPC_WALLET_ERROR, strError);
} }
@ -907,16 +906,15 @@ static UniValue sendmany(const JSONRPCRequest& request)
std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext()); std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext());
// Send // Send
ReserveDestination changedest(pwallet);
CAmount nFeeRequired = 0; CAmount nFeeRequired = 0;
int nChangePosRet = -1; int nChangePosRet = -1;
std::string strFailReason; std::string strFailReason;
CTransactionRef tx; 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) if (!fCreated)
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason);
CValidationState state; 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)); strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state));
throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
} }

5
src/wallet/test/wallet_tests.cpp
View file

@ -361,17 +361,16 @@ public:
CWalletTx& AddTx(CRecipient recipient) CWalletTx& AddTx(CRecipient recipient)
{ {
CTransactionRef tx; CTransactionRef tx;
ReserveDestination reservedest(wallet.get());
CAmount fee; CAmount fee;
int changePos = -1; int changePos = -1;
std::string error; std::string error;
CCoinControl dummy; CCoinControl dummy;
{ {
auto locked_chain = m_chain->lock(); 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; CValidationState state;
BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservedest, state)); BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, state));
CMutableTransaction blocktx; CMutableTransaction blocktx;
{ {
LOCK(wallet->cs_wallet); LOCK(wallet->cs_wallet);

10
src/wallet/wallet.cpp
View file

@ -2666,9 +2666,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
auto locked_chain = chain().lock(); auto locked_chain = chain().lock();
LOCK(cs_wallet); LOCK(cs_wallet);
ReserveDestination reservedest(this);
CTransactionRef tx_new; 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; return false;
} }
@ -2784,10 +2783,11 @@ OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vec
return m_default_address_type; 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) int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
{ {
CAmount nValue = 0; CAmount nValue = 0;
ReserveDestination reservedest(this);
int nChangePosRequest = nChangePosInOut; int nChangePosRequest = nChangePosInOut;
unsigned int nSubtractFeeFromAmount = 0; unsigned int nSubtractFeeFromAmount = 0;
for (const auto& recipient : vecSend) 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 * 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(); 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 */ 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, // Add tx to wallet, because if it has change it's also ours,
// otherwise just for transaction history. // otherwise just for transaction history.

8
src/wallet/wallet.h
View file

@ -256,8 +256,8 @@ public:
/** A wrapper to reserve an address from a wallet /** A wrapper to reserve an address from a wallet
* *
* ReserveDestination is used to reserve an address. It is passed around * ReserveDestination is used to reserve an address.
* during the CreateTransaction/CommitTransaction procedure. * It is currently only used inside of CreateTransaction.
* *
* Instantiating a ReserveDestination does not reserve an address. To do so, * Instantiating a ReserveDestination does not reserve an address. To do so,
* GetReservedDestination() needs to be called on the object. Once an address has been * 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 * selected by SelectCoins(); Also create the change output, when needed
* @note passing nChangePosInOut as -1 will result in setting a random position * @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); 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 bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, bool use_max_sig = false) const
{ {