Update WalletTxBuilder based on review

Some of the more significant changes are - remove release note entry for already-released feature; - rephrase some error messages and comments; - add a missing case to `EstimateTxSize`; - don’t return a selector when we don’t have a UFVK for a UA, which allows some simplifications (and elimination of a failure case) to happen; and - remove a redundant `InsufficientFundsError`.
2023-03-16 13:57:57 -06:00 · 2023-03-16 13:57:57 -06:00 · 9f84ce2858
parent 37ca96e747
commit 9f84ce2858
9 changed files with 62 additions and 85 deletions
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@ -21,12 +21,6 @@ RPC Changes
    also available for testnet and regtest nodes, in which case it does not
    return end-of-service halt information (as testnet and regtest nodes do not
    have an end-of-service halt feature.)
- `z_sendmany` will no longer select transparent coinbase when "ANY\_TADDR" is
-  used as the `fromaddress`. It was already documented to do this, but the
-  previous behavior didn’t match. When coinbase notes were selected in this
-  case, they would (properly) require that the transaction didn’t have any
-  change, but this could be confusing, as the documentation stated that these
-  two conditions (using "ANY\_TADDR" and disallowing change) wouldn’t coincide.
 - Several `z_sendmany` failures have moved from synchronous to asynchronous, so
  while you should already be checking the async operation status, there are now
  more cases that may trigger failure at that stage.
--- a/qa/rpc-tests/wallet_z_sendmany.py
+++ b/qa/rpc-tests/wallet_z_sendmany.py
@ -183,9 +183,10 @@ class WalletZSendmanyTest(BitcoinTestFramework):
        # If we attempt to spend with the default privacy policy, z_sendmany
        # fails because it needs to spend transparent coins in a transaction
        # involving a Unified Address.
+        unified_address_msg = 'Could not select a unified address receiver that allows this transaction to proceed without publicly revealing the transaction amount. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` or weaker if you wish to allow this transaction to proceed anyway.'
        revealed_senders_msg = 'Insufficient funds: have 0.00, need 10.00; note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker.'
-        opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, 'AllowRevealedAmounts')
-        wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg)
+        opid = self.nodes[2].z_sendmany(source, recipients, 1, 0)
+        wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', unified_address_msg)

        # We can't create a transaction with an unknown privacy policy.
        assert_raises_message(
@ -196,14 +197,13 @@ class WalletZSendmanyTest(BitcoinTestFramework):

        # If we set any policy that does not include AllowRevealedSenders,
        # z_sendmany also fails.
-        opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, 'FullPrivacy')
-        wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', 'Could not select a unified address receiver that allows this transaction to proceed without publicly revealing the transaction amount. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` or weaker if you wish to allow this transaction to proceed anyway.')
-        for policy in [
-            'AllowRevealedAmounts',
-            'AllowRevealedRecipients',
+        for (policy, msg) in [
+            ('FullPrivacy', unified_address_msg)
+            ('AllowRevealedAmounts', revealed_senders_msg),
+            ('AllowRevealedRecipients', revealed_senders_msg),
        ]:
            opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, policy)
-            wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg)
+            wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', msg)

        # By setting the correct policy, we can create the transaction.
        opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, 'AllowRevealedSenders')
@ -322,20 +322,19 @@ class WalletZSendmanyTest(BitcoinTestFramework):

        # If we try to send 3 ZEC from n1ua0, it will fail with too-few funds.
        recipients = [{"address":n0ua0, "amount":3}]
+        linked_addrs_msg = 'Insufficient funds: have 2.00, need 3.00; note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker. (This transaction may require selecting transparent coins that were sent to multiple Unified Addresses, which is not enabled by default because it would create a public link between the transparent receivers of these addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to allow this transaction to proceed anyway.)'
        revealed_amounts_msg = 'Could not select a unified address receiver that allows this transaction to proceed without publicly revealing the transaction amount. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` or weaker if you wish to allow this transaction to proceed anyway.'
        opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0)
        wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', revealed_amounts_msg)

        # If we try it again with any policy that is too strong, it also fails.
-        opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, 'FullPrivacy')
-        wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', revealed_amounts_msg)
-        linked_addrs_msg = 'Insufficient funds: have 2.00, need 3.00; note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker. (This transaction may require selecting transparent coins that were sent to multiple Unified Addresses, which is not enabled by default because it would create a public link between the transparent receivers of these addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to allow this transaction to proceed anyway.)'
-        for policy in [
-            'AllowRevealedAmounts',
-            'AllowRevealedRecipients',
+        for (policy, msg) in [
+            ('FullPrivacy', revealed_amounts_msg),
+            ('AllowRevealedAmounts', linked_addrs_msg),
+            ('AllowRevealedRecipients', linked_addrs_msg),
        ]:
            opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, policy)
-            wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', linked_addrs_msg)
+            wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', msg)
        for policy in [
            'AllowRevealedSenders',
            'AllowFullyTransparent',
--- a/src/wallet/asyncrpcoperation_common.cpp
+++ b/src/wallet/asyncrpcoperation_common.cpp
@ -55,7 +55,7 @@ void ThrowInputSelectionError(
                        RPC_INVALID_PARAMETER,
                        "Sending from the Sprout shielded pool to the Sapling "
                        "shielded pool is not enabled by default because it will "
-                        "publicly reveal the transaction amount. THIS MAY AFFECT YOUR PRIVACY. "
+                        "publicly reveal transaction amounts. THIS MAY AFFECT YOUR PRIVACY. "
                        "Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` "
                        "or weaker if you wish to allow this transaction to proceed anyway.");
                case AddressResolutionError::SproutRecipientNotPermitted:
@ -81,15 +81,7 @@ void ThrowInputSelectionError(
                    throw JSONRPCError(
                        RPC_INVALID_PARAMETER,
                        "Could not select a unified address receiver that allows this transaction "
-                        "to proceed without publicly revealing the transaction amount. THIS MAY AFFECT "
-                        "YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to "
-                        "`AllowRevealedAmounts` or weaker if you wish to allow this transaction to "
-                        "proceed anyway.");
-                case AddressResolutionError::ChangeAddressSelectionError:
-                    throw JSONRPCError(
-                        RPC_INVALID_PARAMETER,
-                        "Could not select a change address that allows this transaction "
-                        "to proceed without publicly revealing transaction details. THIS MAY AFFECT "
+                        "to proceed without publicly revealing transaction amounts. THIS MAY AFFECT "
                        "YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to "
                        "`AllowRevealedAmounts` or weaker if you wish to allow this transaction to "
                        "proceed anyway.");
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@ -4670,6 +4670,9 @@ size_t EstimateTxSize(
                    orchardRecipientCount += 1;
                } else if (addr.GetSaplingReceiver().has_value()) {
                    mtx.vShieldedOutput.push_back(OutputDescription());
+                } else if (addr.GetP2PKHReceiver().has_value()
+                           || addr.GetP2SHReceiver().has_value()) {
+                    taddrRecipientCount += 1;
                }
            }
        }, recipient.GetAddress());
--- a/src/wallet/test/rpc_wallet_tests.cpp
+++ b/src/wallet/test/rpc_wallet_tests.cpp
@ -1243,7 +1243,10 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
        auto selector = pwalletMain->ZTXOSelectorForAddress(
                taddr1,
                true,
-                TransparentCoinbasePolicy::Require,
+                // In the real transaction builder we use either Require or Disallow, but here we
+                // are checking that there are no UTXOs at all, so we allow either to be selected to
+                // confirm this.
+                TransparentCoinbasePolicy::Allow,
                false).value();
        WalletTxBuilder builder(Params(), *pwalletMain, minRelayTxFee);
        std::vector<Payment> recipients = { Payment(zaddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) };
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@ -1933,8 +1933,6 @@ std::optional<ZTXOSelector> CWallet::ZTXOSelectorForAddress(
                        pattern = ua;
                    }
                }
-            } else {
-                pattern = ua;
            }
        }
    }, addr);
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@ -896,7 +896,7 @@ typedef std::variant<
 */
 enum class TransparentCoinbasePolicy {
    Disallow, //!< Do not select transparent coinbase
-    Allow,    //!< Select all transparent UTXOs, whether or not they’re coinbase
+    Allow,    //!< Make transparent coinbase available to the selector
    Require   //!< Only select transparent coinbase
 };

--- a/src/wallet/wallet_tx_builder.cpp
+++ b/src/wallet/wallet_tx_builder.cpp
@ -32,20 +32,14 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
    auto resolvedSelection = std::get<InputSelection>(selected);
    auto resolvedPayments = resolvedSelection.GetPayments();

-    if (spendable.Total() < resolvedPayments.Total() + fee) {
-        return InvalidFundsError(
-                spendable.Total(),
-                InsufficientFundsError(resolvedPayments.Total() + fee));
-    }
-
-    // Determine the account we're sending from.
-    auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT);
-
    // We do not set a change address if there is no change.
    std::optional<ChangeAddress> changeAddr;
    auto changeAmount = spendable.Total() - resolvedPayments.Total() - fee;
    if (changeAmount > 0) {
-        auto allowedChangeTypes = [&](const std::set<ReceiverType>& receiverTypes) -> std::set<OutputPool> {
+        // 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) {
@ -56,7 +50,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
                    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.
+                        //       previous behavior, so enable it separately. (#6409)
                        // if (strategy.AllowRevealedRecipients()) {
                        if (!spendable.utxos.empty() || strategy.AllowRevealedRecipients()) {
                            result.insert(OutputPool::Transparent);
@ -78,34 +72,34 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
            return result;
        };

-        std::visit(match {
-            [&](const CKeyID& keyId) {
+        changeAddr = std::visit(match {
+            [&](const CKeyID& keyId) -> ChangeAddress {
                auto sendTo = pwalletMain->GenerateChangeAddressForAccount(
                        sendFromAccount,
-                        allowedChangeTypes({ReceiverType::P2PKH}));
+                        getAllowedChangePools({ReceiverType::P2PKH}));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            },
-            [&](const CScriptID& scriptId) {
+            [&](const CScriptID& scriptId) -> ChangeAddress {
                auto sendTo = pwalletMain->GenerateChangeAddressForAccount(
                        sendFromAccount,
-                        allowedChangeTypes({ReceiverType::P2SH}));
+                        getAllowedChangePools({ReceiverType::P2SH}));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            },
-            [&](const libzcash::SproutPaymentAddress& addr) {
+            [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress {
                // for Sprout, we return change to the originating address using the tx builder.
-                changeAddr = addr;
+                return addr;
            },
-            [&](const libzcash::SproutViewingKey& vk) {
+            [](const libzcash::SproutViewingKey& vk) -> ChangeAddress {
                // for Sprout, we return change to the originating address using the tx builder.
-                changeAddr = vk.address();
+                return vk.address();
            },
-            [&](const libzcash::SaplingPaymentAddress& addr) {
+            [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress {
                // 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.
@ -113,13 +107,13 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
                    ? addr
                    : pwalletMain->GenerateChangeAddressForAccount(
                            sendFromAccount,
-                            allowedChangeTypes({ReceiverType::Sapling}));
+                            getAllowedChangePools({ReceiverType::Sapling}));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            },
-            [&](const libzcash::SaplingExtendedFullViewingKey& fvk) {
+            [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress {
                // 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.
@ -127,46 +121,41 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction(
                    ? fvk.DefaultAddress()
                    : pwalletMain->GenerateChangeAddressForAccount(
                            sendFromAccount,
-                            allowedChangeTypes({ReceiverType::Sapling}));
+                            getAllowedChangePools({ReceiverType::Sapling}));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            },
-            [&](const libzcash::UnifiedAddress& ua) {
+            [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress {
                auto zufvk = pwalletMain->GetUFVKForAddress(ua);
-                if (zufvk.has_value()) {
-                    auto sendTo = zufvk.value().GetChangeAddress(
-                            allowedChangeTypes(ua.GetKnownReceiverTypes()));
-                    assert(sendTo.has_value());
-                    resolvedPayments.AddPayment(
-                            ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                    changeAddr = sendTo.value();
-                }
+                assert(zufvk.has_value());
+                auto sendTo = zufvk.value().GetChangeAddress(
+                        getAllowedChangePools(ua.GetKnownReceiverTypes()));
+                assert(sendTo.has_value());
+                resolvedPayments.AddPayment(
+                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
+                return sendTo.value();
            },
-            [&](const libzcash::UnifiedFullViewingKey& fvk) {
+            [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress {
                auto zufvk = ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk);
                auto sendTo = zufvk.GetChangeAddress(
-                        allowedChangeTypes(fvk.GetKnownReceiverTypes()));
+                        getAllowedChangePools(fvk.GetKnownReceiverTypes()));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            },
-            [&](const AccountZTXOPattern& acct) {
+            [&](const AccountZTXOPattern& acct) -> ChangeAddress {
                auto sendTo = pwalletMain->GenerateChangeAddressForAccount(
                        acct.GetAccountId(),
-                        allowedChangeTypes(acct.GetReceiverTypes()));
+                        getAllowedChangePools(acct.GetReceiverTypes()));
                assert(sendTo.has_value());
                resolvedPayments.AddPayment(
                        ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true));
-                changeAddr = sendTo.value();
+                return sendTo.value();
            }
        }, selector.GetPattern());
-
-        if (!changeAddr.has_value()) {
-            return AddressResolutionError::ChangeAddressSelectionError;
-        }
    }

    auto ovks = SelectOVKs(selector, spendable);
@ -492,10 +481,10 @@ std::pair<uint256, uint256> WalletTxBuilder::SelectOVKs(
 PrivacyPolicy TransactionEffects::GetRequiredPrivacyPolicy() const
 {
    if (!spendable.utxos.empty()) {
-        // TODO: Add a check for whether we need AllowLinkingAccountAddresses here.
+        // TODO: Add a check for whether we need AllowLinkingAccountAddresses here. (#6467)
        if (payments.HasTransparentRecipient()) {
            // TODO: AllowFullyTransparent is the correct policy, but it’s a breaking change from
-            //       previous behavior, so enable it separately.
+            //       previous behavior, so enable it separately. (#6409)
            // maxPrivacy = PrivacyPolicy::AllowFullyTransparent;
            return PrivacyPolicy::AllowRevealedSenders;
        } else {
--- a/src/wallet/wallet_tx_builder.h
+++ b/src/wallet/wallet_tx_builder.h
@ -213,7 +213,6 @@ enum class AddressResolutionError {
    TransparentRecipientNotPermitted,
    InsufficientSaplingFunds,
    UnifiedAddressResolutionError,
-    ChangeAddressSelectionError
 };

 class InsufficientFundsError {