From 51defe9f96822a3aa2cff17b03348a1af180ba4e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 6 Apr 2022 14:38:08 -0600 Subject: [PATCH] Make all privacy policy checks after note selection. This adds tests to wallet_z_sendmany that demonstrates conditions where pool-crossing transfers were not being caught by the privacy policy checks, which were validating only against the transfer request rather than the actual transfer selected. This missed cases where sending funds between unified addresses would reveal amounts via the turnstile. Making all privacy policy checks after note selection guards against this error. --- qa/rpc-tests/wallet_unified_change.py | 2 +- qa/rpc-tests/wallet_z_sendmany.py | 93 ++++++++++++++++++---- src/wallet/asyncrpcoperation_sendmany.cpp | 97 ++++++++++++----------- 3 files changed, 130 insertions(+), 62 deletions(-) diff --git a/qa/rpc-tests/wallet_unified_change.py b/qa/rpc-tests/wallet_unified_change.py index 375c7c422..6df15ce52 100755 --- a/qa/rpc-tests/wallet_unified_change.py +++ b/qa/rpc-tests/wallet_unified_change.py @@ -57,7 +57,7 @@ class WalletUnifiedChangeTest(BitcoinTestFramework): self.nodes[0].z_getbalanceforaccount(account0)) # Send both amounts to ua1 in fully-shielded transactions. This will result - # in account1 having both a Sapling and an Orchard balances. + # in account1 having both Sapling and Orchard balances. recipients = [{'address': ua1_sapling, 'amount': 5}] opid = self.nodes[0].z_sendmany(ua0_sapling, recipients, 1, Decimal('0.00001')) diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index fd6870823..09cad54c7 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -5,11 +5,13 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + DEFAULT_FEE, + NU5_BRANCH_ID, assert_equal, assert_greater_than, assert_raises_message, connect_nodes_bi, - DEFAULT_FEE, + nuparams, start_nodes, wait_and_assert_operationid_status, ) @@ -21,6 +23,7 @@ from decimal import Decimal class WalletZSendmanyTest(BitcoinTestFramework): def setup_network(self, split=False): self.nodes = start_nodes(3, self.options.tmpdir, [[ + nuparams(NU5_BRANCH_ID, 220), ]] * self.num_nodes) connect_nodes_bi(self.nodes,0,1) connect_nodes_bi(self.nodes,1,2) @@ -217,11 +220,9 @@ class WalletZSendmanyTest(BitcoinTestFramework): # If we attempt to spend with the default privacy policy, z_sendmany # returns an error because it needs to create a transparent recipient in # a transaction involving a Unified Address. - assert_raises_message( - JSONRPCException, - 'AllowRevealedRecipients', - self.nodes[0].z_sendmany, - n0ua0, recipients, 1, 0) + revealed_recipients_msg = "This transaction would have transparent recipients, which is not enabled by default because it will publicly reveal transaction 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." + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], opid, 'failed', revealed_recipients_msg) # If we set any policy that does not include AllowRevealedRecipients, # z_sendmany also returns an error. @@ -231,11 +232,8 @@ class WalletZSendmanyTest(BitcoinTestFramework): 'AllowRevealedSenders', 'AllowLinkingAccountAddresses', ]: - assert_raises_message( - JSONRPCException, - 'AllowRevealedRecipients', - self.nodes[0].z_sendmany, - n0ua0, recipients, 1, 0, policy) + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0, policy) + wait_and_assert_operationid_status(self.nodes[0], opid, 'failed', revealed_recipients_msg) # By setting the correct policy, we can create the transaction. opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0, 'AllowRevealedRecipients') @@ -374,14 +372,77 @@ class WalletZSendmanyTest(BitcoinTestFramework): # Send some funds from node 1's account to a transparent address. recipients = [{"address":mytaddr, "amount":5}] # This requires the AllowRevealedRecipients policy... - assert_raises_message( - JSONRPCException, - 'AllowRevealedRecipients', - self.nodes[1].z_sendmany, - n1ua0, recipients, 1, 0) + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', revealed_recipients_msg) # ... which we can always override with the NoPrivacy policy. opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, 'NoPrivacy') wait_and_assert_operationid_status(self.nodes[1], opid) + # Activate NU5 + + self.sync_all() + self.nodes[1].generate(10) + self.sync_all() + + # + # Test AllowRevealedAmounts policy + # + + assert_equal( + {'pools': {'sapling': {'valueZat': 600000000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(n1account)) + + # Sending some funds to the Orchard pool in n0account0 ... + n0ua1 = self.nodes[0].z_getaddressforaccount(n0account0, ["orchard"])['address'] + recipients = [{"address":n0ua1, "amount": 6}] + + # Should fail under default and 'FullPrivacy' policies ... + revealed_amounts_msg = 'Sending from the Sapling shielded pool to the Orchard shielded pool is not enabled by default because it will publicly reveal 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) + + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, 'FullPrivacy') + wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', revealed_amounts_msg) + + # Should succeed under 'AllowRevealedAmounts' + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, 'AllowRevealedAmounts') + wait_and_assert_operationid_status(self.nodes[1], opid) + + self.sync_all() + self.nodes[1].generate(1) + self.sync_all() + + assert_equal( + {'pools': {'sapling': {'valueZat': 1100000000}, 'orchard': {'valueZat': 600000000}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(n0account0)) + + # A total that requires selecting from both pools should fail under default and + # FullPrivacy policies... + + recipients = [{"address":n1ua0, "amount": 15}] + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], opid, 'failed', revealed_amounts_msg) + + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0, 'FullPrivacy') + wait_and_assert_operationid_status(self.nodes[0], opid, 'failed', revealed_amounts_msg) + + # Should succeed under 'AllowRevealedAmounts' + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0, 'AllowRevealedAmounts') + wait_and_assert_operationid_status(self.nodes[0], opid) + + self.sync_all() + self.nodes[1].generate(1) + self.sync_all() + + # All funds should be received to the Orchard pool + assert_equal( + {'pools': {'orchard': {'valueZat': 1500000000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(n1account)) + + # And all change should be optimistically shielded + assert_equal( + {'pools': {'orchard': {'valueZat': 200000000}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(n0account0)) + if __name__ == '__main__': WalletZSendmanyTest().main() diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 0aebcee7b..6c67e403a 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -78,61 +78,16 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( [&](const libzcash::SaplingPaymentAddress& addr) { txOutputAmounts_.sapling_outputs_total += recipient.amount; recipientPools_.insert(OutputPool::Sapling); - if (!(ztxoSelector_.SelectsSapling() || strategy_.AllowRevealedAmounts())) { - if (ztxoSelector_.SelectsSprout()) { - throw JSONRPCError( - 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. " - "Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` " - "or weaker if you wish to allow this transaction to proceed anyway."); - } - if (builder_.SupportsOrchard() && ztxoSelector_.SelectsOrchard()) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "Sending from the Orchard 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. " - "Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` " - "or weaker if you wish to allow this transaction to proceed anyway."); - } - // If the source selects transparent then we don't show an - // error because we are necessarily revealing information. - } }, [&](const libzcash::OrchardRawAddress& addr) { txOutputAmounts_.orchard_outputs_total += recipient.amount; recipientPools_.insert(OutputPool::Orchard); // No transaction allows sends from Sprout to Orchard. assert(!ztxoSelector_.SelectsSprout()); - if (!((builder_.SupportsOrchard() && ztxoSelector_.SelectsOrchard()) || strategy_.AllowRevealedAmounts())) { - if (ztxoSelector_.SelectsSapling()) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "Sending from the Sapling shielded pool to the Orchard " - "shielded pool is not enabled by default because it will " - "publicly reveal 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."); - } - // If the source selects transparent then we don't show an - // error because we are necessarily revealing information. - } } }, recipient.address); } - if (recipientPools_.count(OutputPool::Transparent) && !strategy_.AllowRevealedRecipients()) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "This transaction would have transparent recipients, which is not " - "enabled by default because it will publicly reveal transaction " - "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."); - } - // Log the context info i.e. the call parameters to z_sendmany if (LogAcceptCategory("zrpcunsafe")) { LogPrint("zrpcunsafe", "%s: z_sendmany initialized (params=%s)\n", getId(), contextInfo.write()); @@ -287,6 +242,58 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { "or weaker if you wish to allow this transaction to proceed anyway."); } + if (recipientPools_.count(OutputPool::Transparent) && !strategy_.AllowRevealedRecipients()) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "This transaction would have transparent recipients, which is not " + "enabled by default because it will publicly reveal transaction " + "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."); + } + + if (!spendable.sproutNoteEntries.empty()) { + if (recipientPools_.count(OutputPool::Sapling) && !strategy_.AllowRevealedAmounts()) { + throw JSONRPCError( + 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. " + "Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` " + "or weaker if you wish to allow this transaction to proceed anyway."); + } + } + + if (!spendable.saplingNoteEntries.empty()) { + if (recipientPools_.count(OutputPool::Orchard) && !strategy_.AllowRevealedAmounts()) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Sending from the Sapling shielded pool to the Orchard " + "shielded pool is not enabled by default because it will " + "publicly reveal 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."); + } + // Sending from Sapling to transparent will be caught above in the + // AllowRevealedRecipients check; sending to Sprout is disallowed + // entirely. + } + + if (!spendable.orchardNoteMetadata.empty()) { + if (recipientPools_.count(OutputPool::Sapling) && !strategy_.AllowRevealedAmounts()) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Sending from the Orchard 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. " + "Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` " + "or weaker if you wish to allow this transaction to proceed anyway."); + } + // Sending from Orchard to transparent will be caught above in the + // AllowRevealedRecipients check; sending to Sprout is disallowed + // entirely. + } + spendable.LogInputs(getId()); CAmount t_inputs_total{0};