WalletTxBuilder support for net payments

Previously, `WalletTxBuilder` expected a vector of payments with known amounts. This meant that operations like `z_shieldcoinbase` and `z_mergetoaddress` needed to pre-calculate the amount to be sent to the target address. With ZIP 317 fees, this became harder to do. Now `WalletTxBuilder` accepts either a vector of payments or a single address (and optional memo) for the net amount to be paid to. This also converts `z_shieldcoinbase` to use this approach. `z_mergetoaddress` conversion is blocked by #6569.
2023-04-18 11:48:03 -06:00 · 2023-04-18 11:48:03 -06:00 · f655f482ab
parent f512291ff2
commit f655f482ab
6 changed files with 194 additions and 109 deletions
--- a/qa/rpc-tests/wallet_shieldcoinbase.py
+++ b/qa/rpc-tests/wallet_shieldcoinbase.py
@ -9,6 +9,7 @@ from test_framework.util import assert_equal, initialize_chain_clean, \
    start_node, connect_nodes_bi, sync_blocks, sync_mempools, \
    wait_and_assert_operationid_status, get_coinbase_address, DEFAULT_FEE, \
    NU5_BRANCH_ID, nuparams
+from test_framework.zip317 import conventional_fee

 from decimal import Decimal

@ -117,7 +118,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework):
        # Confirm balances and that do_not_shield_taddr containing funds of 10 was left alone
        assert_equal(self.nodes[0].getbalance(), 10)
        assert_equal(self.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10.0'))
-        self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - DEFAULT_FEE)
+        self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - conventional_fee(4))
        assert_equal(self.nodes[1].getbalance(), 20)
        assert_equal(self.nodes[2].getbalance(), 30)

@ -129,7 +130,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework):
        self.sync_all()

        assert_equal(self.nodes[0].getbalance(), 10)
-        self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - DEFAULT_FEE)
+        self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - conventional_fee(4))
        assert_equal(self.nodes[1].getbalance(), 30)
        assert_equal(self.nodes[2].getbalance(), 0)

--- a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp
+++ b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp
@ -47,7 +47,7 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase(
        PaymentAddress toAddress,
        TransactionStrategy strategy,
        int nUTXOLimit,
-        CAmount fee,
+        std::optional<CAmount> fee,
        UniValue contextInfo) :
    builder_(std::move(builder)),
    ztxoSelector_(ztxoSelector),
@ -57,7 +57,7 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase(
    fee_(fee),
    contextinfo_(contextInfo)
 {
-    assert(MoneyRange(fee));
+    assert(!fee.has_value() || MoneyRange(fee.value()));
    assert(ztxoSelector.RequireSpendingKeys());

    examine(toAddress_, match {
@ -177,23 +177,11 @@ Remaining AsyncRPCOperation_shieldcoinbase::prepare(CWallet& wallet) {

    spendable.LimitTransparentUtxos(numUtxos);

-    // TODO: Don’t check this outside of `PrepareTransaction`
-    if (shieldingValue < fee_) {
-        ThrowInputSelectionError(
-                InvalidFundsError(shieldingValue, InsufficientFundsError(fee_)),
-                ztxoSelector_,
-                strategy_);
-    }
-
-    // FIXME: This should be internal, and shouldn’t be a payment, but more like a change address,
-    //        as we don’t know the amount at this point.
-    std::vector<Payment> payments = { Payment(toAddress_, shieldingValue - fee_, std::nullopt) };
-
    auto preparationResult = builder_.PrepareTransaction(
            wallet,
            ztxoSelector_,
            spendable,
-            payments,
+            std::make_pair(toAddress_, std::nullopt),
            chainActive,
            strategy_,
            fee_,
--- a/src/wallet/asyncrpcoperation_shieldcoinbase.h
+++ b/src/wallet/asyncrpcoperation_shieldcoinbase.h
@ -62,7 +62,7 @@ public:
        PaymentAddress toAddress,
        TransactionStrategy strategy,
        int nUTXOLimit,
-        CAmount fee = DEFAULT_FEE,
+        std::optional<CAmount> fee,
        UniValue contextInfo = NullUniValue);
    virtual ~AsyncRPCOperation_shieldcoinbase();

@ -88,7 +88,7 @@ private:
    PaymentAddress toAddress_;
    TransactionStrategy strategy_;
    int nUTXOLimit_;
-    CAmount fee_;
+    std::optional<CAmount> fee_;
    std::optional<TransactionEffects> effects_;

    UniValue contextinfo_;     // optional data to include in return value from getStatus()
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@ -5188,8 +5188,8 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
            "\nArguments:\n"
            "1. \"fromaddress\"         (string, required) The address is a taddr or \"*\" for all taddrs belonging to the wallet.\n"
            "2. \"toaddress\"           (string, required) The address is a zaddr.\n"
-            "3. fee                   (numeric, optional, default="
-            + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n"
+            "3. fee                   (numeric, optional, default=null) The fee amount in " + CURRENCY_UNIT + " to attach to this transaction. The default behavior\n"
+            "                         is to use a fee calculated according to ZIP 317.\n"
            "4. limit                 (numeric, optional, default="
            + strprintf("%d", SHIELD_COINBASE_DEFAULT_LIMIT) + ") Limit on the maximum number of utxos to shield.  Set to 0 to use as many as will fit in the transaction.\n"
            "5. privacyPolicy         (string, optional, default=\"AllowRevealedSenders\") Policy for what information leakage is acceptable.\n"
@ -5290,13 +5290,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
        throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destStr);
    }

-    CAmount nFee = DEFAULT_FEE;
-    if (params.size() > 2) {
-        if (params[2].get_real() == 0.0) {
-            nFee = 0;
-        } else {
-            nFee = AmountFromValue( params[2] );
-        }
+    std::optional<CAmount> nFee;
+    if (params.size() > 2 && !params[2].isNull()) {
+        nFee = AmountFromValue( params[2] );
    }

    int nUTXOLimit = params.size() > 3 ? params[3].get_int() : SHIELD_COINBASE_DEFAULT_LIMIT;
@ -5308,7 +5304,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
    UniValue contextInfo(UniValue::VOBJ);
    contextInfo.pushKV("fromaddress", params[0]);
    contextInfo.pushKV("toaddress", params[1]);
-    contextInfo.pushKV("fee", ValueFromAmount(nFee));
+    if (nFee.has_value()) {
+        contextInfo.pushKV("fee", ValueFromAmount(nFee.value()));
+    }

    // Create the wallet builder
    WalletTxBuilder builder(chainparams, minRelayTxFee);
--- a/src/wallet/wallet_tx_builder.cpp
+++ b/src/wallet/wallet_tx_builder.cpp
@ -14,6 +14,66 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations)
    return nextBlockHeight - anchorConfirmations;
 }

+static tl::expected<void, InputSelectionError>
+ValidateAmount(const SpendableInputs& spendable, const CAmount& fee)
+{
+    // TODO: The actual requirement should probably be higher than simply `fee` – do we need to
+    //       take into account the dustThreshold when adding an output? But, this was the
+    //       pre-WalletTxBuilder behavior, so it’s fine to maintain it for now.
+    auto targetAmount = fee;
+    if (spendable.Total() < targetAmount)
+        return tl::make_unexpected(ReportInvalidFunds(spendable, false, fee, 0, targetAmount, 0));
+    else
+        return {};
+}
+
+static tl::expected<std::pair<ResolvedPayment, CAmount>, InputSelectionError>
+ResolveNetPayment(
+        const ZTXOSelector& selector,
+        const SpendableInputs& spendable,
+        const NetAmountRecipient& netpay,
+        const std::optional<CAmount>& fee,
+        const TransactionStrategy& strategy,
+        bool afterNU5)
+{
+    bool canResolveOrchard = afterNU5 && !selector.SelectsSprout();
+    CAmount maxSaplingAvailable = spendable.GetSaplingTotal();
+    CAmount maxOrchardAvailable = spendable.GetOrchardTotal();
+    uint32_t orchardOutputs{0};
+
+    // We initially resolve the payment with `MINIMUM_FEE` so that we can use the payment to
+    // calculate the actual fee.
+    auto initialFee = fee.value_or(MINIMUM_FEE);
+    return ValidateAmount(spendable, initialFee)
+        .and_then([&](void) {
+            return ResolvePayment(
+                    Payment(netpay.first, spendable.Total() - initialFee, netpay.second),
+                    canResolveOrchard,
+                    strategy,
+                    maxSaplingAvailable,
+                    maxOrchardAvailable,
+                    orchardOutputs)
+                .map_error([](const auto& error) -> InputSelectionError { return error; })
+                .and_then([&](const auto& rpayment) {
+                    auto finalFee = fee.value_or(CalcZIP317Fee(spendable, {rpayment}, std::nullopt));
+                    return ValidateAmount(spendable, finalFee)
+                        .and_then([&](void) {
+                            return ResolvePayment(
+                                    Payment(netpay.first, spendable.Total() - finalFee, netpay.second),
+                                    canResolveOrchard,
+                                    strategy,
+                                    maxSaplingAvailable,
+                                    maxOrchardAvailable,
+                                    orchardOutputs)
+                                .map([&](const auto& actualPayment) {
+                                    return std::make_pair(actualPayment, finalFee);
+                                })
+                                .map_error([](const auto& error) -> InputSelectionError { return error; });
+                        });
+                });
+        });
+}
+
 tl::expected<ChangeAddress, AddressResolutionError>
 WalletTxBuilder::GetChangeAddress(
        CWallet& wallet,
@ -144,7 +204,7 @@ WalletTxBuilder::PrepareTransaction(
        CWallet& wallet,
        const ZTXOSelector& selector,
        SpendableInputs& spendable,
-        const std::vector<Payment>& payments,
+        const Recipients& payments,
        const CChain& chain,
        TransactionStrategy strategy,
        std::optional<CAmount> fee,
@ -156,7 +216,26 @@ WalletTxBuilder::PrepareTransaction(

    int anchorHeight = GetAnchorHeight(chain, anchorConfirmations);
    bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5);
-    auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, afterNU5);
+    auto selected = examine(payments, match {
+            [&](const std::vector<Payment>& payments) {
+                return ResolveInputsAndPayments(
+                        wallet,
+                        selector,
+                        spendable,
+                        payments,
+                        chain,
+                        strategy,
+                        fee,
+                        afterNU5);
+            },
+            [&](const NetAmountRecipient& netRecipient) {
+                return ResolveNetPayment(selector, spendable, netRecipient, fee, strategy, afterNU5)
+                    .map([&](const auto& pair) {
+                        const auto& [payment, finalFee] = pair;
+                        return InputSelection(spendable, {{payment}}, finalFee, std::nullopt);
+                    });
+            },
+        });
    return selected.map([&](const InputSelection& resolvedSelection) {
        auto ovks = SelectOVKs(wallet, selector, spendable);

@ -446,6 +525,92 @@ WalletTxBuilder::IterateLimit(
    return std::make_tuple(spendableMut, updatedFee, changeAddr);
 }

+static tl::expected<ResolvedPayment, AddressResolutionError>
+ResolvePayment(
+        const Payment& payment,
+        bool canResolveOrchard,
+        const TransactionStrategy& strategy,
+        CAmount& maxSaplingAvailable,
+        CAmount& maxOrchardAvailable,
+        uint32_t& orchardOutputs)
+{
+    return examine(payment.GetAddress(), match {
+        [&](const CKeyID& p2pkh) -> tl::expected<ResolvedPayment, AddressResolutionError> {
+            if (strategy.AllowRevealedRecipients()) {
+                return {{std::nullopt, p2pkh, payment.GetAmount(), payment.GetMemo(), false}};
+            } else {
+                return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed);
+            }
+        },
+        [&](const CScriptID& p2sh) -> tl::expected<ResolvedPayment, AddressResolutionError> {
+            if (strategy.AllowRevealedRecipients()) {
+                return {{std::nullopt, p2sh, payment.GetAmount(), payment.GetMemo(), false}};
+            } else {
+                return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed);
+            }
+        },
+        [&](const SproutPaymentAddress&) -> tl::expected<ResolvedPayment, AddressResolutionError> {
+            return tl::make_unexpected(AddressResolutionError::SproutRecipientsNotSupported);
+        },
+        [&](const SaplingPaymentAddress& addr)
+            -> tl::expected<ResolvedPayment, AddressResolutionError> {
+            if (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) {
+                if (!strategy.AllowRevealedAmounts()) {
+                    maxSaplingAvailable -= payment.GetAmount();
+                }
+                return {{std::nullopt, addr, payment.GetAmount(), payment.GetMemo(), false}};
+            } else {
+                return tl::make_unexpected(AddressResolutionError::RevealingSaplingAmountNotAllowed);
+            }
+        },
+        [&](const UnifiedAddress& ua) -> tl::expected<ResolvedPayment, AddressResolutionError> {
+            if (canResolveOrchard
+                && ua.GetOrchardReceiver().has_value()
+                && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxOrchardAvailable)
+                ) {
+                if (!strategy.AllowRevealedAmounts()) {
+                    maxOrchardAvailable -= payment.GetAmount();
+                }
+                orchardOutputs += 1;
+                return {{
+                    ua,
+                    ua.GetOrchardReceiver().value(),
+                    payment.GetAmount(),
+                    payment.GetMemo(),
+                    false
+                }};
+            } else if (ua.GetSaplingReceiver().has_value()
+                && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable)
+                ) {
+                if (!strategy.AllowRevealedAmounts()) {
+                    maxSaplingAvailable -= payment.GetAmount();
+                }
+                return {{ua, ua.GetSaplingReceiver().value(), payment.GetAmount(), payment.GetMemo(), false}};
+            } else {
+                if (strategy.AllowRevealedRecipients()) {
+                    if (ua.GetP2SHReceiver().has_value()) {
+                        return {{
+                            ua, ua.GetP2SHReceiver().value(), payment.GetAmount(), std::nullopt, false}};
+                    } else if (ua.GetP2PKHReceiver().has_value()) {
+                        return {{
+                            ua, ua.GetP2PKHReceiver().value(), payment.GetAmount(), std::nullopt, false}};
+                    } else {
+                        // This should only occur when we have
+                        // • an Orchard-only UA,
+                        // • `AllowRevealedRecipients`, and
+                        // • can’t resolve Orchard (which means either a Sprout selector or pre-NU5).
+                        return tl::make_unexpected(AddressResolutionError::CouldNotResolveReceiver);
+                    }
+                } else if (strategy.AllowRevealedAmounts()) {
+                    return tl::make_unexpected(AddressResolutionError::TransparentReceiverNotAllowed);
+                } else {
+                    return tl::make_unexpected(AddressResolutionError::RevealingReceiverAmountsNotAllowed);
+                }
+            }
+        }
+    });
+}
+
 tl::expected<InputSelection, InputSelectionError>
 WalletTxBuilder::ResolveInputsAndPayments(
        CWallet& wallet,
@ -474,83 +639,10 @@ WalletTxBuilder::ResolveInputsAndPayments(
    std::vector<ResolvedPayment> resolvedPayments;
    std::optional<AddressResolutionError> resolutionError;
    for (const auto& payment : payments) {
-        examine(payment.GetAddress(), match {
-            [&](const CKeyID& p2pkh) {
-                if (strategy.AllowRevealedRecipients()) {
-                    resolvedPayments.emplace_back(
-                            std::nullopt, p2pkh, payment.GetAmount(), payment.GetMemo(), false);
-                } else {
-                    resolutionError = AddressResolutionError::TransparentRecipientNotAllowed;
-                }
-            },
-            [&](const CScriptID& p2sh) {
-                if (strategy.AllowRevealedRecipients()) {
-                    resolvedPayments.emplace_back(
-                            std::nullopt, p2sh, payment.GetAmount(), payment.GetMemo(), false);
-                } else {
-                    resolutionError = AddressResolutionError::TransparentRecipientNotAllowed;
-                }
-            },
-            [&](const SproutPaymentAddress&) {
-                resolutionError = AddressResolutionError::SproutRecipientsNotSupported;
-            },
-            [&](const SaplingPaymentAddress& addr) {
-                if (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) {
-                    resolvedPayments.emplace_back(
-                            std::nullopt, addr, payment.GetAmount(), payment.GetMemo(), false);
-                    if (!strategy.AllowRevealedAmounts()) {
-                        maxSaplingAvailable -= payment.GetAmount();
-                    }
-                } else {
-                    resolutionError = AddressResolutionError::RevealingSaplingAmountNotAllowed;
-                }
-            },
-            [&](const UnifiedAddress& ua) {
-                if (canResolveOrchard
-                    && ua.GetOrchardReceiver().has_value()
-                    && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxOrchardAvailable)
-                    ) {
-                    resolvedPayments.emplace_back(
-                        ua, ua.GetOrchardReceiver().value(), payment.GetAmount(), payment.GetMemo(), false);
-                    if (!strategy.AllowRevealedAmounts()) {
-                        maxOrchardAvailable -= payment.GetAmount();
-                    }
-                    orchardOutputs += 1;
-                } else if (ua.GetSaplingReceiver().has_value()
-                    && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable)
-                    ) {
-                    resolvedPayments.emplace_back(
-                        ua, ua.GetSaplingReceiver().value(), payment.GetAmount(), payment.GetMemo(), false);
-                    if (!strategy.AllowRevealedAmounts()) {
-                        maxSaplingAvailable -= payment.GetAmount();
-                    }
-                } else {
-                    if (strategy.AllowRevealedRecipients()) {
-                        if (ua.GetP2SHReceiver().has_value()) {
-                            resolvedPayments.emplace_back(
-                                ua, ua.GetP2SHReceiver().value(), payment.GetAmount(), std::nullopt, false);
-                        } else if (ua.GetP2PKHReceiver().has_value()) {
-                            resolvedPayments.emplace_back(
-                                ua, ua.GetP2PKHReceiver().value(), payment.GetAmount(), std::nullopt, false);
-                        } else {
-                            // This should only occur when we have
-                            // • an Orchard-only UA,
-                            // • `AllowRevealedRecipients`, and
-                            // • can’t resolve Orchard (which means either insufficient non-Sprout
-                            //   funds or pre-NU5).
-                            resolutionError = AddressResolutionError::CouldNotResolveReceiver;
-                        }
-                    } else if (strategy.AllowRevealedAmounts()) {
-                        resolutionError = AddressResolutionError::TransparentReceiverNotAllowed;
-                    } else {
-                        resolutionError = AddressResolutionError::RevealingReceiverAmountsNotAllowed;
-                    }
-                }
-            }
-        });
-
-        if (resolutionError.has_value()) {
-            return tl::make_unexpected(resolutionError.value());
+        auto res = ResolvePayment(payment, canResolveOrchard, strategy, maxSaplingAvailable, maxOrchardAvailable, orchardOutputs);
+        res.map([&](const ResolvedPayment& rpayment) { resolvedPayments.emplace_back(rpayment); });
+        if (!res.has_value()) {
+            return tl::make_unexpected(res.error());
        }
    }
    auto resolved = Payments(resolvedPayments);
--- a/src/wallet/wallet_tx_builder.h
+++ b/src/wallet/wallet_tx_builder.h
@ -65,6 +65,12 @@ public:
    }
 };

+typedef std::pair<libzcash::PaymentAddress, std::optional<Memo>> NetAmountRecipient;
+
+typedef std::variant<
+    std::vector<Payment>,
+    NetAmountRecipient> Recipients;
+
 class Payments {
 private:
    std::vector<ResolvedPayment> payments;
@ -426,7 +432,7 @@ public:
            CWallet& wallet,
            const ZTXOSelector& selector,
            SpendableInputs& spendable,
-            const std::vector<Payment>& payments,
+            const Recipients& payments,
            const CChain& chain,
            TransactionStrategy strategy,
            /// A fixed fee is used if provided, otherwise it is calculated based on ZIP 317.