Lock notes (except Orchard) in wallet_tx_builder

This fixes an RPC test failure that tests specifically for this with
z_shieldcoinbase. This also exposed an issue where an overly-high fee resulted
in a negative Payment causing an exception too deep. Added an assert when
creating a Payment and guarded against it for z_shieldcoinbase.

Fixes #2621 and #5654 (but does not handle Orchard locking, which is tracked in
a separate issue).

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -84,7 +84,7 @@ void AsyncRPCOperation_sendmany::main() {
 
     std::optional<uint256> txid;
     try {
-        txid = main_impl();
+        txid = main_impl(*pwalletMain);
     } catch (const UniValue& objError) {
         int code = find_value(objError, "code").get_int();
         std::string message = find_value(objError, "message").get_str();
@@ -137,11 +137,11 @@ void AsyncRPCOperation_sendmany::main() {
 // 4. #3615 There is no padding of inputs or outputs, which may leak information.
 //
 // At least #4 differs from the Rust transaction builder.
-uint256 AsyncRPCOperation_sendmany::main_impl() {
-    auto spendable = builder_.FindAllSpendableInputs(*pwalletMain, ztxoSelector_, mindepth_);
+uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
+    auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, mindepth_);
 
     auto preparedTx = builder_.PrepareTransaction(
-            *pwalletMain,
+            wallet,
             ztxoSelector_,
             spendable,
             recipients_,
@@ -156,6 +156,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
             ThrowInputSelectionError(err, ztxoSelector_, strategy_);
         },
         [&](const TransactionEffects& effects) {
+            try {
                 const auto& spendable = effects.GetSpendable();
                 const auto& payments = effects.GetPayments();
                 spendable.LogInputs(getId());
@@ -178,7 +179,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
 
                 auto buildResult = effects.ApproveAndBuild(
                         Params().GetConsensus(),
-                    *pwalletMain,
+                        wallet,
                         chainActive,
                         strategy_);
                 auto tx = buildResult.GetTxOrThrow();
@@ -187,6 +188,11 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
                 set_result(sendResult);
 
                 txid = tx.GetHash();
+                effects.UnlockSpendable(wallet);
+            } catch (...) {
+                effects.UnlockSpendable(wallet);
+                throw;
+            }
         }
     }, preparedTx);
 

src/wallet/asyncrpcoperation_sendmany.h

@@ -64,7 +64,7 @@ private:
     CAmount fee_;
     UniValue contextinfo_;     // optional data to include in return value from getStatus()
 
-    uint256 main_impl();
+    uint256 main_impl(CWallet& wallet);
 };
 
 // To test private methods, a friend class can act as a proxy
@@ -74,8 +74,8 @@ public:
 
     TEST_FRIEND_AsyncRPCOperation_sendmany(std::shared_ptr<AsyncRPCOperation_sendmany> ptr) : delegate(ptr) {}
 
-    uint256 main_impl() {
-        return delegate->main_impl();
+    uint256 main_impl(CWallet& wallet) {
+        return delegate->main_impl(wallet);
     }
 
     void set_state(OperationStatus state) {

src/wallet/wallet_tx_builder.cpp

@@ -147,7 +147,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
 
     auto ovks = SelectOVKs(wallet, selector, spendable);
 
-    return TransactionEffects(
+    auto effects = TransactionEffects(
             anchorConfirmations,
             spendable,
             resolvedPayments,
@@ -156,6 +156,8 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
             ovks.first,
             ovks.second,
             anchorHeight);
+    effects.LockSpendable(wallet);
+    return effects;
 }
 
 Payments InputSelection::GetPayments() const {
@@ -646,3 +648,35 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild(
     // Build the transaction
     return builder.Build();
 }
+
+// TODO: Lock Orchard notes (#6226)
+void TransactionEffects::LockSpendable(CWallet& wallet) const
+{
+    LOCK2(cs_main, wallet.cs_wallet);
+    for (auto utxo : spendable.utxos) {
+        COutPoint outpt(utxo.tx->GetHash(), utxo.i);
+        wallet.LockCoin(outpt);
+    }
+    for (auto note : spendable.sproutNoteEntries) {
+        wallet.LockNote(note.jsop);
+    }
+    for (auto note : spendable.saplingNoteEntries) {
+        wallet.LockNote(note.op);
+    }
+}
+
+// TODO: Unlock Orchard notes (#6226)
+void TransactionEffects::UnlockSpendable(CWallet& wallet) const
+{
+    LOCK2(cs_main, wallet.cs_wallet);
+    for (auto utxo : spendable.utxos) {
+        COutPoint outpt(utxo.tx->GetHash(), utxo.i);
+        wallet.UnlockCoin(outpt);
+    }
+    for (auto note : spendable.sproutNoteEntries) {
+        wallet.UnlockNote(note.jsop);
+    }
+    for (auto note : spendable.saplingNoteEntries) {
+        wallet.UnlockNote(note.op);
+    }
+}

src/wallet/wallet_tx_builder.h

@@ -48,7 +48,9 @@ public:
             PaymentAddress address,
             CAmount amount,
             std::optional<Memo> memo) :
-        address(address), amount(amount), memo(memo) {}
+        address(address), amount(amount), memo(memo) {
+        assert(amount >= 0);
+    }
 
     const PaymentAddress& GetAddress() const {
         return address;
@@ -158,6 +160,10 @@ private:
     int anchorHeight;
 
 public:
+    /**
+     * This class locks the `SpendableInputs` provided to it. `UnlockSpendable` must be called to
+     * release them before the instance goes out of scope.
+     */
     TransactionEffects(
         uint32_t anchorConfirmations,
         SpendableInputs spendable,
@@ -185,6 +191,17 @@ public:
         return spendable;
     }
 
+    /**
+     * This is automatically called by the constructor, so it’s not generally necessary to call this
+     * otherwise.
+     */
+    void LockSpendable(CWallet& wallet) const;
+
+    /**
+     * This should be called just before the `TransactionEffects` goes out of scope.
+     */
+    void UnlockSpendable(CWallet& wallet) const;
+
     const Payments& GetPayments() const {
         return payments;
     }