From 2596b4920bf079575f53f422d03041f8fcfb5985 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 13 Apr 2023 08:59:00 -0600 Subject: [PATCH] Fix edge case revealed by #6409 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without `AllowRevealedRecipients`, we can’t send transparent change, but previously we asserted if we couldn’t get a transparent change address. Now it returns a new `TransparentChangeNotAllowed` failure, which is just a more specific `TransparentRecipientNotAllowed` to avoid confusion when there are no explicit unshielded recipients. --- qa/rpc-tests/wallet_shieldingcoinbase.py | 2 +- qa/rpc-tests/wallet_z_sendmany.py | 19 +++++ src/wallet/asyncrpcoperation_common.cpp | 8 ++ src/wallet/wallet_tx_builder.cpp | 99 ++++++++++++++++-------- src/wallet/wallet_tx_builder.h | 4 +- 5 files changed, 99 insertions(+), 33 deletions(-) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 44a0ffcdf..2d0f62971 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -105,7 +105,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): recipients = [] recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')}) - myopid = self.nodes[0].z_sendmany(mytaddr, recipients, 10, DEFAULT_FEE, 'AllowRevealedSenders') + myopid = self.nodes[0].z_sendmany(mytaddr, recipients, 10, DEFAULT_FEE, 'AllowFullyTransparent') error_result = wait_and_assert_operationid_status_result( self.nodes[0], myopid, "failed", diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 9b78e1364..7dc57757c 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -494,5 +494,24 @@ class WalletZSendmanyTest(BitcoinTestFramework): {'pools': {'orchard': {'valueZat': 200000000}}, 'minimum_confirmations': 1}, self.nodes[0].z_getbalanceforaccount(n0account0)) + + self.sync_all() + self.nodes[1].generate(1) + self.sync_all() + + # + # Test transparent change + # + + recipients = [{"address":n0ua1, "amount": 4}] + # Should fail because this generates transparent change, but we don’t have + # `AllowRevealedRecipients` + opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0, 'AllowRevealedSenders') + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', "This transaction would have transparent change, which is not enabled by default because it will publicly reveal the change address and amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedRecipients` or weaker if you wish to allow this transaction to proceed anyway.") + + # Should succeed once we include `AllowRevealedRecipients` + opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0, 'AllowFullyTransparent') + wait_and_assert_operationid_status(self.nodes[2], opid) + if __name__ == '__main__': WalletZSendmanyTest().main() diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 30aeee804..f0514ab18 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -62,6 +62,14 @@ void ThrowInputSelectionError( "recipients and amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit " "with the `privacyPolicy` parameter set to `AllowRevealedRecipients` " "or weaker if you wish to allow this transaction to proceed anyway."); + case AddressResolutionError::TransparentChangeNotAllowed: + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "This transaction would have transparent change, which is not " + "enabled by default because it will publicly reveal the change " + "address and amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit " + "with the `privacyPolicy` parameter set to `AllowRevealedRecipients` " + "or weaker if you wish to allow this transaction to proceed anyway."); case AddressResolutionError::RevealingSaplingAmountNotAllowed: throw JSONRPCError( RPC_INVALID_PARAMETER, diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index fe6f4b3fe..ae8f11f2f 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -13,7 +13,7 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) return nextBlockHeight - anchorConfirmations; } -ChangeAddress +tl::expected WalletTxBuilder::GetChangeAddress( CWallet& wallet, const ZTXOSelector& selector, @@ -55,15 +55,20 @@ WalletTxBuilder::GetChangeAddress( return result; }; - auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) { + auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) + -> tl::expected { auto addr = wallet.GenerateChangeAddressForAccount( sendFromAccount, getAllowedChangePools(receiverTypes)); - assert(addr.has_value()); - return addr.value(); + if (addr.has_value()) { + return {addr.value()}; + } else { + return tl::make_unexpected(AddressResolutionError::TransparentChangeNotAllowed); + } }; - auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) -> RecipientAddress { + auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) + -> tl::expected { // 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. @@ -80,45 +85,48 @@ WalletTxBuilder::GetChangeAddress( auto changeAddressForZUFVK = [&]( const ZcashdUnifiedFullViewingKey& zufvk, - const std::set& receiverTypes) { + const std::set& receiverTypes) + -> tl::expected { auto addr = zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes)); assert(addr.has_value()); return addr.value(); }; return examine(selector.GetPattern(), match { - [&](const CKeyID&) -> ChangeAddress { + [&](const CKeyID&) { return changeAddressForTransparentSelector({ReceiverType::P2PKH}); }, - [&](const CScriptID&) -> ChangeAddress { + [&](const CScriptID&) { return changeAddressForTransparentSelector({ReceiverType::P2SH}); }, - [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress { + [](const libzcash::SproutPaymentAddress& addr) + -> tl::expected { // for Sprout, we return change to the originating address using the tx builder. return addr; }, - [](const libzcash::SproutViewingKey& vk) -> ChangeAddress { + [](const libzcash::SproutViewingKey& vk) + -> tl::expected { // for Sprout, we return change to the originating address using the tx builder. return vk.address(); }, - [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress { + [&](const libzcash::SaplingPaymentAddress& addr) { return changeAddressForSaplingAddress(addr); }, - [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress { + [&](const libzcash::SaplingExtendedFullViewingKey& fvk) { return changeAddressForSaplingAddress(fvk.DefaultAddress()); }, - [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress { + [&](const libzcash::UnifiedAddress& ua) { auto zufvk = wallet.GetUFVKForAddress(ua); assert(zufvk.has_value()); return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); }, - [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress { + [&](const libzcash::UnifiedFullViewingKey& fvk) { return changeAddressForZUFVK( ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk), fvk.GetKnownReceiverTypes()); }, - [&](const AccountZTXOPattern& acct) -> ChangeAddress { - auto addr = wallet.GenerateChangeAddressForAccount( + [&](const AccountZTXOPattern& acct) -> tl::expected { + auto addr = wallet.GenerateChangeAddressForAccount( acct.GetAccountId(), getAllowedChangePools(acct.GetReceiverTypes())); assert(addr.has_value()); @@ -297,11 +305,21 @@ InvalidFundsError ReportInvalidFunds( : InvalidFundsReason(InsufficientFundsError(targetAmount)))); } -static void -AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CAmount changeAmount) +static tl::expected +AddChangePayment( + const SpendableInputs& spendable, + Payments& resolvedPayments, + const ChangeAddress& changeAddr, + CAmount changeAmount, + CAmount targetAmount) { assert(changeAmount > 0); + // When spending transparent coinbase outputs, all inputs must be fully consumed. + if (spendable.HasTransparentCoinbase()) { + return tl::make_unexpected(ChangeNotAllowedError(spendable.Total(), targetAmount)); + } + 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. @@ -312,6 +330,8 @@ AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CA ResolvedPayment(std::nullopt, sendTo, changeAmount, std::nullopt, true)); } }); + + return {}; } /// On the initial call, we haven’t yet selected inputs, so we assume the outputs dominate the @@ -344,11 +364,12 @@ WalletTxBuilder::IterateLimit( CAmount bumpTargetAmount{0}; std::optional changeAddr; CAmount changeAmount{0}; + CAmount targetAmount{0}; do { spendableMut = spendable; - auto targetAmount{sendAmount + updatedFee}; + targetAmount = sendAmount + updatedFee; // TODO: the set of recipient pools is not quite sufficient information here; we should // probably perform note selection at the same time as we're performing resolved payment @@ -364,13 +385,19 @@ WalletTxBuilder::IterateLimit( // 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( + auto possibleChangeAddr = GetChangeAddress( wallet, selector, spendableMut, resolved, strategy, afterNU5); + + if (possibleChangeAddr.has_value()) { + changeAddr = possibleChangeAddr.value(); + } else { + return tl::make_unexpected(possibleChangeAddr.error()); + } } previousFee = updatedFee; updatedFee = CalcZIP317Fee( @@ -399,7 +426,12 @@ WalletTxBuilder::IterateLimit( if (changeAmount > 0) { assert(changeAddr.has_value()); - AddChangePayment(resolved, changeAddr.value(), changeAmount); + + auto changeRes = + AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount); + if (!changeRes.has_value()) { + return tl::make_unexpected(changeRes.error()); + } } return std::make_tuple(spendableMut, updatedFee, changeAddr); @@ -557,14 +589,24 @@ WalletTxBuilder::ResolveInputsAndPayments( changeAmount)); } if (changeAmount > 0) { - changeAddr = GetChangeAddress( + auto possibleChangeAddr = GetChangeAddress( wallet, selector, spendableMut, resolved, strategy, afterNU5); - AddChangePayment(resolved, changeAddr.value(), changeAmount); + + if (possibleChangeAddr.has_value()) { + changeAddr = possibleChangeAddr.value(); + } else { + return tl::make_unexpected(possibleChangeAddr.error()); + } + auto changeRes = + AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount); + if (!changeRes.has_value()) { + return tl::make_unexpected(changeRes.error()); + } } } else { auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5); @@ -576,14 +618,9 @@ WalletTxBuilder::ResolveInputsAndPayments( } } - // When spending transparent coinbase outputs, all inputs must be fully - // consumed, and they may only be sent to shielded recipients. - if (spendableMut.HasTransparentCoinbase()) { - if (spendableMut.Total() != targetAmount) { - return tl::make_unexpected(ChangeNotAllowedError(spendableMut.Total(), targetAmount)); - } else if (resolved.HasTransparentRecipient()) { - return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); - } + // When spending transparent coinbase outputs they may only be sent to shielded recipients. + if (spendableMut.HasTransparentCoinbase() && resolved.HasTransparentRecipient()) { + return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); } if (spendableMut.orchardNoteMetadata.size() > this->maxOrchardActions) { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 4ac3e2f08..387313136 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -225,6 +225,8 @@ enum class AddressResolutionError { SproutRecipientsNotSupported, //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedRecipients` TransparentRecipientNotAllowed, + //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedRecipients` + TransparentChangeNotAllowed, //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedAmounts`, but we don’t have enough //! Sapling funds to avoid revealing amounts RevealingSaplingAmountNotAllowed, @@ -338,7 +340,7 @@ private: */ CAmount DefaultDustThreshold() const; - ChangeAddress + tl::expected GetChangeAddress( CWallet& wallet, const ZTXOSelector& selector,