Repository: zcash/zcash (language: rust)

Issue:
z_sendmany crashes zcashd
On version 5.5.0. Starting with a fresh regtest node.
```bash
zcashd -regtest -printtoconsole -nuparams=5ba81b19:1 -nuparams=76b809bb:1 -nuparams=2bb40e60:1 -nuparams=f5b9230b:1 -nuparams=e9ff75a6:2 -nuparams=c2d6d0b4:3
```
```bash
# coinbase maturity = 100
zcash-cli -regtest generate 110
zcash-cli -regtest z_getnewaccount
zcash-cli -regtest z_getaddressforaccount 0
zcash-cli -regtest z_listunifiedreceivers "unified_addr"
# pick out the transparent address and send to ourselves to de-coinbase utxos.
zcash-cli -regtest sendtoaddress "t_addr" 50
zcash-cli -regtest generate 1
zcash-cli -regtest z_getaddressforaccount 0 '["orchard"]'
zcash-cli -regtest z_sendmany ANY_TADDR '[{"address": "orchard_addr", "amount": 2}]' 1 null AllowRevealedSenders
```
zcashd crashes with
```
zcashd: wallet/wallet_tx_builder.cpp:382: auto WalletTxBuilder::GetChangeAddress(CWallet &, const ZTXOSelector &, const SpendableInputs &, const Payments &, const TransactionStrategy &, bool)::(anonymous class)::operator()(const AccountZTXOPattern &) const: Assertion `addr.has_value()' failed.
Aborted (core dumped)
```


Discussion / hints:
Sorry for this crash. Thanks for reporting, and we’re working on a fix.

However, this should only occur when the transaction isn’t valid anyway. In this case what’s happening is that the `z_sendmany` will result in change being sent to a taddr, but `AllowRevealedSenders` doesn’t allow change to taddrs. This tx requires `AllowFullyTransparent`.

That adjustment should avoid the crash, but I think this transaction is still unlikely to succeed. `ANY_TADDR` will only select non-coinbase UTXOs from non-UA taddrs, but it seems like you want it to select from `t_addr`, which is a UA receiver, so you would need to use the UA as the fromaddress. If you simply want to shield the original coinbase UXTOs, then `z_shieldcoinbase '*' orchard_addr` should do it (and doesn’t require `AllowRevealedRecipients`).

Fix patch:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index bb792517aed..c4b3051d764 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2158,9 +2158,11 @@ bool CWallet::SelectorMatchesAddress(
     });
 }
 
-std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
+tl::expected<RecipientAddress, AccountChangeAddressFailure>
+CWallet::GenerateChangeAddressForAccount(
         libzcash::AccountId accountId,
-        std::set<OutputPool> changeOptions) {
+        std::set<OutputPool> changeOptions)
+{
     AssertLockHeld(cs_wallet);
 
     if (accountId == ZCASH_LEGACY_ACCOUNT) {
@@ -2170,6 +2172,7 @@ std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
                 return GenerateNewKey(false).GetID();
             }
         }
+        return tl::make_unexpected(AccountChangeAddressFailure::TransparentChangeNotPermitted);
     } else {
         auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId);
         if (ufvk.has_value()) {
@@ -2197,10 +2200,11 @@ std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
                     return changeAddr.value();
                 }
             }
+            return tl::make_unexpected(AccountChangeAddressFailure::DisjointReceivers);
+        } else {
+            return tl::make_unexpected(AccountChangeAddressFailure::NoSuchAccount);
         }
     }
-
-    return std::nullopt;
 }
 
 SpendableInputs CWallet::FindSpendableInputs(
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index d371eeca6d4..e945743c4f3 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1188,6 +1188,12 @@ class WalletBatchScanner : public BatchScanner {
         const int nHeight);
 };
 
+enum class AccountChangeAddressFailure {
+    DisjointReceivers,
+    TransparentChangeNotPermitted,
+    NoSuchAccount,
+};
+
 /**
  * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
  * and provides the ability to create new transactions.
@@ -1607,7 +1613,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
      * Returns `std::nullopt` if the account does not have an internal spending
      * key matching the requested `OutputPool`.
      */
-    std::optional<libzcash::RecipientAddress> GenerateChangeAddressForAccount(
+    tl::expected<libzcash::RecipientAddress, AccountChangeAddressFailure>
+    GenerateChangeAddressForAccount(
             libzcash::AccountId accountId,
             std::set<libzcash::OutputPool> changeOptions);
 
diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp
index 968e90384ad..fa9877051fe 100644
--- a/src/wallet/wallet_tx_builder.cpp
+++ b/src/wallet/wallet_tx_builder.cpp
@@ -376,11 +376,20 @@ WalletTxBuilder::GetChangeAddress(
                     fvk.GetKnownReceiverTypes());
         },
         [&](const AccountZTXOPattern& acct) -> tl::expected<ChangeAddress, AddressResolutionError> {
-                auto addr = wallet.GenerateChangeAddressForAccount(
+            return wallet.GenerateChangeAddressForAccount(
                     acct.GetAccountId(),
-                    getAllowedChangePools(acct.GetReceiverTypes()));
-            assert(addr.has_value());
-            return addr.value();
+                    getAllowedChangePools(acct.GetReceiverTypes()))
+                .map_error([](auto err) {
+                    switch (err) {
+                        case AccountChangeAddressFailure::DisjointReceivers:
+                            return AddressResolutionError::CouldNotResolveReceiver;
+                        case AccountChangeAddressFailure::TransparentChangeNotPermitted:
+                            return AddressResolutionError::TransparentChangeNotAllowed;
+                        case AccountChangeAddressFailure::NoSuchAccount:
+                            throw std::runtime_error("Selector account doesn’t exist.");
+                    }
+                });
+
         }
     });
 }


Test patch:
diff --git a/src/wallet/gtest/test_rpc_wallet.cpp b/src/wallet/gtest/test_rpc_wallet.cpp
index fc38e99d0c4..fbcd9647a9a 100644
--- a/src/wallet/gtest/test_rpc_wallet.cpp
+++ b/src/wallet/gtest/test_rpc_wallet.cpp
@@ -8,6 +8,7 @@
 #include "transaction_builder.h"
 #include "util/test.h"
 #include "gtest/utils.h"
+#include "wallet/asyncrpcoperation_common.h"
 #include "wallet/asyncrpcoperation_shieldcoinbase.h"
 #include "wallet/asyncrpcoperation_sendmany.h"
 #include "wallet/memo.h"
@@ -16,8 +17,8 @@
 #include <librustzcash.h>
 #include <rust/ed25519.h>
 
-namespace {
-
+namespace
+{
 bool find_error(const UniValue& objError, const std::string& expected) {
     return find_value(objError, "message").get_str().find(expected) != string::npos;
 }
@@ -28,7 +29,108 @@ CWalletTx FakeWalletTx() {
     mtx.vout[0].nValue = 1;
     return CWalletTx(nullptr, mtx);
 }
+}
+
+TEST(WalletRPCTests, PrepareTransaction)
+{
+    LoadProofParameters();
+    SelectParams(CBaseChainParams::TESTNET);
+
+    LoadGlobalWallet();
+
+    RegtestActivateSapling();
+    {
+        LOCK2(cs_main, pwalletMain->cs_wallet);
+
+        if (!pwalletMain->HaveMnemonicSeed()) {
+            pwalletMain->GenerateNewSeed();
+        }
+
+        UniValue retValue;
+
+        KeyIO keyIO(Params());
+        // add keys manually
+        auto taddr = pwalletMain->GenerateNewKey(true).GetID();
+        auto pa = pwalletMain->GenerateNewLegacySaplingZKey();
+
+        const Consensus::Params& consensusParams = Params().GetConsensus();
+
+        int nextBlockHeight = chainActive.Height() + 1;
+
+        // Add a fake transaction to the wallet
+        CMutableTransaction mtx = CreateNewContextualCMutableTransaction(consensusParams, nextBlockHeight, false);
+        CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG;
+        mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey));
+
+        CWalletTx wtx(pwalletMain, mtx);
+        pwalletMain->LoadWalletTx(wtx);
+
+        // Fake-mine the transaction
+        EXPECT_EQ(-1, chainActive.Height());
+        CBlock block;
+        block.vtx.push_back(wtx);
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+        auto blockHash = block.GetHash();
+        CBlockIndex fakeIndex {block};
+        mapBlockIndex.insert(std::make_pair(blockHash, &fakeIndex));
+        chainActive.SetTip(&fakeIndex);
+        EXPECT_TRUE(chainActive.Contains(&fakeIndex));
+        EXPECT_EQ(0, chainActive.Height());
+        wtx.SetMerkleBranch(block);
+        pwalletMain->LoadWalletTx(wtx);
+
+        WalletTxBuilder builder(Params(), minRelayTxFee);
+
+        auto selector = CWallet::LegacyTransparentZTXOSelector(
+                true,
+                TransparentCoinbasePolicy::Disallow);
+
+        { // send from legacy account with change, but insufficient policy
+            auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
+            Payment saplingPayment(saplingKey, 4 * COIN, std::nullopt);
+            std::vector<Payment> payments {saplingPayment};
+
+            TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders);
+
+            SpendableInputs inputs;
+            inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
+
+            builder.PrepareTransaction(
+                    *pwalletMain,
+                    selector,
+                    inputs,
+                    payments,
+                    chainActive,
+                    strategy,
+                    std::nullopt,
+                    1)
+                .map_error([&](const auto& err) {
+                    examine(err, match {
+                        [](AddressResolutionError are) {
+                            EXPECT_EQ(are, AddressResolutionError::TransparentChangeNotAllowed);
+                        },
+                        [&](const auto& e) {
+                            try {
+                                ThrowInputSelectionError(e, selector, strategy);
+                            } catch (const UniValue& value) {
+                                FAIL() << value.write();
+                            }
+                        },
+                    });
+                })
+                .map([](const auto&) {
+                    FAIL() << "Expected an error";
+                });
+        }
+
+        // Tear down
+        chainActive.SetTip(NULL);
+        mapBlockIndex.erase(blockHash);
 
+    }
+    // Revert to default
+    RegtestDeactivateSapling();
+    UnloadGlobalWallet();
 }
 
 // TODO: test private methods