diff --git a/doc/release-notes.md b/doc/release-notes.md index 5d3e57a7f..fad35ea7e 100644 --- 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. diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 3d798a211..377340e75 100755 --- 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', diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index d5119bdc6..8d3294ff1 100644 --- 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."); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8f55af780..7ad2ed292 100644 --- 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()); diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index b330250e8..71cecc9e4 100644 --- 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 recipients = { Payment(zaddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fed0e5fa2..a5a9e28ae 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1933,8 +1933,6 @@ std::optional CWallet::ZTXOSelectorForAddress( pattern = ua; } } - } else { - pattern = ua; } } }, addr); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index eb2c7b20d..a558cc2ef 100644 --- 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 }; diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index b958548bf..64b06891e 100644 --- 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(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 changeAddr; auto changeAmount = spendable.Total() - resolvedPayments.Total() - fee; if (changeAmount > 0) { - auto allowedChangeTypes = [&](const std::set& receiverTypes) -> std::set { + // Determine the account we're sending from. + auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT); + + auto getAllowedChangePools = [&](const std::set& receiverTypes) { std::set 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 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 { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 7330a0dbc..87215970f 100644 --- 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 {