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

Merge pull request #5834 from nuttycom/bug/sendmany_privacy_policy_violations 

Fix privacy policy violations when sending between unified addresses.
This commit is contained in:
str4d 2022-04-08 04:00:31 +01:00 committed by GitHub
parent 5f60f33daf cb64997216
commit 0c32c988fc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 171 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -103,7 +103,7 @@ class WalletOrchardTest(BitcoinTestFramework):
ua3 = addrRes3['address']
recipients = [{"address": ua3, "amount": Decimal('1')}]
myopid = self.nodes[2].z_sendmany(ua2, recipients, 1, 0)
myopid = self.nodes[2].z_sendmany(ua2, recipients, 1, 0, 'AllowRevealedAmounts')
rollback_tx = wait_and_assert_operationid_status(self.nodes[2], myopid)
self.sync_all()

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

@ -65,7 +65,7 @@ class WalletOrchardChangeTest(BitcoinTestFramework):
ua1 = self.nodes[1].z_getaddressforaccount(acct1, ['orchard'])['address']
recipients = [{"address": ua1, "amount": 1}]
myopid = self.nodes[0].z_sendmany(ua0, recipients, 1, 0)
myopid = self.nodes[0].z_sendmany(ua0, recipients, 1, 0, 'AllowRevealedAmounts')
source_tx = wait_and_assert_operationid_status(self.nodes[0], myopid)
self.sync_all()

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

@ -6,6 +6,7 @@
from decimal import Decimal
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
DEFAULT_FEE,
NU5_BRANCH_ID,
assert_equal,
get_coinbase_address,
@ -40,12 +41,12 @@ class WalletUnifiedChangeTest(BitcoinTestFramework):
ua1 = self.nodes[1].z_getaddressforaccount(account1)['address']
# Fund both of ua0_sapling and ua0_orchard
recipients = [{'address': ua0_sapling, 'amount': 10}]
opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders')
recipients = [{'address': ua0_sapling, 'amount': Decimal('9.99999000')}]
opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, DEFAULT_FEE, 'AllowRevealedSenders')
wait_and_assert_operationid_status(self.nodes[0], opid)
recipients = [{'address': ua0_orchard, 'amount': 10}]
opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0, 'AllowRevealedSenders')
recipients = [{'address': ua0_orchard, 'amount': Decimal('9.99999000')}]
opid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, DEFAULT_FEE, 'AllowRevealedSenders')
wait_and_assert_operationid_status(self.nodes[0], opid)
self.sync_all()
@ -53,18 +54,18 @@ class WalletUnifiedChangeTest(BitcoinTestFramework):
self.sync_all()
assert_equal(
{'pools': {'sapling': {'valueZat': 1000000000}, 'orchard': {'valueZat': 1000000000}}, 'minimum_confirmations': 1},
{'pools': {'sapling': {'valueZat': 999999000}, 'orchard': {'valueZat': 999999000}}, 'minimum_confirmations': 1},
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'))
opid = self.nodes[0].z_sendmany(ua0_sapling, recipients, 1, DEFAULT_FEE)
txid_sapling = wait_and_assert_operationid_status(self.nodes[0], opid)
recipients = [{'address': ua1, 'amount': 5}]
opid = self.nodes[0].z_sendmany(ua0_orchard, recipients, 1, Decimal('0.00001'))
opid = self.nodes[0].z_sendmany(ua0_orchard, recipients, 1, DEFAULT_FEE)
txid_orchard = wait_and_assert_operationid_status(self.nodes[0], opid)
assert_equal(set([txid_sapling, txid_orchard]), set(self.nodes[0].getrawmempool()))

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

6
src/coins.cpp
View File

@ -1115,7 +1115,11 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
// use the maximum priority for all (partially or fully) shielded transactions.
// (Note that coinbase transactions cannot contain JoinSplits, or Sapling shielded Spends or Outputs.)
if (tx.vJoinSplit.size() > 0 || tx.vShieldedSpend.size() > 0 || tx.vShieldedOutput.size() > 0) {
if (tx.vJoinSplit.size() > 0 ||
tx.vShieldedSpend.size() > 0 ||
tx.vShieldedOutput.size() > 0 ||
tx.GetOrchardBundle().IsPresent())
{
return MAX_PRIORITY;
}

2
src/main.h
View File

@ -66,7 +66,7 @@ static const unsigned int MAX_REORG_LENGTH = COINBASE_MATURITY - 1;
static const bool DEFAULT_WHITELISTRELAY = true;
/** Default for DEFAULT_WHITELISTFORCERELAY. */
static const bool DEFAULT_WHITELISTFORCERELAY = true;
/** Default for -minrelaytxfee, minimum relay fee for transactions */
/** Default for -minrelaytxfee, minimum relay fee for transactions in zatoshis/kB */
static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 100;
//! -maxtxfee default
static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN;

29
src/miner.cpp
View File

@ -64,9 +64,10 @@ public:
const CTransaction* ptx;
set<uint256> setDependsOn;
CFeeRate feeRate;
CAmount feePaid;
double dPriority;
COrphan(const CTransaction* ptxIn) : ptx(ptxIn), feeRate(0), dPriority(0)
COrphan(const CTransaction* ptxIn) : ptx(ptxIn), feeRate(0), feePaid(0), dPriority(0)
{
}
};
@ -75,7 +76,7 @@ std::optional<uint64_t> last_block_num_txs;
std::optional<uint64_t> last_block_size;
// We want to sort transactions by priority and fee rate, so:
typedef boost::tuple<double, CFeeRate, const CTransaction*> TxPriority;
typedef boost::tuple<double, CFeeRate, CAmount, const CTransaction*> TxPriority;
class TxPriorityCompare
{
bool byFee;
@ -491,15 +492,17 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
uint256 hash = tx.GetHash();
mempool.ApplyDeltas(hash, dPriority, nTotalIn);
CFeeRate feeRate(nTotalIn-tx.GetValueOut(), nTxSize);
CAmount feePaid = nTotalIn - tx.GetValueOut();
CFeeRate feeRate(feePaid, nTxSize);
if (porphan)
{
porphan->dPriority = dPriority;
porphan->feeRate = feeRate;
porphan->feePaid = feePaid;
}
else
vecPriority.push_back(TxPriority(dPriority, feeRate, &(mi->GetTx())));
vecPriority.push_back(TxPriority(dPriority, feeRate, feePaid, &(mi->GetTx())));
}
}
@ -546,7 +549,8 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
// Take highest priority transaction off the priority queue:
double dPriority = vecPriority.front().get<0>();
CFeeRate feeRate = vecPriority.front().get<1>();
const CTransaction& tx = *(vecPriority.front().get<2>());
CAmount feePaid = vecPriority.front().get<2>();
const CTransaction& tx = *(vecPriority.front().get<3>());
const uint256& hash = tx.GetHash();
std::pop_heap(vecPriority.begin(), vecPriority.end(), comparer);
@ -570,8 +574,17 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
double dPriorityDelta = 0;
CAmount nFeeDelta = 0;
mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta);
if (fSortedByFee && (dPriorityDelta <= 0) && (nFeeDelta <= 0) && (feeRate < ::minRelayTxFee) && (nBlockSize + nTxSize >= nBlockMinSize)) {
LogPrintf("%s: skipping free tx %s; already have minimum block size.", __func__, hash.GetHex());
if (fSortedByFee &&
(dPriorityDelta <= 0) &&
(nFeeDelta <= 0) &&
(feeRate < ::minRelayTxFee) &&
(feePaid < DEFAULT_FEE) &&
(nBlockSize + nTxSize >= nBlockMinSize))
{
LogPrintf(
"%s: skipping free tx %s (fee is %i; %s) with size %u, current block size is %u & already have minimum block size %u.",
__func__, hash.GetHex(),
feePaid, feeRate.ToString(), nTxSize, nBlockSize, nBlockMinSize);
continue;
}
@ -673,7 +686,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
porphan->setDependsOn.erase(hash);
if (porphan->setDependsOn.empty())
{
vecPriority.push_back(TxPriority(porphan->dPriority, porphan->feeRate, porphan->ptx));
vecPriority.push_back(TxPriority(porphan->dPriority, porphan->feeRate, porphan->feePaid, porphan->ptx));
std::push_heap(vecPriority.begin(), vecPriority.end(), comparer);
}
}

4
src/txmempool.h
View File

@ -27,6 +27,10 @@ class CAutoFile;
inline double AllowFreeThreshold()
{
// 144 is the number of Bitcoin blocks per day. This has not been updated for Zcash,
// as it would make it harder to get shielded transactions into blocks by lowering the
// threshold at which we switch from priority-based selection of transactions into
// blocks to fee-based selection.
return COIN * 144 / 250;
}

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