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

Merge pull request #5824 from nuttycom/bug/change_reveals_amounts 

Fix condition where change outputs could violate z_sendmany privacy policy.
This commit is contained in:
Kris Nuttycombe 2022-04-06 14:57:08 -06:00 committed by GitHub
parent 834162a25e e6d498e9c9
commit cc892fd48d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 151 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
qa/pull-tester/rpc-tests.py
View File

@ -82,6 +82,7 @@ BASE_SCRIPTS= [
'wallet_sapling.py',
'wallet_sendmany_any_taddr.py',
'wallet_treestate.py',
'wallet_unified_change.py',
'listtransactions.py',
'mempool_resurrect_test.py',
'txn_doublespend.py',

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

@ -139,7 +139,7 @@ class TurnstileTest (BitcoinTestFramework):
assert_equal(block["height"], count + 1)
# Stop node 0 and check logs to verify the miner excluded the transaction from the block
string_to_find = "CreateNewBlock(): tx " + mytxid + " appears to violate " + POOL_NAME.capitalize() + " turnstile"
string_to_find = "CreateNewBlock: tx " + mytxid + " appears to violate " + POOL_NAME.capitalize() + " turnstile"
check_node_log(self, 0, string_to_find)
# Launch node 0 with in-memory size of value pools set to zero.

114
qa/rpc-tests/wallet_unified_change.py Executable file
View File

@ -0,0 +1,114 @@
#!/usr/bin/env python3
# Copyright (c) 2022 The Zcash developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://www.opensource.org/licenses/mit-license.php .
from decimal import Decimal
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
NU5_BRANCH_ID,
assert_equal,
get_coinbase_address,
nuparams,
start_nodes,
wait_and_assert_operationid_status,
)
# Test wallet accounts behaviour
class WalletUnifiedChangeTest(BitcoinTestFramework):
def __init__(self):
super().__init__()
self.num_nodes = 4
def setup_nodes(self):
return start_nodes(self.num_nodes, self.options.tmpdir, [[
nuparams(NU5_BRANCH_ID, 201),
]] * self.num_nodes)
def run_test(self):
# Activate Nu5
self.sync_all()
self.nodes[0].generate(1)
self.sync_all()
account0 = self.nodes[0].z_getnewaccount()['account']
ua0_sapling = self.nodes[0].z_getaddressforaccount(account0, ['sapling'])['address']
ua0_orchard = self.nodes[0].z_getaddressforaccount(account0, ['orchard'])['address']
account1 = self.nodes[1].z_getnewaccount()['account']
ua1_sapling = self.nodes[1].z_getaddressforaccount(account1, ['sapling'])['address']
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')
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')
wait_and_assert_operationid_status(self.nodes[0], opid)
self.sync_all()
self.nodes[0].generate(1)
self.sync_all()
assert_equal(
{'pools': {'sapling': {'valueZat': 1000000000}, 'orchard': {'valueZat': 1000000000}}, '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.
recipients = [{'address': ua1_sapling, 'amount': 5}]
opid = self.nodes[0].z_sendmany(ua0_sapling, recipients, 1, Decimal('0.00001'))
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'))
txid_orchard = wait_and_assert_operationid_status(self.nodes[0], opid)
assert_equal(set([txid_sapling, txid_orchard]), set(self.nodes[0].getrawmempool()))
self.sync_all()
self.nodes[0].generate(1)
self.sync_all()
assert_equal([], self.nodes[0].getrawmempool())
assert_equal(1, self.nodes[0].gettransaction(txid_orchard)['confirmations'])
assert_equal(1, self.nodes[0].gettransaction(txid_sapling)['confirmations'])
assert_equal(
{'pools': {'sapling': {'valueZat': 500000000}, 'orchard': {'valueZat': 500000000}}, 'minimum_confirmations': 1},
self.nodes[1].z_getbalanceforaccount(account1))
# Now send sapling->sapling, generating change.
recipients = [{'address': ua0_sapling, 'amount': Decimal('2.5')}]
opid = self.nodes[1].z_sendmany(ua1_sapling, recipients, 1, 0)
txid_sapling = wait_and_assert_operationid_status(self.nodes[1], opid)
self.sync_all()
self.nodes[1].generate(1)
self.sync_all()
# Since this is entirely sapling->sapling, change should be returned
# to the Sapling pool.
assert_equal(
{'pools': {'sapling': {'valueZat': 250000000}, 'orchard': {'valueZat': 500000000}}, 'minimum_confirmations': 1},
self.nodes[1].z_getbalanceforaccount(account1))
# If we send from an unrestricted UA, change should still not cross
# the pool boundary, since we can build a purely sapling->sapling tx.
recipients = [{'address': ua0_sapling, 'amount': Decimal('1.25')}]
opid = self.nodes[1].z_sendmany(ua1, recipients, 1, 0)
txid_sapling = 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': 125000000}, 'orchard': {'valueZat': 500000000}}, 'minimum_confirmations': 1},
self.nodes[1].z_getbalanceforaccount(account1))
if __name__ == '__main__':
WalletUnifiedChangeTest().main()

40
src/miner.cpp
View File

@ -444,6 +444,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
// Read prev transaction
if (!view.HaveCoins(txin.prevout.hash))
{
LogPrintf("INFO: missing coins for %s", txin.prevout.hash.GetHex());
// This should never happen; all transactions in the memory
// pool should connect to either transactions in the chain
// or other transactions in the memory pool.
@ -539,33 +540,40 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
}
}
LogPrintf("%s: Evaluating %u transactions for inclusion in block.", __func__, vecPriority.size());
while (!vecPriority.empty())
{
// 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>());
const uint256& hash = tx.GetHash();
std::pop_heap(vecPriority.begin(), vecPriority.end(), comparer);
vecPriority.pop_back();
// Size limits
unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
if (nBlockSize + nTxSize >= nBlockMaxSize)
if (nBlockSize + nTxSize >= nBlockMaxSize) {
LogPrintf("%s: skipping tx %s: exceeded maximum block size %u.", __func__, hash.GetHex(), nBlockMaxSize);
continue;
}
// Legacy limits on sigOps:
unsigned int nTxSigOps = GetLegacySigOpCount(tx);
if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS)
if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) {
LogPrintf("%s: skipping tx %s: exceeds legacy max sigops %u.", __func__, hash.GetHex(), MAX_BLOCK_SIGOPS);
continue;
}
// Skip free transactions if we're past the minimum block size:
const uint256& hash = tx.GetHash();
double dPriorityDelta = 0;
CAmount nFeeDelta = 0;
mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta);
if (fSortedByFee && (dPriorityDelta <= 0) && (nFeeDelta <= 0) && (feeRate < ::minRelayTxFee) && (nBlockSize + nTxSize >= nBlockMinSize))
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());
continue;
}
// Prioritise by fee once past the priority size or we run out of high-priority
// transactions:
@ -577,14 +585,18 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
std::make_heap(vecPriority.begin(), vecPriority.end(), comparer);
}
if (!view.HaveInputs(tx))
if (!view.HaveInputs(tx)) {
LogPrintf("%s: not including tx %s; missing inputs.", __func__, hash.GetHex());
continue;
}
CAmount nTxFees = view.GetValueIn(tx)-tx.GetValueOut();
nTxSigOps += GetP2SHSigOpCount(tx, view);
if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS)
if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) {
LogPrintf("%s: skipping tx %s: exceeds p2sh max sigops %u.", __func__, hash.GetHex(), MAX_BLOCK_SIGOPS);
continue;
}
std::vector<CTxOut> allPrevOutputs;
for (const auto& input : tx.vin) {
@ -596,8 +608,10 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
// create only contains transactions that are valid in new blocks.
CValidationState state;
PrecomputedTransactionData txdata(tx, allPrevOutputs);
if (!ContextualCheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, txdata, chainparams.GetConsensus(), consensusBranchId))
if (!ContextualCheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, txdata, chainparams.GetConsensus(), consensusBranchId)) {
LogPrintf("%s: skipping tx %s: Failed contextual inputs check.", __func__, hash.GetHex());
continue;
}
if (chainparams.ZIP209Enabled() && monitoring_pool_balances) {
// Does this transaction lead to a turnstile violation?
@ -615,15 +629,15 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
}
if (sproutValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sprout turnstile\n", tx.GetHash().ToString());
LogPrintf("%s: tx %s appears to violate Sprout turnstile\n", __func__, hash.GetHex());
continue;
}
if (saplingValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sapling turnstile\n", tx.GetHash().ToString());
LogPrintf("%s: tx %s appears to violate Sapling turnstile\n", __func__, hash.GetHex());
continue;
}
if (orchardValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Orchard turnstile\n", tx.GetHash().ToString());
LogPrintf("%s: tx %s appears to violate Orchard turnstile\n", __func__, hash.GetHex());
continue;
}
@ -645,8 +659,8 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
if (fPrintPriority)
{
LogPrintf("priority %.1f fee %s txid %s\n",
dPriority, feeRate.ToString(), tx.GetHash().ToString());
LogPrintf("%s: priority %.1f fee %s txid %s\n",
__func__, dPriority, feeRate.ToString(), hash.GetHex());
}
// Add transactions that depend on this one to the priority queue
@ -669,7 +683,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
last_block_num_txs = nBlockTx;
last_block_size = nBlockSize;
LogPrintf("CreateNewBlock(): total size %u\n", nBlockSize);
LogPrintf("%s: total tx: %u; total size: %u (excluding coinbase)", __func__, nBlockTx, nBlockSize);
// Create coinbase tx
if (next_cb_mtx) {

11
src/wallet/asyncrpcoperation_sendmany.cpp
View File

@ -352,13 +352,18 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
switch (rtype) {
case ReceiverType::P2PKH:
case ReceiverType::P2SH:
allowedChangeTypes.insert(OutputPool::Transparent);
if (!spendable.utxos.empty() || strategy_.AllowRevealedRecipients()) {
allowedChangeTypes.insert(OutputPool::Transparent);
}
break;
case ReceiverType::Sapling:
allowedChangeTypes.insert(OutputPool::Sapling);
if (!spendable.saplingNoteEntries.empty() || strategy_.AllowRevealedAmounts()) {
allowedChangeTypes.insert(OutputPool::Sapling);
}
break;
case ReceiverType::Orchard:
if (builder_.SupportsOrchard()) {
if (builder_.SupportsOrchard() &&
(!spendable.orchardNoteMetadata.empty() || strategy_.AllowRevealedAmounts())) {
allowedChangeTypes.insert(OutputPool::Orchard);
}
break;