Correct change handling for ZIP 317 fees

qa/rpc-tests/mempool_tx_expiry.py

@@ -94,7 +94,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework):
 
         # Create transactions
         blockheight = self.nodes[0].getblockchaininfo()['blocks']
-        zsendamount = Decimal('1.0') - DEFAULT_FEE
+        zsendamount = Decimal('1.0') - compute_conventional_fee(2)
         recipients = []
         recipients.append({"address": z_bob, "amount": zsendamount})
         myopid = self.nodes[0].z_sendmany(z_alice, recipients, 1)
@@ -220,7 +220,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework):
         print("Ensure balance of node 0 is correct")
         bal = self.nodes[0].z_gettotalbalance()
         print("Balance after expire_shielded has expired: ", bal)
-        assert_equal(Decimal(bal["private"]), Decimal('8.0') - compute_conventional_fee(2))
+        assert_equal(Decimal(bal["private"]), Decimal('8.0') - DEFAULT_FEE)
 
         print("Splitting network...")
         self.split_network()

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -84,7 +84,11 @@ void AsyncRPCOperation_sendmany::main() {
 
     std::optional<uint256> txid;
     try {
-        txid = main_impl(*pwalletMain);
+        txid = main_impl(*pwalletMain)
+            .map_error([&](const InputSelectionError& err) {
+                ThrowInputSelectionError(err, ztxoSelector_, strategy_);
+            })
+            .value();
     } catch (const UniValue& objError) {
         int code = find_value(objError, "code").get_int();
         std::string message = find_value(objError, "message").get_str();
@@ -137,7 +141,8 @@ 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(CWallet& wallet) {
+tl::expected<uint256, InputSelectionError>
+AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
     auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, mindepth_);
 
     auto preparedTx = builder_.PrepareTransaction(
@@ -150,12 +155,8 @@ uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
             fee_,
             anchordepth_);
 
-    uint256 txid;
+    return preparedTx
-    examine(preparedTx, match {
+        .map([&](const TransactionEffects& effects) {
-        [&](const InputSelectionError& err) {
-            ThrowInputSelectionError(err, ztxoSelector_, strategy_);
-        },
-        [&](const TransactionEffects& effects) {
             try {
                 const auto& spendable = effects.GetSpendable();
                 const auto& payments = effects.GetPayments();
@@ -187,16 +188,13 @@ uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
                 UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode);
                 set_result(sendResult);
 
-                txid = tx.GetHash();
                 effects.UnlockSpendable(wallet);
+                return tx.GetHash();
             } catch (...) {
                 effects.UnlockSpendable(wallet);
                 throw;
             }
-        }
+        });
-    });
-
-    return txid;
 }
 
 /**

src/wallet/asyncrpcoperation_sendmany.h

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

src/wallet/wallet_tx_builder.cpp

@@ -13,23 +13,125 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations)
     return nextBlockHeight - anchorConfirmations;
 }
 
-/// Returns `std::nullopt` in the case of Sprout, since that isn’t a member of `OutputPool`.
+ChangeAddress
-static std::optional<OutputPool> ChangeAddressPool(const ZTXOSelector& selector) {
+WalletTxBuilder::GetChangeAddress(
-    return std::visit(match {
+        CWallet& wallet,
-        [](const CKeyID&) -> std::optional<OutputPool> { return OutputPool::Transparent; },
+        const ZTXOSelector& selector,
-        [](const CScriptID&) -> std::optional<OutputPool> { return OutputPool::Transparent; },
+        SpendableInputs& spendable,
-        [](const libzcash::SproutPaymentAddress&) -> std::optional<OutputPool> { return std::nullopt; },
+        const Payments& resolvedPayments,
-        [](const libzcash::SproutViewingKey&) -> std::optional<OutputPool> { return std::nullopt; },
+        const TransactionStrategy& strategy,
-        [](const libzcash::SaplingPaymentAddress&) -> std::optional<OutputPool> { return OutputPool::Sapling; },
+        bool afterNU5) const
-        [](const libzcash::SaplingExtendedFullViewingKey&) -> std::optional<OutputPool> { return OutputPool::Sapling; },
+{
-        // TODO: These need some real logic
+    // Determine the account we're sending from.
-        [](const libzcash::UnifiedAddress& ua) -> std::optional<OutputPool> { return OutputPool::Orchard; },
+    auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT);
-        [](const libzcash::UnifiedFullViewingKey& fvk) -> std::optional<OutputPool> { return OutputPool::Orchard; },
+
-        [](const AccountZTXOPattern& acct) -> std::optional<OutputPool> { return OutputPool::Orchard; }
+    auto getAllowedChangePools = [&](const std::set<ReceiverType>& receiverTypes) {
-    }, selector.GetPattern());
+        std::set<OutputPool> result{resolvedPayments.GetRecipientPools()};
+        // We always allow shielded change when not sending from the legacy account.
+        if (sendFromAccount != ZCASH_LEGACY_ACCOUNT) {
+            result.insert(OutputPool::Sapling);
+        }
+        for (ReceiverType rtype : receiverTypes) {
+            switch (rtype) {
+                case ReceiverType::P2PKH:
+                case ReceiverType::P2SH:
+                    // TODO: This is the correct policy, but it’s a breaking change from
+                    //       previous behavior, so enable it separately. (#6409)
+                    // if (strategy.AllowRevealedRecipients()) {
+                    if (!spendable.utxos.empty() || strategy.AllowRevealedRecipients()) {
+                        result.insert(OutputPool::Transparent);
+                    }
+                    break;
+                case ReceiverType::Sapling:
+                    if (!spendable.saplingNoteEntries.empty() || strategy.AllowRevealedAmounts()) {
+                        result.insert(OutputPool::Sapling);
+                    }
+                    break;
+                case ReceiverType::Orchard:
+                    if (afterNU5
+                        && (!spendable.orchardNoteMetadata.empty() || strategy.AllowRevealedAmounts())) {
+                        result.insert(OutputPool::Orchard);
+                    }
+                    break;
+            }
+        }
+        return result;
+    };
+
+    auto changeAddressForTransparentSelector = [&](const std::set<ReceiverType>& receiverTypes) {
+        auto addr = wallet.GenerateChangeAddressForAccount(
+                sendFromAccount,
+                getAllowedChangePools(receiverTypes));
+        assert(addr.has_value());
+        return addr.value();
+    };
+
+    auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) -> RecipientAddress {
+        // for Sapling, if using a legacy address, return change to the
+        // originating address; otherwise return it to the Sapling internal
+        // address corresponding to the UFVK.
+        if (sendFromAccount == ZCASH_LEGACY_ACCOUNT) {
+            return addr;
+        } else {
+            auto addr = wallet.GenerateChangeAddressForAccount(
+                    sendFromAccount,
+                    getAllowedChangePools({ReceiverType::Sapling}));
+            assert(addr.has_value());
+            return addr.value();
+        }
+    };
+
+    auto changeAddressForZUFVK = [&](
+            const ZcashdUnifiedFullViewingKey& zufvk,
+            const std::set<ReceiverType>& receiverTypes) {
+        auto addr = zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes));
+        assert(addr.has_value());
+        return addr.value();
+    };
+
+    return examine(selector.GetPattern(), match {
+        [&](const CKeyID&) -> ChangeAddress {
+            return changeAddressForTransparentSelector({ReceiverType::P2PKH});
+        },
+        [&](const CScriptID&) -> ChangeAddress {
+            return changeAddressForTransparentSelector({ReceiverType::P2SH});
+        },
+        [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress {
+            // for Sprout, we return change to the originating address using the tx builder.
+            return addr;
+        },
+        [](const libzcash::SproutViewingKey& vk) -> ChangeAddress {
+            // for Sprout, we return change to the originating address using the tx builder.
+            return vk.address();
+        },
+        [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress {
+            return changeAddressForSaplingAddress(addr);
+        },
+        [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress {
+            return changeAddressForSaplingAddress(fvk.DefaultAddress());
+        },
+        [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress {
+            auto zufvk = wallet.GetUFVKForAddress(ua);
+            assert(zufvk.has_value());
+            return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes());
+        },
+        [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress {
+            return changeAddressForZUFVK(
+                    ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk),
+                    fvk.GetKnownReceiverTypes());
+        },
+        [&](const AccountZTXOPattern& acct) -> ChangeAddress {
+            auto addr = wallet.GenerateChangeAddressForAccount(
+                    acct.GetAccountId(),
+                    getAllowedChangePools(acct.GetReceiverTypes()));
+            assert(addr.has_value());
+            return addr.value();
+        }
+    });
 }
 
-PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
+tl::expected<TransactionEffects, InputSelectionError>
+WalletTxBuilder::PrepareTransaction(
         CWallet& wallet,
         const ZTXOSelector& selector,
         SpendableInputs& spendable,
@@ -42,140 +144,23 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
     assert(fee < MAX_MONEY);
 
     int anchorHeight = GetAnchorHeight(chain, anchorConfirmations);
-    auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, anchorHeight);
+    bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5);
-    if (std::holds_alternative<InputSelectionError>(selected)) {
+    auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, afterNU5);
-        return std::get<InputSelectionError>(selected);
+    return selected.map([&](const InputSelection& resolvedSelection) {
-    }
+        auto ovks = SelectOVKs(wallet, selector, spendable);
 
-    auto resolvedSelection = std::get<InputSelection>(selected);
+        auto effects = TransactionEffects(
-    // TODO: don’t reassign
+                anchorConfirmations,
-    spendable = resolvedSelection.GetInputs();
+                resolvedSelection.GetInputs(),
-    auto resolvedPayments = resolvedSelection.GetPayments();
+                resolvedSelection.GetPayments(),
-    // TODO: don’t reassign
+                resolvedSelection.GetChangeAddress(),
-    auto finalFee = resolvedSelection.GetFee();
+                resolvedSelection.GetFee(),
-
+                ovks.first,
-    // We do not set a change address if there is no change.
+                ovks.second,
-    std::optional<ChangeAddress> changeAddr;
+                anchorHeight);
-    auto changeAmount = spendable.Total() - resolvedPayments.Total() - finalFee;
+        effects.LockSpendable(wallet);
-    if (changeAmount > 0) {
+        return effects;
-        // Determine the account we're sending from.
+    });
-        auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT);
-
-        auto getAllowedChangePools = [&](const std::set<ReceiverType>& receiverTypes) {
-            std::set<OutputPool> result{resolvedPayments.GetRecipientPools()};
-            // We always allow shielded change when not sending from the legacy account.
-            if (sendFromAccount != ZCASH_LEGACY_ACCOUNT) {
-                result.insert(OutputPool::Sapling);
-            }
-            for (ReceiverType rtype : receiverTypes) {
-                switch (rtype) {
-                    case ReceiverType::P2PKH:
-                    case ReceiverType::P2SH:
-                        if (strategy.AllowRevealedRecipients()) {
-                            result.insert(OutputPool::Transparent);
-                        }
-                        break;
-                    case ReceiverType::Sapling:
-                        if (!spendable.saplingNoteEntries.empty() || strategy.AllowRevealedAmounts()) {
-                            result.insert(OutputPool::Sapling);
-                        }
-                        break;
-                    case ReceiverType::Orchard:
-                        if (params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5)
-                            && (!spendable.orchardNoteMetadata.empty() || strategy.AllowRevealedAmounts())) {
-                            result.insert(OutputPool::Orchard);
-                        }
-                        break;
-                }
-            }
-            return result;
-        };
-
-        auto addChangePayment = [&](const std::optional<RecipientAddress>& sendTo) {
-            assert(sendTo.has_value());
-            resolvedPayments.AddPayment(
-                    ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-            return sendTo.value();
-        };
-
-        auto changeAddressForTransparentSelector = [&](const std::set<ReceiverType>& receiverTypes) {
-            return addChangePayment(
-                    wallet.GenerateChangeAddressForAccount(
-                            sendFromAccount,
-                            getAllowedChangePools(receiverTypes)));
-        };
-
-        auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) {
-            // for Sapling, if using a legacy address, return change to the
-            // originating address; otherwise return it to the Sapling internal
-            // address corresponding to the UFVK.
-            return addChangePayment(
-                    sendFromAccount == ZCASH_LEGACY_ACCOUNT
-                    ? addr
-                    : wallet.GenerateChangeAddressForAccount(
-                            sendFromAccount,
-                            getAllowedChangePools({ReceiverType::Sapling})));
-        };
-
-        auto changeAddressForZUFVK = [&](
-                const ZcashdUnifiedFullViewingKey& zufvk,
-                const std::set<ReceiverType>& receiverTypes) {
-            return addChangePayment(zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes)));
-        };
-
-        changeAddr = examine(selector.GetPattern(), match {
-            [&](const CKeyID&) -> ChangeAddress {
-                return changeAddressForTransparentSelector({ReceiverType::P2PKH});
-            },
-            [&](const CScriptID&) -> ChangeAddress {
-                return changeAddressForTransparentSelector({ReceiverType::P2SH});
-            },
-            [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress {
-                // for Sprout, we return change to the originating address using the tx builder.
-                return addr;
-            },
-            [](const libzcash::SproutViewingKey& vk) -> ChangeAddress {
-                // for Sprout, we return change to the originating address using the tx builder.
-                return vk.address();
-            },
-            [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress {
-                return changeAddressForSaplingAddress(addr);
-            },
-            [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress {
-                return changeAddressForSaplingAddress(fvk.DefaultAddress());
-            },
-            [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress {
-                auto zufvk = wallet.GetUFVKForAddress(ua);
-                assert(zufvk.has_value());
-                return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes());
-            },
-            [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress {
-                return changeAddressForZUFVK(
-                        ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk),
-                        fvk.GetKnownReceiverTypes());
-            },
-            [&](const AccountZTXOPattern& acct) -> ChangeAddress {
-                return addChangePayment(
-                        wallet.GenerateChangeAddressForAccount(
-                                acct.GetAccountId(),
-                                getAllowedChangePools(acct.GetReceiverTypes())));
-            }
-        });
-    }
-
-    auto ovks = SelectOVKs(wallet, selector, spendable);
-
-    auto effects = TransactionEffects(
-            anchorConfirmations,
-            spendable,
-            resolvedPayments,
-            changeAddr,
-            finalFee,
-            ovks.first,
-            ovks.second,
-            anchorHeight);
-    effects.LockSpendable(wallet);
-    return effects;
 }
 
 const SpendableInputs& InputSelection::GetInputs() const {
@@ -190,6 +175,10 @@ CAmount InputSelection::GetFee() const {
     return fee;
 }
 
+const std::optional<ChangeAddress> InputSelection::GetChangeAddress() const {
+    return changeAddr;
+}
+
 CAmount WalletTxBuilder::DefaultDustThreshold() const {
     CKey secret{CKey::TestOnlyRandomKey(true)};
     CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID());
@@ -220,7 +209,7 @@ static CAmount
 CalcZIP317Fee(
         const std::optional<SpendableInputs>& inputs,
         const std::vector<ResolvedPayment>& payments,
-        const std::optional<std::optional<OutputPool>>& changeAddr)
+        const std::optional<ChangeAddress>& changeAddr)
 {
     std::vector<CTxOut> vouts{};
     size_t sproutOutputCount = 0;
@@ -243,22 +232,21 @@ CalcZIP317Fee(
     }
 
     if (changeAddr.has_value()) {
-        auto changePool = changeAddr.value();
+        examine(changeAddr.value(), match {
-        if (changePool.has_value()) {
+            [&](const SproutPaymentAddress&) { ++sproutOutputCount; },
-            switch (changePool.value()) {
+            [&](const RecipientAddress& addr) {
-                case OutputPool::Transparent:
+                examine(addr, match {
-                    // TODO: need to either get an actual change address or make a fake vout here
+                    [&](const CKeyID& taddr) {
-                    break;
+                        vouts.emplace_back(0, GetScriptForDestination(taddr));
-                case OutputPool::Sapling:
+                    },
-                    ++saplingOutputCount;
+                    [&](const CScriptID taddr) {
-                    break;
+                        vouts.emplace_back(0, GetScriptForDestination(taddr));
-                case OutputPool::Orchard:
+                    },
-                    ++orchardOutputCount;
+                    [&](const libzcash::SaplingPaymentAddress&) { ++saplingOutputCount; },
-                    break;
+                    [&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; }
+                });
             }
-        } else {
+        });
-            ++sproutOutputCount;
-        }
     }
 
     size_t logicalActionCount;
@@ -313,6 +301,23 @@ InvalidFundsError ReportInvalidFunds(
                : InvalidFundsReason(InsufficientFundsError(targetAmount))));
 }
 
+static void
+AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CAmount changeAmount)
+{
+    assert(changeAmount > 0);
+
+    examine(changeAddr, match {
+        // TODO: Once we can add Sprout change to `resolvedPayments`, we don’t need to pass
+        //       `changeAddr` around the rest of these functions.
+        [](const libzcash::SproutPaymentAddress&) {},
+        [](const libzcash::SproutViewingKey&) {},
+        [&](const auto& sendTo) {
+            resolvedPayments.AddPayment(
+                    ResolvedPayment(std::nullopt, sendTo, changeAmount, std::nullopt, true));
+        }
+    });
+}
+
 /// On the initial call, we haven’t yet selected inputs, so we assume the outputs dominate the
 /// actions.
 ///
@@ -321,13 +326,18 @@ InvalidFundsError ReportInvalidFunds(
 ///      with change _if_ there is change
 /// 2. iterate over LimitToAmount until the updated fee (now including spends) matches the expected
 ///    fee
-tl::expected<std::pair<SpendableInputs, CAmount>, InputSelectionError>
+tl::expected<
-IterateLimit(
+    std::tuple<SpendableInputs, CAmount, std::optional<ChangeAddress>>,
+    InputSelectionError>
+WalletTxBuilder::IterateLimit(
+        CWallet& wallet,
+        const ZTXOSelector& selector,
+        const TransactionStrategy strategy,
         CAmount sendAmount,
         CAmount dustThreshold,
         const SpendableInputs& spendable,
-        const Payments& resolved,
+        Payments& resolved,
-        const std::optional<std::optional<OutputPool>>& changePool)
+        bool afterNU5) const
 {
     SpendableInputs spendableMut;
 
@@ -336,6 +346,8 @@ IterateLimit(
     // This is used to increase the target amount just enough (generally by 0 or 1) to force
     // selection of additional notes.
     CAmount bumpTargetAmount{0};
+    std::optional<ChangeAddress> changeAddr;
+    CAmount changeAmount{0};
 
     do {
         spendableMut = spendable;
@@ -350,15 +362,25 @@ IterateLimit(
                     targetAmount + bumpTargetAmount,
                     dustThreshold,
                     resolved.GetRecipientPools());
-        CAmount changeAmount{spendableMut.Total() - targetAmount};
+        changeAmount = spendableMut.Total() - targetAmount;
         if (foundSufficientFunds) {
+            // Don’t want to generate a change address if we don’t need one (because it could be
+            // fresh) and once we generate it, hold onto it. But we still don’t have a guarantee
+            // that we won’t end up discarding it.
+            if (changeAmount > 0 && !changeAddr.has_value()) {
+                changeAddr = GetChangeAddress(
+                        wallet,
+                        selector,
+                        spendableMut,
+                        resolved,
+                        strategy,
+                        afterNU5);
+            }
             previousFee = updatedFee;
             updatedFee = CalcZIP317Fee(
                     spendableMut,
                     resolved.GetResolvedPayments(),
-                    changeAmount > 0
+                    changeAmount > 0 ? changeAddr : std::nullopt);
-                    ? std::optional<std::optional<OutputPool>>(changePool)
-                    : std::nullopt);
         } else {
             return tl::make_unexpected(
                     ReportInvalidFunds(
@@ -379,18 +401,24 @@ IterateLimit(
         }
     } while (updatedFee != previousFee);
 
-    return std::make_pair(spendableMut, updatedFee);
+    if (changeAmount > 0) {
+        assert(changeAddr.has_value());
+        AddChangePayment(resolved, changeAddr.value(), changeAmount);
+    }
+
+    return std::make_tuple(spendableMut, updatedFee, changeAddr);
 }
 
-InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
+tl::expected<InputSelection, InputSelectionError>
-        const CWallet& wallet,
+WalletTxBuilder::ResolveInputsAndPayments(
+        CWallet& wallet,
         const ZTXOSelector& selector,
         SpendableInputs& spendableMut,
         const std::vector<Payment>& payments,
         const CChain& chain,
-        TransactionStrategy strategy,
+        const TransactionStrategy& strategy,
         std::optional<CAmount> fee,
-        int anchorHeight) const
+        bool afterNU5) const
 {
     LOCK2(cs_main, wallet.cs_wallet);
 
@@ -405,9 +433,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
 
     // we can only select Orchard addresses if there are sufficient non-Sprout
     // funds to cover the total payments + fee.
-    bool canResolveOrchard =
+    bool canResolveOrchard = afterNU5 && !selector.SelectsSprout();
-        params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5)
-        && !selector.SelectsSprout();
     std::vector<ResolvedPayment> resolvedPayments;
     std::optional<AddressResolutionError> resolutionError;
     for (const auto& payment : payments) {
@@ -487,16 +513,17 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
         });
 
         if (resolutionError.has_value()) {
-            return resolutionError.value();
+            return tl::make_unexpected(resolutionError.value());
         }
     }
     auto resolved = Payments(resolvedPayments);
 
     if (orchardOutputs > this->maxOrchardActions) {
-        return ExcessOrchardActionsError(
+        return tl::make_unexpected(
-                ActionSide::Output,
+                ExcessOrchardActionsError(
-                orchardOutputs,
+                        ActionSide::Output,
-                this->maxOrchardActions);
+                        orchardOutputs,
+                        this->maxOrchardActions));
     }
 
     // Set the dust threshold so that we can select enough inputs to avoid
@@ -511,6 +538,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
 
     CAmount finalFee;
     CAmount targetAmount;
+    std::optional<ChangeAddress> changeAddr;
     if (fee.has_value()) {
         finalFee = fee.value();
         targetAmount = sendAmount + finalFee;
@@ -523,22 +551,32 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
                 resolved.GetRecipientPools());
         CAmount changeAmount{spendableMut.Total() - targetAmount};
         if (!foundSufficientFunds) {
-            return ReportInvalidFunds(
+            return tl::make_unexpected(
+                    ReportInvalidFunds(
+                            spendableMut,
+                            false,
+                            finalFee,
+                            dustThreshold,
+                            targetAmount,
+                            changeAmount));
+        }
+        if (changeAmount > 0) {
+            changeAddr = GetChangeAddress(
+                    wallet,
+                    selector,
                     spendableMut,
-                    false,
+                    resolved,
-                    finalFee,
+                    strategy,
-                    dustThreshold,
+                    afterNU5);
-                    targetAmount,
+            AddChangePayment(resolved, changeAddr.value(), changeAmount);
-                    changeAmount);
         }
     } else {
-        auto limit_result = IterateLimit(sendAmount, dustThreshold, spendableMut, resolved, ChangeAddressPool(selector));
+        auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5);
-        if (limit_result.has_value()) {
+        if (limitResult.has_value()) {
-            spendableMut = limit_result.value().first;
+            std::tie(spendableMut, finalFee, changeAddr) = limitResult.value();
-            finalFee = limit_result.value().second;
             targetAmount = sendAmount - finalFee;
         } else {
-            return limit_result.error();
+            return tl::make_unexpected(limitResult.error());
         }
     }
 
@@ -546,20 +584,21 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments(
     // consumed, and they may only be sent to shielded recipients.
     if (spendableMut.HasTransparentCoinbase()) {
         if (spendableMut.Total() != targetAmount) {
-            return ChangeNotAllowedError(spendableMut.Total(), targetAmount);
+            return tl::make_unexpected(ChangeNotAllowedError(spendableMut.Total(), targetAmount));
         } else if (resolved.HasTransparentRecipient()) {
-            return AddressResolutionError::TransparentRecipientNotAllowed;
+            return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed);
         }
     }
 
     if (spendableMut.orchardNoteMetadata.size() > this->maxOrchardActions) {
-        return ExcessOrchardActionsError(
+        return tl::make_unexpected(
-                ActionSide::Input,
+                ExcessOrchardActionsError(
-                spendableMut.orchardNoteMetadata.size(),
+                        ActionSide::Input,
-                this->maxOrchardActions);
+                        spendableMut.orchardNoteMetadata.size(),
+                        this->maxOrchardActions));
     }
 
-    return InputSelection(spendableMut, resolved, finalFee, anchorHeight);
+    return InputSelection(spendableMut, resolved, finalFee, changeAddr);
 }
 
 std::pair<uint256, uint256>

src/wallet/wallet_tx_builder.h

@@ -315,25 +315,18 @@ private:
     SpendableInputs inputs;
     Payments payments;
     CAmount fee;
-    int orchardAnchorHeight;
+    std::optional<ChangeAddress> changeAddr;
 
 public:
-    InputSelection(SpendableInputs inputs, Payments payments, CAmount fee, int orchardAnchorHeight):
+    InputSelection(SpendableInputs inputs, Payments payments, CAmount fee, std::optional<ChangeAddress> changeAddr):
-        inputs(inputs), payments(payments), fee(fee), orchardAnchorHeight(orchardAnchorHeight) {}
+        inputs(inputs), payments(payments), fee(fee), changeAddr(changeAddr) {}
 
     const SpendableInputs& GetInputs() const;
     const Payments& GetPayments() const;
     CAmount GetFee() const;
+    const std::optional<ChangeAddress> GetChangeAddress() const;
 };
 
-typedef std::variant<
-    InputSelectionError,
-    InputSelection> InputSelectionResult;
-
-typedef std::variant<
-    InputSelectionError,
-    TransactionEffects> PrepareTransactionResult;
-
 class WalletTxBuilder {
 private:
     const CChainParams& params;
@@ -345,20 +338,43 @@ private:
      */
     CAmount DefaultDustThreshold() const;
 
+    ChangeAddress
+    GetChangeAddress(
+            CWallet& wallet,
+            const ZTXOSelector& selector,
+            SpendableInputs& spendable,
+            const Payments& resolvedPayments,
+            const TransactionStrategy& strategy,
+            bool afterNU5) const;
+
+    tl::expected<
+        std::tuple<SpendableInputs, CAmount, std::optional<ChangeAddress>>,
+        InputSelectionError>
+    IterateLimit(
+            CWallet& wallet,
+            const ZTXOSelector& selector,
+            const TransactionStrategy strategy,
+            CAmount sendAmount,
+            CAmount dustThreshold,
+            const SpendableInputs& spendable,
+            Payments& resolved,
+            bool afterNU5) const;
+
     /**
      * Select inputs sufficient to fulfill the specified requested payments,
      * and choose unified address receivers based upon the available inputs
      * and the requested transaction strategy.
      */
-    InputSelectionResult ResolveInputsAndPayments(
+    tl::expected<InputSelection, InputSelectionError>
-            const CWallet& wallet,
+    ResolveInputsAndPayments(
+            CWallet& wallet,
             const ZTXOSelector& selector,
             SpendableInputs& spendable,
             const std::vector<Payment>& payments,
             const CChain& chain,
-            TransactionStrategy strategy,
+            const TransactionStrategy& strategy,
             std::optional<CAmount> fee,
-            int anchorHeight) const;
+            bool afterNU5) const;
     /**
      * Compute the internal and external OVKs to use in transaction construction, given
      * the spendable inputs.
@@ -377,7 +393,8 @@ public:
             const ZTXOSelector& selector,
             int32_t minDepth) const;
 
-    PrepareTransactionResult PrepareTransaction(
+    tl::expected<TransactionEffects, InputSelectionError>
+    PrepareTransaction(
             CWallet& wallet,
             const ZTXOSelector& selector,
             SpendableInputs& spendable,