[Sapling][Refactoring] Save wallet reference inside SaplingOperation

src/qt/addresstablemodel.cpp

@@ -132,7 +132,7 @@ static QString translateTypeToString(AddressTableEntry::Type type)
 class AddressTablePriv
 {
 public:
-    CWallet* wallet;
+    CWallet* wallet{nullptr};
     QList<AddressTableEntry> cachedAddressTable;
     int sendNum = 0;
     int recvNum = 0;

src/qt/transactiontablemodel.cpp

@@ -77,7 +77,7 @@ class TransactionTablePriv
     {
     }
 
-    CWallet* wallet;
+    CWallet* wallet{nullptr};
     TransactionTableModel* parent;
 
     /* Local cache of wallet.

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)

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.

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<COutput> 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<boost::optional<SaplingWitness>> 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++) {

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<SendManyRecipient>& 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};

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<SendManyRecipient> 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<SendManyRecipient> 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<SendManyRecipient> 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,19 +438,16 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling)
     pwalletMain->BlockConnected(std::make_shared<CBlock>(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<SendManyRecipient> recipients = { SendManyRecipient(zaddr1, 1 * COIN, "ABCD") };
-    SaplingOperation operation(builder);
+    SaplingOperation operation(consensusParams, nextBlockHeight, pwalletMain);
     operation.setFromAddress(taddr);
     BOOST_CHECK(operation.setRecipients(recipients)
                          ->setMinDepth(0)
                          ->build());
 
     // try from auto-selected transparent address
     std::vector<SendManyRecipient> recipients2 = { SendManyRecipient(zaddr1, 1 * COIN, "ABCD") };
-    SaplingOperation operation2(builder);
+    SaplingOperation operation2(consensusParams, nextBlockHeight, pwalletMain);
     BOOST_CHECK(operation2.setSelectTransparentCoins(true)
                           ->setRecipients(recipients2)
                           ->setMinDepth(0)

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<SendManyRecipient> 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"

src/wallet/test/wallet_sapling_transactions_validations_tests.cpp

@@ -43,8 +43,7 @@ SaplingOperation createOperationAndBuildTx(std::vector<SendManyRecipient> 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)