diff --git a/doc/release-notes.md b/doc/release-notes.md index 76dd3fffb..7e0fec699 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -81,11 +81,15 @@ Wallet pool boundaries, they must be explicitly enabled via a parameter to the 'z_sendmany' call. -- A new boolean parameter, `allowRevealedAmounts`, has been added to the - list of arguments accepted by 'z_sendmany'. This parameter defaults to - `false` and is only required when the transaction being constructed - would reveal transaction amounts as a consequence of ZEC value crossing - shielded pool boundaries via the turnstile. +- A new string parameter, `privacyPolicy`, has been added to the list of + arguments accepted by `z_sendmany`. This parameter enables the caller to + control what kind of information they permit `zcashd` to reveal when creating + the transaction. If the transaction can only be created by revealing more + information than the given strategy permits, `z_sendmany` will return an + error. The parameter defaults to `LegacyCompat`, which applies the most + restrictive strategy `FullPrivacy` when a Unified Address is present as the + sender or a recipient, and otherwise preserves existing behaviour (which + corresponds to the `AllowFullyTransparent` policy). - Since Sprout outputs are no longer created (with the exception of change) 'z_sendmany' no longer generates payment disclosures (which were only diff --git a/qa/rpc-tests/orchard_reorg.py b/qa/rpc-tests/orchard_reorg.py index b77fb7bd4..58d0bf8d9 100755 --- a/qa/rpc-tests/orchard_reorg.py +++ b/qa/rpc-tests/orchard_reorg.py @@ -70,7 +70,7 @@ class OrchardReorgTest(BitcoinTestFramework): # Create an Orchard note. recipients = [{'address': ua, 'amount': Decimal('12.5')}] - opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[0], opid) # After mining a block, finalorchardroot should have changed. @@ -95,7 +95,7 @@ class OrchardReorgTest(BitcoinTestFramework): # Create another Orchard note on node 0. recipients = [{'address': ua, 'amount': Decimal('12.5')}] - opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[0], opid) # Mine two blocks on node 0. diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index 39d852956..92ff5a8b8 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -118,7 +118,7 @@ class WalletAccountsTest(BitcoinTestFramework): # Send coinbase funds to the UA. print('Sending coinbase funds to account') recipients = [{'address': ua0, 'amount': Decimal('10')}] - opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') txid = wait_and_assert_operationid_status(self.nodes[0], opid) # The wallet should detect the new note as belonging to the UA. @@ -169,7 +169,7 @@ class WalletAccountsTest(BitcoinTestFramework): # Send more coinbase funds to the UA. print('Sending coinbase funds to account') recipients = [{'address': ua0, 'amount': Decimal('10')}] - opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') txid = wait_and_assert_operationid_status(self.nodes[0], opid) # The wallet should detect the new note as belonging to the UA. diff --git a/qa/rpc-tests/wallet_listnotes.py b/qa/rpc-tests/wallet_listnotes.py index c50e7bb7b..198646745 100755 --- a/qa/rpc-tests/wallet_listnotes.py +++ b/qa/rpc-tests/wallet_listnotes.py @@ -68,7 +68,7 @@ class WalletListNotes(BitcoinTestFramework): change_amount_2 = receive_amount_1 - receive_amount_2 - DEFAULT_FEE assert_equal('sapling', self.nodes[0].z_validateaddress(saplingzaddr)['type']) recipients = [{"address": saplingzaddr, "amount":receive_amount_2}] - myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients, 1, DEFAULT_FEE, True) + myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients, 1, DEFAULT_FEE) txid_2 = wait_and_assert_operationid_status(self.nodes[0], myopid) self.sync_all() @@ -107,7 +107,7 @@ class WalletListNotes(BitcoinTestFramework): receive_amount_3 = Decimal('2.0') change_amount_3 = change_amount_2 - receive_amount_3 - DEFAULT_FEE recipients = [{"address": saplingzaddr2, "amount":receive_amount_3}] - myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients, 1, DEFAULT_FEE, True) + myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients, 1, DEFAULT_FEE) txid_3 = wait_and_assert_operationid_status(self.nodes[0], myopid) self.sync_all() unspent_tx = self.nodes[0].z_listunspent(0) diff --git a/qa/rpc-tests/wallet_orchard.py b/qa/rpc-tests/wallet_orchard.py index 48fb3caea..2a1ee85a6 100755 --- a/qa/rpc-tests/wallet_orchard.py +++ b/qa/rpc-tests/wallet_orchard.py @@ -54,7 +54,7 @@ class WalletOrchardTest(BitcoinTestFramework): saplingAddr2 = self.nodes[2].z_listunifiedreceivers(ua2)['sapling'] recipients = [{"address": saplingAddr2, "amount": Decimal('10')}] - myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[0], myopid) # Mine the tx & activate NU5 @@ -70,7 +70,7 @@ class WalletOrchardTest(BitcoinTestFramework): # Node 0 shields some funds # t-coinbase -> Orchard recipients = [{"address": ua1, "amount": Decimal('10')}] - myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[0], myopid) self.sync_all() @@ -86,7 +86,7 @@ class WalletOrchardTest(BitcoinTestFramework): # Send another tx to ua1 recipients = [{"address": ua1, "amount": Decimal('10')}] - myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[0], myopid) # Mine the tx & generate a majority chain on the 0/1 side of the split @@ -105,7 +105,7 @@ class WalletOrchardTest(BitcoinTestFramework): ua3 = addrRes3['unifiedaddress'] recipients = [{"address": ua3, "amount": Decimal('1')}] - myopid = self.nodes[2].z_sendmany(ua2, recipients, 1, 0, True) + myopid = self.nodes[2].z_sendmany(ua2, recipients, 1, 0) rollback_tx = wait_and_assert_operationid_status(self.nodes[2], myopid) self.sync_all() diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 48df09cb0..dd1559305 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -4,8 +4,15 @@ # file COPYING or https://www.opensource.org/licenses/mit-license.php . from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_greater_than, connect_nodes_bi, \ - DEFAULT_FEE, start_nodes, wait_and_assert_operationid_status +from test_framework.util import ( + assert_equal, + assert_greater_than, + assert_raises_message, + connect_nodes_bi, + DEFAULT_FEE, + start_nodes, + wait_and_assert_operationid_status, +) from test_framework.authproxy import JSONRPCException from test_framework.mininode import COIN from decimal import Decimal @@ -164,9 +171,36 @@ class WalletZSendmanyTest(BitcoinTestFramework): # Change went to a fresh address, so use `ANY_TADDR` which # should hold the rest of our transparent funds. + source = 'ANY_TADDR' recipients = [] recipients.append({"address":n0ua0, "amount":10}) - opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0) + + # 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. + revealed_senders_msg = 'This transaction requires selecting transparent coins, which is not enabled by default because it will publicly reveal transaction senders and amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedSenders` or weaker if you wish to allow this transaction to proceed anyway.' + opid = self.nodes[2].z_sendmany(source, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg) + + # We can't create a transaction with an unknown privacy policy. + assert_raises_message( + JSONRPCException, + 'Unknown privacy policy name \'ZcashIsAwesome\'', + self.nodes[2].z_sendmany, + source, recipients, 1, 0, 'ZcashIsAwesome') + + # If we set any policy that does not include AllowRevealedSenders, + # z_sendmany also fails. + for policy in [ + 'FullPrivacy', + 'AllowRevealedAmounts', + 'AllowRevealedRecipients', + ]: + opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, policy) + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg) + + # By setting the correct policy, we can create the transaction. + opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, 'AllowRevealedSenders') wait_and_assert_operationid_status(self.nodes[2], opid) self.nodes[2].generate(1) @@ -181,7 +215,32 @@ class WalletZSendmanyTest(BitcoinTestFramework): # Send some funds to a specific legacy taddr that we can spend from recipients = [] recipients.append({"address":mytaddr, "amount":5}) - opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0) + + # 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) + + # If we set any policy that does not include AllowRevealedRecipients, + # z_sendmany also returns an error. + for policy in [ + 'FullPrivacy', + 'AllowRevealedAmounts', + 'AllowRevealedSenders', + 'AllowLinkingAccountAddresses', + ]: + assert_raises_message( + JSONRPCException, + 'AllowRevealedRecipients', + self.nodes[0].z_sendmany, + n0ua0, recipients, 1, 0, policy) + + # By setting the correct policy, we can create the transaction. + opid = self.nodes[0].z_sendmany(n0ua0, recipients, 1, 0, 'AllowRevealedRecipients') wait_and_assert_operationid_status(self.nodes[0], opid) self.nodes[0].generate(1) @@ -204,10 +263,12 @@ class WalletZSendmanyTest(BitcoinTestFramework): self.check_balance(0, 0, n0ua0, {'sapling': 2}) assert_equal(Decimal(self.nodes[2].z_getbalance(myzaddr)), zbalance) - # Send funds back from the legacy taddr to the UA + # Send funds back from the legacy taddr to the UA. This requires + # AllowRevealedSenders, but we can also use any weaker policy that + # includes it. recipients = [] recipients.append({"address":n0ua0, "amount":4}) - opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0) + opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0, 'AllowFullyTransparent') wait_and_assert_operationid_status(self.nodes[2], opid) self.nodes[2].generate(1) @@ -230,5 +291,99 @@ class WalletZSendmanyTest(BitcoinTestFramework): self.check_balance(0, 0, n0ua0, {'sapling': 8}) assert_equal(Decimal(self.nodes[2].z_getbalance(myzaddr)), zbalance) + # + # Test that z_sendmany avoids UA linkability unless we allow it. + # + + # Generate a new account with two new addresses. + n1account = self.nodes[1].z_getnewaccount()['account'] + n1ua0 = self.nodes[1].z_getaddressforaccount(n1account)['unifiedaddress'] + n1ua1 = self.nodes[1].z_getaddressforaccount(n1account)['unifiedaddress'] + + # Send funds to the transparent receivers of both addresses. + for ua in [n1ua0, n1ua1]: + taddr = self.nodes[1].z_listunifiedreceivers(ua)['transparent'] + self.nodes[0].sendtoaddress(taddr, 2) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # The account should see all funds. + assert_equal( + self.nodes[1].z_getbalanceforaccount(n1account)['pools'], + {'transparent': {'valueZat': 4 * COIN}}, + ) + + # The addresses should see only the transparent funds sent to them. + assert_equal(self.nodes[1].z_getbalance(n1ua0), 2) + assert_equal(self.nodes[1].z_getbalance(n1ua1), 2) + + # 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 (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.)' + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', linked_addrs_msg) + + # If we try it again with any policy that is too strong, it also fails. + for policy in [ + 'FullPrivacy', + 'AllowRevealedAmounts', + 'AllowRevealedRecipients', + 'AllowRevealedSenders', + 'AllowFullyTransparent', + ]: + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, policy) + wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', linked_addrs_msg) + + # Once we provide a sufficiently-weak policy, the transaction succeeds. + opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, 'AllowLinkingAccountAddresses') + wait_and_assert_operationid_status(self.nodes[1], opid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # The account should see the remaining funds, and they should have been + # sent to the Sapling change address (because NU5 is not active). + assert_equal( + self.nodes[1].z_getbalanceforaccount(n1account)['pools'], + {'sapling': {'valueZat': 1 * COIN}}, + ) + + # The addresses should both show the same balance, as they both show the + # Sapling balance. + assert_equal(self.nodes[1].z_getbalance(n1ua0), 1) + assert_equal(self.nodes[1].z_getbalance(n1ua1), 1) + + # + # Test NoPrivacy policy + # + + # Send some legacy transparent funds to n1ua0, creating Sapling outputs. + recipients = [{"address":n1ua0, "amount":10}] + # This requires the AllowRevealedSenders policy... + opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg) + # ... which we can always override with the NoPrivacy policy. + opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0, 'NoPrivacy') + wait_and_assert_operationid_status(self.nodes[2], opid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # 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) + # ... 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) + if __name__ == '__main__': WalletZSendmanyTest().main() diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index c2db7d72e..67a602061 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -134,7 +134,6 @@ static const CRPCConvertParam vRPCConvertParams[] = { "z_sendmany", 1}, { "z_sendmany", 2}, { "z_sendmany", 3}, - { "z_sendmany", 4}, { "z_shieldcoinbase", 2}, { "z_shieldcoinbase", 3}, { "z_getoperationstatus", 0}, diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 84f8bf362..8a22cecfe 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -48,11 +48,11 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( ZTXOSelector ztxoSelector, std::vector recipients, int minDepth, + TransactionStrategy strategy, CAmount fee, - bool allowRevealedAmounts, UniValue contextInfo) : builder_(std::move(builder)), ztxoSelector_(ztxoSelector), recipients_(recipients), - mindepth_(minDepth), fee_(fee), allowRevealedAmounts_(allowRevealedAmounts), + mindepth_(minDepth), strategy_(strategy), fee_(fee), contextinfo_(contextInfo) { assert(fee_ >= 0); @@ -78,15 +78,15 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( [&](const libzcash::SaplingPaymentAddress& addr) { txOutputAmounts_.sapling_outputs_total += recipient.amount; recipientPools_.insert(OutputPool::Sapling); - if (!(ztxoSelector_.SelectsSapling() || allowRevealedAmounts_)) { + 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 `allowRevealedAmounts` parameter set to `true` if " - "you wish to allow this transaction to proceed anyway."); + "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( @@ -94,8 +94,8 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( "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 `allowRevealedAmounts` parameter set to `true` if " - "you wish to allow this transaction to proceed anyway."); + "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. @@ -106,15 +106,15 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( recipientPools_.insert(OutputPool::Orchard); // No transaction allows sends from Sprout to Orchard. assert(!ztxoSelector_.SelectsSprout()); - if (!((builder_.SupportsOrchard() && ztxoSelector_.SelectsOrchard()) || allowRevealedAmounts_)) { + 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 `allowRevealedAmounts` parameter set to `true` if " - "you wish to allow this transaction to proceed anyway."); + "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. @@ -123,6 +123,16 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( }, 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()); @@ -247,6 +257,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { FormatMoney(changeAmount), FormatMoney(dustThreshold))); } else { + bool isFromUa = std::holds_alternative(ztxoSelector_.GetPattern()); throw JSONRPCError( RPC_WALLET_INSUFFICIENT_FUNDS, strprintf( @@ -255,10 +266,27 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { + (allowTransparentCoinbase ? "" : "; note that coinbase outputs will not be selected if you specify " "ANY_TADDR or if any transparent recipients are included.") + + ((!isFromUa || strategy_.AllowLinkingAccountAddresses()) ? "" : + " (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.)") ); } } + if (!(spendable.utxos.empty() || strategy_.AllowRevealedSenders())) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "This transaction requires selecting transparent coins, which is " + "not enabled by default because it will publicly reveal transaction " + "senders and amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit " + "with the `privacyPolicy` parameter set to `AllowRevealedSenders` " + "or weaker if you wish to allow this transaction to proceed anyway."); + } + spendable.LogInputs(getId()); CAmount t_inputs_total{0}; diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index 5ca1f4b02..92b82d955 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -48,8 +48,8 @@ public: ZTXOSelector ztxoSelector, std::vector recipients, int minDepth, + TransactionStrategy strategy, CAmount fee = DEFAULT_FEE, - bool allowRevealedAmounts = false, UniValue contextInfo = NullUniValue); virtual ~AsyncRPCOperation_sendmany(); @@ -77,7 +77,7 @@ private: bool isfromsprout_{false}; bool isfromsapling_{false}; - bool allowRevealedAmounts_{false}; + TransactionStrategy strategy_; uint32_t transparentRecipients_{0}; AccountId sendFromAccount_; std::set recipientPools_; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f23a1b986..3db60c63a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4471,7 +4471,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) if (fHelp || params.size() < 2 || params.size() > 5) throw runtime_error( - "z_sendmany \"fromaddress\" [{\"address\":... ,\"amount\":...},...] ( minconf ) ( fee ) ( allowRevealedAmounts )\n" + "z_sendmany \"fromaddress\" [{\"address\":... ,\"amount\":...},...] ( minconf ) ( fee ) ( privacyPolicy )\n" "\nSend multiple times. Amounts are decimal numbers with at most 8 digits of precision." "\nChange generated from one or more transparent addresses flows to a new transparent" "\naddress, while change generated from a shielded address returns to itself." @@ -4493,7 +4493,26 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) " }, ... ]\n" "3. minconf (numeric, optional, default=1) Only use funds confirmed at least this many times.\n" "4. fee (numeric, optional, default=" + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n" - "5. allowRevealedAmounts (bool, optional, default=false) Permit cross-shielded-pool transfers, which will publicly reveal the amount(s) crossing pool boundaries.\n" + "5. privacyPolicy (string, optional, default=\"LegacyCompat\") Policy for what information leakage is acceptable.\n" + " One of the following strings:\n" + " - \"FullPrivacy\": Only allow fully-shielded transactions (involving a single shielded pool).\n" + " - \"LegacyCompat\": If the transaction involves any Unified Addressess, this is equivalent to\n" + " \"FullPrivacy\". Otherwise, this is equivalent to \"AllowFullyTransparent\".\n" + " - \"AllowRevealedAmounts\": Allow funds to cross between shielded pools, revealing the amount\n" + " that crosses pools.\n" + " - \"AllowRevealedRecipients\": Allow transparent recipients. This also implies revealing\n" + " information described under \"AllowRevealedAmounts\".\n" + " - \"AllowRevealedSenders\": Allow transparent funds to be spent, revealing the sending\n" + " addresses and amounts. This implies revealing information described under \"AllowRevealedAmounts\".\n" + " - \"AllowFullyTransparent\": Allow transaction to both spend transparent funds and have\n" + " transparent recipients. This implies revealing information described under \"AllowRevealedSenders\"\n" + " and \"AllowRevealedRecipients\".\n" + " - \"AllowLinkingAccountAddresses\": Allow selecting transparent coins from the full account,\n" + " rather than just the funds sent to the transparent receiver in the provided Unified Address.\n" + " This implies revealing information described under \"AllowRevealedSenders\".\n" + " - \"NoPrivacy\": Allow the transaction to reveal any information necessary to create it.\n" + " This implies revealing information described under \"AllowFullyTransparent\" and\n" + " \"AllowLinkingAccountAddresses\".\n" "\nResult:\n" "\"operationid\" (string) An operationid to pass to z_getoperationstatus to get the result of the operation.\n" "\nExamples:\n" @@ -4515,6 +4534,28 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) KeyIO keyIO(chainparams); + // We need to know the privacy policy before we construct the ZTXOSelector, + // but we can't determine the default privacy policy without knowing whether + // any UAs are involved. We break this cycle by parsing the privacy policy + // argument first, and then resolving it to the default after parsing the + // rest of the arguments. This works because all possible defaults for the + // privacy policy have the same effect on ZTXOSelector construction (in that + // they don't include AllowLinkingAccountAddresses). + std::optional maybeStrategy; + if (params.size() > 4) { + auto strategyName = params[4].get_str(); + if (strategyName != "LegacyCompat") { + maybeStrategy = TransactionStrategy::FromString(strategyName); + if (!maybeStrategy.has_value()) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + strprintf("Unknown privacy policy name '%s'", strategyName)); + } + } + } + + bool involvesUnifiedAddress = false; + // Check that the from address is valid. // Unified address (UA) allowed here (#5185) auto fromaddress = params[0].get_str(); @@ -4529,7 +4570,11 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) "Invalid from address: should be a taddr, zaddr, UA, or the string 'ANY_TADDR'."); } - auto ztxoSelectorOpt = pwalletMain->ZTXOSelectorForAddress(decoded.value(), true, false); + auto ztxoSelectorOpt = pwalletMain->ZTXOSelectorForAddress( + decoded.value(), + true, + // LegacyCompat does not include AllowLinkingAccountAddresses. + maybeStrategy.has_value() ? maybeStrategy.value().AllowLinkingAccountAddresses() : false); if (!ztxoSelectorOpt.has_value()) { throw JSONRPCError( RPC_INVALID_ADDRESS_OR_KEY, @@ -4544,6 +4589,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) RPC_INVALID_ADDRESS_OR_KEY, "Invalid from address, UA does not correspond to a known account."); } + involvesUnifiedAddress = true; }, [&](const auto& other) { if (selectorAccount.has_value() && selectorAccount.value() != ZCASH_LEGACY_ACCOUNT) { @@ -4628,6 +4674,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) std::optional ua = std::nullopt; if (std::holds_alternative(decoded.value())) { ua = std::get(decoded.value()); + involvesUnifiedAddress = true; } recipients.push_back(SendManyRecipient(ua, addr.value(), nAmount, memo)); @@ -4637,6 +4684,15 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "No recipients"); } + // Now that we've set involvesUnifiedAddress correctly, we can finish + // evaluating the strategy. + TransactionStrategy strategy = maybeStrategy.value_or( + // Default privacy policy is "LegacyCompat". + involvesUnifiedAddress ? + TransactionStrategy(PrivacyPolicy::FullPrivacy) : + TransactionStrategy(PrivacyPolicy::AllowFullyTransparent) + ); + // Sanity check for transaction size // TODO: move this to the builder? auto txsize = EstimateTxSize(ztxoSelector, recipients, nextBlockHeight); @@ -4683,11 +4739,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } } - bool allowRevealedAmounts{false}; - if (params.size() > 4) { - allowRevealedAmounts = params[4].get_bool(); - } - // Use input parameters as the optional context info to be returned by z_getoperationstatus and z_getoperationresult. UniValue o(UniValue::VOBJ); o.pushKV("fromaddress", params[0]); @@ -4710,7 +4761,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) std::shared_ptr q = getAsyncRPCQueue(); std::shared_ptr operation( new AsyncRPCOperation_sendmany( - std::move(builder), ztxoSelector, recipients, nMinDepth, nFee, allowRevealedAmounts, contextInfo) + std::move(builder), ztxoSelector, recipients, nMinDepth, strategy, nFee, contextInfo) ); q->addOperation(operation); AsyncRPCOperationId operationId = operation->getId(); diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index ac61ac966..b2bb27b9e 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/src/wallet/test/rpc_wallet_tests.cpp @@ -1236,7 +1236,8 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) auto selector = pwalletMain->ZTXOSelectorForAddress(taddr1, true, false).value(); TransactionBuilder builder(consensusParams, nHeight + 1, std::nullopt, pwalletMain); std::vector recipients = { SendManyRecipient(std::nullopt, zaddr1, 100*COIN, "DEADBEEF") }; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1)); + TransactionStrategy strategy; + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, strategy)); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); @@ -1248,7 +1249,8 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) auto selector = pwalletMain->ZTXOSelectorForAddress(zaddr1, true, false).value(); TransactionBuilder builder(consensusParams, nHeight + 1, std::nullopt, pwalletMain); std::vector recipients = { SendManyRecipient(std::nullopt, taddr1, 100*COIN, "DEADBEEF") }; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1)); + TransactionStrategy strategy(PrivacyPolicy::AllowRevealedRecipients); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, strategy)); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); @@ -1260,7 +1262,8 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) auto selector = pwalletMain->ZTXOSelectorForAddress(zaddr1, true, false).value(); TransactionBuilder builder(consensusParams, nHeight + 1, std::nullopt, pwalletMain); std::vector recipients = { SendManyRecipient(std::nullopt, zaddr1, 100*COIN, "DEADBEEF") }; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1)); + TransactionStrategy strategy; + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, strategy)); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); @@ -1360,7 +1363,8 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) auto selector = pwalletMain->ZTXOSelectorForAddress(taddr, true, false).value(); std::vector recipients = { SendManyRecipient(std::nullopt, pa, 1*COIN, "ABCD") }; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 0)); + TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 0, strategy)); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); // Enable test mode so tx is not sent @@ -1368,7 +1372,9 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) // Generate the Sapling shielding transaction operation->main(); - BOOST_CHECK(operation->isSuccess()); + if (!operation->isSuccess()) { + BOOST_FAIL(operation->getErrorMessage()); + } // Get the transaction auto result = operation->getResult(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3740c7c7f..5c01d1792 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -7092,6 +7092,99 @@ std::optional UnifiedAddressForReceiver::operator()(co return std::nullopt; } +std::optional TransactionStrategy::FromString(std::string privacyPolicy) { + TransactionStrategy strategy; + + if (privacyPolicy == "FullPrivacy") { + strategy.privacy = PrivacyPolicy::FullPrivacy; + } else if (privacyPolicy == "AllowRevealedAmounts") { + strategy.privacy = PrivacyPolicy::AllowRevealedAmounts; + } else if (privacyPolicy == "AllowRevealedRecipients") { + strategy.privacy = PrivacyPolicy::AllowRevealedRecipients; + } else if (privacyPolicy == "AllowRevealedSenders") { + strategy.privacy = PrivacyPolicy::AllowRevealedSenders; + } else if (privacyPolicy == "AllowFullyTransparent") { + strategy.privacy = PrivacyPolicy::AllowFullyTransparent; + } else if (privacyPolicy == "AllowLinkingAccountAddresses") { + strategy.privacy = PrivacyPolicy::AllowLinkingAccountAddresses; + } else if (privacyPolicy == "NoPrivacy") { + strategy.privacy = PrivacyPolicy::NoPrivacy; + } else { + // Unknown privacy policy. + return std::nullopt; + } + + return strategy; +} + +bool TransactionStrategy::AllowRevealedAmounts() { + switch (privacy) { + case PrivacyPolicy::FullPrivacy: + return false; + case PrivacyPolicy::AllowRevealedAmounts: + case PrivacyPolicy::AllowRevealedRecipients: + case PrivacyPolicy::AllowRevealedSenders: + case PrivacyPolicy::AllowFullyTransparent: + case PrivacyPolicy::AllowLinkingAccountAddresses: + case PrivacyPolicy::NoPrivacy: + return true; + default: + // Fail closed. + return false; + } +} + +bool TransactionStrategy::AllowRevealedRecipients() { + switch (privacy) { + case PrivacyPolicy::FullPrivacy: + case PrivacyPolicy::AllowRevealedAmounts: + case PrivacyPolicy::AllowRevealedSenders: + case PrivacyPolicy::AllowLinkingAccountAddresses: + return false; + case PrivacyPolicy::AllowRevealedRecipients: + case PrivacyPolicy::AllowFullyTransparent: + case PrivacyPolicy::NoPrivacy: + return true; + default: + // Fail closed. + return false; + } +} + +bool TransactionStrategy::AllowRevealedSenders() { + switch (privacy) { + case PrivacyPolicy::FullPrivacy: + case PrivacyPolicy::AllowRevealedAmounts: + case PrivacyPolicy::AllowRevealedRecipients: + return false; + case PrivacyPolicy::AllowRevealedSenders: + case PrivacyPolicy::AllowFullyTransparent: + case PrivacyPolicy::AllowLinkingAccountAddresses: + case PrivacyPolicy::NoPrivacy: + return true; + default: + // Fail closed. + return false; + } +} + +bool TransactionStrategy::AllowLinkingAccountAddresses() { + switch (privacy) { + case PrivacyPolicy::FullPrivacy: + case PrivacyPolicy::AllowRevealedAmounts: + case PrivacyPolicy::AllowRevealedRecipients: + case PrivacyPolicy::AllowRevealedSenders: + case PrivacyPolicy::AllowFullyTransparent: + return false; + case PrivacyPolicy::AllowLinkingAccountAddresses: + case PrivacyPolicy::NoPrivacy: + return true; + default: + // Fail closed. + return false; + } +} + bool ZTXOSelector::SelectsTransparent() const { return std::visit(match { [](const CKeyID& keyId) { return true; }, diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a0553ef1c..27c297ee0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -715,6 +715,35 @@ public: std::string ToString() const; }; +/** + * A strategy to use for managing privacy when constructing a transaction. + */ +enum class PrivacyPolicy { + FullPrivacy, + AllowRevealedAmounts, + AllowRevealedRecipients, + AllowRevealedSenders, + AllowFullyTransparent, + AllowLinkingAccountAddresses, + NoPrivacy, +}; + +class TransactionStrategy { + PrivacyPolicy privacy; + +public: + TransactionStrategy() : privacy(PrivacyPolicy::FullPrivacy) {} + TransactionStrategy(const TransactionStrategy& strategy) : privacy(strategy.privacy) {} + TransactionStrategy(PrivacyPolicy privacyPolicy) : privacy(privacyPolicy) {} + + static std::optional FromString(std::string privacyPolicy); + + bool AllowRevealedAmounts(); + bool AllowRevealedRecipients(); + bool AllowRevealedSenders(); + bool AllowLinkingAccountAddresses(); +}; + /** * A class representing the ZIP 316 unified spending authority associated with * a ZIP 32 account and this wallet's mnemonic seed. This is intended to be