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

Address review feedback re: ZIP 317 in wallet

This commit is contained in:
Greg Pfeil 2023-04-11 12:55:28 -06:00
parent dac6c014d4
commit c6001268c5
No known key found for this signature in database
GPG Key ID: 1193ACD196ED61F2
11 changed files with 55 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
qa/rpc-tests/mempool_limit.py
View File

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

4
qa/rpc-tests/mempool_tx_expiry.py
View File

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

17
qa/rpc-tests/test_framework/zip317.py
View File

@ -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

15
qa/rpc-tests/wallet_listreceived.py
View File

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

4
qa/rpc-tests/wallet_nullifiers.py
View File

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

18
qa/rpc-tests/wallet_shieldingcoinbase.py
View File

@ -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'))

4
qa/rpc-tests/wallet_treestate.py
View File

@ -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...

8
qa/rpc-tests/wallet_z_sendmany.py
View File

@ -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

4
src/wallet/asyncrpcoperation_common.cpp
View File

@ -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 "

9
src/wallet/rpcwallet.cpp
View File

@ -5030,13 +5030,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
// Fee in Zatoshis, not currency format)
std::optional<CAmount> 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

7
src/wallet/wallet_tx_builder.cpp
View File

@ -212,8 +212,7 @@ CalcZIP317Fee(
const std::optional<ChangeAddress>& changeAddr)
{
std::vector<CTxOut> 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());
}