From c6001268c54c1c42a37ccec37d3b9ee93f13fae9 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 11 Apr 2023 12:55:28 -0600 Subject: [PATCH] Address review feedback re: ZIP 317 in wallet --- qa/rpc-tests/mempool_limit.py | 6 +++--- qa/rpc-tests/mempool_tx_expiry.py | 4 ++-- qa/rpc-tests/test_framework/zip317.py | 17 ++++++++++++++--- qa/rpc-tests/wallet_listreceived.py | 15 +++++++-------- qa/rpc-tests/wallet_nullifiers.py | 4 ++-- qa/rpc-tests/wallet_shieldingcoinbase.py | 18 +++++++++++++----- qa/rpc-tests/wallet_treestate.py | 4 ++-- qa/rpc-tests/wallet_z_sendmany.py | 8 ++++---- src/wallet/asyncrpcoperation_common.cpp | 4 +++- src/wallet/rpcwallet.cpp | 9 ++------- src/wallet/wallet_tx_builder.cpp | 7 +++---- 11 files changed, 55 insertions(+), 41 deletions(-) diff --git a/qa/rpc-tests/mempool_limit.py b/qa/rpc-tests/mempool_limit.py index adc2422ba..466444006 100755 --- a/qa/rpc-tests/mempool_limit.py +++ b/qa/rpc-tests/mempool_limit.py @@ -13,7 +13,7 @@ from test_framework.util import ( wait_and_assert_operationid_status, DEFAULT_FEE ) -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee from decimal import Decimal from time import sleep @@ -106,9 +106,9 @@ class MempoolLimit(BitcoinTestFramework): print("Checking mempool size reset after block mined...") self.check_mempool_sizes(0) zaddr4 = self.nodes[0].z_getnewaddress('sapling') - opid4 = self.nodes[0].z_sendmany(zaddr1, [{"address": zaddr4, "amount": Decimal('10.0') - 2*compute_conventional_fee(2)}], 1) + opid4 = self.nodes[0].z_sendmany(zaddr1, [{"address": zaddr4, "amount": Decimal('10.0') - 2*conventional_fee(2)}], 1) wait_and_assert_operationid_status(self.nodes[0], opid4) - opid5 = self.nodes[0].z_sendmany(zaddr2, [{"address": zaddr4, "amount": Decimal('10.0') - 2*compute_conventional_fee(2)}], 1) + opid5 = self.nodes[0].z_sendmany(zaddr2, [{"address": zaddr4, "amount": Decimal('10.0') - 2*conventional_fee(2)}], 1) wait_and_assert_operationid_status(self.nodes[0], opid5) self.sync_all() diff --git a/qa/rpc-tests/mempool_tx_expiry.py b/qa/rpc-tests/mempool_tx_expiry.py index 0215d9e3c..a5d77ef99 100755 --- a/qa/rpc-tests/mempool_tx_expiry.py +++ b/qa/rpc-tests/mempool_tx_expiry.py @@ -12,7 +12,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, \ connect_nodes_bi, sync_blocks, start_nodes, \ wait_and_assert_operationid_status, DEFAULT_FEE -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee from decimal import Decimal @@ -94,7 +94,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework): # Create transactions blockheight = self.nodes[0].getblockchaininfo()['blocks'] - zsendamount = Decimal('1.0') - compute_conventional_fee(2) + zsendamount = Decimal('1.0') - conventional_fee(2) recipients = [] recipients.append({"address": z_bob, "amount": zsendamount}) myopid = self.nodes[0].z_sendmany(z_alice, recipients, 1) diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py index 5fc045826..b5192f7eb 100644 --- a/qa/rpc-tests/test_framework/zip317.py +++ b/qa/rpc-tests/test_framework/zip317.py @@ -6,14 +6,25 @@ # # zip317.py # -# Utilities for ZIP 317 conventional fee specification. +# Utilities for ZIP 317 conventional fee specification, as defined in https://zips.z.cash/zip-0317. # from test_framework.mininode import COIN from decimal import Decimal -MARGINAL_FEE = Decimal(5000)/COIN +# The fee per logical action, in ZAT. See https://zips.z.cash/zip-0317#fee-calculation. +MARGINAL_FEE = 5000 + +# The lower bound on the number of logical actions in a tx, for purposes of fee calculation. See +# https://zips.z.cash/zip-0317#fee-calculation. GRACE_ACTIONS = 2 -def compute_conventional_fee(logical_actions): +# The Zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is +# required. +ZIP_317_FEE = -1 + +def conventional_fee_zats(logical_actions): return MARGINAL_FEE * max(GRACE_ACTIONS, logical_actions) + +def conventional_fee(logical_actions): + return Decimal(conventional_fee_zats(logical_actions)) / COIN diff --git a/qa/rpc-tests/wallet_listreceived.py b/qa/rpc-tests/wallet_listreceived.py index 89357ef27..22fcbb63e 100755 --- a/qa/rpc-tests/wallet_listreceived.py +++ b/qa/rpc-tests/wallet_listreceived.py @@ -16,8 +16,7 @@ from test_framework.util import ( NU5_BRANCH_ID, ) from test_framework.util import wait_and_assert_operationid_status, start_nodes -from test_framework.mininode import COIN -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee, conventional_fee_zats from decimal import Decimal my_memo_str = 'c0ffee' # stay awake @@ -158,8 +157,8 @@ class ListReceivedTest (BitcoinTestFramework): outputs = sorted(pt['outputs'], key=lambda x: x['valueZat']) assert_equal(outputs[0]['pool'], 'sapling') assert_equal(outputs[0]['address'], zaddr1) - assert_equal(outputs[0]['value'], Decimal('0.4') - compute_conventional_fee(2)) - assert_equal(outputs[0]['valueZat'], 40000000 - compute_conventional_fee(2)*COIN) + assert_equal(outputs[0]['value'], Decimal('0.4') - conventional_fee(2)) + assert_equal(outputs[0]['valueZat'], 40000000 - conventional_fee_zats(2)) assert_equal(outputs[0]['output'], 1) assert_equal(outputs[0]['outgoing'], False) assert_equal(outputs[0]['memo'], no_memo) @@ -179,8 +178,8 @@ class ListReceivedTest (BitcoinTestFramework): assert_equal(2, len(r), "zaddr1 Should have received 2 notes") r = sorted(r, key = lambda received: received['amount']) assert_equal(txid, r[0]['txid']) - assert_equal(Decimal('0.4')-compute_conventional_fee(2), r[0]['amount']) - assert_equal(40000000-compute_conventional_fee(2)*COIN, r[0]['amountZat']) + assert_equal(Decimal('0.4')-conventional_fee(2), r[0]['amount']) + assert_equal(40000000-conventional_fee_zats(2), r[0]['amountZat']) assert_equal(r[0]['change'], True, "Note valued at (0.4-"+str(DEFAULT_FEE)+") should be change") assert_equal(no_memo, r[0]['memo']) @@ -383,8 +382,8 @@ class ListReceivedTest (BitcoinTestFramework): # Verify that we observe the change output assert_equal(outputs[2]['pool'], 'orchard') - assert_equal(outputs[2]['value'], Decimal('0.5') - compute_conventional_fee(3)) - assert_equal(outputs[2]['valueZat'], 50000000 - compute_conventional_fee(3)*COIN) + assert_equal(outputs[2]['value'], Decimal('0.5') - conventional_fee(3)) + assert_equal(outputs[2]['valueZat'], 50000000 - conventional_fee_zats(3)) assert_equal(outputs[2]['outgoing'], False) assert_equal(outputs[2]['walletInternal'], True) assert_equal(outputs[2]['memo'], no_memo) diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py index e5ade4794..1a16df993 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -7,7 +7,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_true, bitcoind_processes, \ connect_nodes_bi, start_node, start_nodes, wait_and_assert_operationid_status, \ get_coinbase_address, DEFAULT_FEE -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee from decimal import Decimal @@ -93,7 +93,7 @@ class WalletNullifiersTest (BitcoinTestFramework): # check zaddr balance zsendmany2notevalue = Decimal('2.0') - zsendmanyfee = compute_conventional_fee(2) + zsendmanyfee = conventional_fee(2) zaddrremaining = zsendmanynotevalue - zsendmany2notevalue - zsendmanyfee assert_equal(self.nodes[3].z_getbalance(myzaddr3), zsendmany2notevalue) assert_equal(self.nodes[2].z_getbalance(myzaddr), zaddrremaining) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 60fec3326..44a0ffcdf 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -10,7 +10,7 @@ from test_framework.util import assert_equal, initialize_chain_clean, \ start_nodes, connect_nodes_bi, wait_and_assert_operationid_status, \ wait_and_assert_operationid_status_result, get_coinbase_address, \ check_node_log, DEFAULT_FEE -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee, ZIP_317_FEE import sys import timeit @@ -189,7 +189,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # A custom fee of 0 is okay. Here the node will send the note value back to itself. recipients = [] - recipients.append({"address":myzaddr, "amount": Decimal('20.0') - compute_conventional_fee(2)}) + recipients.append({"address":myzaddr, "amount": Decimal('20.0') - conventional_fee(2)}) myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, Decimal('0.0')) mytxid = wait_and_assert_operationid_status(self.nodes[0], myopid) self.sync_all() @@ -231,7 +231,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # UTXO selection in z_sendmany sorts in ascending order, so smallest utxos are consumed first. # At this point in time, unspent notes all have a value of 10.0. recipients = [] - amount = Decimal('10.0') - compute_conventional_fee(2) - Decimal('0.00000001') # this leaves change at 1 zatoshi less than dust threshold + amount = Decimal('10.0') - conventional_fee(2) - Decimal('0.00000001') # this leaves change at 1 zatoshi less than dust threshold recipients.append({"address":self.nodes[0].getnewaddress(), "amount":amount }) myopid = self.nodes[0].z_sendmany(mytaddr, recipients, 1) wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 10.00, need 0.00000053 more to avoid creating invalid change output 0.00000001 (dust threshold is 0.00000054); note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker.") @@ -285,7 +285,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): self.nodes[0].getinfo() # Issue #2263 Workaround END - myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, -1, 'AllowRevealedRecipients') + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, ZIP_317_FEE, 'AllowRevealedRecipients') try: wait_and_assert_operationid_status(self.nodes[0], myopid) except JSONRPCException as e: @@ -301,10 +301,18 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # check balance node2balance = amount_per_recipient * num_t_recipients - saplingvalue -= node2balance + compute_conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change + saplingvalue -= node2balance + conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change assert_equal(self.nodes[2].getbalance(), node2balance) check_value_pool(self.nodes[0], 'sapling', saplingvalue) + # Send will fail because fee is negative + try: + # NB: Using -2 as the fee because -1 is the sentinel for using the conventional fee. + self.nodes[0].z_sendmany(myzaddr, recipients, 1, -2) + except JSONRPCException as e: + errorString = e.error['message'] + assert_equal("Amount out of range" in errorString, True) + # Send will fail because fee is larger than MAX_MONEY try: self.nodes[0].z_sendmany(myzaddr, recipients, 1, Decimal('21000000.00000001')) diff --git a/qa/rpc-tests/wallet_treestate.py b/qa/rpc-tests/wallet_treestate.py index 807ee1896..4fa1a245c 100755 --- a/qa/rpc-tests/wallet_treestate.py +++ b/qa/rpc-tests/wallet_treestate.py @@ -7,7 +7,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, initialize_chain_clean, \ start_nodes, connect_nodes_bi, wait_and_assert_operationid_status, \ get_coinbase_address, DEFAULT_FEE -from test_framework.zip317 import compute_conventional_fee +from test_framework.zip317 import conventional_fee import time from decimal import Decimal @@ -80,7 +80,7 @@ class WalletTreeStateTest (BitcoinTestFramework): # the z_sendmany implementation because there are only two inputs per joinsplit. recipients = [] recipients.append({"address": self.nodes[2].z_getnewaddress(), "amount": Decimal('18.0')}) - recipients.append({"address": self.nodes[2].z_getnewaddress(), "amount": Decimal('12.0') - 4 * compute_conventional_fee(3)}) + recipients.append({"address": self.nodes[2].z_getnewaddress(), "amount": Decimal('12.0') - 4 * conventional_fee(3)}) myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1) # Wait for Tx 2 to begin executing... diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 3b0c269c1..9b78e1364 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -424,15 +424,15 @@ class WalletZSendmanyTest(BitcoinTestFramework): # Test sending Sprout funds to Orchard-only UA # - sproutAddr = self.nodes[0].listaddresses()[0]['sprout']['addresses'][0] + sproutAddr = self.nodes[2].listaddresses()[0]['sprout']['addresses'][0] recipients = [{"address":n0orchard_only, "amount":100}] for (policy, msg) in [ ('FullPrivacy', 'Could not send to a shielded receiver of a unified address without spending funds from a different pool, which would reveal transaction amounts. 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.'), ('AllowRevealedAmounts', 'This transaction would send to a transparent receiver of a unified address, 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.'), - ('AllowRevealedRecipients', 'Could not send to an Orchard-only receiver, despite a lax privacy policy. Either there are insufficient non-Sprout funds (there is no transaction version that supports both Sprout and Orchard), or NU5 has not been activated yet.'), + ('AllowRevealedRecipients', 'Could not send to an Orchard-only receiver despite a lax privacy policy, because you are sending from the Sprout pool and there is no transaction version that supports both Sprout and Orchard.'), ]: - opid = self.nodes[0].z_sendmany(sproutAddr, recipients, 1, 0, policy) - wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', msg) + opid = self.nodes[2].z_sendmany(sproutAddr, recipients, 1, 0, policy) + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', msg) # # Test AllowRevealedAmounts policy diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 11412b46d..30aeee804 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -77,7 +77,9 @@ void ThrowInputSelectionError( strategy.AllowRevealedAmounts() ? strprintf("despite a lax privacy policy, because %s", selector.SelectsSprout() - ? "you are sending from the Sprout pool" + ? "you are sending from the Sprout pool and there is " + "no transaction version that supports both Sprout " + "and Orchard" : "NU5 has not been activated yet") : "without spending non-Orchard funds, which would reveal " "transaction amounts. THIS MAY AFFECT YOUR PRIVACY. Resubmit " diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 582dab962..7acf6fbcc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5030,13 +5030,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Fee in Zatoshis, not currency format) std::optional nFee; - if (params.size() > 3 && params[3].get_real() >= 0.0) { - CAmount fixedFee; - if (params[3].get_real() == 0.0) { - fixedFee = 0; - } else { - fixedFee = AmountFromValue( params[3] ); - } + if (params.size() > 3 && params[3].get_real() != -1.0) { + CAmount fixedFee = AmountFromValue( params[3] ); // Check that the user specified fee is not absurd. // This allows amount=0 (and all amount < DEFAULT_FEE) transactions to use the default network fee diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 568717a7b..14ac949df 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -212,8 +212,7 @@ CalcZIP317Fee( const std::optional& changeAddr) { std::vector vouts{}; - size_t sproutOutputCount = 0; - size_t saplingOutputCount{}, orchardOutputCount{}; + size_t sproutOutputCount{}, saplingOutputCount{}, orchardOutputCount{}; for (const auto& payment : payments) { std::visit(match { [&](const CKeyID& addr) { @@ -546,7 +545,7 @@ WalletTxBuilder::ResolveInputsAndPayments( // probably perform note selection at the same time as we're performing resolved payment // construction above. bool foundSufficientFunds = spendableMut.LimitToAmount( - sendAmount + finalFee, + targetAmount, dustThreshold, resolved.GetRecipientPools()); CAmount changeAmount{spendableMut.Total() - targetAmount}; @@ -574,7 +573,7 @@ WalletTxBuilder::ResolveInputsAndPayments( auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5); if (limitResult.has_value()) { std::tie(spendableMut, finalFee, changeAddr) = limitResult.value(); - targetAmount = sendAmount - finalFee; + targetAmount = sendAmount + finalFee; } else { return tl::make_unexpected(limitResult.error()); }