diff --git a/qa/rpc-tests/wallet_orchard.py b/qa/rpc-tests/wallet_orchard.py index f5a66918e..218c5e57b 100755 --- a/qa/rpc-tests/wallet_orchard.py +++ b/qa/rpc-tests/wallet_orchard.py @@ -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() diff --git a/qa/rpc-tests/wallet_orchard_change.py b/qa/rpc-tests/wallet_orchard_change.py index c329778c6..bded74614 100755 --- a/qa/rpc-tests/wallet_orchard_change.py +++ b/qa/rpc-tests/wallet_orchard_change.py @@ -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() diff --git a/qa/rpc-tests/wallet_unified_change.py b/qa/rpc-tests/wallet_unified_change.py index 375c7c422..1384d642d 100755 --- a/qa/rpc-tests/wallet_unified_change.py +++ b/qa/rpc-tests/wallet_unified_change.py @@ -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())) 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/coins.cpp b/src/coins.cpp index 4ed2907af..eae717c7b 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -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; } diff --git a/src/main.h b/src/main.h index 9335ed5ff..62e11a248 100644 --- a/src/main.h +++ b/src/main.h @@ -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; diff --git a/src/miner.cpp b/src/miner.cpp index a04183577..b17980713 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -64,9 +64,10 @@ public: const CTransaction* ptx; set 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 last_block_num_txs; std::optional last_block_size; // We want to sort transactions by priority and fee rate, so: -typedef boost::tuple TxPriority; +typedef boost::tuple 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); } } diff --git a/src/txmempool.h b/src/txmempool.h index 4b6962dfe..2337bff4b 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -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; } 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};