From 7f0592a66438cdc2fca7e72c27f13d0a56c9defd Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 4 Apr 2023 08:46:40 -0600 Subject: [PATCH 01/10] Simplify canResolveOrchard logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When deciding whether we can pay Orchard receivers, rather than checking whether we have “sufficient non-Sprout funds”, we can just check whether the selector selects Sprout – if it doesn’t then we can select Orchard receivers regardless, and we’ll catch insufficient funds later. This is also helpful for ZIP 317, because in that case we don’t even know the total non-Sprout funds necessary until after we decide which receivers to pay. --- qa/rpc-tests/wallet_z_sendmany.py | 22 ++++++++++++++-------- src/wallet/asyncrpcoperation_common.cpp | 17 ++++++++++++----- src/wallet/wallet_tx_builder.cpp | 16 ++++++++-------- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 10734465e..3b0c269c1 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -22,6 +22,10 @@ from decimal import Decimal # Test wallet address behaviour across network upgrades class WalletZSendmanyTest(BitcoinTestFramework): + def __init__(self): + super().__init__() + self.cache_behavior = 'sprout' + def setup_network(self, split=False): self.nodes = start_nodes(3, self.options.tmpdir, [[ nuparams(NU5_BRANCH_ID, 238), @@ -110,12 +114,13 @@ class WalletZSendmanyTest(BitcoinTestFramework): # check balances zsendmanynotevalue = Decimal('7.0') zsendmanyfee = DEFAULT_FEE - node2utxobalance = Decimal('260.00000000') - zsendmanynotevalue - zsendmanyfee + node2sproutbalance = Decimal('50.00000000') + node2utxobalance = Decimal('210.00000000') - zsendmanynotevalue - zsendmanyfee # check shielded balance status with getwalletinfo wallet_info = self.nodes[2].getwalletinfo() assert_equal(Decimal(wallet_info["shielded_unconfirmed_balance"]), zsendmanynotevalue) - assert_equal(Decimal(wallet_info["shielded_balance"]), Decimal('0.0')) + assert_equal(Decimal(wallet_info["shielded_balance"]), node2sproutbalance) self.nodes[2].generate(10) self.sync_all() @@ -130,13 +135,13 @@ class WalletZSendmanyTest(BitcoinTestFramework): # check via z_gettotalbalance resp = self.nodes[2].z_gettotalbalance() assert_equal(Decimal(resp["transparent"]), node2utxobalance) - assert_equal(Decimal(resp["private"]), zbalance) - assert_equal(Decimal(resp["total"]), node2utxobalance + zbalance) + assert_equal(Decimal(resp["private"]), node2sproutbalance + zbalance) + assert_equal(Decimal(resp["total"]), node2utxobalance + node2sproutbalance + zbalance) # check confirmed shielded balance with getwalletinfo wallet_info = self.nodes[2].getwalletinfo() assert_equal(Decimal(wallet_info["shielded_unconfirmed_balance"]), Decimal('0.0')) - assert_equal(Decimal(wallet_info["shielded_balance"]), zsendmanynotevalue) + assert_equal(Decimal(wallet_info["shielded_balance"]), node2sproutbalance + zsendmanynotevalue) # there should be at least one Sapling output mytxdetails = self.nodes[2].getrawtransaction(mytxid, 1) @@ -377,7 +382,7 @@ class WalletZSendmanyTest(BitcoinTestFramework): 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 NU5 has not been activated yet.'), ]: opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, policy) wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', msg) @@ -416,16 +421,17 @@ class WalletZSendmanyTest(BitcoinTestFramework): self.sync_all() # - # Test Orchard-only UA with insufficient non-Sprout funds + # Test sending Sprout funds to Orchard-only UA # + sproutAddr = self.nodes[0].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.'), ]: - opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0, policy) + opid = self.nodes[0].z_sendmany(sproutAddr, recipients, 1, 0, policy) wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', msg) # diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index f03dad03a..3fcbe38a8 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -48,7 +48,7 @@ void ThrowInputSelectionError( const TransactionStrategy& strategy) { examine(err, match { - [](const AddressResolutionError& err) { + [&](const AddressResolutionError& err) { switch (err) { case AddressResolutionError::SproutRecipientsNotSupported: throw JSONRPCError( @@ -73,10 +73,17 @@ void ThrowInputSelectionError( case AddressResolutionError::CouldNotResolveReceiver: throw JSONRPCError( RPC_INVALID_PARAMETER, - "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."); + strprintf("Could not send to an Orchard-only receiver %s.", + strategy.AllowRevealedAmounts() + ? strprintf("despite a lax privacy policy, because %s", + selector.SelectsSprout() + ? "you are sending from the Sprout pool" + : "NU5 has not been activated yet") + : "without spending non-Orchard funds, 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")); case AddressResolutionError::TransparentReceiverNotAllowed: throw JSONRPCError( RPC_INVALID_PARAMETER, diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 28605b137..646a768d7 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -189,13 +189,6 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( { LOCK2(cs_main, wallet.cs_wallet); - // Determine the target totals - CAmount sendAmount{0}; - for (const auto& payment : payments) { - sendAmount += payment.GetAmount(); - } - CAmount targetAmount = sendAmount + fee; - // This is a simple greedy algorithm to attempt to preserve requested // transactional privacy while moving as much value to the most recent pool // as possible. This will also perform opportunistic shielding if the @@ -209,7 +202,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( // funds to cover the total payments + fee. bool canResolveOrchard = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5) - && spendableMut.Total() - spendableMut.GetSproutTotal() >= targetAmount; + && !selector.SelectsSprout(); std::vector resolvedPayments; std::optional resolutionError; for (const auto& payment : payments) { @@ -305,6 +298,13 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( // creating dust change amounts. CAmount dustThreshold{this->DefaultDustThreshold()}; + // Determine the target totals + CAmount sendAmount{0}; + for (const auto& payment : payments) { + sendAmount += payment.GetAmount(); + } + CAmount targetAmount{sendAmount - fee}; + // TODO: the set of recipient pools is not quite sufficient information here; we should // probably perform note selection at the same time as we're performing resolved payment // construction above. From fc6eca86e2a8dbf168105da2e9af975bc383c636 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Mon, 3 Apr 2023 12:05:55 -0600 Subject: [PATCH 02/10] Support ZIP 317 fees in the zcashd wallet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - still support explicit fixed fees everywhere – if a caller provides a fee, we don’t adjust it - treat negative fees as signifier for use of ZIP 317 fees when a fee needs to be provided because of additional positional arguments --- qa/rpc-tests/mempool_limit.py | 5 +- qa/rpc-tests/mempool_tx_expiry.py | 3 +- qa/rpc-tests/test_framework/zip317.py | 19 ++ qa/rpc-tests/wallet_listreceived.py | 15 +- qa/rpc-tests/wallet_nullifiers.py | 5 +- qa/rpc-tests/wallet_shieldingcoinbase.py | 20 +- qa/rpc-tests/wallet_treestate.py | 3 +- src/wallet/asyncrpcoperation_common.cpp | 11 + src/wallet/asyncrpcoperation_sendmany.cpp | 4 +- src/wallet/asyncrpcoperation_sendmany.h | 4 +- src/wallet/gtest/test_rpc_wallet.cpp | 2 +- src/wallet/rpcwallet.cpp | 25 +- src/wallet/test/rpc_wallet_tests.cpp | 4 +- src/wallet/wallet_tx_builder.cpp | 266 ++++++++++++++++++++-- src/wallet/wallet_tx_builder.h | 25 +- src/zip317.cpp | 11 +- src/zip317.h | 3 + 17 files changed, 351 insertions(+), 74 deletions(-) create mode 100644 qa/rpc-tests/test_framework/zip317.py diff --git a/qa/rpc-tests/mempool_limit.py b/qa/rpc-tests/mempool_limit.py index 9a3c2d5c2..adc2422ba 100755 --- a/qa/rpc-tests/mempool_limit.py +++ b/qa/rpc-tests/mempool_limit.py @@ -13,6 +13,7 @@ from test_framework.util import ( wait_and_assert_operationid_status, DEFAULT_FEE ) +from test_framework.zip317 import compute_conventional_fee from decimal import Decimal from time import sleep @@ -105,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*DEFAULT_FEE}], 1) + opid4 = self.nodes[0].z_sendmany(zaddr1, [{"address": zaddr4, "amount": Decimal('10.0') - 2*compute_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*DEFAULT_FEE}], 1) + opid5 = self.nodes[0].z_sendmany(zaddr2, [{"address": zaddr4, "amount": Decimal('10.0') - 2*compute_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 526328768..f57e8bb4a 100755 --- a/qa/rpc-tests/mempool_tx_expiry.py +++ b/qa/rpc-tests/mempool_tx_expiry.py @@ -12,6 +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 decimal import Decimal @@ -219,7 +220,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework): print("Ensure balance of node 0 is correct") bal = self.nodes[0].z_gettotalbalance() print("Balance after expire_shielded has expired: ", bal) - assert_equal(Decimal(bal["private"]), Decimal('8.0') - DEFAULT_FEE) + assert_equal(Decimal(bal["private"]), Decimal('8.0') - compute_conventional_fee(2)) print("Splitting network...") self.split_network() diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py new file mode 100644 index 000000000..5fc045826 --- /dev/null +++ b/qa/rpc-tests/test_framework/zip317.py @@ -0,0 +1,19 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +# +# zip317.py +# +# Utilities for ZIP 317 conventional fee specification. +# + +from test_framework.mininode import COIN +from decimal import Decimal + +MARGINAL_FEE = Decimal(5000)/COIN +GRACE_ACTIONS = 2 + +def compute_conventional_fee(logical_actions): + return MARGINAL_FEE * max(GRACE_ACTIONS, logical_actions) diff --git a/qa/rpc-tests/wallet_listreceived.py b/qa/rpc-tests/wallet_listreceived.py index 96eea5aa6..89357ef27 100755 --- a/qa/rpc-tests/wallet_listreceived.py +++ b/qa/rpc-tests/wallet_listreceived.py @@ -13,10 +13,11 @@ from test_framework.util import ( connect_nodes_bi, nuparams, DEFAULT_FEE, - DEFAULT_FEE_ZATS, 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 decimal import Decimal my_memo_str = 'c0ffee' # stay awake @@ -157,8 +158,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') - DEFAULT_FEE) - assert_equal(outputs[0]['valueZat'], 40000000 - DEFAULT_FEE_ZATS) + 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]['output'], 1) assert_equal(outputs[0]['outgoing'], False) assert_equal(outputs[0]['memo'], no_memo) @@ -178,8 +179,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')-DEFAULT_FEE, r[0]['amount']) - assert_equal(40000000-DEFAULT_FEE_ZATS, r[0]['amountZat']) + 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(r[0]['change'], True, "Note valued at (0.4-"+str(DEFAULT_FEE)+") should be change") assert_equal(no_memo, r[0]['memo']) @@ -382,8 +383,8 @@ class ListReceivedTest (BitcoinTestFramework): # Verify that we observe the change output assert_equal(outputs[2]['pool'], 'orchard') - assert_equal(outputs[2]['value'], Decimal('0.49999')) - assert_equal(outputs[2]['valueZat'], 49999000) + 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]['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 938945bc6..e5ade4794 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -7,6 +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 decimal import Decimal @@ -92,7 +93,7 @@ class WalletNullifiersTest (BitcoinTestFramework): # check zaddr balance zsendmany2notevalue = Decimal('2.0') - zsendmanyfee = DEFAULT_FEE + zsendmanyfee = compute_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) @@ -117,7 +118,7 @@ class WalletNullifiersTest (BitcoinTestFramework): # check zaddr balance zsendmany3notevalue = Decimal('1.0') - zaddrremaining2 = zaddrremaining - zsendmany3notevalue - zsendmanyfee + zaddrremaining2 = zaddrremaining - zsendmany3notevalue - DEFAULT_FEE assert_equal(self.nodes[1].z_getbalance(myzaddr), zaddrremaining2) assert_equal(self.nodes[2].z_getbalance(myzaddr), zaddrremaining2) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 7e1ccc42d..60fec3326 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -10,6 +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 import sys import timeit @@ -188,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') - DEFAULT_FEE}) + recipients.append({"address":myzaddr, "amount": Decimal('20.0') - compute_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() @@ -230,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') - DEFAULT_FEE - Decimal('0.00000001') # this leaves change at 1 zatoshi less than dust threshold + amount = Decimal('10.0') - compute_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.") @@ -247,7 +248,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): recipients = [] recipients.append({"address":self.nodes[1].getnewaddress(), "amount":Decimal('10000.0')}) 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 10000.00001; 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.") + wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 10.00, need 10000.0001; 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.") myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, DEFAULT_FEE, 'AllowRevealedRecipients') wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 9.99998, need 10000.00001; 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.") @@ -264,7 +265,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # given the tx size, resulting in mempool rejection. errorString = '' recipients = [] - num_t_recipients = 2500 + num_t_recipients = 1998 # stay just under the absurdly-high-fee error amount_per_recipient = Decimal('0.00000546') # dust threshold # Note that regtest chainparams does not require standard tx, so setting the amount to be # less than the dust threshold, e.g. 0.00000001 will not result in mempool rejection. @@ -284,7 +285,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): self.nodes[0].getinfo() # Issue #2263 Workaround END - myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, DEFAULT_FEE, 'AllowRevealedRecipients') + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, -1, 'AllowRevealedRecipients') try: wait_and_assert_operationid_status(self.nodes[0], myopid) except JSONRPCException as e: @@ -300,17 +301,10 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # check balance node2balance = amount_per_recipient * num_t_recipients - saplingvalue -= node2balance + DEFAULT_FEE + saplingvalue -= node2balance + compute_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: - self.nodes[0].z_sendmany(myzaddr, recipients, 1, -1) - 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 e1afb60c5..807ee1896 100755 --- a/qa/rpc-tests/wallet_treestate.py +++ b/qa/rpc-tests/wallet_treestate.py @@ -7,6 +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 import time from decimal import Decimal @@ -79,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*DEFAULT_FEE}) + recipients.append({"address": self.nodes[2].z_getnewaddress(), "amount": Decimal('12.0') - 4 * compute_conventional_fee(3)}) myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1) # Wait for Tx 2 to begin executing... diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 3fcbe38a8..11412b46d 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -113,9 +113,20 @@ void ThrowInputSelectionError( "Insufficient funds: have %s, %s", FormatMoney(err.available), examine(err.reason, match { + [](const QuasiChangeError& qce) { + return strprintf( + "need %s more to surpass the dust threshold and avoid being " + "forced to over-pay the fee. Alternatively, you could specify " + "a fee of %s to allow overpayment of the conventional fee and " + "have this transaction proceed.", + FormatMoney(qce.dustThreshold), + FormatMoney(qce.finalFee)); + }, [](const InsufficientFundsError& ife) { return strprintf("need %s", FormatMoney(ife.required)); }, + // TODO: Add the fee here, so we can suggest specifying an explicit fee (see + // `QuasiChangeError`). [](const DustThresholdError& dte) { return strprintf( "need %s more to avoid creating invalid change output %s (dust threshold is %s)", diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 01af89535..75ed71c91 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -49,13 +49,13 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( int minDepth, unsigned int anchorDepth, TransactionStrategy strategy, - CAmount fee, + std::optional fee, UniValue contextInfo) : builder_(std::move(builder)), ztxoSelector_(ztxoSelector), recipients_(recipients), mindepth_(minDepth), anchordepth_(anchorDepth), strategy_(strategy), fee_(fee), contextinfo_(contextInfo) { - assert(fee_ >= 0); + assert(!fee_.has_value() || fee_.value() >= 0); assert(mindepth_ >= 0); assert(!recipients_.empty()); assert(ztxoSelector.RequireSpendingKeys()); diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index ec59470d2..352691a7b 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -35,7 +35,7 @@ public: int minDepth, unsigned int anchorDepth, TransactionStrategy strategy, - CAmount fee = DEFAULT_FEE, + std::optional fee, UniValue contextInfo = NullUniValue); virtual ~AsyncRPCOperation_sendmany(); @@ -61,7 +61,7 @@ private: TransactionStrategy strategy_; int mindepth_{1}; unsigned int anchordepth_{nAnchorConfirmations}; - CAmount fee_; + std::optional fee_; UniValue contextinfo_; // optional data to include in return value from getStatus() uint256 main_impl(CWallet& wallet); diff --git a/src/wallet/gtest/test_rpc_wallet.cpp b/src/wallet/gtest/test_rpc_wallet.cpp index 716654c31..5bcef4ad2 100644 --- a/src/wallet/gtest/test_rpc_wallet.cpp +++ b/src/wallet/gtest/test_rpc_wallet.cpp @@ -296,7 +296,7 @@ TEST(WalletRPCTests, RPCZsendmanyTaddrToSapling) TransparentCoinbasePolicy::Disallow, false).value(); std::vector recipients = { Payment(pa, 1*COIN, Memo::FromHexOrThrow("ABCD")) }; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 0, 0, strategy)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 0, 0, strategy, std::nullopt)); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); // Enable test mode so tx is not sent diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ff4c40190..582dab962 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4813,7 +4813,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) " the output is being sent to a transparent address, it’s an error to include this field.\n" " }, ... ]\n" "3. minconf (numeric, optional, default=" + strprintf("%u", DEFAULT_NOTE_CONFIRMATIONS) + ") Only use funds confirmed at least this many times.\n" - "4. fee (numeric, optional, default=" + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n" + "4. fee (numeric, optional, default=-1) The fee amount to attach to this transaction. The default behavior\n" + " is to use a fee calculated according to ZIP 317.\n" "5. privacyPolicy (string, optional, default=\"LegacyCompat\") Policy for what information leakage is acceptable.\n" " One of the following strings:\n" " - \"FullPrivacy\": Only allow fully-shielded transactions (involving a single shielded value pool).\n" @@ -5028,31 +5029,33 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) int nMinDepth = parseMinconf(DEFAULT_NOTE_CONFIRMATIONS, params, 2, std::nullopt); // Fee in Zatoshis, not currency format) - CAmount nFee = DEFAULT_FEE; - if (params.size() > 3) { + std::optional nFee; + if (params.size() > 3 && params[3].get_real() >= 0.0) { + CAmount fixedFee; if (params[3].get_real() == 0.0) { - nFee = 0; + fixedFee = 0; } else { - nFee = AmountFromValue( params[3] ); + 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 // or anything less than DEFAULT_FEE instead of being forced to use a custom fee and leak metadata if (nTotalOut < DEFAULT_FEE) { - if (nFee > DEFAULT_FEE) { + if (fixedFee > DEFAULT_FEE) { throw JSONRPCError( RPC_INVALID_PARAMETER, - strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(nFee), FormatMoney(DEFAULT_FEE))); + strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(fixedFee), FormatMoney(DEFAULT_FEE))); } } else { // Check that the user specified fee is not absurd. - if (nFee > nTotalOut) { + if (fixedFee > nTotalOut) { throw JSONRPCError( RPC_INVALID_PARAMETER, - strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(nFee), FormatMoney(nTotalOut))); + strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(fixedFee), FormatMoney(nTotalOut))); } } + nFee = fixedFee; } // Use input parameters as the optional context info to be returned by z_getoperationstatus and z_getoperationresult. @@ -5060,7 +5063,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) o.pushKV("fromaddress", params[0]); o.pushKV("amounts", params[1]); o.pushKV("minconf", nMinDepth); - o.pushKV("fee", std::stod(FormatMoney(nFee))); + if (nFee.has_value()) { + o.pushKV("fee", std::stod(FormatMoney(nFee.value()))); + } UniValue contextInfo = o; // Create operation and add to global queue diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index 58583c556..cd4b4454a 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/src/wallet/test/rpc_wallet_tests.cpp @@ -1251,7 +1251,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) WalletTxBuilder builder(Params(), minRelayTxFee); std::vector recipients = { Payment(zaddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) }; TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders); - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy, std::nullopt)); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); @@ -1268,7 +1268,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) WalletTxBuilder builder(Params(), minRelayTxFee); std::vector recipients = { Payment(taddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) }; TransactionStrategy strategy(PrivacyPolicy::AllowRevealedRecipients); - std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy, std::nullopt)); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 646a768d7..8d1846761 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -3,6 +3,7 @@ // file COPYING or https://www.opensource.org/licenses/mit-license.php . #include "wallet/wallet_tx_builder.h" +#include "zip317.h" using namespace libzcash; @@ -12,6 +13,22 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) return nextBlockHeight - anchorConfirmations; } +/// Returns `std::nullopt` in the case of Sprout, since that isn’t a member of `OutputPool`. +static std::optional ChangeAddressPool(const ZTXOSelector& selector) { + return std::visit(match { + [](const CKeyID&) -> std::optional { return OutputPool::Transparent; }, + [](const CScriptID&) -> std::optional { return OutputPool::Transparent; }, + [](const libzcash::SproutPaymentAddress&) -> std::optional { return std::nullopt; }, + [](const libzcash::SproutViewingKey&) -> std::optional { return std::nullopt; }, + [](const libzcash::SaplingPaymentAddress&) -> std::optional { return OutputPool::Sapling; }, + [](const libzcash::SaplingExtendedFullViewingKey&) -> std::optional { return OutputPool::Sapling; }, + // TODO: These need some real logic + [](const libzcash::UnifiedAddress& ua) -> std::optional { return OutputPool::Orchard; }, + [](const libzcash::UnifiedFullViewingKey& fvk) -> std::optional { return OutputPool::Orchard; }, + [](const AccountZTXOPattern& acct) -> std::optional { return OutputPool::Orchard; } + }, selector.GetPattern()); +} + PrepareTransactionResult WalletTxBuilder::PrepareTransaction( CWallet& wallet, const ZTXOSelector& selector, @@ -19,7 +36,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( const std::vector& payments, const CChain& chain, TransactionStrategy strategy, - CAmount fee, + std::optional fee, uint32_t anchorConfirmations) const { assert(fee < MAX_MONEY); @@ -31,11 +48,15 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( } auto resolvedSelection = std::get(selected); + // TODO: don’t reassign + spendable = resolvedSelection.GetInputs(); auto resolvedPayments = resolvedSelection.GetPayments(); + // TODO: don’t reassign + auto finalFee = resolvedSelection.GetFee(); // We do not set a change address if there is no change. std::optional changeAddr; - auto changeAmount = spendable.Total() - resolvedPayments.Total() - fee; + auto changeAmount = spendable.Total() - resolvedPayments.Total() - finalFee; if (changeAmount > 0) { // Determine the account we're sending from. auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT); @@ -149,7 +170,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( spendable, resolvedPayments, changeAddr, - fee, + finalFee, ovks.first, ovks.second, anchorHeight); @@ -157,8 +178,16 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( return effects; } -Payments InputSelection::GetPayments() const { - return this->payments; +const SpendableInputs& InputSelection::GetInputs() const { + return inputs; +} + +const Payments& InputSelection::GetPayments() const { + return payments; +} + +CAmount InputSelection::GetFee() const { + return fee; } CAmount WalletTxBuilder::DefaultDustThreshold() const { @@ -177,6 +206,182 @@ SpendableInputs WalletTxBuilder::FindAllSpendableInputs( return wallet.FindSpendableInputs(selector, minDepth, std::nullopt); } +static size_t NotOne(size_t n) +{ + return n == 1 ? 2 : n; +} + +/// ISSUES: +/// • z_shieldcoinbase currently checks the fee in advance of calling PrepareTransaction in order to +/// determine the payment amount. But that fee possibly isn’t correct. We could perhaps have _no_ +/// payments in z_shieldcoinbase, but provide an explicit change address? (Don’t expose this via +/// RPC, and give it a different name, e.g. `NetFundsRecipient` in the WalletTxBuilder interface) +static CAmount +CalcZIP317Fee( + const std::optional& inputs, + const std::vector& payments, + const std::optional>& changeAddr) +{ + std::vector vouts{}; + size_t sproutOutputCount = 0; + size_t saplingOutputCount{}, orchardOutputCount{}; + for (const auto& payment : payments) { + std::visit(match { + [&](const CKeyID& addr) { + vouts.emplace_back(payment.amount, GetScriptForDestination(addr)); + }, + [&](const CScriptID& addr) { + vouts.emplace_back(payment.amount, GetScriptForDestination(addr)); + }, + [&](const libzcash::SaplingPaymentAddress&) { + ++saplingOutputCount; + }, + [&](const libzcash::OrchardRawAddress&) { + ++orchardOutputCount; + } + }, payment.address); + } + + if (changeAddr.has_value()) { + auto changePool = changeAddr.value(); + if (changePool.has_value()) { + switch (changePool.value()) { + case OutputPool::Transparent: + // TODO: need to either get an actual change address or make a fake vout here + break; + case OutputPool::Sapling: + ++saplingOutputCount; + break; + case OutputPool::Orchard: + ++orchardOutputCount; + break; + } + } else { + ++sproutOutputCount; + } + } + + size_t logicalActionCount; + if (inputs.has_value()) { + std::vector vins{}; + for (const auto& utxo : inputs.value().utxos) { + vins.emplace_back( + COutPoint(utxo.tx->GetHash(), utxo.i), + utxo.tx->vout[utxo.i].scriptPubKey); + } + logicalActionCount = CalculateLogicalActionCount( + vins, + vouts, + std::max(inputs.value().sproutNoteEntries.size(), sproutOutputCount), + inputs.value().saplingNoteEntries.size(), + NotOne(saplingOutputCount), + NotOne(std::max(inputs.value().orchardNoteMetadata.size(), orchardOutputCount))); + + } else { + logicalActionCount = CalculateLogicalActionCount( + {}, + vouts, + sproutOutputCount, + 0, + NotOne(saplingOutputCount), + NotOne(orchardOutputCount)); + } + + return CalculateConventionalFee(logicalActionCount); +} + +InvalidFundsError ReportInvalidFunds( + const SpendableInputs& spendable, + bool hasQuasiChange, + CAmount fee, + CAmount dustThreshold, + CAmount targetAmount, + CAmount changeAmount) +{ + return InvalidFundsError( + spendable.Total(), + hasQuasiChange + // TODO: NEED TESTS TO EXERCISE THIS + ? InvalidFundsReason(QuasiChangeError(fee, dustThreshold)) + : (changeAmount > 0 && changeAmount < dustThreshold + // TODO: we should provide the option for the caller to explicitly forego change + // (definitionally an amount below the dust amount) and send the extra to the + // recipient or the miner fee to avoid creating dust change, rather than prohibit + // them from sending entirely in this circumstance. (Daira disagrees, as this could + // leak information to the recipient or publicly in the fee.) + ? InvalidFundsReason(DustThresholdError(dustThreshold, changeAmount)) + : InvalidFundsReason(InsufficientFundsError(targetAmount)))); +} + +/// On the initial call, we haven’t yet selected inputs, so we assume the outputs dominate the +/// actions. +/// +/// 1. calc fee using only resolvedPayments to set a lower bound on the actual fee +/// • this also needs to know which pool change is going to, so it can determine what the fee is +/// with change _if_ there is change +/// 2. iterate over LimitToAmount until the updated fee (now including spends) matches the expected +/// fee +tl::expected, InputSelectionError> +IterateLimit( + CAmount sendAmount, + CAmount dustThreshold, + const SpendableInputs& spendable, + const Payments& resolved, + const std::optional>& changePool) +{ + SpendableInputs spendableMut; + + auto previousFee = MINIMUM_FEE; + auto updatedFee = CalcZIP317Fee(std::nullopt, resolved.GetResolvedPayments(), std::nullopt); + // This is used to increase the target amount just enough (generally by 0 or 1) to force + // selection of additional notes. + CAmount bumpTargetAmount{0}; + + do { + spendableMut = spendable; + + auto targetAmount{sendAmount + updatedFee}; + + // TODO: the set of recipient pools is not quite sufficient information here; we should + // probably perform note selection at the same time as we're performing resolved payment + // construction above. + bool foundSufficientFunds = + spendableMut.LimitToAmount( + targetAmount + bumpTargetAmount, + dustThreshold, + resolved.GetRecipientPools()); + CAmount changeAmount{spendableMut.Total() - targetAmount}; + if (foundSufficientFunds) { + previousFee = updatedFee; + updatedFee = CalcZIP317Fee( + spendableMut, + resolved.GetResolvedPayments(), + changeAmount > 0 + ? std::optional>(changePool) + : std::nullopt); + } else { + return tl::make_unexpected( + ReportInvalidFunds( + spendableMut, + bumpTargetAmount != 0, + previousFee, + dustThreshold, + targetAmount, + changeAmount)); + } + // This happens when we have exactly `MARGINAL_FEE` change, then add a change output that + // causes the conventional fee to consume that change, leaving us with no change, which then + // lowers the fee. + if (updatedFee < previousFee) { + // Bump the updated fee so that we don’t exit the loop, but should force us to take an + // extra note (or fail) in the next `LimitToAmount`. + bumpTargetAmount = 1; + } + } while (updatedFee != previousFee); + + return std::make_pair(spendableMut, updatedFee); +} + InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( const CWallet& wallet, const ZTXOSelector& selector, @@ -184,7 +389,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( const std::vector& payments, const CChain& chain, TransactionStrategy strategy, - CAmount fee, + std::optional fee, int anchorHeight) const { LOCK2(cs_main, wallet.cs_wallet); @@ -303,25 +508,38 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( for (const auto& payment : payments) { sendAmount += payment.GetAmount(); } - CAmount targetAmount{sendAmount - fee}; - // TODO: the set of recipient pools is not quite sufficient information here; we should - // probably perform note selection at the same time as we're performing resolved payment - // construction above. - if (!spendableMut.LimitToAmount(targetAmount, dustThreshold, resolved.GetRecipientPools())) { + CAmount finalFee; + CAmount targetAmount; + if (fee.has_value()) { + finalFee = fee.value(); + targetAmount = sendAmount + finalFee; + // TODO: the set of recipient pools is not quite sufficient information here; we should + // probably perform note selection at the same time as we're performing resolved payment + // construction above. + bool foundSufficientFunds = spendableMut.LimitToAmount( + sendAmount + finalFee, + dustThreshold, + resolved.GetRecipientPools()); CAmount changeAmount{spendableMut.Total() - targetAmount}; - return InvalidFundsError( - spendableMut.Total(), - changeAmount > 0 && changeAmount < dustThreshold - // TODO: we should provide the option for the caller to explicitly - // forego change (definitionally an amount below the dust amount) - // and send the extra to the recipient or the miner fee to avoid - // creating dust change, rather than prohibit them from sending - // entirely in this circumstance. - // (Daira disagrees, as this could leak information to the recipient - // or publicly in the fee.) - ? InvalidFundsReason(DustThresholdError(dustThreshold, changeAmount)) - : InvalidFundsReason(InsufficientFundsError(targetAmount))); + if (!foundSufficientFunds) { + return ReportInvalidFunds( + spendableMut, + false, + finalFee, + dustThreshold, + targetAmount, + changeAmount); + } + } else { + auto limit_result = IterateLimit(sendAmount, dustThreshold, spendableMut, resolved, ChangeAddressPool(selector)); + if (limit_result.has_value()) { + spendableMut = limit_result.value().first; + finalFee = limit_result.value().second; + targetAmount = sendAmount - finalFee; + } else { + return limit_result.error(); + } } // When spending transparent coinbase outputs, all inputs must be fully @@ -341,7 +559,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( this->maxOrchardActions); } - return InputSelection(resolved, anchorHeight); + return InputSelection(spendableMut, resolved, finalFee, anchorHeight); } std::pair diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index cb1b5441c..1a322b36d 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -239,6 +239,15 @@ enum class AddressResolutionError { RevealingReceiverAmountsNotAllowed, }; +class QuasiChangeError { +public: + CAmount finalFee; + CAmount dustThreshold; + + QuasiChangeError(CAmount finalFee, CAmount dustThreshold): + finalFee(finalFee), dustThreshold(dustThreshold) { } +}; + class InsufficientFundsError { public: CAmount required; @@ -257,6 +266,7 @@ public: }; typedef std::variant< + QuasiChangeError, InsufficientFundsError, DustThresholdError> InvalidFundsReason; @@ -302,14 +312,18 @@ typedef std::variant< class InputSelection { private: + SpendableInputs inputs; Payments payments; + CAmount fee; int orchardAnchorHeight; public: - InputSelection(Payments payments, int orchardAnchorHeight): - payments(payments), orchardAnchorHeight(orchardAnchorHeight) {} + InputSelection(SpendableInputs inputs, Payments payments, CAmount fee, int orchardAnchorHeight): + inputs(inputs), payments(payments), fee(fee), orchardAnchorHeight(orchardAnchorHeight) {} - Payments GetPayments() const; + const SpendableInputs& GetInputs() const; + const Payments& GetPayments() const; + CAmount GetFee() const; }; typedef std::variant< @@ -343,7 +357,7 @@ private: const std::vector& payments, const CChain& chain, TransactionStrategy strategy, - CAmount fee, + std::optional fee, int anchorHeight) const; /** * Compute the internal and external OVKs to use in transaction construction, given @@ -370,7 +384,8 @@ public: const std::vector& payments, const CChain& chain, TransactionStrategy strategy, - CAmount fee, + /// A fixed fee is used if provided, otherwise it is calculated based on ZIP 317. + std::optional fee, uint32_t anchorConfirmations) const; }; diff --git a/src/zip317.cpp b/src/zip317.cpp index e0bd1c640..b78ab1a61 100644 --- a/src/zip317.cpp +++ b/src/zip317.cpp @@ -14,6 +14,13 @@ CAmount CalculateConventionalFee(size_t logicalActionCount) { return MARGINAL_FEE * std::max(GRACE_ACTIONS, logicalActionCount); } +template +size_t GetUTXOFieldSize(const std::vector& utxos) { + auto size = GetSerializeSize(utxos, SER_NETWORK, PROTOCOL_VERSION); + auto countSize = GetSizeOfCompactSize(utxos.size()); + return size - countSize; +} + size_t CalculateLogicalActionCount( const std::vector& vin, const std::vector& vout, @@ -21,8 +28,8 @@ size_t CalculateLogicalActionCount( unsigned int saplingSpendCount, unsigned int saplingOutputCount, unsigned int orchardActionCount) { - const size_t tx_in_total_size = GetSerializeSize(vin, SER_NETWORK, PROTOCOL_VERSION); - const size_t tx_out_total_size = GetSerializeSize(vout, SER_NETWORK, PROTOCOL_VERSION); + const size_t tx_in_total_size = GetUTXOFieldSize(vin); + const size_t tx_out_total_size = GetUTXOFieldSize(vout); return std::max(ceil_div(tx_in_total_size, P2PKH_STANDARD_INPUT_SIZE), ceil_div(tx_out_total_size, P2PKH_STANDARD_OUTPUT_SIZE)) + diff --git a/src/zip317.h b/src/zip317.h index 342b35e99..10657368c 100644 --- a/src/zip317.h +++ b/src/zip317.h @@ -23,6 +23,9 @@ static const int64_t WEIGHT_RATIO_SCALE = INT64_C(10000000000000000); static const int64_t WEIGHT_RATIO_CAP = 4; static const size_t BLOCK_UNPAID_ACTION_LIMIT = 50; +/// This is the lowest the conventional fee can be in ZIP 317. +static const CAmount MINIMUM_FEE = MARGINAL_FEE * GRACE_ACTIONS; + /// Return the conventional fee for the given `logicalActionCount` calculated according to /// . CAmount CalculateConventionalFee(size_t logicalActionCount); From dac6c014d4964e802a2d759605038bbe76fa4859 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Fri, 7 Apr 2023 13:28:13 -0600 Subject: [PATCH 03/10] Correct change handling for ZIP 317 fees --- qa/rpc-tests/mempool_tx_expiry.py | 4 +- src/wallet/asyncrpcoperation_sendmany.cpp | 24 +- src/wallet/asyncrpcoperation_sendmany.h | 4 +- src/wallet/wallet_tx_builder.cpp | 445 ++++++++++++---------- src/wallet/wallet_tx_builder.h | 49 ++- 5 files changed, 290 insertions(+), 236 deletions(-) diff --git a/qa/rpc-tests/mempool_tx_expiry.py b/qa/rpc-tests/mempool_tx_expiry.py index f57e8bb4a..0215d9e3c 100755 --- a/qa/rpc-tests/mempool_tx_expiry.py +++ b/qa/rpc-tests/mempool_tx_expiry.py @@ -94,7 +94,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework): # Create transactions blockheight = self.nodes[0].getblockchaininfo()['blocks'] - zsendamount = Decimal('1.0') - DEFAULT_FEE + zsendamount = Decimal('1.0') - compute_conventional_fee(2) recipients = [] recipients.append({"address": z_bob, "amount": zsendamount}) myopid = self.nodes[0].z_sendmany(z_alice, recipients, 1) @@ -220,7 +220,7 @@ class MempoolTxExpiryTest(BitcoinTestFramework): print("Ensure balance of node 0 is correct") bal = self.nodes[0].z_gettotalbalance() print("Balance after expire_shielded has expired: ", bal) - assert_equal(Decimal(bal["private"]), Decimal('8.0') - compute_conventional_fee(2)) + assert_equal(Decimal(bal["private"]), Decimal('8.0') - DEFAULT_FEE) print("Splitting network...") self.split_network() diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 75ed71c91..904e841fa 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -84,7 +84,11 @@ void AsyncRPCOperation_sendmany::main() { std::optional txid; try { - txid = main_impl(*pwalletMain); + txid = main_impl(*pwalletMain) + .map_error([&](const InputSelectionError& err) { + ThrowInputSelectionError(err, ztxoSelector_, strategy_); + }) + .value(); } catch (const UniValue& objError) { int code = find_value(objError, "code").get_int(); std::string message = find_value(objError, "message").get_str(); @@ -137,7 +141,8 @@ void AsyncRPCOperation_sendmany::main() { // 4. #3615 There is no padding of inputs or outputs, which may leak information. // // At least #4 differs from the Rust transaction builder. -uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) { +tl::expected +AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) { auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, mindepth_); auto preparedTx = builder_.PrepareTransaction( @@ -150,12 +155,8 @@ uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) { fee_, anchordepth_); - uint256 txid; - examine(preparedTx, match { - [&](const InputSelectionError& err) { - ThrowInputSelectionError(err, ztxoSelector_, strategy_); - }, - [&](const TransactionEffects& effects) { + return preparedTx + .map([&](const TransactionEffects& effects) { try { const auto& spendable = effects.GetSpendable(); const auto& payments = effects.GetPayments(); @@ -187,16 +188,13 @@ uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) { UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode); set_result(sendResult); - txid = tx.GetHash(); effects.UnlockSpendable(wallet); + return tx.GetHash(); } catch (...) { effects.UnlockSpendable(wallet); throw; } - } - }); - - return txid; + }); } /** diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index 352691a7b..1ccc9dfc6 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -64,7 +64,7 @@ private: std::optional fee_; UniValue contextinfo_; // optional data to include in return value from getStatus() - uint256 main_impl(CWallet& wallet); + tl::expected main_impl(CWallet& wallet); }; // To test private methods, a friend class can act as a proxy @@ -74,7 +74,7 @@ public: TEST_FRIEND_AsyncRPCOperation_sendmany(std::shared_ptr ptr) : delegate(ptr) {} - uint256 main_impl(CWallet& wallet) { + tl::expected main_impl(CWallet& wallet) { return delegate->main_impl(wallet); } diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 8d1846761..568717a7b 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -13,23 +13,125 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) return nextBlockHeight - anchorConfirmations; } -/// Returns `std::nullopt` in the case of Sprout, since that isn’t a member of `OutputPool`. -static std::optional ChangeAddressPool(const ZTXOSelector& selector) { - return std::visit(match { - [](const CKeyID&) -> std::optional { return OutputPool::Transparent; }, - [](const CScriptID&) -> std::optional { return OutputPool::Transparent; }, - [](const libzcash::SproutPaymentAddress&) -> std::optional { return std::nullopt; }, - [](const libzcash::SproutViewingKey&) -> std::optional { return std::nullopt; }, - [](const libzcash::SaplingPaymentAddress&) -> std::optional { return OutputPool::Sapling; }, - [](const libzcash::SaplingExtendedFullViewingKey&) -> std::optional { return OutputPool::Sapling; }, - // TODO: These need some real logic - [](const libzcash::UnifiedAddress& ua) -> std::optional { return OutputPool::Orchard; }, - [](const libzcash::UnifiedFullViewingKey& fvk) -> std::optional { return OutputPool::Orchard; }, - [](const AccountZTXOPattern& acct) -> std::optional { return OutputPool::Orchard; } - }, selector.GetPattern()); +ChangeAddress +WalletTxBuilder::GetChangeAddress( + CWallet& wallet, + const ZTXOSelector& selector, + SpendableInputs& spendable, + const Payments& resolvedPayments, + const TransactionStrategy& strategy, + bool afterNU5) const +{ + // Determine the account we're sending from. + auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT); + + auto getAllowedChangePools = [&](const std::set& receiverTypes) { + std::set result{resolvedPayments.GetRecipientPools()}; + // We always allow shielded change when not sending from the legacy account. + if (sendFromAccount != ZCASH_LEGACY_ACCOUNT) { + result.insert(OutputPool::Sapling); + } + for (ReceiverType rtype : receiverTypes) { + switch (rtype) { + case ReceiverType::P2PKH: + case ReceiverType::P2SH: + // TODO: This is the correct policy, but it’s a breaking change from + // previous behavior, so enable it separately. (#6409) + // if (strategy.AllowRevealedRecipients()) { + if (!spendable.utxos.empty() || strategy.AllowRevealedRecipients()) { + result.insert(OutputPool::Transparent); + } + break; + case ReceiverType::Sapling: + if (!spendable.saplingNoteEntries.empty() || strategy.AllowRevealedAmounts()) { + result.insert(OutputPool::Sapling); + } + break; + case ReceiverType::Orchard: + if (afterNU5 + && (!spendable.orchardNoteMetadata.empty() || strategy.AllowRevealedAmounts())) { + result.insert(OutputPool::Orchard); + } + break; + } + } + return result; + }; + + auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) { + auto addr = wallet.GenerateChangeAddressForAccount( + sendFromAccount, + getAllowedChangePools(receiverTypes)); + assert(addr.has_value()); + return addr.value(); + }; + + auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) -> RecipientAddress { + // for Sapling, if using a legacy address, return change to the + // originating address; otherwise return it to the Sapling internal + // address corresponding to the UFVK. + if (sendFromAccount == ZCASH_LEGACY_ACCOUNT) { + return addr; + } else { + auto addr = wallet.GenerateChangeAddressForAccount( + sendFromAccount, + getAllowedChangePools({ReceiverType::Sapling})); + assert(addr.has_value()); + return addr.value(); + } + }; + + auto changeAddressForZUFVK = [&]( + const ZcashdUnifiedFullViewingKey& zufvk, + const std::set& receiverTypes) { + auto addr = zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes)); + assert(addr.has_value()); + return addr.value(); + }; + + return examine(selector.GetPattern(), match { + [&](const CKeyID&) -> ChangeAddress { + return changeAddressForTransparentSelector({ReceiverType::P2PKH}); + }, + [&](const CScriptID&) -> ChangeAddress { + return changeAddressForTransparentSelector({ReceiverType::P2SH}); + }, + [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress { + // for Sprout, we return change to the originating address using the tx builder. + return addr; + }, + [](const libzcash::SproutViewingKey& vk) -> ChangeAddress { + // for Sprout, we return change to the originating address using the tx builder. + return vk.address(); + }, + [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress { + return changeAddressForSaplingAddress(addr); + }, + [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress { + return changeAddressForSaplingAddress(fvk.DefaultAddress()); + }, + [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress { + auto zufvk = wallet.GetUFVKForAddress(ua); + assert(zufvk.has_value()); + return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); + }, + [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress { + return changeAddressForZUFVK( + ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk), + fvk.GetKnownReceiverTypes()); + }, + [&](const AccountZTXOPattern& acct) -> ChangeAddress { + auto addr = wallet.GenerateChangeAddressForAccount( + acct.GetAccountId(), + getAllowedChangePools(acct.GetReceiverTypes())); + assert(addr.has_value()); + return addr.value(); + } + }); } -PrepareTransactionResult WalletTxBuilder::PrepareTransaction( +tl::expected +WalletTxBuilder::PrepareTransaction( CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, @@ -42,140 +144,23 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( assert(fee < MAX_MONEY); int anchorHeight = GetAnchorHeight(chain, anchorConfirmations); - auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, anchorHeight); - if (std::holds_alternative(selected)) { - return std::get(selected); - } + bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5); + auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, afterNU5); + return selected.map([&](const InputSelection& resolvedSelection) { + auto ovks = SelectOVKs(wallet, selector, spendable); - auto resolvedSelection = std::get(selected); - // TODO: don’t reassign - spendable = resolvedSelection.GetInputs(); - auto resolvedPayments = resolvedSelection.GetPayments(); - // TODO: don’t reassign - auto finalFee = resolvedSelection.GetFee(); - - // We do not set a change address if there is no change. - std::optional changeAddr; - auto changeAmount = spendable.Total() - resolvedPayments.Total() - finalFee; - if (changeAmount > 0) { - // Determine the account we're sending from. - auto sendFromAccount = wallet.FindAccountForSelector(selector).value_or(ZCASH_LEGACY_ACCOUNT); - - auto getAllowedChangePools = [&](const std::set& receiverTypes) { - std::set result{resolvedPayments.GetRecipientPools()}; - // We always allow shielded change when not sending from the legacy account. - if (sendFromAccount != ZCASH_LEGACY_ACCOUNT) { - result.insert(OutputPool::Sapling); - } - for (ReceiverType rtype : receiverTypes) { - switch (rtype) { - case ReceiverType::P2PKH: - case ReceiverType::P2SH: - if (strategy.AllowRevealedRecipients()) { - result.insert(OutputPool::Transparent); - } - break; - case ReceiverType::Sapling: - if (!spendable.saplingNoteEntries.empty() || strategy.AllowRevealedAmounts()) { - result.insert(OutputPool::Sapling); - } - break; - case ReceiverType::Orchard: - if (params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5) - && (!spendable.orchardNoteMetadata.empty() || strategy.AllowRevealedAmounts())) { - result.insert(OutputPool::Orchard); - } - break; - } - } - return result; - }; - - auto addChangePayment = [&](const std::optional& sendTo) { - assert(sendTo.has_value()); - resolvedPayments.AddPayment( - ResolvedPayment(std::nullopt, sendTo.value(), changeAmount, std::nullopt, true)); - return sendTo.value(); - }; - - auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) { - return addChangePayment( - wallet.GenerateChangeAddressForAccount( - sendFromAccount, - getAllowedChangePools(receiverTypes))); - }; - - auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) { - // for Sapling, if using a legacy address, return change to the - // originating address; otherwise return it to the Sapling internal - // address corresponding to the UFVK. - return addChangePayment( - sendFromAccount == ZCASH_LEGACY_ACCOUNT - ? addr - : wallet.GenerateChangeAddressForAccount( - sendFromAccount, - getAllowedChangePools({ReceiverType::Sapling}))); - }; - - auto changeAddressForZUFVK = [&]( - const ZcashdUnifiedFullViewingKey& zufvk, - const std::set& receiverTypes) { - return addChangePayment(zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes))); - }; - - changeAddr = examine(selector.GetPattern(), match { - [&](const CKeyID&) -> ChangeAddress { - return changeAddressForTransparentSelector({ReceiverType::P2PKH}); - }, - [&](const CScriptID&) -> ChangeAddress { - return changeAddressForTransparentSelector({ReceiverType::P2SH}); - }, - [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress { - // for Sprout, we return change to the originating address using the tx builder. - return addr; - }, - [](const libzcash::SproutViewingKey& vk) -> ChangeAddress { - // for Sprout, we return change to the originating address using the tx builder. - return vk.address(); - }, - [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress { - return changeAddressForSaplingAddress(addr); - }, - [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress { - return changeAddressForSaplingAddress(fvk.DefaultAddress()); - }, - [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress { - auto zufvk = wallet.GetUFVKForAddress(ua); - assert(zufvk.has_value()); - return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); - }, - [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress { - return changeAddressForZUFVK( - ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk), - fvk.GetKnownReceiverTypes()); - }, - [&](const AccountZTXOPattern& acct) -> ChangeAddress { - return addChangePayment( - wallet.GenerateChangeAddressForAccount( - acct.GetAccountId(), - getAllowedChangePools(acct.GetReceiverTypes()))); - } - }); - } - - auto ovks = SelectOVKs(wallet, selector, spendable); - - auto effects = TransactionEffects( - anchorConfirmations, - spendable, - resolvedPayments, - changeAddr, - finalFee, - ovks.first, - ovks.second, - anchorHeight); - effects.LockSpendable(wallet); - return effects; + auto effects = TransactionEffects( + anchorConfirmations, + resolvedSelection.GetInputs(), + resolvedSelection.GetPayments(), + resolvedSelection.GetChangeAddress(), + resolvedSelection.GetFee(), + ovks.first, + ovks.second, + anchorHeight); + effects.LockSpendable(wallet); + return effects; + }); } const SpendableInputs& InputSelection::GetInputs() const { @@ -190,6 +175,10 @@ CAmount InputSelection::GetFee() const { return fee; } +const std::optional InputSelection::GetChangeAddress() const { + return changeAddr; +} + CAmount WalletTxBuilder::DefaultDustThreshold() const { CKey secret{CKey::TestOnlyRandomKey(true)}; CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID()); @@ -220,7 +209,7 @@ static CAmount CalcZIP317Fee( const std::optional& inputs, const std::vector& payments, - const std::optional>& changeAddr) + const std::optional& changeAddr) { std::vector vouts{}; size_t sproutOutputCount = 0; @@ -243,22 +232,21 @@ CalcZIP317Fee( } if (changeAddr.has_value()) { - auto changePool = changeAddr.value(); - if (changePool.has_value()) { - switch (changePool.value()) { - case OutputPool::Transparent: - // TODO: need to either get an actual change address or make a fake vout here - break; - case OutputPool::Sapling: - ++saplingOutputCount; - break; - case OutputPool::Orchard: - ++orchardOutputCount; - break; + examine(changeAddr.value(), match { + [&](const SproutPaymentAddress&) { ++sproutOutputCount; }, + [&](const RecipientAddress& addr) { + examine(addr, match { + [&](const CKeyID& taddr) { + vouts.emplace_back(0, GetScriptForDestination(taddr)); + }, + [&](const CScriptID taddr) { + vouts.emplace_back(0, GetScriptForDestination(taddr)); + }, + [&](const libzcash::SaplingPaymentAddress&) { ++saplingOutputCount; }, + [&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; } + }); } - } else { - ++sproutOutputCount; - } + }); } size_t logicalActionCount; @@ -313,6 +301,23 @@ InvalidFundsError ReportInvalidFunds( : InvalidFundsReason(InsufficientFundsError(targetAmount)))); } +static void +AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CAmount changeAmount) +{ + assert(changeAmount > 0); + + examine(changeAddr, match { + // TODO: Once we can add Sprout change to `resolvedPayments`, we don’t need to pass + // `changeAddr` around the rest of these functions. + [](const libzcash::SproutPaymentAddress&) {}, + [](const libzcash::SproutViewingKey&) {}, + [&](const auto& sendTo) { + resolvedPayments.AddPayment( + ResolvedPayment(std::nullopt, sendTo, changeAmount, std::nullopt, true)); + } + }); +} + /// On the initial call, we haven’t yet selected inputs, so we assume the outputs dominate the /// actions. /// @@ -321,13 +326,18 @@ InvalidFundsError ReportInvalidFunds( /// with change _if_ there is change /// 2. iterate over LimitToAmount until the updated fee (now including spends) matches the expected /// fee -tl::expected, InputSelectionError> -IterateLimit( +tl::expected< + std::tuple>, + InputSelectionError> +WalletTxBuilder::IterateLimit( + CWallet& wallet, + const ZTXOSelector& selector, + const TransactionStrategy strategy, CAmount sendAmount, CAmount dustThreshold, const SpendableInputs& spendable, - const Payments& resolved, - const std::optional>& changePool) + Payments& resolved, + bool afterNU5) const { SpendableInputs spendableMut; @@ -336,6 +346,8 @@ IterateLimit( // This is used to increase the target amount just enough (generally by 0 or 1) to force // selection of additional notes. CAmount bumpTargetAmount{0}; + std::optional changeAddr; + CAmount changeAmount{0}; do { spendableMut = spendable; @@ -350,15 +362,25 @@ IterateLimit( targetAmount + bumpTargetAmount, dustThreshold, resolved.GetRecipientPools()); - CAmount changeAmount{spendableMut.Total() - targetAmount}; + changeAmount = spendableMut.Total() - targetAmount; if (foundSufficientFunds) { + // Don’t want to generate a change address if we don’t need one (because it could be + // fresh) and once we generate it, hold onto it. But we still don’t have a guarantee + // that we won’t end up discarding it. + if (changeAmount > 0 && !changeAddr.has_value()) { + changeAddr = GetChangeAddress( + wallet, + selector, + spendableMut, + resolved, + strategy, + afterNU5); + } previousFee = updatedFee; updatedFee = CalcZIP317Fee( spendableMut, resolved.GetResolvedPayments(), - changeAmount > 0 - ? std::optional>(changePool) - : std::nullopt); + changeAmount > 0 ? changeAddr : std::nullopt); } else { return tl::make_unexpected( ReportInvalidFunds( @@ -379,18 +401,24 @@ IterateLimit( } } while (updatedFee != previousFee); - return std::make_pair(spendableMut, updatedFee); + if (changeAmount > 0) { + assert(changeAddr.has_value()); + AddChangePayment(resolved, changeAddr.value(), changeAmount); + } + + return std::make_tuple(spendableMut, updatedFee, changeAddr); } -InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( - const CWallet& wallet, +tl::expected +WalletTxBuilder::ResolveInputsAndPayments( + CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendableMut, const std::vector& payments, const CChain& chain, - TransactionStrategy strategy, + const TransactionStrategy& strategy, std::optional fee, - int anchorHeight) const + bool afterNU5) const { LOCK2(cs_main, wallet.cs_wallet); @@ -405,9 +433,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( // we can only select Orchard addresses if there are sufficient non-Sprout // funds to cover the total payments + fee. - bool canResolveOrchard = - params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5) - && !selector.SelectsSprout(); + bool canResolveOrchard = afterNU5 && !selector.SelectsSprout(); std::vector resolvedPayments; std::optional resolutionError; for (const auto& payment : payments) { @@ -487,16 +513,17 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( }); if (resolutionError.has_value()) { - return resolutionError.value(); + return tl::make_unexpected(resolutionError.value()); } } auto resolved = Payments(resolvedPayments); if (orchardOutputs > this->maxOrchardActions) { - return ExcessOrchardActionsError( - ActionSide::Output, - orchardOutputs, - this->maxOrchardActions); + return tl::make_unexpected( + ExcessOrchardActionsError( + ActionSide::Output, + orchardOutputs, + this->maxOrchardActions)); } // Set the dust threshold so that we can select enough inputs to avoid @@ -511,6 +538,7 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( CAmount finalFee; CAmount targetAmount; + std::optional changeAddr; if (fee.has_value()) { finalFee = fee.value(); targetAmount = sendAmount + finalFee; @@ -523,22 +551,32 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( resolved.GetRecipientPools()); CAmount changeAmount{spendableMut.Total() - targetAmount}; if (!foundSufficientFunds) { - return ReportInvalidFunds( + return tl::make_unexpected( + ReportInvalidFunds( + spendableMut, + false, + finalFee, + dustThreshold, + targetAmount, + changeAmount)); + } + if (changeAmount > 0) { + changeAddr = GetChangeAddress( + wallet, + selector, spendableMut, - false, - finalFee, - dustThreshold, - targetAmount, - changeAmount); + resolved, + strategy, + afterNU5); + AddChangePayment(resolved, changeAddr.value(), changeAmount); } } else { - auto limit_result = IterateLimit(sendAmount, dustThreshold, spendableMut, resolved, ChangeAddressPool(selector)); - if (limit_result.has_value()) { - spendableMut = limit_result.value().first; - finalFee = limit_result.value().second; + 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; } else { - return limit_result.error(); + return tl::make_unexpected(limitResult.error()); } } @@ -546,20 +584,21 @@ InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( // consumed, and they may only be sent to shielded recipients. if (spendableMut.HasTransparentCoinbase()) { if (spendableMut.Total() != targetAmount) { - return ChangeNotAllowedError(spendableMut.Total(), targetAmount); + return tl::make_unexpected(ChangeNotAllowedError(spendableMut.Total(), targetAmount)); } else if (resolved.HasTransparentRecipient()) { - return AddressResolutionError::TransparentRecipientNotAllowed; + return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); } } if (spendableMut.orchardNoteMetadata.size() > this->maxOrchardActions) { - return ExcessOrchardActionsError( - ActionSide::Input, - spendableMut.orchardNoteMetadata.size(), - this->maxOrchardActions); + return tl::make_unexpected( + ExcessOrchardActionsError( + ActionSide::Input, + spendableMut.orchardNoteMetadata.size(), + this->maxOrchardActions)); } - return InputSelection(spendableMut, resolved, finalFee, anchorHeight); + return InputSelection(spendableMut, resolved, finalFee, changeAddr); } std::pair diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 1a322b36d..4ac3e2f08 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -315,25 +315,18 @@ private: SpendableInputs inputs; Payments payments; CAmount fee; - int orchardAnchorHeight; + std::optional changeAddr; public: - InputSelection(SpendableInputs inputs, Payments payments, CAmount fee, int orchardAnchorHeight): - inputs(inputs), payments(payments), fee(fee), orchardAnchorHeight(orchardAnchorHeight) {} + InputSelection(SpendableInputs inputs, Payments payments, CAmount fee, std::optional changeAddr): + inputs(inputs), payments(payments), fee(fee), changeAddr(changeAddr) {} const SpendableInputs& GetInputs() const; const Payments& GetPayments() const; CAmount GetFee() const; + const std::optional GetChangeAddress() const; }; -typedef std::variant< - InputSelectionError, - InputSelection> InputSelectionResult; - -typedef std::variant< - InputSelectionError, - TransactionEffects> PrepareTransactionResult; - class WalletTxBuilder { private: const CChainParams& params; @@ -345,20 +338,43 @@ private: */ CAmount DefaultDustThreshold() const; + ChangeAddress + GetChangeAddress( + CWallet& wallet, + const ZTXOSelector& selector, + SpendableInputs& spendable, + const Payments& resolvedPayments, + const TransactionStrategy& strategy, + bool afterNU5) const; + + tl::expected< + std::tuple>, + InputSelectionError> + IterateLimit( + CWallet& wallet, + const ZTXOSelector& selector, + const TransactionStrategy strategy, + CAmount sendAmount, + CAmount dustThreshold, + const SpendableInputs& spendable, + Payments& resolved, + bool afterNU5) const; + /** * Select inputs sufficient to fulfill the specified requested payments, * and choose unified address receivers based upon the available inputs * and the requested transaction strategy. */ - InputSelectionResult ResolveInputsAndPayments( - const CWallet& wallet, + tl::expected + ResolveInputsAndPayments( + CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, const std::vector& payments, const CChain& chain, - TransactionStrategy strategy, + const TransactionStrategy& strategy, std::optional fee, - int anchorHeight) const; + bool afterNU5) const; /** * Compute the internal and external OVKs to use in transaction construction, given * the spendable inputs. @@ -377,7 +393,8 @@ public: const ZTXOSelector& selector, int32_t minDepth) const; - PrepareTransactionResult PrepareTransaction( + tl::expected + PrepareTransaction( CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, From c6001268c54c1c42a37ccec37d3b9ee93f13fae9 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 11 Apr 2023 12:55:28 -0600 Subject: [PATCH 04/10] 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()); } From 1c005916996493bcfaac4b28879afe5359f85dcd Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Wed, 12 Apr 2023 10:32:30 -0600 Subject: [PATCH 05/10] Fix accidental reversion of #6409 Rearranging some code caused a partial reversion. --- src/wallet/wallet_tx_builder.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 14ac949df..fe6f4b3fe 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -35,10 +35,7 @@ WalletTxBuilder::GetChangeAddress( switch (rtype) { case ReceiverType::P2PKH: case ReceiverType::P2SH: - // TODO: This is the correct policy, but it’s a breaking change from - // previous behavior, so enable it separately. (#6409) - // if (strategy.AllowRevealedRecipients()) { - if (!spendable.utxos.empty() || strategy.AllowRevealedRecipients()) { + if (strategy.AllowRevealedRecipients()) { result.insert(OutputPool::Transparent); } break; From 2596b4920bf079575f53f422d03041f8fcfb5985 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 13 Apr 2023 08:59:00 -0600 Subject: [PATCH 06/10] Fix edge case revealed by #6409 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without `AllowRevealedRecipients`, we can’t send transparent change, but previously we asserted if we couldn’t get a transparent change address. Now it returns a new `TransparentChangeNotAllowed` failure, which is just a more specific `TransparentRecipientNotAllowed` to avoid confusion when there are no explicit unshielded recipients. --- qa/rpc-tests/wallet_shieldingcoinbase.py | 2 +- qa/rpc-tests/wallet_z_sendmany.py | 19 +++++ src/wallet/asyncrpcoperation_common.cpp | 8 ++ src/wallet/wallet_tx_builder.cpp | 99 ++++++++++++++++-------- src/wallet/wallet_tx_builder.h | 4 +- 5 files changed, 99 insertions(+), 33 deletions(-) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 44a0ffcdf..2d0f62971 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -105,7 +105,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): recipients = [] recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')}) - myopid = self.nodes[0].z_sendmany(mytaddr, recipients, 10, DEFAULT_FEE, 'AllowRevealedSenders') + myopid = self.nodes[0].z_sendmany(mytaddr, recipients, 10, DEFAULT_FEE, 'AllowFullyTransparent') error_result = wait_and_assert_operationid_status_result( self.nodes[0], myopid, "failed", diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 9b78e1364..7dc57757c 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -494,5 +494,24 @@ class WalletZSendmanyTest(BitcoinTestFramework): {'pools': {'orchard': {'valueZat': 200000000}}, 'minimum_confirmations': 1}, self.nodes[0].z_getbalanceforaccount(n0account0)) + + self.sync_all() + self.nodes[1].generate(1) + self.sync_all() + + # + # Test transparent change + # + + recipients = [{"address":n0ua1, "amount": 4}] + # Should fail because this generates transparent change, but we don’t have + # `AllowRevealedRecipients` + opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0, 'AllowRevealedSenders') + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', "This transaction would have transparent change, which is not enabled by default because it will publicly reveal the change address 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.") + + # Should succeed once we include `AllowRevealedRecipients` + opid = self.nodes[2].z_sendmany(mytaddr, recipients, 1, 0, 'AllowFullyTransparent') + wait_and_assert_operationid_status(self.nodes[2], opid) + if __name__ == '__main__': WalletZSendmanyTest().main() diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 30aeee804..f0514ab18 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -62,6 +62,14 @@ void ThrowInputSelectionError( "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."); + case AddressResolutionError::TransparentChangeNotAllowed: + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "This transaction would have transparent change, which is not " + "enabled by default because it will publicly reveal the change " + "address 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."); case AddressResolutionError::RevealingSaplingAmountNotAllowed: throw JSONRPCError( RPC_INVALID_PARAMETER, diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index fe6f4b3fe..ae8f11f2f 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -13,7 +13,7 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) return nextBlockHeight - anchorConfirmations; } -ChangeAddress +tl::expected WalletTxBuilder::GetChangeAddress( CWallet& wallet, const ZTXOSelector& selector, @@ -55,15 +55,20 @@ WalletTxBuilder::GetChangeAddress( return result; }; - auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) { + auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) + -> tl::expected { auto addr = wallet.GenerateChangeAddressForAccount( sendFromAccount, getAllowedChangePools(receiverTypes)); - assert(addr.has_value()); - return addr.value(); + if (addr.has_value()) { + return {addr.value()}; + } else { + return tl::make_unexpected(AddressResolutionError::TransparentChangeNotAllowed); + } }; - auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) -> RecipientAddress { + auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) + -> tl::expected { // for Sapling, if using a legacy address, return change to the // originating address; otherwise return it to the Sapling internal // address corresponding to the UFVK. @@ -80,45 +85,48 @@ WalletTxBuilder::GetChangeAddress( auto changeAddressForZUFVK = [&]( const ZcashdUnifiedFullViewingKey& zufvk, - const std::set& receiverTypes) { + const std::set& receiverTypes) + -> tl::expected { auto addr = zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes)); assert(addr.has_value()); return addr.value(); }; return examine(selector.GetPattern(), match { - [&](const CKeyID&) -> ChangeAddress { + [&](const CKeyID&) { return changeAddressForTransparentSelector({ReceiverType::P2PKH}); }, - [&](const CScriptID&) -> ChangeAddress { + [&](const CScriptID&) { return changeAddressForTransparentSelector({ReceiverType::P2SH}); }, - [](const libzcash::SproutPaymentAddress& addr) -> ChangeAddress { + [](const libzcash::SproutPaymentAddress& addr) + -> tl::expected { // for Sprout, we return change to the originating address using the tx builder. return addr; }, - [](const libzcash::SproutViewingKey& vk) -> ChangeAddress { + [](const libzcash::SproutViewingKey& vk) + -> tl::expected { // for Sprout, we return change to the originating address using the tx builder. return vk.address(); }, - [&](const libzcash::SaplingPaymentAddress& addr) -> ChangeAddress { + [&](const libzcash::SaplingPaymentAddress& addr) { return changeAddressForSaplingAddress(addr); }, - [&](const libzcash::SaplingExtendedFullViewingKey& fvk) -> ChangeAddress { + [&](const libzcash::SaplingExtendedFullViewingKey& fvk) { return changeAddressForSaplingAddress(fvk.DefaultAddress()); }, - [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress { + [&](const libzcash::UnifiedAddress& ua) { auto zufvk = wallet.GetUFVKForAddress(ua); assert(zufvk.has_value()); return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); }, - [&](const libzcash::UnifiedFullViewingKey& fvk) -> ChangeAddress { + [&](const libzcash::UnifiedFullViewingKey& fvk) { return changeAddressForZUFVK( ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk), fvk.GetKnownReceiverTypes()); }, - [&](const AccountZTXOPattern& acct) -> ChangeAddress { - auto addr = wallet.GenerateChangeAddressForAccount( + [&](const AccountZTXOPattern& acct) -> tl::expected { + auto addr = wallet.GenerateChangeAddressForAccount( acct.GetAccountId(), getAllowedChangePools(acct.GetReceiverTypes())); assert(addr.has_value()); @@ -297,11 +305,21 @@ InvalidFundsError ReportInvalidFunds( : InvalidFundsReason(InsufficientFundsError(targetAmount)))); } -static void -AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CAmount changeAmount) +static tl::expected +AddChangePayment( + const SpendableInputs& spendable, + Payments& resolvedPayments, + const ChangeAddress& changeAddr, + CAmount changeAmount, + CAmount targetAmount) { assert(changeAmount > 0); + // When spending transparent coinbase outputs, all inputs must be fully consumed. + if (spendable.HasTransparentCoinbase()) { + return tl::make_unexpected(ChangeNotAllowedError(spendable.Total(), targetAmount)); + } + examine(changeAddr, match { // TODO: Once we can add Sprout change to `resolvedPayments`, we don’t need to pass // `changeAddr` around the rest of these functions. @@ -312,6 +330,8 @@ AddChangePayment(Payments& resolvedPayments, const ChangeAddress& changeAddr, CA ResolvedPayment(std::nullopt, sendTo, changeAmount, std::nullopt, true)); } }); + + return {}; } /// On the initial call, we haven’t yet selected inputs, so we assume the outputs dominate the @@ -344,11 +364,12 @@ WalletTxBuilder::IterateLimit( CAmount bumpTargetAmount{0}; std::optional changeAddr; CAmount changeAmount{0}; + CAmount targetAmount{0}; do { spendableMut = spendable; - auto targetAmount{sendAmount + updatedFee}; + targetAmount = sendAmount + updatedFee; // TODO: the set of recipient pools is not quite sufficient information here; we should // probably perform note selection at the same time as we're performing resolved payment @@ -364,13 +385,19 @@ WalletTxBuilder::IterateLimit( // fresh) and once we generate it, hold onto it. But we still don’t have a guarantee // that we won’t end up discarding it. if (changeAmount > 0 && !changeAddr.has_value()) { - changeAddr = GetChangeAddress( + auto possibleChangeAddr = GetChangeAddress( wallet, selector, spendableMut, resolved, strategy, afterNU5); + + if (possibleChangeAddr.has_value()) { + changeAddr = possibleChangeAddr.value(); + } else { + return tl::make_unexpected(possibleChangeAddr.error()); + } } previousFee = updatedFee; updatedFee = CalcZIP317Fee( @@ -399,7 +426,12 @@ WalletTxBuilder::IterateLimit( if (changeAmount > 0) { assert(changeAddr.has_value()); - AddChangePayment(resolved, changeAddr.value(), changeAmount); + + auto changeRes = + AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount); + if (!changeRes.has_value()) { + return tl::make_unexpected(changeRes.error()); + } } return std::make_tuple(spendableMut, updatedFee, changeAddr); @@ -557,14 +589,24 @@ WalletTxBuilder::ResolveInputsAndPayments( changeAmount)); } if (changeAmount > 0) { - changeAddr = GetChangeAddress( + auto possibleChangeAddr = GetChangeAddress( wallet, selector, spendableMut, resolved, strategy, afterNU5); - AddChangePayment(resolved, changeAddr.value(), changeAmount); + + if (possibleChangeAddr.has_value()) { + changeAddr = possibleChangeAddr.value(); + } else { + return tl::make_unexpected(possibleChangeAddr.error()); + } + auto changeRes = + AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount); + if (!changeRes.has_value()) { + return tl::make_unexpected(changeRes.error()); + } } } else { auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5); @@ -576,14 +618,9 @@ WalletTxBuilder::ResolveInputsAndPayments( } } - // When spending transparent coinbase outputs, all inputs must be fully - // consumed, and they may only be sent to shielded recipients. - if (spendableMut.HasTransparentCoinbase()) { - if (spendableMut.Total() != targetAmount) { - return tl::make_unexpected(ChangeNotAllowedError(spendableMut.Total(), targetAmount)); - } else if (resolved.HasTransparentRecipient()) { - return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); - } + // When spending transparent coinbase outputs they may only be sent to shielded recipients. + if (spendableMut.HasTransparentCoinbase() && resolved.HasTransparentRecipient()) { + return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); } if (spendableMut.orchardNoteMetadata.size() > this->maxOrchardActions) { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 4ac3e2f08..387313136 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -225,6 +225,8 @@ enum class AddressResolutionError { SproutRecipientsNotSupported, //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedRecipients` TransparentRecipientNotAllowed, + //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedRecipients` + TransparentChangeNotAllowed, //! Requested `PrivacyPolicy` doesn’t include `AllowRevealedAmounts`, but we don’t have enough //! Sapling funds to avoid revealing amounts RevealingSaplingAmountNotAllowed, @@ -338,7 +340,7 @@ private: */ CAmount DefaultDustThreshold() const; - ChangeAddress + tl::expected GetChangeAddress( CWallet& wallet, const ZTXOSelector& selector, From c54c4ee987bb5b9f46b9c48467d0e93201c69538 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 13 Apr 2023 18:38:21 -0600 Subject: [PATCH 07/10] Adjust wallet absurd fee check for ZIP 317 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It now happens async (in `PrepareTransaction`) and ensures that the fee doesn’t exceed the maximum useful value for a transaction. --- qa/rpc-tests/test_framework/zip317.py | 5 +++++ qa/rpc-tests/wallet_parsing_amounts.py | 13 ------------- qa/rpc-tests/wallet_shieldingcoinbase.py | 15 +++++++-------- src/wallet/asyncrpcoperation_common.cpp | 13 +++++++++++++ src/wallet/rpcwallet.cpp | 21 +-------------------- src/wallet/wallet_tx_builder.cpp | 18 +++++++++++++++++- src/wallet/wallet_tx_builder.h | 12 ++++++++++++ 7 files changed, 55 insertions(+), 42 deletions(-) diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py index b5192f7eb..93fd12dc9 100644 --- a/qa/rpc-tests/test_framework/zip317.py +++ b/qa/rpc-tests/test_framework/zip317.py @@ -19,6 +19,11 @@ MARGINAL_FEE = 5000 # https://zips.z.cash/zip-0317#fee-calculation. GRACE_ACTIONS = 2 +# Limits the relative probability of picking a given transaction to be at most `WEIGHT_RATIO_CAP` +# times greater than a transaction that pays exactly the conventional fee. See +# https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction +WEIGHT_RATIO_CAP = 4 + # The Zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is # required. ZIP_317_FEE = -1 diff --git a/qa/rpc-tests/wallet_parsing_amounts.py b/qa/rpc-tests/wallet_parsing_amounts.py index bdfdf42c3..ff792b934 100755 --- a/qa/rpc-tests/wallet_parsing_amounts.py +++ b/qa/rpc-tests/wallet_parsing_amounts.py @@ -70,19 +70,6 @@ class WalletAmountParsingTest(BitcoinTestFramework): print(errorString) assert(False) - # This fee is larger than the default fee and since amount=0 - # it should trigger error - fee = Decimal('0.1') - recipients = [ {"address": myzaddr, "amount": Decimal('0.0') } ] - minconf = 1 - errorString = '' - - try: - myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee) - except JSONRPCException as e: - errorString = e.error['message'] - assert('Small transaction amount' in errorString) - # This fee is less than default and greater than amount, but still valid fee = Decimal('0.0000001') recipients = [ {"address": myzaddr, "amount": Decimal('0.00000001') } ] diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 2d0f62971..1ff7050a0 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 conventional_fee, ZIP_317_FEE +from test_framework.zip317 import conventional_fee, WEIGHT_RATIO_CAP, ZIP_317_FEE import sys import timeit @@ -299,9 +299,11 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): self.nodes[1].generate(1) self.sync_all() + many_recipient_fee = conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change + # check balance node2balance = amount_per_recipient * num_t_recipients - saplingvalue -= node2balance + conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change + saplingvalue -= node2balance + many_recipient_fee assert_equal(self.nodes[2].getbalance(), node2balance) check_value_pool(self.nodes[0], 'sapling', saplingvalue) @@ -320,12 +322,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): errorString = e.error['message'] assert_equal("Amount out of range" in errorString, True) - # Send will fail because fee is larger than sum of outputs - try: - self.nodes[0].z_sendmany(myzaddr, recipients, 1, (amount_per_recipient * num_t_recipients) + Decimal('0.00000001')) - except JSONRPCException as e: - errorString = e.error['message'] - assert_equal("is greater than the sum of outputs" in errorString, True) + # Send will fail because fee is more than `WEIGHT_RATIO_CAP * conventional_fee` + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, (WEIGHT_RATIO_CAP * many_recipient_fee) + Decimal('0.00000001')) + wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Fee 0.40000001 is greater than 4 times the conventional fee for this tx (which is 0.10). There is no prioritization benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) and likely indicates a mistake in setting the fee.') # Send will succeed because the balance of non-coinbase utxos is 10.0 try: diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index f0514ab18..87f3b5243 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -4,6 +4,7 @@ #include "init.h" #include "rpc/protocol.h" #include "util/moneystr.h" +#include "zip317.h" extern UniValue signrawtransaction(const UniValue& params, bool fHelp); @@ -166,6 +167,18 @@ void ThrowInputSelectionError( "The proposed transaction would result in %s in change.", FormatMoney(err.available - err.required))); }, + [](const AbsurdFeeError& err) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + strprintf( + "Fee %s is greater than %d times the conventional fee for this tx (which is " + "%s). There is no prioritization benefit to a fee this large (see " + "https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) " + "and likely indicates a mistake in setting the fee.", + FormatMoney(err.fixedFee), + WEIGHT_RATIO_CAP, + FormatMoney(err.conventionalFee))); + }, [](const ExcessOrchardActionsError& err) { std::string side; switch (err.side) { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7acf6fbcc..2958b11cb 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5031,26 +5031,7 @@ 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() != -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 - // or anything less than DEFAULT_FEE instead of being forced to use a custom fee and leak metadata - if (nTotalOut < DEFAULT_FEE) { - if (fixedFee > DEFAULT_FEE) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(fixedFee), FormatMoney(DEFAULT_FEE))); - } - } else { - // Check that the user specified fee is not absurd. - if (fixedFee > nTotalOut) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(fixedFee), FormatMoney(nTotalOut))); - } - } - nFee = fixedFee; + nFee = AmountFromValue( params[3] ); } // Use input parameters as the optional context info to be returned by z_getoperationstatus and z_getoperationresult. diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index ae8f11f2f..63bacc4e3 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -602,11 +602,27 @@ WalletTxBuilder::ResolveInputsAndPayments( } else { return tl::make_unexpected(possibleChangeAddr.error()); } + + // TODO: This duplicates the check in the `else` branch of the containing `if`. Until we + // can add Sprout change to `Payments` (#5660), we need to check this before + // adding the change payment. We can remove this check and make the later one + // unconditional once that’s fixed. + auto conventionalFee = + CalcZIP317Fee(spendableMut, resolved.GetResolvedPayments(), changeAddr); + if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) { + return tl::make_unexpected(AbsurdFeeError(conventionalFee, finalFee)); + } auto changeRes = AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount); if (!changeRes.has_value()) { return tl::make_unexpected(changeRes.error()); } + } else { + auto conventionalFee = + CalcZIP317Fee(spendableMut, resolved.GetResolvedPayments(), std::nullopt); + if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) { + return tl::make_unexpected(AbsurdFeeError(resolved.Total(), finalFee)); + } } } else { auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5); @@ -915,7 +931,7 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild( // TODO: We currently can’t store Sprout change in `Payments`, so we only validate the // spend/output balance in the case that `TransactionBuilder` doesn’t need to // (re)calculate the change. In future, we shouldn’t rely on `TransactionBuilder` ever - // calculating change. + // calculating change. (#5660) if (changeAddr.has_value()) { examine(changeAddr.value(), match { [&](const SproutPaymentAddress& addr) { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 387313136..3c1e53b5d 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -290,6 +290,17 @@ public: available(available), required(required) { } }; +/// Error when a fee is higher than can be useful. This reduces the chance of accidentally +/// overpaying with explicit fees. +class AbsurdFeeError { +public: + CAmount conventionalFee; + CAmount fixedFee; + + AbsurdFeeError(CAmount conventionalFee, CAmount fixedFee): + conventionalFee(conventionalFee), fixedFee(fixedFee) { } +}; + enum ActionSide { Input, Output, @@ -310,6 +321,7 @@ typedef std::variant< AddressResolutionError, InvalidFundsError, ChangeNotAllowedError, + AbsurdFeeError, ExcessOrchardActionsError> InputSelectionError; class InputSelection { From 0b9b5c8dc11f58a4be59edca7a41686039bfc2c9 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 13 Apr 2023 19:16:35 -0600 Subject: [PATCH 08/10] Address review feedback for ZIP 317 fees in wallet --- qa/rpc-tests/test_framework/zip317.py | 4 +- src/wallet/asyncrpcoperation_common.cpp | 4 +- src/wallet/rpcwallet.cpp | 2 +- src/wallet/wallet_tx_builder.cpp | 64 +++++++++++-------------- src/wallet/wallet_tx_builder.h | 10 ++-- src/zip317.cpp | 10 ++-- 6 files changed, 45 insertions(+), 49 deletions(-) diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py index 93fd12dc9..dbe8f7e51 100644 --- a/qa/rpc-tests/test_framework/zip317.py +++ b/qa/rpc-tests/test_framework/zip317.py @@ -12,7 +12,7 @@ from test_framework.mininode import COIN from decimal import Decimal -# The fee per logical action, in ZAT. See https://zips.z.cash/zip-0317#fee-calculation. +# The fee per logical action, in zatoshis. 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 @@ -24,7 +24,7 @@ GRACE_ACTIONS = 2 # https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction WEIGHT_RATIO_CAP = 4 -# The Zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is +# The zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is # required. ZIP_317_FEE = -1 diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 87f3b5243..796a58d81 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -124,7 +124,7 @@ void ThrowInputSelectionError( "Insufficient funds: have %s, %s", FormatMoney(err.available), examine(err.reason, match { - [](const QuasiChangeError& qce) { + [](const PhantomChangeError& qce) { return strprintf( "need %s more to surpass the dust threshold and avoid being " "forced to over-pay the fee. Alternatively, you could specify " @@ -137,7 +137,7 @@ void ThrowInputSelectionError( return strprintf("need %s", FormatMoney(ife.required)); }, // TODO: Add the fee here, so we can suggest specifying an explicit fee (see - // `QuasiChangeError`). + // `PhantomChangeError`). [](const DustThresholdError& dte) { return strprintf( "need %s more to avoid creating invalid change output %s (dust threshold is %s)", diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2958b11cb..27d0732d2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4813,7 +4813,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) " the output is being sent to a transparent address, it’s an error to include this field.\n" " }, ... ]\n" "3. minconf (numeric, optional, default=" + strprintf("%u", DEFAULT_NOTE_CONFIRMATIONS) + ") Only use funds confirmed at least this many times.\n" - "4. fee (numeric, optional, default=-1) The fee amount to attach to this transaction. The default behavior\n" + "4. fee (numeric, optional, default=-1) The fee amount in " + CURRENCY_UNIT + " to attach to this transaction. The default behavior\n" " is to use a fee calculated according to ZIP 317.\n" "5. privacyPolicy (string, optional, default=\"LegacyCompat\") Policy for what information leakage is acceptable.\n" " One of the following strings:\n" diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 63bacc4e3..057274614 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -200,31 +200,26 @@ SpendableInputs WalletTxBuilder::FindAllSpendableInputs( return wallet.FindSpendableInputs(selector, minDepth, std::nullopt); } -static size_t NotOne(size_t n) +static size_t PadCount(size_t n) { return n == 1 ? 2 : n; } -/// ISSUES: -/// • z_shieldcoinbase currently checks the fee in advance of calling PrepareTransaction in order to -/// determine the payment amount. But that fee possibly isn’t correct. We could perhaps have _no_ -/// payments in z_shieldcoinbase, but provide an explicit change address? (Don’t expose this via -/// RPC, and give it a different name, e.g. `NetFundsRecipient` in the WalletTxBuilder interface) static CAmount CalcZIP317Fee( const std::optional& inputs, const std::vector& payments, const std::optional& changeAddr) { - std::vector vouts{}; + std::vector vout{}; size_t sproutOutputCount{}, saplingOutputCount{}, orchardOutputCount{}; for (const auto& payment : payments) { std::visit(match { [&](const CKeyID& addr) { - vouts.emplace_back(payment.amount, GetScriptForDestination(addr)); + vout.emplace_back(payment.amount, GetScriptForDestination(addr)); }, [&](const CScriptID& addr) { - vouts.emplace_back(payment.amount, GetScriptForDestination(addr)); + vout.emplace_back(payment.amount, GetScriptForDestination(addr)); }, [&](const libzcash::SaplingPaymentAddress&) { ++saplingOutputCount; @@ -241,10 +236,10 @@ CalcZIP317Fee( [&](const RecipientAddress& addr) { examine(addr, match { [&](const CKeyID& taddr) { - vouts.emplace_back(0, GetScriptForDestination(taddr)); + vout.emplace_back(0, GetScriptForDestination(taddr)); }, [&](const CScriptID taddr) { - vouts.emplace_back(0, GetScriptForDestination(taddr)); + vout.emplace_back(0, GetScriptForDestination(taddr)); }, [&](const libzcash::SaplingPaymentAddress&) { ++saplingOutputCount; }, [&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; } @@ -253,38 +248,34 @@ CalcZIP317Fee( }); } - size_t logicalActionCount; + std::vector vin{}; + size_t sproutInputCount = 0; + size_t saplingInputCount = 0; + size_t orchardInputCount = 0; if (inputs.has_value()) { - std::vector vins{}; - for (const auto& utxo : inputs.value().utxos) { - vins.emplace_back( + for (const auto& utxo : inputs->utxos) { + vin.emplace_back( COutPoint(utxo.tx->GetHash(), utxo.i), utxo.tx->vout[utxo.i].scriptPubKey); } - logicalActionCount = CalculateLogicalActionCount( - vins, - vouts, - std::max(inputs.value().sproutNoteEntries.size(), sproutOutputCount), - inputs.value().saplingNoteEntries.size(), - NotOne(saplingOutputCount), - NotOne(std::max(inputs.value().orchardNoteMetadata.size(), orchardOutputCount))); - - } else { - logicalActionCount = CalculateLogicalActionCount( - {}, - vouts, - sproutOutputCount, - 0, - NotOne(saplingOutputCount), - NotOne(orchardOutputCount)); + sproutInputCount = inputs->sproutNoteEntries.size(); + saplingInputCount = inputs->saplingNoteEntries.size(); + orchardInputCount = inputs->orchardNoteMetadata.size(); } + size_t logicalActionCount = CalculateLogicalActionCount( + vin, + vout, + std::max(sproutInputCount, sproutOutputCount), + saplingInputCount, + PadCount(saplingOutputCount), + PadCount(std::max(orchardInputCount, orchardOutputCount))); return CalculateConventionalFee(logicalActionCount); } InvalidFundsError ReportInvalidFunds( const SpendableInputs& spendable, - bool hasQuasiChange, + bool hasPhantomChange, CAmount fee, CAmount dustThreshold, CAmount targetAmount, @@ -292,9 +283,9 @@ InvalidFundsError ReportInvalidFunds( { return InvalidFundsError( spendable.Total(), - hasQuasiChange + hasPhantomChange // TODO: NEED TESTS TO EXERCISE THIS - ? InvalidFundsReason(QuasiChangeError(fee, dustThreshold)) + ? InvalidFundsReason(PhantomChangeError(fee, dustThreshold)) : (changeAmount > 0 && changeAmount < dustThreshold // TODO: we should provide the option for the caller to explicitly forego change // (definitionally an amount below the dust amount) and send the extra to the @@ -367,6 +358,7 @@ WalletTxBuilder::IterateLimit( CAmount targetAmount{0}; do { + // NB: This makes a fresh copy so that we start from the full set of notes when we re-limit. spendableMut = spendable; targetAmount = sendAmount + updatedFee; @@ -459,8 +451,8 @@ WalletTxBuilder::ResolveInputsAndPayments( CAmount maxOrchardAvailable = spendableMut.GetOrchardTotal(); uint32_t orchardOutputs{0}; - // we can only select Orchard addresses if there are sufficient non-Sprout - // funds to cover the total payments + fee. + // we can only select Orchard addresses if we’re not sending from Sprout, since there is no tx + // version where both Sprout and Orchard are valid. bool canResolveOrchard = afterNU5 && !selector.SelectsSprout(); std::vector resolvedPayments; std::optional resolutionError; diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 3c1e53b5d..d44b665bc 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -241,12 +241,16 @@ enum class AddressResolutionError { RevealingReceiverAmountsNotAllowed, }; -class QuasiChangeError { +/// Phantom change is change that appears to exist until we add the output for it, at which point it +/// is consumed by the increase to the conventional fee. When we are at the limit of selectable +/// notes, this makes it impossible to create the transaction without either creating a 0-valued +/// output or overpaying the fee. +class PhantomChangeError { public: CAmount finalFee; CAmount dustThreshold; - QuasiChangeError(CAmount finalFee, CAmount dustThreshold): + PhantomChangeError(CAmount finalFee, CAmount dustThreshold): finalFee(finalFee), dustThreshold(dustThreshold) { } }; @@ -268,7 +272,7 @@ public: }; typedef std::variant< - QuasiChangeError, + PhantomChangeError, InsufficientFundsError, DustThresholdError> InvalidFundsReason; diff --git a/src/zip317.cpp b/src/zip317.cpp index b78ab1a61..b9b65e7c8 100644 --- a/src/zip317.cpp +++ b/src/zip317.cpp @@ -15,9 +15,9 @@ CAmount CalculateConventionalFee(size_t logicalActionCount) { } template -size_t GetUTXOFieldSize(const std::vector& utxos) { - auto size = GetSerializeSize(utxos, SER_NETWORK, PROTOCOL_VERSION); - auto countSize = GetSizeOfCompactSize(utxos.size()); +static size_t GetTxIOFieldSize(const std::vector& txIOs) { + auto size = GetSerializeSize(txIOs, SER_NETWORK, PROTOCOL_VERSION); + auto countSize = GetSizeOfCompactSize(txIOs.size()); return size - countSize; } @@ -28,8 +28,8 @@ size_t CalculateLogicalActionCount( unsigned int saplingSpendCount, unsigned int saplingOutputCount, unsigned int orchardActionCount) { - const size_t tx_in_total_size = GetUTXOFieldSize(vin); - const size_t tx_out_total_size = GetUTXOFieldSize(vout); + const size_t tx_in_total_size = GetTxIOFieldSize(vin); + const size_t tx_out_total_size = GetTxIOFieldSize(vout); return std::max(ceil_div(tx_in_total_size, P2PKH_STANDARD_INPUT_SIZE), ceil_div(tx_out_total_size, P2PKH_STANDARD_OUTPUT_SIZE)) + From 6ae7c532cebf29890368a1543f52631687e600ab Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Fri, 14 Apr 2023 19:49:58 +0100 Subject: [PATCH 09/10] Change spelling of prioritisation in an error message For consistency with the `prioritisetransaction` RPC method. --- qa/rpc-tests/wallet_shieldingcoinbase.py | 2 +- src/wallet/asyncrpcoperation_common.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 1ff7050a0..f3b9b15be 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/qa/rpc-tests/wallet_shieldingcoinbase.py @@ -324,7 +324,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework): # Send will fail because fee is more than `WEIGHT_RATIO_CAP * conventional_fee` myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, (WEIGHT_RATIO_CAP * many_recipient_fee) + Decimal('0.00000001')) - wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Fee 0.40000001 is greater than 4 times the conventional fee for this tx (which is 0.10). There is no prioritization benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) and likely indicates a mistake in setting the fee.') + wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Fee 0.40000001 is greater than 4 times the conventional fee for this tx (which is 0.10). There is no prioritisation benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction), and it likely indicates a mistake in setting the fee.') # Send will succeed because the balance of non-coinbase utxos is 10.0 try: diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 796a58d81..0bd6e6037 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -172,9 +172,9 @@ void ThrowInputSelectionError( RPC_INVALID_PARAMETER, strprintf( "Fee %s is greater than %d times the conventional fee for this tx (which is " - "%s). There is no prioritization benefit to a fee this large (see " - "https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) " - "and likely indicates a mistake in setting the fee.", + "%s). There is no prioritisation benefit to a fee this large (see " + "https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction), " + "and it likely indicates a mistake in setting the fee.", FormatMoney(err.fixedFee), WEIGHT_RATIO_CAP, FormatMoney(err.conventionalFee))); From d78cee968006a309672e0fe9fe81acdc19c0ef97 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Fri, 14 Apr 2023 12:57:07 -0600 Subject: [PATCH 10/10] Address more ZIP 317 fee feedback --- src/wallet/rpcwallet.cpp | 3 +-- src/wallet/wallet_tx_builder.cpp | 41 +++++++++++++++++--------------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 27d0732d2..560ab9ac4 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5028,7 +5028,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Minimum confirmations int nMinDepth = parseMinconf(DEFAULT_NOTE_CONFIRMATIONS, params, 2, std::nullopt); - // Fee in Zatoshis, not currency format) std::optional nFee; if (params.size() > 3 && params[3].get_real() != -1.0) { nFee = AmountFromValue( params[3] ); @@ -5040,7 +5039,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) o.pushKV("amounts", params[1]); o.pushKV("minconf", nMinDepth); if (nFee.has_value()) { - o.pushKV("fee", std::stod(FormatMoney(nFee.value()))); + o.pushKV("fee", ValueFromAmount(nFee.value())); } UniValue contextInfo = o; diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 057274614..1576d1ba2 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -68,7 +68,7 @@ WalletTxBuilder::GetChangeAddress( }; auto changeAddressForSaplingAddress = [&](const libzcash::SaplingPaymentAddress& addr) - -> tl::expected { + -> RecipientAddress { // for Sapling, if using a legacy address, return change to the // originating address; otherwise return it to the Sapling internal // address corresponding to the UFVK. @@ -85,8 +85,7 @@ WalletTxBuilder::GetChangeAddress( auto changeAddressForZUFVK = [&]( const ZcashdUnifiedFullViewingKey& zufvk, - const std::set& receiverTypes) - -> tl::expected { + const std::set& receiverTypes) { auto addr = zufvk.GetChangeAddress(getAllowedChangePools(receiverTypes)); assert(addr.has_value()); return addr.value(); @@ -109,18 +108,22 @@ WalletTxBuilder::GetChangeAddress( // for Sprout, we return change to the originating address using the tx builder. return vk.address(); }, - [&](const libzcash::SaplingPaymentAddress& addr) { + [&](const libzcash::SaplingPaymentAddress& addr) + -> tl::expected { return changeAddressForSaplingAddress(addr); }, - [&](const libzcash::SaplingExtendedFullViewingKey& fvk) { + [&](const libzcash::SaplingExtendedFullViewingKey& fvk) + -> tl::expected { return changeAddressForSaplingAddress(fvk.DefaultAddress()); }, - [&](const libzcash::UnifiedAddress& ua) { + [&](const libzcash::UnifiedAddress& ua) + -> tl::expected { auto zufvk = wallet.GetUFVKForAddress(ua); assert(zufvk.has_value()); return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); }, - [&](const libzcash::UnifiedFullViewingKey& fvk) { + [&](const libzcash::UnifiedFullViewingKey& fvk) + -> tl::expected { return changeAddressForZUFVK( ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(params, fvk), fvk.GetKnownReceiverTypes()); @@ -253,14 +256,14 @@ CalcZIP317Fee( size_t saplingInputCount = 0; size_t orchardInputCount = 0; if (inputs.has_value()) { - for (const auto& utxo : inputs->utxos) { + for (const auto& utxo : inputs.value().utxos) { vin.emplace_back( COutPoint(utxo.tx->GetHash(), utxo.i), utxo.tx->vout[utxo.i].scriptPubKey); } - sproutInputCount = inputs->sproutNoteEntries.size(); - saplingInputCount = inputs->saplingNoteEntries.size(); - orchardInputCount = inputs->orchardNoteMetadata.size(); + sproutInputCount = inputs.value().sproutNoteEntries.size(); + saplingInputCount = inputs.value().saplingNoteEntries.size(); + orchardInputCount = inputs.value().orchardNoteMetadata.size(); } size_t logicalActionCount = CalculateLogicalActionCount( vin, @@ -377,7 +380,7 @@ WalletTxBuilder::IterateLimit( // fresh) and once we generate it, hold onto it. But we still don’t have a guarantee // that we won’t end up discarding it. if (changeAmount > 0 && !changeAddr.has_value()) { - auto possibleChangeAddr = GetChangeAddress( + auto maybeChangeAddr = GetChangeAddress( wallet, selector, spendableMut, @@ -385,10 +388,10 @@ WalletTxBuilder::IterateLimit( strategy, afterNU5); - if (possibleChangeAddr.has_value()) { - changeAddr = possibleChangeAddr.value(); + if (maybeChangeAddr.has_value()) { + changeAddr = maybeChangeAddr.value(); } else { - return tl::make_unexpected(possibleChangeAddr.error()); + return tl::make_unexpected(maybeChangeAddr.error()); } } previousFee = updatedFee; @@ -581,7 +584,7 @@ WalletTxBuilder::ResolveInputsAndPayments( changeAmount)); } if (changeAmount > 0) { - auto possibleChangeAddr = GetChangeAddress( + auto maybeChangeAddr = GetChangeAddress( wallet, selector, spendableMut, @@ -589,10 +592,10 @@ WalletTxBuilder::ResolveInputsAndPayments( strategy, afterNU5); - if (possibleChangeAddr.has_value()) { - changeAddr = possibleChangeAddr.value(); + if (maybeChangeAddr.has_value()) { + changeAddr = maybeChangeAddr.value(); } else { - return tl::make_unexpected(possibleChangeAddr.error()); + return tl::make_unexpected(maybeChangeAddr.error()); } // TODO: This duplicates the check in the `else` branch of the containing `if`. Until we