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

Apply suggestions from code review 

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: str4d <jack@electriccoin.co>
This commit is contained in:
Kris Nuttycombe 2022-01-11 15:17:56 -07:00
parent 9554ed2dc4
commit 0ca7d49626
7 changed files with 88 additions and 69 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -26,7 +26,7 @@ class WalletSendManyAnyTaddr(BitcoinTestFramework):
self.nodes[3],
self.nodes[3].z_sendmany('ANY_TADDR', [{'address': recipient, 'amount': 100}]),
'failed',
'Insufficient funds: have 0.00, need 100.00001; if you are attempting to shield transparent coinbase funds, ensure that you have specified only a single recipient address.',
'Insufficient funds: have 0.00, need 100.00001; note that coinbase outputs will not be selected if you specify ANY_TADDR or if any transparent recipients are included.',
)
# Prepare some non-coinbase UTXOs

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

@ -88,8 +88,11 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
errorString = e.error['message']
assert_equal("Invalid from address, no spending key found for address", errorString);
# This send will fail because our wallet does not allow any change when shielding a coinbase utxo,
# as it's currently not possible to specify a change address in z_sendmany.
# 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.
recipients = []
recipients.append({"address":myzaddr, "amount":Decimal('1.23456789')})
@ -233,9 +236,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
recipients = []
recipients.append({"address":self.nodes[1].getnewaddress(), "amount":Decimal('10000.0')})
myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 10.00, need 10000.00001; if you are attempting to shield transparent coinbase funds, ensure that you have specified only a single recipient address.")
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 or if any transparent recipients are included.")
myopid = self.nodes[0].z_sendmany(myzaddr, recipients)
wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient funds: have 9.99998, need 10000.00001; if you are attempting to shield transparent coinbase funds, ensure that you have specified only a single recipient address.")
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 or if any transparent recipients are included.")
# Send will fail because of insufficient funds unless sender uses coinbase utxos
try:

73
src/wallet/asyncrpcoperation_sendmany.cpp
View File

@ -93,8 +93,8 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany(
}
}, paymentSource);
if ((isfromsprout_ || isfromsapling_) && minDepth==0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Minconf cannot be zero when sending from zaddr");
if ((isfromsprout_ || isfromsapling_) && minDepth == 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Minconf cannot be zero when sending from a shielded address");
}
// Log the context info i.e. the call parameters to z_sendmany
@ -171,10 +171,6 @@ public:
CAmount t_outputs_total{0};
CAmount z_outputs_total{0};
CAmount targetAmount{0};
CAmount GetChangeAmount() {
return t_inputs_total + z_inputs_total - targetAmount;
}
};
bool IsFromAnyTaddr(const PaymentSource& paymentSource) {
@ -192,9 +188,16 @@ bool IsFromAnyTaddr(const PaymentSource& paymentSource) {
// Errors in transaction construction will throw.
//
// Notes:
// 1. #1159 Currently there is no limit set on the number of joinsplits, so size of tx could be invalid.
// 2. #1360 Note selection is not optimal
// 3. #1277 Spendable notes are not locked, so an operation running in parallel could also try to use them
// 1. #1159 Currently there is no limit set on the number of elements, which could
// make the tx too large.
// 2. #1360 Note selection is not optimal.
// 3. #1277 Spendable notes are not locked, so an operation running in parallel
// could also try to use them.
// 4. #1614 Anchors are chosen at the most recent block; this is unreliable and leaks
// information in case of rollback.
// 5. #3615 There is no padding of inputs or outputs, which may leak information.
//
// At least 4. and 5. differ from the Rust transaction builder.
uint256 AsyncRPCOperation_sendmany::main_impl() {
// TODO UA: this check will become meaningless.
bool isfromzaddr_ = isfromsprout_ || isfromsapling_;
@ -225,7 +228,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
if (isfromsprout_ && !allowRevealedAmounts_) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
"Sending from Sprout to Sapling is not enabled by default because it will "
"Sending between shielded pools is not enabled by default because it will "
"publicly reveal the transaction amount. THIS MAY AFFECT YOUR PRIVACY. "
"Resubmit with the `allowRevealedAmounts` parameter set to `true` if "
"you wish to allow this transaction to proceed anyway.");
@ -258,10 +261,11 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
if (!spendable.LimitToAmount(txValues.targetAmount, dustThreshold)) {
CAmount changeAmount{spendable.Total() - txValues.targetAmount};
if (changeAmount > 0 && changeAmount < dustThreshold) {
// TODO: this condition is silly; we should provide the option for the caller to
// forego change (definitionally 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.
// 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.
throw JSONRPCError(
RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf(
@ -278,8 +282,8 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
"Insufficient funds: have %s, need %s",
FormatMoney(spendable.Total()), FormatMoney(txValues.targetAmount))
+ (allowTransparentCoinbase ? "" :
"; if you are attempting to shield transparent coinbase funds, "
"ensure that you have specified only a single recipient address.")
"; note that coinbase outputs will not be selected if you specify "
"ANY_TADDR or if any transparent recipients are included.")
);
}
}
@ -409,12 +413,12 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
CAmount sum = 0;
// Create Sapling outpoints
std::vector<SaplingOutPoint> ops;
std::vector<SaplingOutPoint> saplingOutPoints;
std::vector<SaplingNote> saplingNotes;
std::vector<SaplingExtendedSpendingKey> saplingKeys;
for (const auto& t : spendable.saplingNoteEntries) {
ops.push_back(t.op);
saplingOutPoints.push_back(t.op);
saplingNotes.push_back(t.note);
libzcash::SaplingExtendedSpendingKey saplingKey;
@ -432,7 +436,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
std::vector<std::optional<SaplingWitness>> witnesses;
{
LOCK2(cs_main, pwalletMain->cs_wallet);
pwalletMain->GetSaplingNoteWitnesses(ops, witnesses, anchor);
pwalletMain->GetSaplingNoteWitnesses(saplingOutPoints, witnesses, anchor);
}
// Add Sapling spends
@ -491,6 +495,9 @@ uint256 AsyncRPCOperation_sendmany::main_impl() {
// to happen as creating a chained joinsplit transaction can take longer than the block interval.
// So, we need to take locks on cs_main and pwalletMain->cs_wallet so that the witnesses aren't
// updated.
//
// TODO: these locks would ideally be shared for selection of Sapling anchors and witnesses
// as well.
std::vector<std::optional<SproutWitness>> vSproutWitnesses;
{
LOCK2(cs_main, pwalletMain->cs_wallet);
@ -585,7 +592,8 @@ bool SpendableInputs::LimitToAmount(const CAmount amountRequired, const CAmount
return totalSelected == amountRequired || totalSelected - amountRequired > dustThreshold;
};
// select Sprout notes for spending first
// Select Sprout notes for spending first - if possible, we want users to
// spend any notes that they still have in the Sprout pool.
if (!haveSufficientFunds()) {
std::sort(sproutNoteEntries.begin(), sproutNoteEntries.end(),
[](SproutNoteEntry i, SproutNoteEntry j) -> bool {
@ -599,7 +607,10 @@ bool SpendableInputs::LimitToAmount(const CAmount amountRequired, const CAmount
sproutNoteEntries.erase(sproutIt, sproutNoteEntries.end());
}
// next select transparent utxos
// Next select transparent utxos. We preferentially spend transparent funds,
// with the intent that we'd like to opportunistically shield whatever is
// possible, and we will always shield change after the introduction of
// unified addresses.
if (!haveSufficientFunds()) {
std::sort(utxos.begin(), utxos.end(),
[](COutput i, COutput j) -> bool {
@ -613,7 +624,10 @@ bool SpendableInputs::LimitToAmount(const CAmount amountRequired, const CAmount
utxos.erase(utxoIt, utxos.end());
}
// finally select Sapling outputs
// Finally select Sapling outputs. After the introduction of Orchard to the
// wallet, the selection of Sapling and Orchard notes, and the
// determination of change amounts, should be done in a fashion that
// minimizes information leakage whenever possible.
if (!haveSufficientFunds()) {
std::sort(saplingNoteEntries.begin(), saplingNoteEntries.end(),
[](SaplingNoteEntry i, SaplingNoteEntry j) -> bool {
@ -641,9 +655,18 @@ bool SpendableInputs::HasTransparentCoinbase() const {
}
void SpendableInputs::LogInputs(const AsyncRPCOperationId& id) const {
for (const auto& utxo : utxos) {
LogPrint("zrpcunsafe", "%s: found unspent transparent UTXO (txid=%s, index=%d, amount=%s, isCoinbase=%s)\n",
id,
utxo.tx->GetHash().ToString(),
utxo.i,
FormatMoney(utxo.Value()),
utxo.fIsCoinbase);
}
for (const auto& entry : sproutNoteEntries) {
std::string data(entry.memo.begin(), entry.memo.end());
LogPrint("zrpcunsafe", "%s: found unspent Sprout note (opid=%s, vJoinSplit=%d, jsoutindex=%d, amount=%s, memo=%s)\n",
LogPrint("zrpcunsafe", "%s: found unspent Sprout note (txid=%s, vJoinSplit=%d, jsoutindex=%d, amount=%s, memo=%s)\n",
id,
entry.jsop.hash.ToString().substr(0, 10),
entry.jsop.js,
@ -655,7 +678,7 @@ void SpendableInputs::LogInputs(const AsyncRPCOperationId& id) const {
for (const auto& entry : saplingNoteEntries) {
std::string data(entry.memo.begin(), entry.memo.end());
LogPrint("zrpcunsafe", "%s: found unspent Sapling note (opid=%s, vShieldedSpend=%d, amount=%s, memo=%s)\n",
LogPrint("zrpcunsafe", "%s: found unspent Sapling note (txid=%s, vShieldedSpend=%d, amount=%s, memo=%s)\n",
id,
entry.op.hash.ToString().substr(0, 10),
entry.op.n,
@ -691,7 +714,7 @@ std::array<unsigned char, ZC_MEMO_SIZE> AsyncRPCOperation_sendmany::get_memo_fro
if (rawMemo.size() > ZC_MEMO_SIZE) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
strprintf("Memo size of %d is too big, maximum allowed is %d", rawMemo.size(), ZC_MEMO_SIZE));
strprintf("Memo size of %d bytes is too big, maximum allowed is %d bytes", rawMemo.size(), ZC_MEMO_SIZE));
}
// copy vector into boost array

59
src/wallet/rpcwallet.cpp
View File

@ -3681,8 +3681,8 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
size_t EstimateTxSize(
const PaymentSource& paymentSource,
const std::vector<SendManyRecipient>& recipients) {
int nextBlockHeight = chainActive.Height() + 1;
const std::vector<SendManyRecipient>& recipients,
int nextBlockHeight) {
CMutableTransaction mtx;
mtx.fOverwintered = true;
mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID;
@ -3732,9 +3732,7 @@ size_t EstimateTxSize(
},
[&](const libzcash::SproutPaymentAddress& addr) {
JSDescription jsdesc;
if (mtx.fOverwintered && (mtx.nVersion >= SAPLING_TX_VERSION)) {
jsdesc.proof = GrothProof();
}
jsdesc.proof = GrothProof();
mtx.vJoinSplit.push_back(jsdesc);
},
[&](const libzcash::UnifiedAddress& ua) {
@ -3794,15 +3792,20 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
LOCK2(cs_main, pwalletMain->cs_wallet);
const auto& chainparams = Params();
int nextBlockHeight = chainActive.Height() + 1;
ThrowIfInitialBlockDownload();
KeyIO keyIO(Params());
if (!chainparams.GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_SAPLING)) {
throw JSONRPCError(
RPC_INVALID_PARAMETER, "Cannot create shielded transactions before Sapling has activated");
}
KeyIO keyIO(chainparams);
// Check that the from address is valid.
auto fromaddress = params[0].get_str();
PaymentSource paymentSource;
bool fromTaddr = false;
bool fromSprout = false;
bool fromSapling = false;
if (fromaddress == "ANY_TADDR") {
paymentSource = FromAnyTaddr();
} else {
@ -3813,26 +3816,12 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
"Invalid from address: should be a taddr, a zaddr, or the string 'ANY_TADDR'.");
}
// Remember what sort of address this is
std::visit(match {
[&](const CKeyID&) {
fromTaddr = true;
},
[&](const CScriptID&) {
fromTaddr = true;
},
[&](const libzcash::SaplingPaymentAddress& addr) {
fromSapling = true;
},
[&](const libzcash::SproutPaymentAddress& addr) {
fromSprout = true;
},
[&](const libzcash::UnifiedAddress& ua) {
throw JSONRPCError(
RPC_INVALID_ADDRESS_OR_KEY,
"Invalid from address: unified addresses are not yet supported.");
}
}, addr.value());
// Unified addresses are not yet supported.
if (std::holds_alternative<libzcash::UnifiedAddress>(addr.value())) {
throw JSONRPCError(
RPC_INVALID_ADDRESS_OR_KEY,
"Invalid from address: unified addresses are not yet supported.");
}
paymentSource = addr.value();
}
@ -3842,6 +3831,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, amounts array is empty.");
}
std::set<std::string> addrStrings;
std::vector<SendManyRecipient> recipients;
CAmount nTotalOut = 0;
for (const UniValue& o : outputs.getValues()) {
@ -3861,7 +3851,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
// restriction here to allow sprout->sprout; these transfers will not be forbidden
// by later code.
bool toSprout = std::holds_alternative<libzcash::SproutPaymentAddress>(addr.value());
if (toSprout) { // && !fromSprout) {
if (toSprout) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
"Sending funds into the Sprout pool is not supported by z_sendmany");
@ -3872,6 +3862,10 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
std::string("Invalid parameter, unknown address format: ") + addrStr);
}
if (!addrStrings.insert(addrStr).second) {
throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ") + addrStr);
};
UniValue memoValue = find_value(o, "memo");
std::optional<std::string> memo;
if (!memoValue.isNull()) {
@ -3902,7 +3896,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
// Sanity check for transaction size
// TODO: move this to the builder?
auto txsize = EstimateTxSize(paymentSource, recipients);
auto txsize = EstimateTxSize(paymentSource, recipients, nextBlockHeight);
if (txsize > MAX_TX_SIZE_AFTER_SAPLING) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
@ -3959,8 +3953,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
o.pushKV("fee", std::stod(FormatMoney(nFee)));
UniValue contextInfo = o;
int nextBlockHeight = chainActive.Height() + 1;
TransactionBuilder builder(Params().GetConsensus(), nextBlockHeight, pwalletMain);
TransactionBuilder builder(chainparams.GetConsensus(), nextBlockHeight, pwalletMain);
// Create operation and add to global queue
std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();

4
src/wallet/test/rpc_wallet_tests.cpp
View File

@ -1256,7 +1256,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_sendmany(builder, zaddr1, recipients, 0));
BOOST_CHECK(false); // Fail test if an exception is not thrown
} catch (const UniValue& objError) {
BOOST_CHECK(find_error(objError, "Minconf cannot be zero when sending from zaddr"));
BOOST_CHECK(find_error(objError, "Minconf cannot be zero when sending from a shielded address"));
}
}
@ -1373,7 +1373,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling)
mtx = CreateNewContextualCMutableTransaction(consensusParams, nextBlockHeight);
std::vector<SendManyRecipient> recipients = { SendManyRecipient(pa, 1*COIN, "ABCD") };
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_sendmany(builder, taddr, recipients, 1));
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_sendmany(builder, taddr, recipients, 0));
std::shared_ptr<AsyncRPCOperation_sendmany> ptr = std::dynamic_pointer_cast<AsyncRPCOperation_sendmany> (operation);
// Enable test mode so tx is not sent

4
src/wallet/wallet.cpp
View File

@ -2033,7 +2033,7 @@ bool CWallet::IsSaplingNullifierFromMe(const uint256& nullifier) const
return false;
}
void CWallet::GetSproutNoteWitnesses(std::vector<JSOutPoint> notes,
void CWallet::GetSproutNoteWitnesses(const std::vector<JSOutPoint>& notes,
std::vector<std::optional<SproutWitness>>& witnesses,
uint256 &final_anchor)
{
@ -2060,7 +2060,7 @@ void CWallet::GetSproutNoteWitnesses(std::vector<JSOutPoint> notes,
}
}
void CWallet::GetSaplingNoteWitnesses(std::vector<SaplingOutPoint> notes,
void CWallet::GetSaplingNoteWitnesses(const std::vector<SaplingOutPoint>& notes,
std::vector<std::optional<SaplingWitness>>& witnesses,
uint256 &final_anchor)
{

4
src/wallet/wallet.h
View File

@ -1195,11 +1195,11 @@ public:
bool IsSaplingNullifierFromMe(const uint256& nullifier) const;
void GetSproutNoteWitnesses(
std::vector<JSOutPoint> notes,
const std::vector<JSOutPoint>& notes,
std::vector<std::optional<SproutWitness>>& witnesses,
uint256 &final_anchor);
void GetSaplingNoteWitnesses(
std::vector<SaplingOutPoint> notes,
const std::vector<SaplingOutPoint>& notes,
std::vector<std::optional<SaplingWitness>>& witnesses,
uint256 &final_anchor);