From e22c115e7828e4e75af652ec69c4cb056f0d107d Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 4 Aug 2018 12:06:05 +0100 Subject: [PATCH 01/10] Move GetSpendingKeyForPaymentAddress visitor into wallet.h Also fixes it to not use the global pwalletMain. --- src/wallet/rpcdump.cpp | 40 ++++------------------------------------ src/wallet/wallet.cpp | 34 ++++++++++++++++++++++++++++++++++ src/wallet/wallet.h | 12 ++++++++++++ 3 files changed, 50 insertions(+), 36 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index dea3ea245..e4a6e179d 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -784,41 +784,6 @@ UniValue z_importviewingkey(const UniValue& params, bool fHelp) return NullUniValue; } -class GetSpendingKeyForPaymentAddress : public boost::static_visitor -{ -private: - CWallet *m_wallet; -public: - GetSpendingKeyForPaymentAddress(CWallet *wallet) : m_wallet(wallet) {} - - libzcash::SpendingKey operator()(const libzcash::SproutPaymentAddress &zaddr) const - { - libzcash::SproutSpendingKey k; - if (!pwalletMain->GetSproutSpendingKey(zaddr, k)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet does not hold private zkey for this zaddr"); - } - return k; - } - - libzcash::SpendingKey operator()(const libzcash::SaplingPaymentAddress &zaddr) const - { - libzcash::SaplingIncomingViewingKey ivk; - libzcash::SaplingFullViewingKey fvk; - libzcash::SaplingSpendingKey sk; - - if (!pwalletMain->GetSaplingIncomingViewingKey(zaddr, ivk) || - !pwalletMain->GetSaplingFullViewingKey(ivk, fvk) || - !pwalletMain->GetSaplingSpendingKey(fvk, sk)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet does not hold private zkey for this zaddr"); - } - return sk; - } - - libzcash::SpendingKey operator()(const libzcash::InvalidEncoding& no) const { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid zaddr"); - } -}; - UniValue z_exportkey(const UniValue& params, bool fHelp) { if (!EnsureWalletIsAvailable(fHelp)) @@ -852,7 +817,10 @@ UniValue z_exportkey(const UniValue& params, bool fHelp) // Sapling support auto sk = boost::apply_visitor(GetSpendingKeyForPaymentAddress(pwalletMain), address); - return EncodeSpendingKey(sk); + if (!sk) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet does not hold private zkey for this zaddr"); + } + return EncodeSpendingKey(sk.get()); } UniValue z_exportviewingkey(const UniValue& params, bool fHelp) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 04a61ed13..e016cc476 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4373,3 +4373,37 @@ bool PaymentAddressBelongsToWallet::operator()(const libzcash::InvalidEncoding& { return false; } + +boost::optional GetSpendingKeyForPaymentAddress::operator()( + const libzcash::SproutPaymentAddress &zaddr) const +{ + libzcash::SproutSpendingKey k; + if (m_wallet->GetSproutSpendingKey(zaddr, k)) { + return libzcash::SpendingKey(k); + } else { + return boost::none; + } +} + +boost::optional GetSpendingKeyForPaymentAddress::operator()( + const libzcash::SaplingPaymentAddress &zaddr) const +{ + libzcash::SaplingIncomingViewingKey ivk; + libzcash::SaplingFullViewingKey fvk; + libzcash::SaplingSpendingKey sk; + + if (m_wallet->GetSaplingIncomingViewingKey(zaddr, ivk) && + m_wallet->GetSaplingFullViewingKey(ivk, fvk) && + m_wallet->GetSaplingSpendingKey(fvk, sk)) { + return libzcash::SpendingKey(sk); + } else { + return boost::none; + } +} + +boost::optional GetSpendingKeyForPaymentAddress::operator()( + const libzcash::InvalidEncoding& no) const +{ + // Defaults to InvalidEncoding + return libzcash::SpendingKey(); +} diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index acefa01d8..2e23b2169 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1319,4 +1319,16 @@ public: bool operator()(const libzcash::InvalidEncoding& no) const; }; +class GetSpendingKeyForPaymentAddress : public boost::static_visitor> +{ +private: + CWallet *m_wallet; +public: + GetSpendingKeyForPaymentAddress(CWallet *wallet) : m_wallet(wallet) {} + + boost::optional operator()(const libzcash::SproutPaymentAddress &zaddr) const; + boost::optional operator()(const libzcash::SaplingPaymentAddress &zaddr) const; + boost::optional operator()(const libzcash::InvalidEncoding& no) const; +}; + #endif // BITCOIN_WALLET_WALLET_H From 81e0fd2eb94d4db216face9d30ad7368b2f4c532 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 4 Aug 2018 12:07:18 +0100 Subject: [PATCH 02/10] wallet: Add HaveSpendingKeyForPaymentAddress visitor --- src/wallet/wallet.cpp | 20 ++++++++++++++++++++ src/wallet/wallet.h | 12 ++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e016cc476..62b344522 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4374,6 +4374,26 @@ bool PaymentAddressBelongsToWallet::operator()(const libzcash::InvalidEncoding& return false; } +bool HaveSpendingKeyForPaymentAddress::operator()(const libzcash::SproutPaymentAddress &zaddr) const +{ + return m_wallet->HaveSproutSpendingKey(zaddr); +} + +bool HaveSpendingKeyForPaymentAddress::operator()(const libzcash::SaplingPaymentAddress &zaddr) const +{ + libzcash::SaplingIncomingViewingKey ivk; + libzcash::SaplingFullViewingKey fvk; + + return m_wallet->GetSaplingIncomingViewingKey(zaddr, ivk) && + m_wallet->GetSaplingFullViewingKey(ivk, fvk) && + m_wallet->HaveSaplingSpendingKey(fvk); +} + +bool HaveSpendingKeyForPaymentAddress::operator()(const libzcash::InvalidEncoding& no) const +{ + return false; +} + boost::optional GetSpendingKeyForPaymentAddress::operator()( const libzcash::SproutPaymentAddress &zaddr) const { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2e23b2169..6c57d6112 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1319,6 +1319,18 @@ public: bool operator()(const libzcash::InvalidEncoding& no) const; }; +class HaveSpendingKeyForPaymentAddress : public boost::static_visitor +{ +private: + CWallet *m_wallet; +public: + HaveSpendingKeyForPaymentAddress(CWallet *wallet) : m_wallet(wallet) {} + + bool operator()(const libzcash::SproutPaymentAddress &zaddr) const; + bool operator()(const libzcash::SaplingPaymentAddress &zaddr) const; + bool operator()(const libzcash::InvalidEncoding& no) const; +}; + class GetSpendingKeyForPaymentAddress : public boost::static_visitor> { private: From 36e2141d92572f1b4f3a6c5c57c6a3eecbbddcb6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 4 Aug 2018 12:23:13 +0100 Subject: [PATCH 03/10] rpcwallet: Add TransactionBuilder argument to AsyncRPCOperation_sendmany --- src/test/rpc_wallet_tests.cpp | 28 +++++++++++------------ src/transaction_builder.h | 1 + src/wallet/asyncrpcoperation_sendmany.cpp | 9 +++++++- src/wallet/asyncrpcoperation_sendmany.h | 15 ++++++++++-- src/wallet/rpcwallet.cpp | 10 +++++++- 5 files changed, 45 insertions(+), 18 deletions(-) diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index edbcb6552..680cd0490 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -970,26 +970,26 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_parameters) // Test constructor of AsyncRPCOperation_sendmany try { - std::shared_ptr operation(new AsyncRPCOperation_sendmany(mtx, "",{}, {}, -1)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(boost::none, mtx, "",{}, {}, -1)); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "Minconf cannot be negative")); } try { - std::shared_ptr operation(new AsyncRPCOperation_sendmany(mtx, "",{}, {}, 1)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(boost::none, mtx, "",{}, {}, 1)); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "From address parameter missing")); } try { - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ", {}, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ", {}, {}, 1) ); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "No recipients")); } try { std::vector recipients = { SendManyRecipient("dummy",1.0, "") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, "INVALID", recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, "INVALID", recipients, {}, 1) ); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "Invalid from address")); } @@ -997,7 +997,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_parameters) // Testnet payment addresses begin with 'zt'. This test detects an incorrect prefix. try { std::vector recipients = { SendManyRecipient("dummy",1.0, "") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, "zcMuhvq8sEkHALuSU2i4NbNQxshSAYrpCExec45ZjtivYPbuiFPwk6WHy4SvsbeZ4siy1WheuRGjtaJmoD1J8bFqNXhsG6U", recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, "zcMuhvq8sEkHALuSU2i4NbNQxshSAYrpCExec45ZjtivYPbuiFPwk6WHy4SvsbeZ4siy1WheuRGjtaJmoD1J8bFqNXhsG6U", recipients, {}, 1) ); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "Invalid from address")); } @@ -1006,7 +1006,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_parameters) // invokes a method on pwalletMain, which is undefined in the google test environment. try { std::vector recipients = { SendManyRecipient("dummy",1.0, "") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, "ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP", recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, "ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP", recipients, {}, 1) ); } catch (const UniValue& objError) { BOOST_CHECK( find_error(objError, "no spending key found for zaddr")); } @@ -1039,7 +1039,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // there are no utxos to spend { std::vector recipients = { SendManyRecipient(zaddr1,100.0, "DEADBEEF") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, taddr1, {}, recipients, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, taddr1, {}, recipients, 1) ); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); @@ -1050,7 +1050,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) { try { std::vector recipients = {SendManyRecipient(taddr1, 100.0, "DEADBEEF")}; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(mtx, zaddr1, recipients, {}, 0)); + std::shared_ptr operation(new AsyncRPCOperation_sendmany(boost::none, mtx, 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")); @@ -1061,7 +1061,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // there are no unspent notes to spend { std::vector recipients = { SendManyRecipient(taddr1,100.0, "DEADBEEF") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, recipients, {}, 1) ); operation->main(); BOOST_CHECK(operation->isFailed()); std::string msg = operation->getErrorMessage(); @@ -1071,7 +1071,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // get_memo_from_hex_string()) { std::vector recipients = { SendManyRecipient(zaddr1,100.0, "DEADBEEF") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, recipients, {}, 1) ); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); @@ -1122,7 +1122,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // add_taddr_change_output_to_tx() will append a vout to a raw transaction { std::vector recipients = { SendManyRecipient(zaddr1,100.0, "DEADBEEF") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, recipients, {}, 1) ); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); @@ -1151,7 +1151,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) SendManyRecipient("tmUSbHz3vxnwLvRyNDXbwkZxjVyDodMJEhh",CAmount(4.56), ""), SendManyRecipient("tmYZAXYPCP56Xa5JQWWPZuK7o7bfUQW6kkd",CAmount(7.89), ""), }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, recipients, {}, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, recipients, {}, 1) ); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); @@ -1174,7 +1174,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // we have the spending key for the dummy recipient zaddr1 std::vector recipients = { SendManyRecipient(zaddr1, 0.0005, "ABCD") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, {}, recipients, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, {}, recipients, 1) ); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); @@ -1199,7 +1199,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // Dummy input so the operation object can be instantiated. std::vector recipients = { SendManyRecipient(zaddr1, 0.0005, "ABCD") }; - std::shared_ptr operation( new AsyncRPCOperation_sendmany(mtx, zaddr1, {}, recipients, 1) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(boost::none, mtx, zaddr1, {}, recipients, 1) ); std::shared_ptr ptr = std::dynamic_pointer_cast (operation); TEST_FRIEND_AsyncRPCOperation_sendmany proxy(ptr); diff --git a/src/transaction_builder.h b/src/transaction_builder.h index 0291c7ad1..90f2eff2d 100644 --- a/src/transaction_builder.h +++ b/src/transaction_builder.h @@ -69,6 +69,7 @@ private: boost::optional tChangeAddr; public: + TransactionBuilder() {} TransactionBuilder(const Consensus::Params& consensusParams, int nHeight, CKeyStore* keyStore = nullptr); void SetFee(CAmount fee); diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 405cde231..661943762 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -53,6 +53,7 @@ int find_output(UniValue obj, int n) { } AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( + boost::optional builder, CMutableTransaction contextualTx, std::string fromAddress, std::vector tOutputs, @@ -75,7 +76,13 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( if (tOutputs.size() == 0 && zOutputs.size() == 0) { throw JSONRPCError(RPC_INVALID_PARAMETER, "No recipients"); } - + + isUsingBuilder_ = false; + if (builder) { + isUsingBuilder_ = true; + builder_ = builder.get(); + } + fromtaddr_ = DecodeDestination(fromAddress); isfromtaddr_ = IsValidDestination(fromtaddr_); isfromzaddr_ = false; diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index d3729804a..c4078160b 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -8,6 +8,7 @@ #include "asyncrpcoperation.h" #include "amount.h" #include "primitives/transaction.h" +#include "transaction_builder.h" #include "zcash/JoinSplit.hpp" #include "zcash/Address.hpp" #include "wallet.h" @@ -51,7 +52,15 @@ struct WitnessAnchorData { class AsyncRPCOperation_sendmany : public AsyncRPCOperation { public: - AsyncRPCOperation_sendmany(CMutableTransaction contextualTx, std::string fromAddress, std::vector tOutputs, std::vector zOutputs, int minDepth, CAmount fee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE, UniValue contextInfo = NullUniValue); + AsyncRPCOperation_sendmany( + boost::optional builder, + CMutableTransaction contextualTx, + std::string fromAddress, + std::vector tOutputs, + std::vector zOutputs, + int minDepth, + CAmount fee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE, + UniValue contextInfo = NullUniValue); virtual ~AsyncRPCOperation_sendmany(); // We don't want to be copied or moved around @@ -73,6 +82,7 @@ private: UniValue contextinfo_; // optional data to include in return value from getStatus() + bool isUsingBuilder_; // Indicates that no Sprout addresses are involved uint32_t consensusBranchId_; CAmount fee_; int mindepth_; @@ -93,7 +103,8 @@ private: std::vector z_outputs_; std::vector t_inputs_; std::vector z_inputs_; - + + TransactionBuilder builder_; CTransaction tx_; void add_taddr_change_output_to_tx(CAmount amount); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 133f4a5ea..0ce97276c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -13,6 +13,7 @@ #include "netbase.h" #include "rpc/server.h" #include "timedata.h" +#include "transaction_builder.h" #include "util.h" #include "utilmoneystr.h" #include "wallet.h" @@ -3787,7 +3788,14 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) o.push_back(Pair("fee", std::stod(FormatMoney(nFee)))); UniValue contextInfo = o; + // Builder (used if Sapling addresses are involved) + boost::optional builder; + if (false) { // TODO: Sapling support + builder = TransactionBuilder(Params().GetConsensus(), nextBlockHeight, pwalletMain); + } + // Contextual transaction we will build on + // (used if no Sapling addresses are involved) CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(Params().GetConsensus(), nextBlockHeight); bool isShielded = !fromTaddr || zaddrRecipients.size() > 0; if (contextualTx.nVersion == 1 && isShielded) { @@ -3796,7 +3804,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Create operation and add to global queue std::shared_ptr q = getAsyncRPCQueue(); - std::shared_ptr operation( new AsyncRPCOperation_sendmany(contextualTx, fromaddress, taddrRecipients, zaddrRecipients, nMinDepth, nFee, contextInfo) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(builder, contextualTx, fromaddress, taddrRecipients, zaddrRecipients, nMinDepth, nFee, contextInfo) ); q->addOperation(operation); AsyncRPCOperationId operationId = operation->getId(); return operationId; From af4057b90448fe4f588a89594435c0eb0a43cc0d Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 4 Aug 2018 12:34:52 +0100 Subject: [PATCH 04/10] rpcwallet: Prevent use of both Sprout and Sapling addresses in z_sendmany --- src/wallet/rpcwallet.cpp | 44 +++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0ce97276c..cc3f5b85f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3610,25 +3610,24 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Check that the from address is valid. auto fromaddress = params[0].get_str(); bool fromTaddr = false; + bool fromSapling = false; CTxDestination taddr = DecodeDestination(fromaddress); fromTaddr = IsValidDestination(taddr); - libzcash::SproutPaymentAddress zaddr; if (!fromTaddr) { auto res = DecodePaymentAddress(fromaddress); if (!IsValidPaymentAddress(res)) { // invalid throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid from address, should be a taddr or zaddr."); } - // TODO: Add Sapling support. For now, ensure we can freely convert. - assert(boost::get(&res) != nullptr); - zaddr = boost::get(res); - } - // Check that we have the spending key - if (!fromTaddr) { - if (!pwalletMain->HaveSproutSpendingKey(zaddr)) { + // Check that we have the spending key + if (!boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), res)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "From address does not belong to this node, zaddr spending key not found."); } + + // Remember whether this is a Sprout or Sapling address + // !fromTaddr && !fromSapling -> Sprout + fromSapling = boost::get(&res) != nullptr; } UniValue outputs = params[1].get_array(); @@ -3639,6 +3638,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Keep track of addresses to spot duplicates set setAddress; + // Track whether we see any Sprout addresses + bool noSproutAddrs = fromTaddr || fromSapling; + // Recipients std::vector taddrRecipients; std::vector zaddrRecipients; @@ -3659,8 +3661,27 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) bool isZaddr = false; CTxDestination taddr = DecodeDestination(address); if (!IsValidDestination(taddr)) { - if (IsValidPaymentAddressString(address)) { + auto res = DecodePaymentAddress(address); + if (IsValidPaymentAddress(res)) { isZaddr = true; + + bool toSapling = boost::get(&res) != nullptr; + noSproutAddrs = noSproutAddrs && toSapling; + + // If we are sending from a shielded address, all recipient + // shielded addresses must be of the same type. + if (!fromTaddr) { + if (!fromSapling && toSapling) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Cannot send from a Sprout address to a Sapling address using z_sendmany"); + } + if (fromSapling && !toSapling) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Cannot send from a Sapling address to a Sprout address using z_sendmany"); + } + } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ")+address ); } @@ -3790,7 +3811,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Builder (used if Sapling addresses are involved) boost::optional builder; - if (false) { // TODO: Sapling support + if (noSproutAddrs) { builder = TransactionBuilder(Params().GetConsensus(), nextBlockHeight, pwalletMain); } @@ -3802,6 +3823,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) contextualTx.nVersion = 2; // Tx format should support vjoinsplits } + // TODO: Add Sapling support to AsyncRPCOperation_sendmany() + assert(!noSproutAddrs); + // Create operation and add to global queue std::shared_ptr q = getAsyncRPCQueue(); std::shared_ptr operation( new AsyncRPCOperation_sendmany(builder, contextualTx, fromaddress, taddrRecipients, zaddrRecipients, nMinDepth, nFee, contextInfo) ); From e54c4d2ca120c6bbc632a95542056d3587632fee Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 4 Aug 2018 23:19:52 +0100 Subject: [PATCH 05/10] rpcwallet: Add Sapling support to z_sendmany --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/wallet_sapling.py | 104 ++++++++++ src/wallet/asyncrpcoperation_sendmany.cpp | 220 +++++++++++++++++++--- src/wallet/asyncrpcoperation_sendmany.h | 1 + src/wallet/rpcwallet.cpp | 3 - 5 files changed, 298 insertions(+), 31 deletions(-) create mode 100644 qa/rpc-tests/wallet_sapling.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 73d9ddb76..d6eea9c68 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -24,6 +24,7 @@ testScripts=( 'wallet_nullifiers.py' 'wallet_1941.py' 'wallet_addresses.py' + 'wallet_sapling.py' 'listtransactions.py' 'mempool_resurrect_test.py' 'txn_doublespend.py' diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py new file mode 100644 index 000000000..517869820 --- /dev/null +++ b/qa/rpc-tests/wallet_sapling.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python2 +# Copyright (c) 2018 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + start_nodes, + wait_and_assert_operationid_status, +) + +from decimal import Decimal + +# Test wallet behaviour with Sapling addresses +class WalletSaplingTest(BitcoinTestFramework): + + def setup_nodes(self): + return start_nodes(4, self.options.tmpdir, [[ + '-nuparams=5ba81b19:201', # Overwinter + '-nuparams=76b809bb:201', # Sapling + ]] * 4) + + def run_test(self): + # Sanity-check the test harness + assert_equal(self.nodes[0].getblockcount(), 200) + + # Activate Sapling + self.nodes[2].generate(1) + self.sync_all() + + taddr0 = self.nodes[0].getnewaddress() + # Skip over the address containing node 1's coinbase + self.nodes[1].getnewaddress() + taddr1 = self.nodes[1].getnewaddress() + saplingAddr0 = self.nodes[0].z_getnewaddress('sapling') + saplingAddr1 = self.nodes[1].z_getnewaddress('sapling') + + # Verify addresses + assert(saplingAddr0 in self.nodes[0].z_listaddresses()) + assert(saplingAddr1 in self.nodes[1].z_listaddresses()) + assert_equal(self.nodes[0].z_validateaddress(saplingAddr0)['type'], 'sapling') + assert_equal(self.nodes[0].z_validateaddress(saplingAddr1)['type'], 'sapling') + + # Verify balance + assert_equal(self.nodes[0].z_getbalance(saplingAddr0), Decimal('0')) + assert_equal(self.nodes[1].z_getbalance(saplingAddr1), Decimal('0')) + assert_equal(self.nodes[1].z_getbalance(taddr1), Decimal('0')) + + # Node 0 shields some funds + # taddr -> Sapling + # -> taddr (change) + recipients = [] + recipients.append({"address": saplingAddr0, "amount": Decimal('20')}) + myopid = self.nodes[0].z_sendmany(taddr0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # Verify balance + assert_equal(self.nodes[0].z_getbalance(saplingAddr0), Decimal('20')) + assert_equal(self.nodes[1].z_getbalance(saplingAddr1), Decimal('0')) + assert_equal(self.nodes[1].z_getbalance(taddr1), Decimal('0')) + + # Node 0 sends some shielded funds to node 1 + # Sapling -> Sapling + # -> Sapling (change) + recipients = [] + recipients.append({"address": saplingAddr1, "amount": Decimal('15')}) + myopid = self.nodes[0].z_sendmany(saplingAddr0, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # Verify balance + assert_equal(self.nodes[0].z_getbalance(saplingAddr0), Decimal('5')) + assert_equal(self.nodes[1].z_getbalance(saplingAddr1), Decimal('15')) + assert_equal(self.nodes[1].z_getbalance(taddr1), Decimal('0')) + + # Node 1 sends some shielded funds to node 0, as well as unshielding + # Sapling -> Sapling + # -> taddr + # -> Sapling (change) + recipients = [] + recipients.append({"address": saplingAddr0, "amount": Decimal('5')}) + recipients.append({"address": taddr1, "amount": Decimal('5')}) + myopid = self.nodes[1].z_sendmany(saplingAddr1, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[1], myopid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # Verify balance + assert_equal(self.nodes[0].z_getbalance(saplingAddr0), Decimal('10')) + assert_equal(self.nodes[1].z_getbalance(saplingAddr1), Decimal('5')) + assert_equal(self.nodes[1].z_getbalance(taddr1), Decimal('5')) + +if __name__ == '__main__': + WalletSaplingTest().main() diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 661943762..52bf598f7 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -35,6 +35,9 @@ using namespace libzcash; +extern UniValue signrawtransaction(const UniValue& params, bool fHelp); +extern UniValue sendrawtransaction(const UniValue& params, bool fHelp); + int find_output(UniValue obj, int n) { UniValue outputMapValue = find_value(obj, "outputmap"); if (!outputMapValue.isArray()) { @@ -90,19 +93,14 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( if (!isfromtaddr_) { auto address = DecodePaymentAddress(fromAddress); if (IsValidPaymentAddress(address)) { - // TODO: Add Sapling support. For now, ensure we can freely convert. - assert(boost::get(&address) != nullptr); - SproutPaymentAddress addr = boost::get(address); - // We don't need to lock on the wallet as spending key related methods are thread-safe - SproutSpendingKey key; - if (!pwalletMain->GetSproutSpendingKey(addr, key)) { + if (!boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), address)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid from address, no spending key found for zaddr"); } - + isfromzaddr_ = true; - frompaymentaddress_ = addr; - spendingkey_ = key; + frompaymentaddress_ = address; + spendingkey_ = boost::apply_visitor(GetSpendingKeyForPaymentAddress(pwalletMain), address).get(); } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid from address"); } @@ -251,6 +249,9 @@ bool AsyncRPCOperation_sendmany::main_impl() { for (SendManyInputJSOP & t : z_inputs_) { z_inputs_total += std::get<2>(t); } + for (auto t : z_sapling_inputs_) { + z_inputs_total += t.note.value(); + } CAmount t_outputs_total = 0; for (SendManyRecipient & t : t_outputs_) { @@ -335,15 +336,25 @@ bool AsyncRPCOperation_sendmany::main_impl() { } // update the transaction with these inputs - CMutableTransaction rawTx(tx_); - for (SendManyInputUTXO & t : t_inputs_) { - uint256 txid = std::get<0>(t); - int vout = std::get<1>(t); - CAmount amount = std::get<2>(t); - CTxIn in(COutPoint(txid, vout)); - rawTx.vin.push_back(in); + if (isUsingBuilder_) { + CScript scriptPubKey = GetScriptForDestination(fromtaddr_); + for (auto t : t_inputs_) { + uint256 txid = std::get<0>(t); + int vout = std::get<1>(t); + CAmount amount = std::get<2>(t); + builder_.AddTransparentInput(COutPoint(txid, vout), scriptPubKey, amount); + } + } else { + CMutableTransaction rawTx(tx_); + for (SendManyInputUTXO & t : t_inputs_) { + uint256 txid = std::get<0>(t); + int vout = std::get<1>(t); + CAmount amount = std::get<2>(t); + CTxIn in(COutPoint(txid, vout)); + rawTx.vin.push_back(in); + } + tx_ = CTransaction(rawTx); } - tx_ = CTransaction(rawTx); } LogPrint((isfromtaddr_) ? "zrpc" : "zrpcunsafe", "%s: spending %s to send %s with fee %s\n", @@ -354,6 +365,141 @@ bool AsyncRPCOperation_sendmany::main_impl() { LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(z_outputs_total)); LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(minersFee)); + + /** + * SCENARIO #0 + * + * Sprout not involved, so we just use the TransactionBuilder and we're done. + * We added the transparent inputs to the builder earlier. + */ + if (isUsingBuilder_) { + builder_.SetFee(minersFee); + + // Get various necessary keys + SaplingExpandedSpendingKey expsk; + SaplingFullViewingKey from; + if (isfromzaddr_) { + auto sk = boost::get(spendingkey_); + expsk = sk.expanded_spending_key(); + from = expsk.full_viewing_key(); + } else { + // TODO: Set "from" to something! + } + + // Set change address if we are using transparent funds + // TODO: Should we just use fromtaddr_ as the change address? + if (isfromtaddr_) { + LOCK2(cs_main, pwalletMain->cs_wallet); + + EnsureWalletIsUnlocked(); + CReserveKey keyChange(pwalletMain); + CPubKey vchPubKey; + bool ret = keyChange.GetReservedKey(vchPubKey); + if (!ret) { + // should never fail, as we just unlocked + throw JSONRPCError( + RPC_WALLET_KEYPOOL_RAN_OUT, + "Could not generate a taddr to use as a change address"); + } + + CTxDestination changeAddr = vchPubKey.GetID(); + assert(builder_.SendChangeTo(changeAddr)); + } + + // Select Sapling notes + std::vector ops; + std::vector notes; + CAmount sum = 0; + for (auto t : z_sapling_inputs_) { + ops.push_back(t.op); + notes.push_back(t.note); + sum += t.note.value(); + if (sum >= targetAmount) { + break; + } + } + + // Fetch Sapling anchor and witnesses + uint256 anchor; + std::vector> witnesses; + { + LOCK2(cs_main, pwalletMain->cs_wallet); + pwalletMain->GetSaplingNoteWitnesses(ops, witnesses, anchor); + } + + // Add Sapling spends + for (size_t i = 0; i < notes.size(); i++) { + if (!witnesses[i]) { + throw JSONRPCError(RPC_WALLET_ERROR, "Missing witness for Sapling note"); + } + assert(builder_.AddSaplingSpend(expsk, notes[i], anchor, witnesses[i].get())); + } + + // Add Sapling outputs + for (auto r : z_outputs_) { + auto address = std::get<0>(r); + auto value = std::get<1>(r); + auto hexMemo = std::get<2>(r); + + auto addr = DecodePaymentAddress(address); + assert(boost::get(&addr) != nullptr); + auto to = boost::get(addr); + + auto memo = get_memo_from_hex_string(hexMemo); + + builder_.AddSaplingOutput(from, to, value, memo); + } + + // Add transparent outputs + for (auto r : t_outputs_) { + auto outputAddress = std::get<0>(r); + auto amount = std::get<1>(r); + + auto address = DecodeDestination(outputAddress); + if (!builder_.AddTransparentOutput(address, amount)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid output address, not a valid taddr."); + } + } + + // Build the transaction + auto maybe_tx = builder_.Build(); + if (!maybe_tx) { + throw JSONRPCError(RPC_WALLET_ERROR, "Failed to build transaction."); + } + tx_ = maybe_tx.get(); + + // Send the transaction + // TODO: Use CWallet::CommitTransaction instead of sendrawtransaction + auto signedtxn = EncodeHexTx(tx_); + if (!testmode) { + UniValue params = UniValue(UniValue::VARR); + params.push_back(signedtxn); + UniValue sendResultValue = sendrawtransaction(params, false); + if (sendResultValue.isNull()) { + throw JSONRPCError(RPC_WALLET_ERROR, "sendrawtransaction did not return an error or a txid."); + } + + auto txid = sendResultValue.get_str(); + + UniValue o(UniValue::VOBJ); + o.push_back(Pair("txid", txid)); + set_result(o); + } else { + // Test mode does not send the transaction to the network. + UniValue o(UniValue::VOBJ); + o.push_back(Pair("test", 1)); + o.push_back(Pair("txid", tx_.GetHash().ToString())); + o.push_back(Pair("hex", signedtxn)); + set_result(o); + } + + return true; + } + /** + * END SCENARIO #0 + */ + + // Grab the current consensus branch ID { LOCK(cs_main); @@ -411,9 +557,6 @@ bool AsyncRPCOperation_sendmany::main_impl() { } std::deque zOutputsDeque; for (auto o : z_outputs_) { - // TODO: Add Sapling support. For now, ensure we can later convert freely. - auto addr = DecodePaymentAddress(std::get<0>(o)); - assert(boost::get(&addr) != nullptr); zOutputsDeque.push_back(o); } @@ -741,6 +884,7 @@ bool AsyncRPCOperation_sendmany::main_impl() { info.vjsout.push_back(JSOutput()); // dummy output while we accumulate funds into a change note for vpub_new } else { PaymentAddress pa = DecodePaymentAddress(address); + // If we are here, we know we have no Sapling outputs. JSOutput jso = JSOutput(boost::get(pa), value); if (hexMemo.size() > 0) { jso.memo = get_memo_from_hex_string(hexMemo); @@ -775,9 +919,6 @@ bool AsyncRPCOperation_sendmany::main_impl() { } -extern UniValue signrawtransaction(const UniValue& params, bool fHelp); -extern UniValue sendrawtransaction(const UniValue& params, bool fHelp); - /** * Sign and send a raw transaction. * Raw transaction as hex string should be in object field "rawtxn" @@ -879,7 +1020,7 @@ bool AsyncRPCOperation_sendmany::find_utxos(bool fAcceptCoinbase=false) { if (isCoinbase && fAcceptCoinbase==false) { continue; } - + CAmount nValue = out.tx->vout[out.i].nValue; SendManyInputUTXO utxo(out.tx->GetHash(), out.i, nValue, isCoinbase); t_inputs_.push_back(utxo); @@ -902,10 +1043,19 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, fromaddress_, mindepth_); } + // If using the TransactionBuilder, we only want Sapling notes. + // If not using it, we only want Sprout notes. + // TODO: Refactor `GetFilteredNotes()` so we only fetch what we need. + if (isUsingBuilder_) { + sproutEntries.clear(); + } else { + saplingEntries.clear(); + } + for (CSproutNotePlaintextEntry & entry : sproutEntries) { z_inputs_.push_back(SendManyInputJSOP(entry.jsop, entry.plaintext.note(boost::get(frompaymentaddress_)), CAmount(entry.plaintext.value()))); std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end()); - LogPrint("zrpcunsafe", "%s: found unspent note (txid=%s, vjoinsplit=%d, ciphertext=%d, amount=%s, memo=%s)\n", + LogPrint("zrpcunsafe", "%s: found unspent Sprout note (txid=%s, vjoinsplit=%d, ciphertext=%d, amount=%s, memo=%s)\n", getId(), entry.jsop.hash.ToString().substr(0, 10), entry.jsop.js, @@ -914,9 +1064,19 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { HexStr(data).substr(0, 10) ); } - // TODO: Do something with Sapling notes - - if (z_inputs_.size() == 0) { + + for (auto entry : saplingEntries) { + z_sapling_inputs_.push_back(entry); + std::string data(entry.memo.begin(), entry.memo.end()); + LogPrint("zrpcunsafe", "%s: found unspent Sapling note (txid=%s, vShieldedSpend=%d, amount=%s, memo=%s)\n", + getId(), + entry.op.hash.ToString().substr(0, 10), + entry.op.n, + FormatMoney(entry.note.value()), + HexStr(data).substr(0, 10)); + } + + if (z_inputs_.empty() && z_sapling_inputs_.empty()) { return false; } @@ -924,6 +1084,10 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { std::sort(z_inputs_.begin(), z_inputs_.end(), [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool { return ( std::get<2>(i) > std::get<2>(j)); }); + std::sort(z_sapling_inputs_.begin(), z_sapling_inputs_.end(), + [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool { + return ( i.note.value() > j.note.value()); + }); return true; } diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index c4078160b..4f62ef8b2 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -103,6 +103,7 @@ private: std::vector z_outputs_; std::vector t_inputs_; std::vector z_inputs_; + std::vector z_sapling_inputs_; TransactionBuilder builder_; CTransaction tx_; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cc3f5b85f..e1cf19865 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3823,9 +3823,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) contextualTx.nVersion = 2; // Tx format should support vjoinsplits } - // TODO: Add Sapling support to AsyncRPCOperation_sendmany() - assert(!noSproutAddrs); - // Create operation and add to global queue std::shared_ptr q = getAsyncRPCQueue(); std::shared_ptr operation( new AsyncRPCOperation_sendmany(builder, contextualTx, fromaddress, taddrRecipients, zaddrRecipients, nMinDepth, nFee, contextInfo) ); From 07d85a64559a8a95be6800f3551d199b1062d680 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 30 Aug 2018 09:42:21 +0100 Subject: [PATCH 06/10] Define additional booleans for readability --- src/wallet/rpcwallet.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e1cf19865..89719aa2d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3626,9 +3626,10 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } // Remember whether this is a Sprout or Sapling address - // !fromTaddr && !fromSapling -> Sprout fromSapling = boost::get(&res) != nullptr; } + // This logic will need to be updated if we add a new shielded pool + bool fromSprout = !(fromTaddr || fromSapling); UniValue outputs = params[1].get_array(); @@ -3639,7 +3640,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) set setAddress; // Track whether we see any Sprout addresses - bool noSproutAddrs = fromTaddr || fromSapling; + bool noSproutAddrs = !fromSprout; // Recipients std::vector taddrRecipients; @@ -3666,17 +3667,18 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) isZaddr = true; bool toSapling = boost::get(&res) != nullptr; + bool toSprout = !toSapling; noSproutAddrs = noSproutAddrs && toSapling; // If we are sending from a shielded address, all recipient // shielded addresses must be of the same type. if (!fromTaddr) { - if (!fromSapling && toSapling) { + if (fromSprout && toSapling) { throw JSONRPCError( RPC_INVALID_PARAMETER, "Cannot send from a Sprout address to a Sapling address using z_sendmany"); } - if (fromSapling && !toSapling) { + if (fromSapling && toSprout) { throw JSONRPCError( RPC_INVALID_PARAMETER, "Cannot send from a Sapling address to a Sprout address using z_sendmany"); From 1ec06e93b12c644b605cdbd8e2a808d4b2017828 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 3 Sep 2018 01:13:18 +0100 Subject: [PATCH 07/10] Rename z_inputs_ to z_sprout_inputs_ --- src/wallet/asyncrpcoperation_sendmany.cpp | 14 +++++++------- src/wallet/asyncrpcoperation_sendmany.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 52bf598f7..780b30ea1 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -246,7 +246,7 @@ bool AsyncRPCOperation_sendmany::main_impl() { } CAmount z_inputs_total = 0; - for (SendManyInputJSOP & t : z_inputs_) { + for (SendManyInputJSOP & t : z_sprout_inputs_) { z_inputs_total += std::get<2>(t); } for (auto t : z_sapling_inputs_) { @@ -548,7 +548,7 @@ bool AsyncRPCOperation_sendmany::main_impl() { // Copy zinputs and zoutputs to more flexible containers std::deque zInputsDeque; // zInputsDeque stores minimum numbers of notes for target amount CAmount tmp = 0; - for (auto o : z_inputs_) { + for (auto o : z_sprout_inputs_) { zInputsDeque.push_back(o); tmp += std::get<2>(o); if (tmp >= targetAmount) { @@ -563,9 +563,9 @@ bool AsyncRPCOperation_sendmany::main_impl() { // When spending notes, take a snapshot of note witnesses and anchors as the treestate will // change upon arrival of new blocks which contain joinsplit transactions. This is likely // to happen as creating a chained joinsplit transaction can take longer than the block interval. - if (z_inputs_.size() > 0) { + if (z_sprout_inputs_.size() > 0) { LOCK2(cs_main, pwalletMain->cs_wallet); - for (auto t : z_inputs_) { + for (auto t : z_sprout_inputs_) { JSOutPoint jso = std::get<0>(t); std::vector vOutPoints = { jso }; uint256 inputAnchor; @@ -1053,7 +1053,7 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { } for (CSproutNotePlaintextEntry & entry : sproutEntries) { - z_inputs_.push_back(SendManyInputJSOP(entry.jsop, entry.plaintext.note(boost::get(frompaymentaddress_)), CAmount(entry.plaintext.value()))); + z_sprout_inputs_.push_back(SendManyInputJSOP(entry.jsop, entry.plaintext.note(boost::get(frompaymentaddress_)), CAmount(entry.plaintext.value()))); std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end()); LogPrint("zrpcunsafe", "%s: found unspent Sprout note (txid=%s, vjoinsplit=%d, ciphertext=%d, amount=%s, memo=%s)\n", getId(), @@ -1076,12 +1076,12 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { HexStr(data).substr(0, 10)); } - if (z_inputs_.empty() && z_sapling_inputs_.empty()) { + if (z_sprout_inputs_.empty() && z_sapling_inputs_.empty()) { return false; } // sort in descending order, so big notes appear first - std::sort(z_inputs_.begin(), z_inputs_.end(), [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool { + std::sort(z_sprout_inputs_.begin(), z_sprout_inputs_.end(), [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool { return ( std::get<2>(i) > std::get<2>(j)); }); std::sort(z_sapling_inputs_.begin(), z_sapling_inputs_.end(), diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index 4f62ef8b2..862ae02a2 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -102,7 +102,7 @@ private: std::vector t_outputs_; std::vector z_outputs_; std::vector t_inputs_; - std::vector z_inputs_; + std::vector z_sprout_inputs_; std::vector z_sapling_inputs_; TransactionBuilder builder_; From 7c02acc5b64e8ef51517331682603d1ed9fe342c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 3 Sep 2018 01:28:31 +0100 Subject: [PATCH 08/10] Minor cleanups --- src/wallet/asyncrpcoperation_sendmany.cpp | 16 ++++++++++------ src/wallet/rpcwallet.cpp | 20 +++++++++----------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 780b30ea1..d37614dbe 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -240,6 +240,9 @@ bool AsyncRPCOperation_sendmany::main_impl() { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds, no unspent notes found for zaddr from address."); } + // At least one of z_sprout_inputs_ and z_sapling_inputs_ must be empty by design + assert(z_sprout_inputs_.empty() || z_sapling_inputs_.empty()); + CAmount t_inputs_total = 0; for (SendManyInputUTXO & t : t_inputs_) { t_inputs_total += std::get<2>(t); @@ -277,7 +280,7 @@ bool AsyncRPCOperation_sendmany::main_impl() { if (isfromzaddr_ && (z_inputs_total < targetAmount)) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, - strprintf("Insufficient protected funds, have %s, need %s", + strprintf("Insufficient shielded funds, have %s, need %s", FormatMoney(z_inputs_total), FormatMoney(targetAmount))); } @@ -598,7 +601,7 @@ bool AsyncRPCOperation_sendmany::main_impl() { if (selectedUTXOCoinbase) { assert(isSingleZaddrOutput); throw JSONRPCError(RPC_WALLET_ERROR, strprintf( - "Change %s not allowed. When protecting coinbase funds, the wallet does not " + "Change %s not allowed. When shielding coinbase funds, the wallet does not " "allow any change as there is currently no way to specify a change address " "in z_sendmany.", FormatMoney(change))); } else { @@ -1081,12 +1084,13 @@ bool AsyncRPCOperation_sendmany::find_unspent_notes() { } // sort in descending order, so big notes appear first - std::sort(z_sprout_inputs_.begin(), z_sprout_inputs_.end(), [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool { - return ( std::get<2>(i) > std::get<2>(j)); - }); + std::sort(z_sprout_inputs_.begin(), z_sprout_inputs_.end(), + [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool { + return std::get<2>(i) > std::get<2>(j); + }); std::sort(z_sapling_inputs_.begin(), z_sapling_inputs_.end(), [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool { - return ( i.note.value() > j.note.value()); + return i.note.value() > j.note.value(); }); return true; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 89719aa2d..c090a8234 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3672,17 +3672,15 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // If we are sending from a shielded address, all recipient // shielded addresses must be of the same type. - if (!fromTaddr) { - if (fromSprout && toSapling) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "Cannot send from a Sprout address to a Sapling address using z_sendmany"); - } - if (fromSapling && toSprout) { - throw JSONRPCError( - RPC_INVALID_PARAMETER, - "Cannot send from a Sapling address to a Sprout address using z_sendmany"); - } + if (fromSprout && toSapling) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Cannot send from a Sprout address to a Sapling address using z_sendmany"); + } + if (fromSapling && toSprout) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Cannot send from a Sapling address to a Sprout address using z_sendmany"); } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ")+address ); From 0f436a0a26d1fb8a912cf3b636f5895322b10dce Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 3 Sep 2018 08:23:09 +0100 Subject: [PATCH 09/10] Fix RPC test that checks exact wording of cleaned-up error message --- qa/rpc-tests/wallet_protectcoinbase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/wallet_protectcoinbase.py b/qa/rpc-tests/wallet_protectcoinbase.py index 5854449ab..a27896df7 100755 --- a/qa/rpc-tests/wallet_protectcoinbase.py +++ b/qa/rpc-tests/wallet_protectcoinbase.py @@ -262,7 +262,7 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): myopid = self.nodes[0].z_sendmany(mytaddr, recipients) wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient transparent funds, have 10.00, need 10000.0001") myopid = self.nodes[0].z_sendmany(myzaddr, recipients) - wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient protected funds, have 9.9998, need 10000.0001") + wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Insufficient shielded funds, have 9.9998, need 10000.0001") # Send will fail because of insufficient funds unless sender uses coinbase utxos try: From af04224522e1a55afae56146930f16915b118185 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 3 Sep 2018 08:27:33 +0100 Subject: [PATCH 10/10] Fix file permissions of wallet_sapling RPC test --- qa/rpc-tests/wallet_sapling.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 qa/rpc-tests/wallet_sapling.py diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py old mode 100644 new mode 100755