From a759f99845a69f3a3622efd44fb1c9b5c62d7d62 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 2 Feb 2017 15:30:03 -0500 Subject: [PATCH 1/3] [wallet] Construct CWalletTx objects in CommitTransaction Construct CWalletTx objects in CWallet::CommitTransaction, instead of having callers do it. This ensures CWalletTx objects are constructed in a uniform way and all fields are set. This also makes it possible to avoid confusing and wasteful CWalletTx copies in https://github.com/bitcoin/bitcoin/pull/9381 There is no change in behavior. --- src/privatesend/privatesend-util.cpp | 10 +++--- src/qt/sendcoinsdialog.cpp | 4 +-- src/qt/walletmodel.cpp | 35 +++++++++--------- src/qt/walletmodel.h | 2 +- src/qt/walletmodeltransaction.cpp | 14 +++----- src/qt/walletmodeltransaction.h | 5 ++- src/rpc/governance.cpp | 10 +++--- src/rpc/rpcevo.cpp | 8 ++--- src/wallet/rpcwallet.cpp | 54 +++++++++++++++------------- src/wallet/test/wallet_tests.cpp | 10 +++--- src/wallet/wallet.cpp | 39 ++++++++++---------- src/wallet/wallet.h | 6 ++-- 12 files changed, 97 insertions(+), 100 deletions(-) diff --git a/src/privatesend/privatesend-util.cpp b/src/privatesend/privatesend-util.cpp index fbfe55ac4d3e..507bf6f677dd 100644 --- a/src/privatesend/privatesend-util.cpp +++ b/src/privatesend/privatesend-util.cpp @@ -251,7 +251,6 @@ bool CTransactionBuilder::IsDust(CAmount nAmount) const bool CTransactionBuilder::Commit(std::string& strResult) { - CWalletTx wtx; CAmount nFeeRet = 0; int nChangePosRet = -1; @@ -265,7 +264,8 @@ bool CTransactionBuilder::Commit(std::string& strResult) } } - if (!pwallet->CreateTransaction(vecSend, wtx, dummyReserveKey, nFeeRet, nChangePosRet, strResult, coinControl)) { + CTransactionRef tx; + if (!pwallet->CreateTransaction(vecSend, tx, dummyReserveKey, nFeeRet, nChangePosRet, strResult, coinControl)) { return false; } @@ -273,7 +273,7 @@ bool CTransactionBuilder::Commit(std::string& strResult) bool fDust = IsDust(nAmountLeft); // If there is a either remainder which is considered to be dust (will be added to fee in this case) or no amount left there should be no change output, return if there is a change output. if (nChangePosRet != -1 && fDust) { - strResult = strprintf("Unexpected change output %s at position %d", wtx.tx->vout[nChangePosRet].ToString(), nChangePosRet); + strResult = strprintf("Unexpected change output %s at position %d", tx->vout[nChangePosRet].ToString(), nChangePosRet); return false; } @@ -301,14 +301,14 @@ bool CTransactionBuilder::Commit(std::string& strResult) } CValidationState state; - if (!pwallet->CommitTransaction(wtx, dummyReserveKey, g_connman.get(), state)) { + if (!pwallet->CommitTransaction(tx, {}, {}, {}, dummyReserveKey, g_connman.get(), state)) { strResult = state.GetRejectReason(); return false; } fKeepKeys = true; - strResult = wtx.GetHash().ToString(); + strResult = tx->GetHash().ToString(); return true; } diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 2c218cede582..16eb6acbf9dc 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -421,7 +421,7 @@ void SendCoinsDialog::send(QList recipients) if (ctrl.IsUsingPrivateSend()) { // append number of inputs questionString.append("
"); - int nInputs = currentTransaction.getTransaction()->tx->vin.size(); + int nInputs = currentTransaction.getTransaction()->vin.size(); questionString.append(tr("This transaction will consume %n input(s)", "", nInputs)); // warn about potential privacy issues when spending too many inputs at once @@ -461,7 +461,7 @@ void SendCoinsDialog::send(QList recipients) } // now send the prepared transaction - WalletModel::SendCoinsReturn sendStatus = model->sendCoins(currentTransaction); + WalletModel::SendCoinsReturn sendStatus = model->sendCoins(currentTransaction, ctrl.IsUsingPrivateSend()); // process sendStatus and on error generate message shown to user processSendCoinsReturn(sendStatus); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 728cb239a7da..f01c5d49be37 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -344,16 +344,15 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact int nChangePosRet = -1; - CWalletTx* newTx = transaction.getTransaction(); + CTransactionRef& newTx = transaction.getTransaction(); CReserveKey *keyChange = transaction.getPossibleKeyChange(); - - fCreated = wallet->CreateTransaction(vecSend, *newTx, *keyChange, nFeeRequired, nChangePosRet, strFailReason, coinControl); + fCreated = wallet->CreateTransaction(vecSend, newTx, *keyChange, nFeeRequired, nChangePosRet, strFailReason, coinControl); transaction.setTransactionFee(nFeeRequired); if (fSubtractFeeFromAmount && fCreated) transaction.reassignAmounts(); - nValueOut = newTx->tx->GetValueOut(); - nVinSize = newTx->tx->vin.size(); + nValueOut = newTx->GetValueOut(); + nVinSize = newTx->vin.size(); } if(!fCreated) @@ -376,7 +375,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact return SendCoinsReturn(OK); } -WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &transaction) +WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &transaction, bool fIsPrivateSend) { QByteArray transaction_array; /* store serialized transaction */ @@ -384,10 +383,8 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran LOCK2(cs_main, mempool.cs); LOCK(wallet->cs_wallet); - CWalletTx *newTx = transaction.getTransaction(); - QList recipients = transaction.getRecipients(); - - for (const SendCoinsRecipient &rcp : recipients) + std::vector> vOrderForm; + for (const SendCoinsRecipient &rcp : transaction.getRecipients()) { if (rcp.paymentRequest.IsInitialized()) { @@ -397,25 +394,27 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran } // Store PaymentRequests in wtx.vOrderForm in wallet. - std::string key("PaymentRequest"); std::string value; rcp.paymentRequest.SerializeToString(&value); - newTx->vOrderForm.push_back(make_pair(key, value)); + vOrderForm.emplace_back("PaymentRequest", std::move(value)); } else if (!rcp.message.isEmpty()) // Message from normal dash:URI (dash:XyZ...?message=example) - { - newTx->vOrderForm.push_back(make_pair("Message", rcp.message.toStdString())); - } + vOrderForm.emplace_back("Message", rcp.message.toStdString()); + } + + if (fIsPrivateSend) { + vOrderForm.emplace_back("DS", "1"); } + CTransactionRef& newTx = transaction.getTransaction(); CReserveKey *keyChange = transaction.getPossibleKeyChange(); CValidationState state; - if(!wallet->CommitTransaction(*newTx, *keyChange, g_connman.get(), state)) + if (!wallet->CommitTransaction(newTx, {} /* mapValue */, std::move(vOrderForm), {} /* fromAccount */, *keyChange, g_connman.get(), state)) return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason())); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); - ssTx << *newTx->tx; - transaction_array.append(ssTx.data(), ssTx.size()); + ssTx << newTx; + transaction_array.append(&(ssTx[0]), ssTx.size()); } // Add addresses / update labels that we've sent to the address book, diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 28cbc6c0bf56..1ee10d4980b7 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -167,7 +167,7 @@ class WalletModel : public QObject SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const CCoinControl& coinControl); // Send coins to a list of recipients - SendCoinsReturn sendCoins(WalletModelTransaction &transaction); + SendCoinsReturn sendCoins(WalletModelTransaction &transaction, bool fIsPrivateSend); // Wallet encryption bool setWalletEncrypted(bool encrypted, const SecureString &passphrase); diff --git a/src/qt/walletmodeltransaction.cpp b/src/qt/walletmodeltransaction.cpp index 20710d7639b4..cd5f1a6a2b22 100644 --- a/src/qt/walletmodeltransaction.cpp +++ b/src/qt/walletmodeltransaction.cpp @@ -11,12 +11,6 @@ WalletModelTransaction::WalletModelTransaction(const QList & walletTransaction(0), fee(0) { - walletTransaction = new CWalletTx(); -} - -WalletModelTransaction::~WalletModelTransaction() -{ - delete walletTransaction; } QList WalletModelTransaction::getRecipients() const @@ -24,14 +18,14 @@ QList WalletModelTransaction::getRecipients() const return recipients; } -CWalletTx *WalletModelTransaction::getTransaction() const +CTransactionRef& WalletModelTransaction::getTransaction() { return walletTransaction; } unsigned int WalletModelTransaction::getTransactionSize() { - return (!walletTransaction ? 0 : (::GetSerializeSize(*walletTransaction->tx, SER_NETWORK, PROTOCOL_VERSION))); + return (!walletTransaction ? 0 : (::GetSerializeSize(*walletTransaction, SER_NETWORK, PROTOCOL_VERSION))); } CAmount WalletModelTransaction::getTransactionFee() const @@ -61,7 +55,7 @@ void WalletModelTransaction::reassignAmounts() if (out.amount() <= 0) continue; const unsigned char* scriptStr = (const unsigned char*)out.script().data(); CScript scriptPubKey(scriptStr, scriptStr+out.script().size()); - for (const auto& txout : walletTransaction->tx->vout) { + for (const auto& txout : walletTransaction->vout) { if (txout.scriptPubKey == scriptPubKey) { subtotal += txout.nValue; break; @@ -72,7 +66,7 @@ void WalletModelTransaction::reassignAmounts() } else // normal recipient (no payment request) { - for (const auto& txout : walletTransaction->tx->vout) { + for (const auto& txout : walletTransaction->vout) { CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString())); if (txout.scriptPubKey == scriptPubKey) { rcp.amount = txout.nValue; diff --git a/src/qt/walletmodeltransaction.h b/src/qt/walletmodeltransaction.h index 60888aa914d7..2e40afca22d5 100644 --- a/src/qt/walletmodeltransaction.h +++ b/src/qt/walletmodeltransaction.h @@ -22,11 +22,10 @@ class WalletModelTransaction { public: explicit WalletModelTransaction(const QList &recipients); - ~WalletModelTransaction(); QList getRecipients() const; - CWalletTx *getTransaction() const; + CTransactionRef& getTransaction(); unsigned int getTransactionSize(); void setTransactionFee(const CAmount& newFee); @@ -41,7 +40,7 @@ class WalletModelTransaction private: QList recipients; - CWalletTx *walletTransaction; + CTransactionRef walletTransaction; std::unique_ptr keyChange; CAmount fee; }; diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 833a77a82f91..8e272cb9a2aa 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -206,8 +206,8 @@ UniValue gobject_prepare(const JSONRPCRequest& request) outpoint = COutPoint(collateralHash, (uint32_t)collateralIndex); } - CWalletTx wtx; - if (!pwallet->GetBudgetSystemCollateralTX(wtx, govobj.GetHash(), govobj.GetMinCollateralFee(), outpoint)) { + CTransactionRef tx; + if (!pwallet->GetBudgetSystemCollateralTX(tx, govobj.GetHash(), govobj.GetMinCollateralFee(), outpoint)) { std::string err = "Error making collateral transaction for governance object. Please check your wallet balance and make sure your wallet is unlocked."; if (!request.params[6].isNull() && !request.params[7].isNull()) { err += "Please verify your specified output is valid and is enough for the combined proposal fee and transaction fee."; @@ -219,14 +219,14 @@ UniValue gobject_prepare(const JSONRPCRequest& request) CReserveKey reservekey(pwallet); // -- send the tx to the network CValidationState state; - if (!pwallet->CommitTransaction(wtx, reservekey, g_connman.get(), state)) { + if (!pwallet->CommitTransaction(tx, {}, {}, {}, reservekey, g_connman.get(), state)) { throw JSONRPCError(RPC_INTERNAL_ERROR, "CommitTransaction failed! Reason given: " + state.GetRejectReason()); } LogPrint(BCLog::GOBJECT, "gobject_prepare -- GetDataAsPlainString = %s, hash = %s, txid = %s\n", - govobj.GetDataAsPlainString(), govobj.GetHash().ToString(), wtx.GetHash().ToString()); + govobj.GetDataAsPlainString(), govobj.GetHash().ToString(), tx->GetHash().ToString()); - return wtx.GetHash().ToString(); + return tx->GetHash().ToString(); } #endif // ENABLE_WALLET diff --git a/src/rpc/rpcevo.cpp b/src/rpc/rpcevo.cpp index 73907753323a..3233557bcaed 100644 --- a/src/rpc/rpcevo.cpp +++ b/src/rpc/rpcevo.cpp @@ -229,18 +229,18 @@ static void FundSpecialTx(CWallet* pwallet, CMutableTransaction& tx, const Speci throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("No funds at specified address %s", EncodeDestination(fundDest))); } - CWalletTx wtx; + CTransactionRef newTx; CReserveKey reservekey(pwallet); CAmount nFee; int nChangePos = -1; std::string strFailReason; - if (!pwallet->CreateTransaction(vecSend, wtx, reservekey, nFee, nChangePos, strFailReason, coinControl, false, tx.vExtraPayload.size())) { + if (!pwallet->CreateTransaction(vecSend, newTx, reservekey, nFee, nChangePos, strFailReason, coinControl, false, tx.vExtraPayload.size())) { throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); } - tx.vin = wtx.tx->vin; - tx.vout = wtx.tx->vout; + tx.vin = newTx->vin; + tx.vout = newTx->vout; if (dummyTxOutAdded && tx.vout.size() > 1) { // CreateTransaction added a change output, so we don't need the dummy txout anymore. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 89be10b19857..6d0559d8bc42 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -381,7 +381,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request) return ret; } -static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew, const CCoinControl& coin_control) +static CTransactionRef SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue, std::string fromAccount) { CAmount curBalance = pwallet->GetBalance(); @@ -396,6 +396,10 @@ static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CA throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); } + if (coin_control.IsUsingPrivateSend()) { + mapValue["DS"] = "1"; + } + // Parse Dash address CScript scriptPubKey = GetScriptForDestination(address); @@ -407,17 +411,18 @@ static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CA int nChangePosRet = -1; CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; vecSend.push_back(recipient); - if (!pwallet->CreateTransaction(vecSend, wtxNew, reservekey, nFeeRequired, nChangePosRet, - strError, coin_control)) { + CTransactionRef tx; + if (!pwallet->CreateTransaction(vecSend, tx, reservekey, 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(wtxNew, reservekey, g_connman.get(), state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(fromAccount), reservekey, g_connman.get(), state)) { strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } + return tx; } UniValue sendtoaddress(const JSONRPCRequest& request) @@ -476,11 +481,11 @@ UniValue sendtoaddress(const JSONRPCRequest& request) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); // Wallet comments - CWalletTx wtx; + mapValue_t mapValue; if (!request.params[2].isNull() && !request.params[2].get_str().empty()) - wtx.mapValue["comment"] = request.params[2].get_str(); + mapValue["comment"] = request.params[2].get_str(); if (!request.params[3].isNull() && !request.params[3].get_str().empty()) - wtx.mapValue["to"] = request.params[3].get_str(); + mapValue["to"] = request.params[3].get_str(); bool fSubtractFeeFromAmount = false; if (!request.params[4].isNull()) { @@ -505,9 +510,8 @@ UniValue sendtoaddress(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, wtx, coin_control); - - return wtx.GetHash().GetHex(); + CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue), {} /* fromAccount */); + return tx->GetHash().GetHex(); } // DEPRECATED @@ -1013,12 +1017,11 @@ UniValue sendfrom(const JSONRPCRequest& request) nMinDepth = request.params[3].get_int(); bool fAddLocked = (!request.params[4].isNull() && request.params[4].get_bool()); - CWalletTx wtx; - wtx.strFromAccount = strAccount; + mapValue_t mapValue; if (!request.params[5].isNull() && !request.params[5].get_str().empty()) - wtx.mapValue["comment"] = request.params[5].get_str(); + mapValue["comment"] = request.params[5].get_str(); if (!request.params[6].isNull() && !request.params[6].get_str().empty()) - wtx.mapValue["to"] = request.params[6].get_str(); + mapValue["to"] = request.params[6].get_str(); EnsureWalletIsUnlocked(pwallet); @@ -1028,9 +1031,8 @@ UniValue sendfrom(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds"); CCoinControl no_coin_control; // This is a deprecated API - SendMoney(pwallet, dest, nAmount, false, wtx, no_coin_control); - - return wtx.GetHash().GetHex(); + CTransactionRef tx = SendMoney(pwallet, dest, nAmount, false, no_coin_control, std::move(mapValue), std::move(strAccount)); + return tx->GetHash().GetHex(); } @@ -1101,10 +1103,9 @@ UniValue sendmany(const JSONRPCRequest& request) nMinDepth = request.params[2].get_int(); bool fAddLocked = (!request.params[3].isNull() && request.params[3].get_bool()); - CWalletTx wtx; - wtx.strFromAccount = strAccount; + mapValue_t mapValue; if (!request.params[4].isNull() && !request.params[4].get_str().empty()) - wtx.mapValue["comment"] = request.params[4].get_str(); + mapValue["comment"] = request.params[4].get_str(); UniValue subtractFeeFrom(UniValue::VARR); if (!request.params[5].isNull()) @@ -1128,6 +1129,10 @@ UniValue sendmany(const JSONRPCRequest& request) } } + if (coin_control.IsUsingPrivateSend()) { + mapValue["DS"] = "1"; + } + std::set destinations; std::vector vecSend; @@ -1173,18 +1178,17 @@ UniValue sendmany(const JSONRPCRequest& request) CAmount nFeeRequired = 0; int nChangePosRet = -1; std::string strFailReason; - - bool fCreated = pwallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason, - coin_control); + CTransactionRef tx; + bool fCreated = pwallet->CreateTransaction(vecSend, tx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); CValidationState state; - if (!pwallet->CommitTransaction(wtx, keyChange, g_connman.get(), state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(strAccount), keyChange, g_connman.get(), state)) { strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); } - return wtx.GetHash().GetHex(); + return tx->GetHash().GetHex(); } UniValue addmultisigaddress(const JSONRPCRequest& request) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c1950fd299ab..d852db0b15ff 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -621,24 +621,24 @@ class ListCoinsTestingSetup : public TestChain100Setup CWalletTx& AddTx(CRecipient recipient) { - CWalletTx wtx; + CTransactionRef tx; CReserveKey reservekey(wallet.get()); CAmount fee; int changePos = -1; std::string error; CCoinControl dummy; - BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy)); + BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, reservekey, fee, changePos, error, dummy)); CValidationState state; - BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state)); + BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, {}, reservekey, nullptr, state)); CMutableTransaction blocktx; { LOCK(wallet->cs_wallet); - blocktx = CMutableTransaction(*wallet->mapWallet.at(wtx.tx->GetHash()).tx); + blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx); } CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); LOCK(cs_main); LOCK(wallet->cs_wallet); - auto it = wallet->mapWallet.find(wtx.GetHash()); + auto it = wallet->mapWallet.find(tx->GetHash()); BOOST_CHECK(it != wallet->mapWallet.end()); it->second.SetMerkleBranch(chainActive.Tip(), 1); return it->second; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf3bcae33be3..51e44fa13a98 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3318,13 +3318,13 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC nExtraPayloadSize = (int)tx.vExtraPayload.size(); CReserveKey reservekey(this); - CWalletTx wtx; - if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false, nExtraPayloadSize)) { + CTransactionRef tx_new; + if (!CreateTransaction(vecSend, tx_new, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false, nExtraPayloadSize)) { return false; } if (nChangePosInOut != -1) { - tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]); + 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. reservekey.KeepKey(); @@ -3333,11 +3333,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC // 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++) { - tx.vout[idx].nValue = wtx.tx->vout[idx].nValue; + tx.vout[idx].nValue = tx_new->vout[idx].nValue; } // Add new txins while keeping original txin scriptSig/order. - for (const CTxIn& txin : wtx.tx->vin) { + for (const CTxIn& txin : tx_new->vin) { if (!coinControl.IsSelected(txin.prevout)) { tx.vin.push_back(txin); @@ -3678,7 +3678,7 @@ bool CWallet::CreateCollateralTransaction(CMutableTransaction& txCollateral, std return true; } -bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, CAmount amount, const COutPoint& outpoint) +bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef tx, uint256 hash, CAmount amount, const COutPoint& outpoint) { // make our change address CReserveKey reservekey(this); @@ -3705,7 +3705,7 @@ bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, CAmount a return true; } -bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, +bool CWallet::CreateTransaction(const std::vector& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign, int nExtraPayloadSize) { CAmount nValue = 0; @@ -3729,12 +3729,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT return false; } - wtxNew.fTimeReceivedIsTxTime = true; - wtxNew.BindWallet(this); - if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - wtxNew.mapValue["DS"] = "1"; - } - CMutableTransaction txNew; // Discourage fee sniping. @@ -3822,7 +3816,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = nChangePosRequest; txNew.vin.clear(); txNew.vout.clear(); - wtxNew.fFromMe = true; bool fFirst = true; CAmount nValueToSelect = nValue; @@ -4085,14 +4078,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - // Embed the constructed transaction data in wtxNew. - wtxNew.SetTx(MakeTransactionRef(std::move(txNew))); + // Return the constructed transaction data. + tx = MakeTransactionRef(std::move(txNew)); } if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits LockPoints lp; - CTxMemPoolEntry entry(wtxNew.tx, 0, 0, 0, false, 0, lp); + CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); CTxMemPool::setEntries setAncestors; size_t nLimitAncestors = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); size_t nLimitAncestorSize = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000; @@ -4119,12 +4112,20 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT /** * Call after CreateTransaction unless you want to abort */ -bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CConnman* connman, CValidationState& state) +bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, std::string fromAccount, CReserveKey& reservekey, CConnman* connman, CValidationState& state) { { LOCK2(cs_main, mempool.cs); LOCK(cs_wallet); - LogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ + + CWalletTx wtxNew(this, std::move(tx)); + wtxNew.mapValue = std::move(mapValue); + wtxNew.vOrderForm = std::move(orderForm); + wtxNew.strFromAccount = std::move(fromAccount); + wtxNew.fTimeReceivedIsTxTime = true; + wtxNew.fFromMe = true; + + LogPrintf("CommitTransaction:\n%s\n", wtxNew.tx->ToString()); { // Take key pair from key pool so it won't be used again reservekey.KeepKey(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c94ff9b771f5..cd9fc48075df 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1040,7 +1040,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface CAmount GetNormalizedAnonymizedBalance() const; CAmount GetDenominatedBalance(bool unconfirmed=false) const; - bool GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, CAmount amount, const COutPoint& outpoint=COutPoint()/*defaults null*/); + bool GetBudgetSystemCollateralTX(CTransactionRef tx, uint256 hash, CAmount amount, const COutPoint& outpoint=COutPoint()/*defaults null*/); CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const; /** @@ -1055,9 +1055,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface * selected by SelectCoins(); Also create the change output, when needed * @note passing nChangePosInOut as -1 will result in setting a random position */ - bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, + bool CreateTransaction(const std::vector& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true, int nExtraPayloadSize = 0); - bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CConnman* connman, CValidationState& state); + bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, std::string fromAccount, CReserveKey& reservekey, CConnman* connman, CValidationState& state); bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason); From ceece041a6c95c701bcff7ab9510e3e9e06e1501 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 19 Jan 2017 16:08:03 -0500 Subject: [PATCH 2/3] [wallet] Get rid of CWalletTx default constructor No change in behavior in the normal case. But buggy mapWallet lookups with invalid txids will now throw exceptions instead of inserting dummy entries into the map, and potentially causing segfaults and other failures. This also makes it a compiler error to use the mapWallet[hash] syntax which could create dummy entries. --- src/wallet/test/accounting_tests.cpp | 8 ++++---- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 5 ----- src/wallet/walletdb.cpp | 6 +++--- 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/src/wallet/test/accounting_tests.cpp b/src/wallet/test/accounting_tests.cpp index fa37257c957d..4ed621bf7225 100644 --- a/src/wallet/test/accounting_tests.cpp +++ b/src/wallet/test/accounting_tests.cpp @@ -29,7 +29,7 @@ GetResults(CWallet& wallet, std::map& results) BOOST_AUTO_TEST_CASE(acc_orderupgrade) { std::vector vpwtx; - CWalletTx wtx; + CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); CAccountingEntry ae; std::map results; @@ -44,7 +44,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade) wtx.mapValue["comment"] = "z"; m_wallet.AddToWallet(wtx); - vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]); + vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash())); vpwtx[0]->nTimeReceived = (unsigned int)1333333335; vpwtx[0]->nOrderPos = -1; @@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade) wtx.SetTx(MakeTransactionRef(std::move(tx))); } m_wallet.AddToWallet(wtx); - vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]); + vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash())); vpwtx[1]->nTimeReceived = (unsigned int)1333333336; wtx.mapValue["comment"] = "x"; @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade) wtx.SetTx(MakeTransactionRef(std::move(tx))); } m_wallet.AddToWallet(wtx); - vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]); + vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash())); vpwtx[2]->nTimeReceived = (unsigned int)1333333329; vpwtx[2]->nOrderPos = -1; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 51e44fa13a98..d4e197083009 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -720,7 +720,7 @@ void CWallet::SyncMetaData(std::pair ran int nMinOrderPos = std::numeric_limits::max(); const CWalletTx* copyFrom = nullptr; for (TxSpends::iterator it = range.first; it != range.second; ++it) { - const CWalletTx* wtx = &mapWallet[it->second]; + const CWalletTx* wtx = &mapWallet.at(it->second); if (wtx->nOrderPos < nMinOrderPos) { nMinOrderPos = wtx->nOrderPos; copyFrom = wtx; @@ -735,7 +735,7 @@ void CWallet::SyncMetaData(std::pair ran for (TxSpends::iterator it = range.first; it != range.second; ++it) { const uint256& hash = it->second; - CWalletTx* copyTo = &mapWallet[hash]; + CWalletTx* copyTo = &mapWallet.at(hash); if (copyFrom == copyTo) continue; assert(copyFrom && "Oldest wallet transaction in range assumed to have been found."); if (!copyFrom->IsEquivalentTo(*copyTo)) continue; @@ -4141,7 +4141,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve // notify only once if(updated_hahes.find(txin.prevout.hash) != updated_hahes.end()) continue; - CWalletTx &coin = mapWallet[txin.prevout.hash]; + CWalletTx &coin = mapWallet.at(txin.prevout.hash); coin.BindWallet(this); NotifyTransactionChanged(this, txin.prevout.hash, CT_UPDATED); updated_hahes.insert(txin.prevout.hash); @@ -4153,7 +4153,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve // Get the inserted-CWalletTx from mapWallet so that the // fInMempool flag is cached properly - CWalletTx& wtx = mapWallet[wtxNew.GetHash()]; + CWalletTx& wtx = mapWallet.at(wtxNew.GetHash()); if (fBroadcastTransactions) { @@ -4670,7 +4670,7 @@ std::set< std::set > CWallet::GetAddressGroupings() CTxDestination address; if(!IsMine(txin)) /* If this input isn't mine, ignore it */ continue; - if(!ExtractDestination(mapWallet[txin.prevout.hash].tx->vout[txin.prevout.n].scriptPubKey, address)) + if(!ExtractDestination(mapWallet.at(txin.prevout.hash).tx->vout[txin.prevout.n].scriptPubKey, address)) continue; grouping.insert(address); any_mine = true; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cd9fc48075df..76b1ab6fb671 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -358,11 +358,6 @@ class CWalletTx : public CMerkleTx mutable CAmount nAvailableWatchCreditCached; mutable CAmount nChangeCached; - CWalletTx() - { - Init(nullptr); - } - CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg)) { Init(pwalletIn); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 2bc01ac01c98..c9d2ede73787 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -278,7 +278,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, { uint256 hash; ssKey >> hash; - CWalletTx wtx; + CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); ssValue >> wtx; CValidationState state; if (!(CheckTransaction(*wtx.tx, state) && (wtx.GetHash() == hash) && state.IsValid())) @@ -651,7 +651,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) pwallet->UpdateTimeFirstKey(1); for (uint256 hash : wss.vWalletUpgrade) - WriteTx(pwallet->mapWallet[hash]); + WriteTx(pwallet->mapWallet.at(hash)); // Rewrite encrypted wallets of versions 0.4.0 and 0.5.0rc: if (wss.fIsEncrypted && (wss.nFileVersion == 40000 || wss.nFileVersion == 50000)) @@ -712,7 +712,7 @@ DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::vector> hash; - CWalletTx wtx; + CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); ssValue >> wtx; vTxHash.push_back(hash); From adcfa7553d9b20dbfb5bfca9a44c25a870028ca2 Mon Sep 17 00:00:00 2001 From: dustinface <35775977+xdustinface@users.noreply.github.com> Date: Thu, 3 Sep 2020 12:33:00 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: UdjinM6 --- src/qt/walletmodel.cpp | 7 ++++--- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index f01c5d49be37..864191a7e777 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -402,19 +402,20 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran vOrderForm.emplace_back("Message", rcp.message.toStdString()); } + mapValue_t mapValue; if (fIsPrivateSend) { - vOrderForm.emplace_back("DS", "1"); + mapValue["DS"] = "1"; } CTransactionRef& newTx = transaction.getTransaction(); CReserveKey *keyChange = transaction.getPossibleKeyChange(); CValidationState state; - if (!wallet->CommitTransaction(newTx, {} /* mapValue */, std::move(vOrderForm), {} /* fromAccount */, *keyChange, g_connman.get(), state)) + if (!wallet->CommitTransaction(newTx, std::move(mapValue), std::move(vOrderForm), {} /* fromAccount */, *keyChange, g_connman.get(), state)) return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason())); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << newTx; - transaction_array.append(&(ssTx[0]), ssTx.size()); + transaction_array.append(ssTx.data(), ssTx.size()); } // Add addresses / update labels that we've sent to the address book, diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d4e197083009..f61ec52b6a70 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3678,7 +3678,7 @@ bool CWallet::CreateCollateralTransaction(CMutableTransaction& txCollateral, std return true; } -bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef tx, uint256 hash, CAmount amount, const COutPoint& outpoint) +bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint) { // make our change address CReserveKey reservekey(this); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 76b1ab6fb671..b91d0f8c026e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1035,7 +1035,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface CAmount GetNormalizedAnonymizedBalance() const; CAmount GetDenominatedBalance(bool unconfirmed=false) const; - bool GetBudgetSystemCollateralTX(CTransactionRef tx, uint256 hash, CAmount amount, const COutPoint& outpoint=COutPoint()/*defaults null*/); + bool GetBudgetSystemCollateralTX(CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint=COutPoint()/*defaults null*/); CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const; /**