From af2526d755ffc5e5b5e8435a9cea3c33e0ef682b Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 9 Mar 2023 18:06:10 -0700 Subject: [PATCH 1/2] Improve taddr no-memo check Do the check deeper, preventing test_bitcoin from being able to bypass it. This also moves it out of z_sendmany-specific code, which will be helpful when we add other operations, like sendfromaccount. --- qa/rpc-tests/wallet_z_sendmany.py | 5 +++ src/wallet/rpcwallet.cpp | 9 ++---- src/wallet/wallet_tx_builder.cpp | 52 ++++++++++++++++++++----------- 3 files changed, 40 insertions(+), 26 deletions(-) diff --git a/qa/rpc-tests/wallet_z_sendmany.py b/qa/rpc-tests/wallet_z_sendmany.py index 07372d80a..5a14319fc 100755 --- a/qa/rpc-tests/wallet_z_sendmany.py +++ b/qa/rpc-tests/wallet_z_sendmany.py @@ -152,6 +152,11 @@ class WalletZSendmanyTest(BitcoinTestFramework): # The following assertion might fail nondeterministically # assert_equal(node2balance, Decimal('16.99799000')) + # try sending with a memo to a taddr, which should fail + recipients = [{"address":self.nodes[0].getnewaddress(), "amount":1, "memo":"DEADBEEF"}] + opid = self.nodes[2].z_sendmany(myzaddr, recipients, 1, DEFAULT_FEE, 'AllowRevealedRecipients') + wait_and_assert_operationid_status(self.nodes[2], opid, 'failed', 'Failed to build transaction: Memos cannot be sent to transparent addresses.') + recipients = [] recipients.append({"address":self.nodes[0].getnewaddress(), "amount":1}) recipients.append({"address":self.nodes[2].getnewaddress(), "amount":1.0}) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 32144fb04..90a3344ce 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4736,7 +4736,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) " [{\n" " \"address\":address (string, required) The address is a taddr, zaddr, or Unified Address\n" " \"amount\":amount (numeric, required) The numeric amount in " + CURRENCY_UNIT + " is the value\n" - " \"memo\":memo (string, optional) If the address is a zaddr, raw data represented in hexadecimal string format\n" + " \"memo\":memo (string, optional) If the address is a zaddr, raw data represented in hexadecimal string format. If\n" + " it’s a taddr, 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" @@ -4841,12 +4842,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) std::optional memo; if (!memoValue.isNull()) { auto memoHex = memoValue.get_str(); - if (!std::visit(libzcash::HasShieldedRecipient(), addr.value())) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "Invalid parameter, memos cannot be sent to transparent addresses."); - } - std::visit(match { [&](MemoError err) { switch (err) { diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 55f480cd0..bcb7ed822 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -544,25 +544,39 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild( } // Add outputs - for (const auto& r : payments.GetResolvedPayments()) { - std::visit(match { - [&](const CKeyID& keyId) { - builder.AddTransparentOutput(keyId, r.amount); - }, - [&](const CScriptID& scriptId) { - builder.AddTransparentOutput(scriptId, r.amount); - }, - [&](const libzcash::SaplingPaymentAddress& addr) { - builder.AddSaplingOutput( - r.isInternal ? internalOVK : externalOVK, addr, r.amount, - r.memo.has_value() ? r.memo.value().ToBytes() : Memo::NoMemo().ToBytes()); - }, - [&](const libzcash::OrchardRawAddress& addr) { - builder.AddOrchardOutput( - r.isInternal ? internalOVK : externalOVK, addr, r.amount, - r.memo.has_value() ? std::optional(r.memo.value().ToBytes()) : std::nullopt); - } - }, r.address); + try { + for (const auto& r : payments.GetResolvedPayments()) { + std::visit(match { + [&](const CKeyID& keyId) { + if (r.memo.has_value()) { + throw TransactionBuilderResult( + "Memos cannot be sent to transparent addresses."); + } else { + builder.AddTransparentOutput(keyId, r.amount); + } + }, + [&](const CScriptID& scriptId) { + if (r.memo.has_value()) { + throw TransactionBuilderResult( + "Memos cannot be sent to transparent addresses."); + } else { + builder.AddTransparentOutput(scriptId, r.amount); + } + }, + [&](const libzcash::SaplingPaymentAddress& addr) { + builder.AddSaplingOutput( + r.isInternal ? internalOVK : externalOVK, addr, r.amount, + r.memo.has_value() ? r.memo.value().ToBytes() : Memo::NoMemo().ToBytes()); + }, + [&](const libzcash::OrchardRawAddress& addr) { + builder.AddOrchardOutput( + r.isInternal ? internalOVK : externalOVK, addr, r.amount, + r.memo.has_value() ? std::optional(r.memo.value().ToBytes()) : std::nullopt); + } + }, r.address); + } + } catch (const TransactionBuilderResult& result) { + return result; } // Add transparent utxos From 7bf5f598eea8d4bee979b3855100906b0ea81b90 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 28 Mar 2023 16:30:47 -0600 Subject: [PATCH 2/2] Update src/wallet/rpcwallet.cpp Co-authored-by: Kris Nuttycombe --- src/wallet/rpcwallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 90a3344ce..2cffd1693 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4737,7 +4737,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) " \"address\":address (string, required) The address is a taddr, zaddr, or Unified Address\n" " \"amount\":amount (numeric, required) The numeric amount in " + CURRENCY_UNIT + " is the value\n" " \"memo\":memo (string, optional) If the address is a zaddr, raw data represented in hexadecimal string format. If\n" - " it’s a taddr, it’s an error to include this field.\n" + " 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"