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

Merge pull request #6265 from sellout/any_taddr-sans-coinbase 

Don’t select transparent coinbase with ANY_TADDR
This commit is contained in:
Kris Nuttycombe 2022-12-12 09:18:32 -07:00 committed by GitHub
parent 121f84c122 54ecf53619
commit 80a1366a05
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 80 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
doc/release-notes.md
View File

@ -4,3 +4,12 @@ release-notes at release time)
Notable changes
===============
RPC Changes
-----------
- `z_sendmany` will no longer select transparent coinbase when "ANY\_TADDR" is
used as the `fromaddress`. It was already documented to do this, but the
previous behavior didn’t match. When coinbase notes were selected in this
case, they would (properly) require that the transaction didn’t have any
change, but this could be confusing, as the documentation stated that these
two conditions (using "ANY\_TADDR" and disallowing change) wouldn’t coincide.

30
qa/rpc-tests/wallet_listunspent.py
View File

@ -80,51 +80,39 @@ class WalletListUnspent(BitcoinTestFramework):
opid = self.nodes[0].z_sendmany(
'ANY_TADDR',
# FIXME: #6262 The amount here _should_ be 2, but because of a
# bug in the selector for `ANY_TADDR`, it’s selecting
# transparent coinbase, which also means we can’t have
# change. When that bug is fixed, the test should fail
# here, and we can switch it back to 2 (and cascade the
# corrected amounts mentioned below.
[{'address': n1uaddr, 'amount': 10}],
[{'address': n1uaddr, 'amount': 2}],
1, 0, 'AllowRevealedSenders')
wait_and_assert_operationid_status(self.nodes[0], opid)
self.nodes[0].generate(2)
self.sync_all() # height 207
# FIXME: #6262, should be `expected_matured_at_height(207) - 10 + 10 - 2`
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(207) - 10 + 10 - 10)
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(207) - 10 + 10 - 2)
opid = self.nodes[0].z_sendmany(
'ANY_TADDR',
# FIXME: Should be 3 (see above)
[{'address': n1uaddr, 'amount': 10}],
[{'address': n1uaddr, 'amount': 3}],
1, 0, 'AllowRevealedSenders')
wait_and_assert_operationid_status(self.nodes[0], opid)
self.nodes[0].generate(2)
self.sync_all() # height 209
# FIXME: #6262, should be `expected_matured_at_height(209) - 10 + 10 - 2 - 3`
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(209) - 10 + 10 - 10 - 10)
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(209) - 10 + 10 - 2 - 3)
opid = self.nodes[0].z_sendmany(
'ANY_TADDR',
# FIXME: Should be 5 (see above)
[{'address': n1uaddr, 'amount': 10}],
[{'address': n1uaddr, 'amount': 5}],
1, 0, 'AllowRevealedSenders')
wait_and_assert_operationid_status(self.nodes[0], opid)
self.nodes[0].generate(2)
self.sync_all() # height 211
# FIXME: #6262, should be `expected_matured_at_height(211) - 10 + 10 - 2 - 3 - 5`
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(211) - 10 + 10 - 10 - 10 - 10)
assert_equal(self.nodes[0].getbalance(), expected_matured_at_height(211) - 10 + 10 - 2 - 3 - 5)
# check balances at various past points in the chain
# FIXME: #6262, change the comparison amounts when the above changes are made.
assert_equal(self.matured_at_height(205), expected_matured_at_height(205) - 10 + 10)
assert_equal(self.matured_at_height(207), expected_matured_at_height(207) - 10 + 10 - 10)
assert_equal(self.matured_at_height(209), expected_matured_at_height(209) - 10 + 10 - 10 - 10)
assert_equal(self.matured_at_height(211), expected_matured_at_height(211) - 10 + 10 - 10 - 10 - 10)
assert_equal(self.matured_at_height(207), expected_matured_at_height(207) - 10 + 10 - 2)
assert_equal(self.matured_at_height(209), expected_matured_at_height(209) - 10 + 10 - 2 - 3)
assert_equal(self.matured_at_height(211), expected_matured_at_height(211) - 10 + 10 - 2 - 3 - 5)
if __name__ == '__main__':
WalletListUnspent().main()

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

@ -97,7 +97,7 @@ class WalletSendManyAnyTaddr(BitcoinTestFramework):
# Check that ANY_TADDR note selection doesn't attempt a double-spend
myopid = self.nodes[3].z_sendmany('ANY_TADDR', [{'address': recipient, 'amount': 20}], 1)
wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "Insufficient funds: have 14.99998, need 20.00001")
wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "Insufficient funds: have 14.99998, need 20.00001; note that coinbase outputs will not be selected if you specify ANY_TADDR or if any transparent recipients are included.")
# Create an expired transaction on node 3.
self.split_network()
@ -123,7 +123,7 @@ class WalletSendManyAnyTaddr(BitcoinTestFramework):
assert_equal(0, self.nodes[2].getbalance())
# Check that ANY_TADDR doesn't select an expired output.
wait_and_assert_operationid_status(self.nodes[2], self.nodes[2].z_sendmany('ANY_TADDR', [{'address': recipient, 'amount': 13}]), "failed", "Insufficient funds: have 0.00, need 13.00001")
wait_and_assert_operationid_status(self.nodes[2], self.nodes[2].z_sendmany('ANY_TADDR', [{'address': recipient, 'amount': 13}]), "failed", "Insufficient funds: have 0.00, need 13.00001; note that coinbase outputs will not be selected if you specify ANY_TADDR or if any transparent recipients are included.")
if __name__ == '__main__':
WalletSendManyAnyTaddr().main()

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

@ -88,8 +88,8 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
errorString = e.error['message']
assert_equal("Invalid from address, no payment source found for address.", errorString);
# This send will fail because our consensus does not allow transparent change when
# shielding a coinbase utxo.
# This send will fail because our consensus does not allow transparent change when
# shielding a coinbase utxo.
# TODO: After upgrading to unified address support, change will be sent to the most
# recent shielded spend authority corresponding to the account of the source address
# and this send will succeed, causing this test to fail.
@ -98,9 +98,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
error_result = wait_and_assert_operationid_status_result(
self.nodes[0],
myopid, "failed",
"When shielding coinbase funds, the wallet does not allow any change. The proposed transaction would result in 8.76542211 in change.",
self.nodes[0],
myopid, "failed",
"When shielding coinbase funds, the wallet does not allow any change. The proposed transaction would result in 8.76542211 in change.",
10)
# Test that the returned status object contains a params field with the operation's input parameters
@ -225,7 +225,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
amount = Decimal('10.0') - DEFAULT_FEE - 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)")
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 or if any transparent recipients are included.")
# Send will fail because send amount is too big, even when including coinbase utxos
errorString = ""

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

@ -11,6 +11,7 @@ from test_framework.util import (
assert_greater_than,
assert_raises_message,
connect_nodes_bi,
get_coinbase_address,
nuparams,
start_nodes,
wait_and_assert_operationid_status,
@ -170,9 +171,8 @@ class WalletZSendmanyTest(BitcoinTestFramework):
n0account0 = self.nodes[0].z_getnewaccount()['account']
n0ua0 = self.nodes[0].z_getaddressforaccount(n0account0)['address']
# Change went to a fresh address, so use `ANY_TADDR` which
# should hold the rest of our transparent funds.
source = 'ANY_TADDR'
# Prepare to fund the UA from coinbase
source = get_coinbase_address(self.nodes[2])
recipients = []
recipients.append({"address":n0ua0, "amount":10})
@ -317,7 +317,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
# If we try to send 3 ZEC from n1ua0, it will fail with too-few funds.
recipients = [{"address":n0ua0, "amount":3}]
linked_addrs_msg = 'Insufficient funds: have 2.00, need 3.00 (This transaction may require selecting transparent coins that were sent to multiple Unified Addresses, which is not enabled by default because it would create a public link between the transparent receivers of these addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to allow this transaction to proceed anyway.)'
linked_addrs_msg = 'Insufficient funds: have 2.00, need 3.00. (This transaction may require selecting transparent coins that were sent to multiple Unified Addresses, which is not enabled by default because it would create a public link between the transparent receivers of these addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to allow this transaction to proceed anyway.)'
opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', linked_addrs_msg)
@ -357,12 +357,13 @@ class WalletZSendmanyTest(BitcoinTestFramework):
#
# Send some legacy transparent funds to n1ua0, creating Sapling outputs.
source = get_coinbase_address(self.nodes[2])
recipients = [{"address":n1ua0, "amount":10}]
# This requires the AllowRevealedSenders policy...
opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0)
opid = self.nodes[2].z_sendmany(source, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', revealed_senders_msg)
# ... which we can always override with the NoPrivacy policy.
opid = self.nodes[2].z_sendmany('ANY_TADDR', recipients, 1, 0, 'NoPrivacy')
opid = self.nodes[2].z_sendmany(source, recipients, 1, 0, 'NoPrivacy')
wait_and_assert_operationid_status(self.nodes[2], opid)
self.sync_all()
@ -396,7 +397,7 @@ class WalletZSendmanyTest(BitcoinTestFramework):
n0ua1 = self.nodes[0].z_getaddressforaccount(n0account0, ["orchard"])['address']
recipients = [{"address":n0ua1, "amount": 6}]
# Should fail under default and 'FullPrivacy' policies ...
# Should fail under default and 'FullPrivacy' policies ...
revealed_amounts_msg = 'Sending from the Sapling shielded pool to the Orchard shielded pool is not enabled by default because it will publicly reveal the transaction amount. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` parameter set to `AllowRevealedAmounts` or weaker if you wish to allow this transaction to proceed anyway.'
opid = self.nodes[1].z_sendmany(n1ua0, recipients, 1, 0)
wait_and_assert_operationid_status(self.nodes[1], opid, 'failed', revealed_amounts_msg)

56
src/wallet/asyncrpcoperation_sendmany.cpp
View File

@ -68,12 +68,10 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany(
for (const ResolvedPayment& recipient : recipients_) {
std::visit(match {
[&](const CKeyID& addr) {
transparentRecipients_ += 1;
txOutputAmounts_.t_outputs_total += recipient.amount;
recipientPools_.insert(OutputPool::Transparent);
},
[&](const CScriptID& addr) {
transparentRecipients_ += 1;
txOutputAmounts_.t_outputs_total += recipient.amount;
recipientPools_.insert(OutputPool::Transparent);
},
@ -182,7 +180,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
// Allow transparent coinbase inputs if there are no transparent
// recipients.
bool allowTransparentCoinbase = transparentRecipients_ == 0;
bool allowTransparentCoinbase = !recipientPools_.count(OutputPool::Transparent);
// Set the dust threshold so that we can select enough inputs to avoid
// creating dust change amounts.
@ -197,41 +195,39 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
}
if (!spendable.LimitToAmount(targetAmount, dustThreshold, recipientPools_)) {
CAmount changeAmount{spendable.Total() - targetAmount};
std::string insufficientFundsMessage =
strprintf("Insufficient funds: have %s", FormatMoney(spendable.Total()));
if (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)
throw JSONRPCError(
RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf(
"Insufficient funds: have %s, need %s more to avoid creating invalid change output %s "
"(dust threshold is %s)",
FormatMoney(spendable.Total()),
FormatMoney(dustThreshold - changeAmount),
FormatMoney(changeAmount),
FormatMoney(dustThreshold)));
// (Daira disagrees, as this could leak information to the recipient
// or to an external viewing key holder.)
insufficientFundsMessage +=
strprintf(
", need %s more to avoid creating invalid change output %s (dust threshold is %s)",
FormatMoney(dustThreshold - changeAmount),
FormatMoney(changeAmount),
FormatMoney(dustThreshold));
} else {
bool isFromUa = std::holds_alternative<libzcash::UnifiedAddress>(ztxoSelector_.GetPattern());
throw JSONRPCError(
RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf(
"Insufficient funds: have %s, need %s",
FormatMoney(spendable.Total()), FormatMoney(targetAmount))
+ (allowTransparentCoinbase ? "" :
"; note that coinbase outputs will not be selected if you specify "
"ANY_TADDR or if any transparent recipients are included.")
+ ((!isFromUa || strategy_.AllowLinkingAccountAddresses()) ? "" :
" (This transaction may require selecting transparent coins that were sent "
"to multiple Unified Addresses, which is not enabled by default because "
"it would create a public link between the transparent receivers of these "
"addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` "
"parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to "
"allow this transaction to proceed anyway.)")
);
insufficientFundsMessage += strprintf(", need %s", FormatMoney(targetAmount));
}
bool isFromUa = std::holds_alternative<libzcash::UnifiedAddress>(ztxoSelector_.GetPattern());
throw JSONRPCError(
RPC_WALLET_INSUFFICIENT_FUNDS,
insufficientFundsMessage
+ (allowTransparentCoinbase && ztxoSelector_.SelectsTransparentCoinbase() ? "." :
"; note that coinbase outputs will not be selected if you specify "
"ANY_TADDR or if any transparent recipients are included.")
+ ((!isFromUa || strategy_.AllowLinkingAccountAddresses()) ? "" :
" (This transaction may require selecting transparent coins that were sent "
"to multiple Unified Addresses, which is not enabled by default because "
"it would create a public link between the transparent receivers of these "
"addresses. THIS MAY AFFECT YOUR PRIVACY. Resubmit with the `privacyPolicy` "
"parameter set to `AllowLinkingAccountAddresses` or weaker if you wish to "
"allow this transaction to proceed anyway.)"));
}
if (!(spendable.utxos.empty() || strategy_.AllowRevealedSenders())) {

1
src/wallet/asyncrpcoperation_sendmany.h
View File

@ -72,7 +72,6 @@ private:
bool isfromsprout_{false};
bool isfromsapling_{false};
TransactionStrategy strategy_;
uint32_t transparentRecipients_{0};
AccountId sendFromAccount_;
std::set<OutputPool> recipientPools_;
TxOutputAmounts txOutputAmounts_;

20
src/wallet/wallet.cpp
View File

@ -2214,6 +2214,7 @@ SpendableInputs CWallet::FindSpendableInputs(
KeyIO keyIO(Params());
bool selectTransparent{selector.SelectsTransparent()};
bool selectTransparentCoinbase{selector.SelectsTransparentCoinbase()};
bool selectSprout{selector.SelectsSprout()};
bool selectSapling{selector.SelectsSapling()};
bool selectOrchard{selector.SelectsOrchard()};
@ -2229,7 +2230,7 @@ SpendableInputs CWallet::FindSpendableInputs(
if (selectTransparent &&
// skip transparent utxo selection if coinbase spend restrictions are not met
(!isCoinbase || (allowTransparentCoinbase && wtx.GetBlocksToMaturity(asOfHeight) <= 0))) {
(!isCoinbase || (selectTransparentCoinbase && allowTransparentCoinbase && wtx.GetBlocksToMaturity(asOfHeight) <= 0))) {
for (int i = 0; i < wtx.vout.size(); i++) {
const auto& output = wtx.vout[i];
@ -7822,6 +7823,23 @@ bool ZTXOSelector::SelectsTransparent() const {
[](const AccountZTXOPattern& acct) { return acct.IncludesP2PKH() || acct.IncludesP2SH(); }
}, this->pattern);
}
bool ZTXOSelector::SelectsTransparentCoinbase() const {
return std::visit(match {
[](const CKeyID& keyId) { return true; },
[](const CScriptID& scriptId) { return true; },
[](const libzcash::SproutPaymentAddress& addr) { return false; },
[](const libzcash::SproutViewingKey& vk) { return false; },
[](const libzcash::SaplingPaymentAddress& addr) { return false; },
[](const libzcash::SaplingExtendedFullViewingKey& vk) { return false; },
[](const libzcash::UnifiedAddress& ua) {
return ua.GetP2PKHReceiver().has_value() || ua.GetP2SHReceiver().has_value();
},
[](const libzcash::UnifiedFullViewingKey& ufvk) { return ufvk.GetTransparentKey().has_value(); },
[](const AccountZTXOPattern& acct) {
return (acct.IncludesP2PKH() || acct.IncludesP2SH()) && acct.GetAccountId() != ZCASH_LEGACY_ACCOUNT;
}
}, this->pattern);
}
bool ZTXOSelector::SelectsSprout() const {
return std::visit(match {
[](const libzcash::SproutViewingKey& addr) { return true; },

1
src/wallet/wallet.h
View File

@ -872,6 +872,7 @@ public:
}
bool SelectsTransparent() const;
bool SelectsTransparentCoinbase() const;
bool SelectsSprout() const;
bool SelectsSapling() const;
bool SelectsOrchard() const;