zecfoundation
/
zcashd
mirror of https://github.com/ZcashFoundation/zcashd.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Kris Nuttycombe 2022-04-06 14:38:08 -06:00
parent a86220ee62
commit 51defe9f96
3 changed files with 130 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
qa/rpc-tests/wallet_unified_change.py
View File

@ -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'))

93
qa/rpc-tests/wallet_z_sendmany.py
View File

@ -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()

97
src/wallet/asyncrpcoperation_sendmany.cpp
View File

@ -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};