diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index bf759b4336c0..f2967d519d50 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -132,7 +132,7 @@ static QString translateTypeToString(AddressTableEntry::Type type) class AddressTablePriv { public: - CWallet* wallet; + CWallet* wallet{nullptr}; QList cachedAddressTable; int sendNum = 0; int recvNum = 0; diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 8472467c2d7a..da2aaa375ede 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -77,7 +77,7 @@ class TransactionTablePriv { } - CWallet* wallet; + CWallet* wallet{nullptr}; TransactionTableModel* parent; /* Local cache of wallet. diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 5caf65e2a216..7dd3ae145e6d 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -607,8 +607,7 @@ OperationResult WalletModel::PrepareShieldedTransaction(WalletModelTransaction* if (!opResult) return opResult; // Create the operation - TransactionBuilder txBuilder = TransactionBuilder(Params().GetConsensus(), nextBlockHeight, wallet); - SaplingOperation operation(txBuilder); + SaplingOperation operation(Params().GetConsensus(), nextBlockHeight, wallet); auto operationResult = operation.setRecipients(recipients) ->setTransparentKeyChange(modelTransaction->getPossibleKeyChange()) ->setSelectTransparentCoins(fromTransparent) diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index ce0ffb91c61a..b054e570fa6b 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -346,7 +346,7 @@ class WalletModel : public QObject void stop(); private: - CWallet* wallet; + CWallet* wallet{nullptr}; // Simple Wallet interface. // todo: Goal would be to move every CWallet* call to the wallet wrapper and // in the model only perform the data organization (and QT wrappers) to be presented on the UI. diff --git a/src/sapling/sapling_operation.cpp b/src/sapling/sapling_operation.cpp index 6d9b72b5ab2a..ad7dc55d2c2b 100644 --- a/src/sapling/sapling_operation.cpp +++ b/src/sapling/sapling_operation.cpp @@ -19,6 +19,18 @@ struct TxValues CAmount target{0}; }; +SaplingOperation::SaplingOperation(const Consensus::Params& consensusParams, int nHeight, CWallet* _wallet) : + wallet(_wallet), + txBuilder(consensusParams, nHeight, _wallet) +{ + assert (wallet != nullptr); +}; + +SaplingOperation::~SaplingOperation() +{ + delete tkeyChange; +} + OperationResult SaplingOperation::checkTxValues(TxValues& txValues, bool isFromtAddress, bool isFromShielded) { assert(!isFromtAddress || txValues.shieldedInTotal == 0); @@ -36,13 +48,14 @@ OperationResult SaplingOperation::checkTxValues(TxValues& txValues, bool isFromt return OperationResult(true); } -OperationResult loadKeysFromShieldedFrom(const libzcash::SaplingPaymentAddress &addr, +OperationResult loadKeysFromShieldedFrom(const CWallet* pwallet, + const libzcash::SaplingPaymentAddress &addr, libzcash::SaplingExpandedSpendingKey& expskOut, uint256& ovkOut) { // Get spending key for address libzcash::SaplingExtendedSpendingKey sk; - if (!pwalletMain->GetSaplingExtendedSpendingKey(addr, sk)) { + if (!pwallet->GetSaplingExtendedSpendingKey(addr, sk)) { return errorOut("Spending key not in the wallet"); } expskOut = sk.expsk; @@ -126,7 +139,7 @@ OperationResult SaplingOperation::build() // Get the common OVK for recovering t->shield outputs. // If not already databased, a new one will be generated from the HD seed. // It is safe to do it here, as the wallet is unlocked. - ovk = pwalletMain->GetSaplingScriptPubKeyMan()->getCommonOVK(); + ovk = wallet->GetSaplingScriptPubKeyMan()->getCommonOVK(); } // Add outputs @@ -158,7 +171,7 @@ OperationResult SaplingOperation::build() // Set change address if we are using transparent funds if (isFromtAddress) { if (!tkeyChange) { - tkeyChange = new CReserveKey(pwalletMain); + tkeyChange = new CReserveKey(wallet); } CPubKey vchPubKey; if (!tkeyChange->GetReservedKey(vchPubKey, true)) { @@ -227,7 +240,7 @@ OperationResult SaplingOperation::build() OperationResult SaplingOperation::send(std::string& retTxHash) { - const CWallet::CommitResult& res = pwalletMain->CommitTransaction(finalTx, tkeyChange, g_connman.get()); + const CWallet::CommitResult& res = wallet->CommitTransaction(finalTx, tkeyChange, g_connman.get()); if (res.status != CWallet::CommitStatus::OK) { return errorOut(res.ToString()); } @@ -271,7 +284,7 @@ OperationResult SaplingOperation::loadUtxos(TxValues& txValues) std::vector selectedUTXOInputs; CAmount nSelectedValue = 0; for (const auto& outpoint : vCoins) { - const auto* tx = pwalletMain->GetWalletTx(outpoint.outPoint.hash); + const auto* tx = wallet->GetWalletTx(outpoint.outPoint.hash); if (!tx) continue; nSelectedValue += tx->tx->vout[outpoint.outPoint.n].nValue; selectedUTXOInputs.emplace_back(tx, outpoint.outPoint.n, 0, true, true); @@ -289,7 +302,7 @@ OperationResult SaplingOperation::loadUtxos(TxValues& txValues) true, &destinations, mindepth); - if (!pwalletMain->AvailableCoins(&transInputs, nullptr, coinsFilter)) { + if (!wallet->AvailableCoins(&transInputs, nullptr, coinsFilter)) { return errorOut("Insufficient funds, no available UTXO to spend"); } @@ -353,10 +366,12 @@ OperationResult SaplingOperation::loadUtxos(TxValues& txValues, const std::vecto * recover it from the note (now that we have the spending key). */ enum CacheCheckResult {OK, SPENT, INVALID}; -static CacheCheckResult CheckCachedNote(const SaplingNoteEntry& t, const libzcash::SaplingExpandedSpendingKey& expsk) +static CacheCheckResult CheckCachedNote(CWallet* pwallet, + const SaplingNoteEntry& t, + const libzcash::SaplingExpandedSpendingKey& expsk) { - auto sspkm = pwalletMain->GetSaplingScriptPubKeyMan(); - CWalletTx& prevTx = pwalletMain->mapWallet.at(t.op.hash); + auto sspkm = pwallet->GetSaplingScriptPubKeyMan(); + CWalletTx& prevTx = pwallet->mapWallet.at(t.op.hash); SaplingNoteData& nd = prevTx.mapSaplingNoteData.at(t.op); if (nd.witnesses.empty()) { return CacheCheckResult::INVALID; @@ -372,13 +387,13 @@ static CacheCheckResult CheckCachedNote(const SaplingNoteEntry& t, const libzcas LogPrintf("ERROR: Unable to recover nullifier for note %s.\n", noteStr); return CacheCheckResult::INVALID; } - WITH_LOCK(pwalletMain->cs_wallet, sspkm->UpdateSaplingNullifierNoteMap(nd, t.op, nf)); + WITH_LOCK(pwallet->cs_wallet, sspkm->UpdateSaplingNullifierNoteMap(nd, t.op, nf)); // re-check the spent status if (sspkm->IsSaplingSpent(*(nd.nullifier))) { LogPrintf("Removed note %s as it appears to be already spent.\n", noteStr); prevTx.MarkDirty(); - CWalletDB(pwalletMain->GetDBHandle(), "r+").WriteTx(prevTx); - pwalletMain->NotifyTransactionChanged(pwalletMain, t.op.hash, CT_UPDATED); + CWalletDB(pwallet->GetDBHandle(), "r+").WriteTx(prevTx); + pwallet->NotifyTransactionChanged(pwallet, t.op.hash, CT_UPDATED); return CacheCheckResult::SPENT; } } @@ -388,7 +403,7 @@ static CacheCheckResult CheckCachedNote(const SaplingNoteEntry& t, const libzcas OperationResult SaplingOperation::loadUnspentNotes(TxValues& txValues, uint256& ovk) { shieldedInputs.clear(); - auto sspkm = pwalletMain->GetSaplingScriptPubKeyMan(); + auto sspkm = wallet->GetSaplingScriptPubKeyMan(); // if we already have selected the notes, let's directly set them. bool hasCoinControl = coinControl && coinControl->HasSelected(); if (hasCoinControl) { @@ -437,12 +452,12 @@ OperationResult SaplingOperation::loadUnspentNotes(TxValues& txValues, uint256& // Get the spending key for the address. libzcash::SaplingExpandedSpendingKey expsk; uint256 ovkIn; - auto resLoadKeys = loadKeysFromShieldedFrom(t.address, expsk, ovkIn); + auto resLoadKeys = loadKeysFromShieldedFrom(wallet, t.address, expsk, ovkIn); if (!resLoadKeys) return resLoadKeys; // If the noteData is not properly cached, for whatever reason, // try to update it here, now that we have the spending key. - CacheCheckResult res = CheckCachedNote(t, expsk); + CacheCheckResult res = CheckCachedNote(wallet, t, expsk); if (res == CacheCheckResult::INVALID) { // This should never happen. User would be forced to zap. LogPrintf("ERROR: Witness/Nullifier invalid for note %s. Restart with --zapwallettxes\n", t.op.ToString()); @@ -481,7 +496,7 @@ OperationResult SaplingOperation::loadUnspentNotes(TxValues& txValues, uint256& // Fetch Sapling anchor and witnesses uint256 anchor; std::vector> witnesses; - pwalletMain->GetSaplingScriptPubKeyMan()->GetSaplingNoteWitnesses(ops, witnesses, anchor); + wallet->GetSaplingScriptPubKeyMan()->GetSaplingNoteWitnesses(ops, witnesses, anchor); // Add Sapling spends for (size_t i = 0; i < notes.size(); i++) { diff --git a/src/sapling/sapling_operation.h b/src/sapling/sapling_operation.h index c1c4cb74bc11..2df64a1c8abc 100644 --- a/src/sapling/sapling_operation.h +++ b/src/sapling/sapling_operation.h @@ -81,10 +81,8 @@ class FromAddress { class SaplingOperation { public: - explicit SaplingOperation(const Consensus::Params& consensusParams, int chainHeight) : txBuilder(consensusParams, chainHeight) {}; - explicit SaplingOperation(TransactionBuilder& _builder) : txBuilder(_builder) {}; - - ~SaplingOperation() { delete tkeyChange; } + explicit SaplingOperation(const Consensus::Params& consensusParams, int nHeight, CWallet* _wallet); + ~SaplingOperation(); OperationResult build(); OperationResult send(std::string& retTxHash); @@ -99,7 +97,6 @@ class SaplingOperation { SaplingOperation* setRecipients(std::vector& vec) { recipients = std::move(vec); return this; }; SaplingOperation* setFee(CAmount _fee) { fee = _fee; return this; } SaplingOperation* setMinDepth(int _mindepth) { assert(_mindepth >= 0); mindepth = _mindepth; return this; } - SaplingOperation* setTxBuilder(TransactionBuilder& builder) { txBuilder = builder; return this; } SaplingOperation* setTransparentKeyChange(CReserveKey* reserveKey) { tkeyChange = reserveKey; return this; } SaplingOperation* setCoinControl(const CCoinControl* _coinControl) { coinControl = _coinControl; return this; } @@ -108,6 +105,13 @@ class SaplingOperation { CTransactionRef getFinalTxRef() { return finalTx; } private: + /* + * Cannot be nullptr. A pointer to the wallet, used to retrieve the inputs to spend, the keys to create the outputs, + * sapling notes and nullifiers, as well as to commit transactions. + * The same keystore is passed to the transaction builder in order to produce the required signatures. + */ + CWallet* wallet{nullptr}; + FromAddress fromAddress; // In case of no addressFrom filter selected, it will accept any utxo in the wallet as input. bool selectFromtaddrs{false}; diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index c3a86997c2e2..c0ce6b108831 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -333,7 +333,7 @@ BOOST_AUTO_TEST_CASE(saplingOperationTests) { // there are no utxos to spend { std::vector recipients = { SendManyRecipient(zaddr1, COIN, "DEADBEEF") }; - SaplingOperation operation(consensusParams, 1); + SaplingOperation operation(consensusParams, 1, pwalletMain); operation.setFromAddress(taddr1); auto res = operation.setRecipients(recipients)->buildAndSend(ret); BOOST_CHECK(!res); @@ -343,7 +343,7 @@ BOOST_AUTO_TEST_CASE(saplingOperationTests) { // minconf cannot be zero when sending from zaddr { std::vector recipients = { SendManyRecipient(zaddr1, COIN, "DEADBEEF") }; - SaplingOperation operation(consensusParams, 1); + SaplingOperation operation(consensusParams, 1, pwalletMain); operation.setFromAddress(zaddr1); auto res = operation.setRecipients(recipients)->setMinDepth(0)->buildAndSend(ret); BOOST_CHECK(!res); @@ -353,7 +353,7 @@ BOOST_AUTO_TEST_CASE(saplingOperationTests) { // there are no unspent notes to spend { std::vector recipients = { SendManyRecipient(taddr1, COIN) }; - SaplingOperation operation(consensusParams, 1); + SaplingOperation operation(consensusParams, 1, pwalletMain); operation.setFromAddress(zaddr1); auto res = operation.setRecipients(recipients)->buildAndSend(ret); BOOST_CHECK(!res); @@ -438,11 +438,8 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) pwalletMain->BlockConnected(std::make_shared(block), mi->second, vtxConflicted); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); - // Context that shieldsendmany requires - auto builder = TransactionBuilder(consensusParams, nextBlockHeight, pwalletMain); - std::vector recipients = { SendManyRecipient(zaddr1, 1 * COIN, "ABCD") }; - SaplingOperation operation(builder); + SaplingOperation operation(consensusParams, nextBlockHeight, pwalletMain); operation.setFromAddress(taddr); BOOST_CHECK(operation.setRecipients(recipients) ->setMinDepth(0) @@ -450,7 +447,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) // try from auto-selected transparent address std::vector recipients2 = { SendManyRecipient(zaddr1, 1 * COIN, "ABCD") }; - SaplingOperation operation2(builder); + SaplingOperation operation2(consensusParams, nextBlockHeight, pwalletMain); BOOST_CHECK(operation2.setSelectTransparentCoins(true) ->setRecipients(recipients2) ->setMinDepth(0) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6a150e2c32d9..15b1fe97db6b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1168,8 +1168,7 @@ UniValue CreateColdStakeDelegation(const UniValue& params, CTransactionRef& txNe throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling not active yet"); } std::vector recipients = {SendManyRecipient(ownerKey, *stakeKey, nValue)}; - TransactionBuilder txBuilder = TransactionBuilder(consensus, nextBlockHeight, pwalletMain); - SaplingOperation operation(txBuilder); + SaplingOperation operation(consensus, nextBlockHeight, pwalletMain); OperationResult res = operation.setSelectShieldedCoins(true) ->setRecipients(recipients) ->build(); @@ -1520,8 +1519,7 @@ static SaplingOperation CreateShieldedTransaction(const JSONRPCRequest& request) EnsureWalletIsUnlocked(); LOCK2(cs_main, pwalletMain->cs_wallet); int nextBlockHeight = chainActive.Height() + 1; - TransactionBuilder txBuilder = TransactionBuilder(Params().GetConsensus(), nextBlockHeight, pwalletMain); - SaplingOperation operation(txBuilder); + SaplingOperation operation(Params().GetConsensus(), nextBlockHeight, pwalletMain); // Param 0: source of funds. Can either be a valid address, sapling address, // or the string "from_transparent"|"from_trans_cold"|"from_shield" diff --git a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp index 161aa00e026d..45e652f072fc 100644 --- a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp +++ b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp @@ -43,8 +43,7 @@ SaplingOperation createOperationAndBuildTx(std::vector recipi bool selectTransparentCoins) { // Create the operation - TransactionBuilder txBuilder = TransactionBuilder(Params().GetConsensus(), nextBlockHeight, pwalletMain); - SaplingOperation operation(txBuilder); + SaplingOperation operation(Params().GetConsensus(), nextBlockHeight, pwalletMain); auto operationResult = operation.setRecipients(recipients) ->setSelectTransparentCoins(selectTransparentCoins) ->setSelectShieldedCoins(!selectTransparentCoins)