From c21ffff790d18c392089f9b9cc2ced79745ef9a9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 19 Jan 2022 17:00:13 -0700 Subject: [PATCH] Add correct selection of change addresses to z_sendmany This also alters `TransactionBuilder::SendChangeTo` to take a `libzcash::ChangeAddress` value and thus avoids the invalid t-addr case of CTxDestination. --- Cargo.lock | 19 +-- Cargo.toml | 10 +- src/gtest/test_transaction_builder.cpp | 14 +- src/rust/include/librustzcash.h | 11 ++ src/rust/src/rustzcash.rs | 21 ++- src/transaction_builder.cpp | 42 ++--- src/transaction_builder.h | 8 +- src/wallet/asyncrpcoperation_common.cpp | 2 + src/wallet/asyncrpcoperation_sendmany.cpp | 152 ++++++------------ src/wallet/asyncrpcoperation_sendmany.h | 9 +- .../asyncrpcoperation_shieldcoinbase.cpp | 1 + src/wallet/gtest/test_wallet.cpp | 2 +- src/wallet/rpcwallet.cpp | 4 +- src/wallet/wallet.cpp | 84 ++++++++-- src/wallet/wallet.h | 27 +++- src/zcash/Address.hpp | 38 ----- src/zcash/address/sapling.hpp | 3 +- src/zcash/address/unified.cpp | 4 + src/zcash/address/unified.h | 58 +++++++ src/zcash/address/zip32.cpp | 23 +++ src/zcash/address/zip32.h | 5 +- 21 files changed, 320 insertions(+), 217 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6f0342048..e5376a43f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -472,7 +472,7 @@ checksum = "c34f04666d835ff5d62e058c3995147c06f42fe86ff053337632bca83e42702d" [[package]] name = "equihash" version = "0.1.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "blake2b_simd 1.0.0", "byteorder", @@ -481,7 +481,7 @@ dependencies = [ [[package]] name = "f4jumble" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "blake2b_simd 1.0.0", ] @@ -1841,10 +1841,9 @@ dependencies = [ [[package]] name = "zcash_address" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "bech32", - "blake2b_simd 1.0.0", "bs58", "f4jumble", "zcash_encoding", @@ -1853,7 +1852,7 @@ dependencies = [ [[package]] name = "zcash_encoding" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "byteorder", "nonempty", @@ -1862,7 +1861,7 @@ dependencies = [ [[package]] name = "zcash_history" version = "0.2.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "bigint", "blake2b_simd 1.0.0", @@ -1872,7 +1871,7 @@ dependencies = [ [[package]] name = "zcash_note_encryption" version = "0.1.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "chacha20", "chacha20poly1305", @@ -1883,7 +1882,7 @@ dependencies = [ [[package]] name = "zcash_primitives" version = "0.5.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "aes", "bip0039", @@ -1901,11 +1900,9 @@ dependencies = [ "incrementalmerkletree", "jubjub", "lazy_static", - "log", "memuse", "nonempty", "orchard", - "pasta_curves", "rand", "rand_core 0.6.3", "sha2", @@ -1917,7 +1914,7 @@ dependencies = [ [[package]] name = "zcash_proofs" version = "0.5.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=7801dddf35ca247345cf4f5c5e48791297cad531#7801dddf35ca247345cf4f5c5e48791297cad531" +source = "git+https://github.com/zcash/librustzcash.git?rev=a01290869a4f0c4e05bdcca550795ea0e76e8550#a01290869a4f0c4e05bdcca550795ea0e76e8550" dependencies = [ "bellman", "blake2b_simd 1.0.0", diff --git a/Cargo.toml b/Cargo.toml index e067901e9..02f5cc8cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,8 +71,8 @@ panic = 'abort' codegen-units = 1 [patch.crates-io] -zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" } -zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" } -zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" } -zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" } -zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" } +zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "a01290869a4f0c4e05bdcca550795ea0e76e8550" } +zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "a01290869a4f0c4e05bdcca550795ea0e76e8550" } +zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "a01290869a4f0c4e05bdcca550795ea0e76e8550" } +zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "a01290869a4f0c4e05bdcca550795ea0e76e8550" } +zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "a01290869a4f0c4e05bdcca550795ea0e76e8550" } diff --git a/src/gtest/test_transaction_builder.cpp b/src/gtest/test_transaction_builder.cpp index ad8c3465a..c17fafb7d 100644 --- a/src/gtest/test_transaction_builder.cpp +++ b/src/gtest/test_transaction_builder.cpp @@ -268,17 +268,6 @@ TEST(TransactionBuilder, RejectsInvalidTransparentOutput) ASSERT_THROW(builder.AddTransparentOutput(taddr, 50), UniValue); } -TEST(TransactionBuilder, RejectsInvalidTransparentChangeAddress) -{ - SelectParams(CBaseChainParams::REGTEST); - const Consensus::Params& consensusParams = Params().GetConsensus(); - - // Default CTxDestination type is an invalid address - CTxDestination taddr; - auto builder = TransactionBuilder(consensusParams, 1); - ASSERT_THROW(builder.SendChangeTo(taddr), UniValue); -} - TEST(TransactionBuilder, FailsWithNegativeChange) { auto consensusParams = RegtestActivateSapling(); @@ -344,7 +333,6 @@ TEST(TransactionBuilder, ChangeOutput) CKey tsk = AddTestCKeyToKeyStore(keystore); auto tkeyid = tsk.GetPubKey().GetID(); auto scriptPubKey = GetScriptForDestination(tkeyid); - CTxDestination taddr = tkeyid; // No change address and no Sapling spends { @@ -387,7 +375,7 @@ TEST(TransactionBuilder, ChangeOutput) { auto builder = TransactionBuilder(consensusParams, 1, &keystore); builder.AddTransparentInput(COutPoint(), scriptPubKey, 25000); - builder.SendChangeTo(taddr); + builder.SendChangeTo(tkeyid, {}); auto tx = builder.Build().GetTxOrThrow(); EXPECT_EQ(tx.vin.size(), 1); diff --git a/src/rust/include/librustzcash.h b/src/rust/include/librustzcash.h index 43b31031a..9424bd789 100644 --- a/src/rust/include/librustzcash.h +++ b/src/rust/include/librustzcash.h @@ -306,6 +306,17 @@ extern "C" { unsigned char *xfvk_i ); + /** + * Derive the Sapling FVK for an internal BIP44 chain from the + * corresponding external chain FVK. + */ + void librustzcash_zip32_sapling_derive_internal_fvk( + const unsigned char *fvk, + const unsigned char *dk, + unsigned char *fvk_ret, + unsigned char *dk_ret + ); + /** * Derive a PaymentAddress from a (SaplingFullViewingKey, DiversifierKey) * pair. Returns 'false' if no valid address can be derived for the diff --git a/src/rust/src/rustzcash.rs b/src/rust/src/rustzcash.rs index 6f138608c..b44199c2b 100644 --- a/src/rust/src/rustzcash.rs +++ b/src/rust/src/rustzcash.rs @@ -55,7 +55,7 @@ use zcash_primitives::{ }, sapling::{merkle_hash, spend_sig}, transaction::components::Amount, - zip32::{self, sapling_address, sapling_find_address}, + zip32::{self, sapling_address, sapling_derive_internal_fvk, sapling_find_address}, }; use zcash_proofs::{ circuit::sapling::TREE_DEPTH as SAPLING_TREE_DEPTH, @@ -1081,6 +1081,25 @@ pub extern "C" fn librustzcash_zip32_xfvk_derive( true } +/// Derive the Sapling internal +#[no_mangle] +pub extern "C" fn librustzcash_zip32_sapling_derive_internal_fvk( + fvk: *const [c_uchar; 96], + dk: *const [c_uchar; 32], + fvk_ret: *mut [c_uchar; 96], + dk_ret: *mut [c_uchar; 32], +) { + let fvk = FullViewingKey::read(&unsafe { *fvk }[..]).expect("valid Sapling FullViewingKey"); + let dk = zip32::DiversifierKey(unsafe { *dk }); + + let (fvk_internal, dk_internal) = sapling_derive_internal_fvk(&fvk, &dk); + let fvk_ret = unsafe { &mut *fvk_ret }; + let dk_ret = unsafe { &mut *dk_ret }; + + fvk_ret.copy_from_slice(&fvk_internal.to_bytes()); + dk_ret.copy_from_slice(&dk_internal.0); +} + /// Derive a PaymentAddress from an ExtendedFullViewingKey. #[no_mangle] pub extern "C" fn librustzcash_zip32_sapling_address( diff --git a/src/transaction_builder.cpp b/src/transaction_builder.cpp index 50ed89110..94705ebed 100644 --- a/src/transaction_builder.cpp +++ b/src/transaction_builder.cpp @@ -280,29 +280,31 @@ void TransactionBuilder::SetFee(CAmount fee) this->fee = fee; } -void TransactionBuilder::SendChangeTo(libzcash::SaplingPaymentAddress changeAddr, uint256 ovk) -{ - saplingChangeAddr = std::make_pair(ovk, changeAddr); - sproutChangeAddr = std::nullopt; +// TODO: remove support for transparent change? +void TransactionBuilder::SendChangeTo( + const libzcash::RecipientAddress& changeAddr, + const uint256& ovk) { tChangeAddr = std::nullopt; -} - -void TransactionBuilder::SendChangeTo(libzcash::SproutPaymentAddress changeAddr) -{ - sproutChangeAddr = changeAddr; - saplingChangeAddr = std::nullopt; - tChangeAddr = std::nullopt; -} - -void TransactionBuilder::SendChangeTo(CTxDestination& changeAddr) -{ - if (!IsValidDestination(changeAddr)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid change address, not a valid taddr."); - } - - tChangeAddr = changeAddr; saplingChangeAddr = std::nullopt; sproutChangeAddr = std::nullopt; + + std::visit(match { + [&](const CKeyID& keyId) { + tChangeAddr = keyId; + }, + [&](const CScriptID& scriptId) { + tChangeAddr = scriptId; + }, + [&](const libzcash::SaplingPaymentAddress& changeDest) { + saplingChangeAddr = std::make_pair(ovk, changeDest); + } + }, changeAddr); +} + +void TransactionBuilder::SendChangeToSprout(const libzcash::SproutPaymentAddress& zaddr) { + tChangeAddr = std::nullopt; + saplingChangeAddr = std::nullopt; + sproutChangeAddr = zaddr; } TransactionBuilderResult TransactionBuilder::Build() diff --git a/src/transaction_builder.h b/src/transaction_builder.h index 9873010fd..9d40abe45 100644 --- a/src/transaction_builder.h +++ b/src/transaction_builder.h @@ -13,6 +13,7 @@ #include "script/script.h" #include "script/standard.h" #include "uint256.h" +#include "util/match.h" #include "zcash/Address.hpp" #include "zcash/IncrementalMerkleTree.hpp" #include "zcash/JoinSplit.hpp" @@ -170,11 +171,8 @@ public: void AddTransparentOutput(const CTxDestination& to, CAmount value); - void SendChangeTo(libzcash::SaplingPaymentAddress changeAddr, uint256 ovk); - - void SendChangeTo(libzcash::SproutPaymentAddress); - - void SendChangeTo(CTxDestination& changeAddr); + void SendChangeTo(const libzcash::RecipientAddress& changeAddr, const uint256& ovk); + void SendChangeToSprout(const libzcash::SproutPaymentAddress& changeAddr); TransactionBuilderResult Build(); diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 97f697a4e..7d7ba7fbf 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -6,6 +6,8 @@ extern UniValue signrawtransaction(const UniValue& params, bool fHelp); +// TODO: instead of passing a TestMode flag, tests should override `CommitTransaction` +// on the wallet. UniValue SendTransaction(CTransaction& tx, std::optional> reservekey, bool testmode) { UniValue o(UniValue::VOBJ); // Send the transaction diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index f657c2d0b..dd8e00a66 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -60,31 +60,6 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( assert(!recipients_.empty()); assert(ztxoSelector.RequireSpendingKeys()); - std::visit(match { - [&](const AccountZTXOPattern& acct) { - isfromtaddr_ = - acct.GetReceiverTypes().empty() || - acct.GetReceiverTypes().count(ReceiverType::P2PKH) > 0 || - acct.GetReceiverTypes().count(ReceiverType::P2SH) > 0; - }, - [&](const CKeyID& keyId) { - isfromtaddr_ = true; - }, - [&](const CScriptID& scriptId) { - isfromtaddr_ = true; - }, - [&](const libzcash::SproutPaymentAddress& addr) { - isfromsprout_ = true; - }, - [&](const libzcash::SaplingPaymentAddress& addr) { - isfromsapling_ = true; - } - }, ztxoSelector.GetPattern()); - - if ((isfromsprout_ || isfromsapling_) && minDepth == 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Minconf cannot be zero when sending from a shielded address"); - } - // calculate the target totals for (const SendManyRecipient& recipient : recipients_) { std::visit(match { @@ -98,7 +73,7 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( }, [&](const libzcash::SaplingPaymentAddress& addr) { txOutputAmounts_.z_outputs_total += recipient.amount; - if (isfromsprout_ && !allowRevealedAmounts_) { + if (ztxoSelector_.SelectsSprout() && !allowRevealedAmounts_) { throw JSONRPCError( RPC_INVALID_PARAMETER, "Sending between shielded pools is not enabled by default because it will " @@ -192,10 +167,6 @@ void AsyncRPCOperation_sendmany::main() { // // 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_; - assert(isfromtaddr_ != isfromzaddr_); - CAmount sendAmount = txOutputAmounts_.z_outputs_total + txOutputAmounts_.t_outputs_total; CAmount targetAmount = sendAmount + fee_; @@ -245,14 +216,6 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { spendable.LogInputs(getId()); - // At least one of z_sprout_inputs_ and z_sapling_inputs_ must be empty by design - // - // TODO: This restriction is true by construction as we have no mechanism - // for filtering for notes that will select both Sprout and Sapling notes - // simultaneously, but even if we did it would likely be safe to remove - // this limitation. - assert(spendable.sproutNoteEntries.empty() || spendable.saplingNoteEntries.empty()); - CAmount t_inputs_total{0}; CAmount z_inputs_total{0}; for (const auto& t : spendable.utxos) { @@ -265,25 +228,16 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { z_inputs_total += t.note.value(); } - // TODO UA: these restrictions should be removed. - assert(!isfromtaddr_ || z_inputs_total == 0); - assert(!isfromzaddr_ || t_inputs_total == 0); - - if (isfromtaddr_ && (t_inputs_total < targetAmount)) { - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, - strprintf("Insufficient transparent funds, have %s, need %s", - FormatMoney(t_inputs_total), FormatMoney(targetAmount))); - } - if (isfromzaddr_ && (z_inputs_total < targetAmount)) { - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, - strprintf("Insufficient shielded funds, have %s, need %s", - FormatMoney(z_inputs_total), FormatMoney(targetAmount))); + if (z_inputs_total > 0 && mindepth_ == 0) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "Minconf cannot be zero when sending from a shielded address"); } // When spending transparent coinbase outputs, all inputs must be fully // consumed, and they may only be sent to shielded recipients. if (spendable.HasTransparentCoinbase()) { - if (t_inputs_total != targetAmount) { + if (t_inputs_total + z_inputs_total != targetAmount) { throw JSONRPCError( RPC_WALLET_ERROR, strprintf( @@ -299,66 +253,55 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { } } - if (isfromtaddr_) { - LogPrint("zrpc", "%s: spending %s to send %s with fee %s\n", - getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(fee_)); - } else { - LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", - getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(fee_)); - } + LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", + getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(fee_)); LogPrint("zrpc", "%s: transparent input: %s (to choose from)\n", getId(), FormatMoney(t_inputs_total)); LogPrint("zrpcunsafe", "%s: private input: %s (to choose from)\n", getId(), FormatMoney(z_inputs_total)); LogPrint("zrpc", "%s: transparent output: %s\n", getId(), FormatMoney(txOutputAmounts_.t_outputs_total)); LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(txOutputAmounts_.z_outputs_total)); LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(fee_)); - CReserveKey keyChange(pwalletMain); - uint256 ovk; - - auto getDefaultOVK = [&]() { - HDSeed seed = pwalletMain->GetHDSeedForRPC(); - return ovkForShieldingFromTaddr(seed); - }; - - auto setTransparentChangeRecipient = [&]() { - LOCK2(cs_main, pwalletMain->cs_wallet); - - EnsureWalletIsUnlocked(); - 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(); - builder_.SendChangeTo(changeAddr); - }; - - // FIXME: use the appropriate shielded pool change address for the - // source unified address account (or the legacy account), and the - // associated OVK + auto ovks = this->SelectOVKs(); + auto selectorAccountId = pwalletMain->FindAccountForSelector(ztxoSelector_); std::visit(match { + [&](const CKeyID& keyId) { + auto accountId = selectorAccountId.value_or(ZCASH_LEGACY_ACCOUNT); + builder_.SendChangeTo( + pwalletMain->GenerateChangeAddressForAccount(accountId, { libzcash::ChangeType::Sapling }).value(), + ovks.first); + }, + [&](const CScriptID& scriptId) { + auto accountId = selectorAccountId.value_or(ZCASH_LEGACY_ACCOUNT); + builder_.SendChangeTo( + pwalletMain->GenerateChangeAddressForAccount(accountId, { libzcash::ChangeType::Sapling }).value(), + ovks.first); + }, [&](const libzcash::SproutPaymentAddress& addr) { - ovk = getDefaultOVK(); - builder_.SendChangeTo(addr); + // for Sprout, we return change to the originating address. + builder_.SendChangeToSprout(addr); }, [&](const libzcash::SaplingPaymentAddress& addr) { - libzcash::SaplingExtendedSpendingKey saplingKey; - assert(pwalletMain->GetSaplingExtendedSpendingKey(addr, saplingKey)); - - ovk = saplingKey.expsk.full_viewing_key().ovk; - builder_.SendChangeTo(addr, ovk); + // for Sapling, if using a legacy address, return change to the + // originating address; otherwise return it to the Sapling internal + // address corresponding to the UFVK. + if (selectorAccountId.has_value()) { + auto changeAddr = pwalletMain->GenerateChangeAddressForAccount( + selectorAccountId.value(), { libzcash::ChangeType::Sapling }).value(); + builder_.SendChangeTo(changeAddr, ovks.first); + } else { + builder_.SendChangeTo(addr, ovks.first); + } }, - [&](const auto& other) { - ovk = getDefaultOVK(); - setTransparentChangeRecipient(); + [&](const AccountZTXOPattern& acct) { + auto changeAddr = pwalletMain->GenerateChangeAddressForAccount( + selectorAccountId.value(), { libzcash::ChangeType::Sapling }); + assert(changeAddr.has_value()); + builder_.SendChangeTo(changeAddr.value(), ovks.first); } }, ztxoSelector_.GetPattern()); - // Track the total of notes that we've added to the builder + // Track the total of notes that we've added to the builder. This + // shouldn't strictly be necessary, given `spendable.LimitToAmount` CAmount sum = 0; // Create Sapling outpoints @@ -410,7 +353,7 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { auto value = r.amount; auto memo = get_memo_from_hex_string(r.memo.has_value() ? r.memo.value() : ""); - builder_.AddSaplingOutput(ovk, addr, value, memo); + builder_.AddSaplingOutput(ovks.second, addr, value, memo); } }, r.address); } @@ -466,12 +409,20 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { auto buildResult = builder_.Build(); auto tx = buildResult.GetTxOrThrow(); - UniValue sendResult = SendTransaction(tx, keyChange, testmode); + UniValue sendResult = SendTransaction(tx, std::nullopt, testmode); set_result(sendResult); return tx.GetHash(); } +std::pair AsyncRPCOperation_sendmany::SelectOVKs() const { + //TODO + uint256 internalOVK; + uint256 externalOVK; + + return std::make_pair(internalOVK, externalOVK); +} + /** * Compute a dust threshold based upon a standard p2pkh txout. */ @@ -523,4 +474,3 @@ UniValue AsyncRPCOperation_sendmany::getStatus() const { obj.pushKV("params", contextinfo_ ); return obj; } - diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index ec5f2a6bb..b57450253 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -75,13 +75,19 @@ private: CAmount fee_; UniValue contextinfo_; // optional data to include in return value from getStatus() - bool isfromtaddr_{false}; bool isfromsprout_{false}; bool isfromsapling_{false}; bool allowRevealedAmounts_{false}; uint32_t transparentRecipients_{0}; TxOutputAmounts txOutputAmounts_; + /** + * Compute the internal and external OVKs to use in transaction construction, given + * the payment source and the set of types that correspond to outputs selected for + * being spent in the transaction. + */ + std::pair SelectOVKs() const; + static CAmount DefaultDustThreshold(); static std::array get_memo_from_hex_string(std::string s); @@ -89,7 +95,6 @@ private: uint256 main_impl(); }; - // To test private methods, a friend class can act as a proxy class TEST_FRIEND_AsyncRPCOperation_sendmany { public: diff --git a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp index 81f16343a..cd714b8dc 100644 --- a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp @@ -258,6 +258,7 @@ bool ShieldToAddress::operator()(const libzcash::SaplingPaymentAddress &zaddr) c // generate a common one from the HD seed. This ensures the data is // recoverable, while keeping it logically separate from the ZIP 32 // Sapling key hierarchy, which the user might not be using. + // FIXME: update to use the ZIP-316 OVK HDSeed seed = pwalletMain->GetHDSeedForRPC(); uint256 ovk = ovkForShieldingFromTaddr(seed); diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 3cc97a9ca..6446ceda7 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -2199,7 +2199,7 @@ TEST(WalletTests, GenerateUnifiedAddress) { ua->first.GetSaplingReceiver(), ufvk.GetSaplingKey().value().Address(ua->second)); - auto u4r = wallet.GetUnifiedForReceiver(ua->first.GetSaplingReceiver().value()); + auto u4r = wallet.FindUnifiedAddressByReceiver(ua->first.GetSaplingReceiver().value()); EXPECT_EQ(u4r, ua->first); // Explicitly trigger the invalid transparent child index failure diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index a5358fa85..42f7290db 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4044,7 +4044,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp) // If the note belongs to a Sapling address that is part of an account in the // wallet, show the corresponding Unified Address. std::string address; - const auto ua = pwalletMain->GetUnifiedForReceiver(pa); + const auto ua = pwalletMain->FindUnifiedAddressByReceiver(pa); if (ua.has_value()) { address = keyIO.EncodePaymentAddress(ua.value()); } else { @@ -4097,7 +4097,7 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp) // If the note belongs to a Sapling address that is part of an account in the // wallet, show the corresponding Unified Address. std::string address; - const auto ua = pwalletMain->GetUnifiedForReceiver(pa); + const auto ua = pwalletMain->FindUnifiedAddressByReceiver(pa); if (ua.has_value()) { address = keyIO.EncodePaymentAddress(ua.value()); } else { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 195b259f6..d4a13d150 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -536,7 +536,7 @@ bool CWallet::AddUnifiedFullViewingKey(const libzcash::UnifiedFullViewingKey &uf return CWalletDB(strWalletFile).WriteUnifiedFullViewingKey(ufvk); } -std::optional CWallet::GetUnifiedFullViewingKeyByAccount(libzcash::AccountId accountId) { +std::optional CWallet::GetUnifiedFullViewingKeyByAccount(libzcash::AccountId accountId) const { if (!mnemonicHDChain.has_value()) { throw std::runtime_error( "CWallet::GetUnifiedFullViewingKeyByAccount(): Wallet is missing mnemonic seed metadata."); @@ -1546,6 +1546,22 @@ bool CWallet::SelectorMatchesAddress( }, selector.GetPattern()); } +std::optional CWallet::GenerateChangeAddressForAccount( + libzcash::AccountId accountId, + std::set changeOptions) { + auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId); + if (ufvk.has_value()) { + for (libzcash::ChangeType t : changeOptions) { + if (t == libzcash::ChangeType::Transparent && accountId == ZCASH_LEGACY_ACCOUNT) { + return GenerateNewKey().GetID(); + } else { + return ufvk.value().GetChangeAddress(changeOptions); + } + } + } + return std::nullopt; +} + SpendableInputs CWallet::FindSpendableInputs( ZTXOSelector selector, bool allowTransparentCoinbase, @@ -5980,8 +5996,12 @@ std::optional CWallet::GetUnifiedAccountId(const libzcash:: } } -std::optional CWallet::GetUnifiedForReceiver(const Receiver& receiver) const { - return std::visit(LookupUnifiedAddress(*this), receiver); +std::optional CWallet::FindUFVKByReceiver(const libzcash::Receiver& receiver) const { + return std::visit(UFVKForReceiver(*this), receiver); +} + +std::optional CWallet::FindUnifiedAddressByReceiver(const Receiver& receiver) const { + return std::visit(UnifiedAddressForReceiver(*this), receiver); } // @@ -6213,7 +6233,6 @@ KeyAddResult AddViewingKeyToWallet::operator()(const libzcash::SaplingExtendedFu return KeyNotAdded; } } - KeyAddResult AddViewingKeyToWallet::operator()(const libzcash::UnifiedFullViewingKey& no) const { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unified full viewing key import is not yet supported."); } @@ -6271,14 +6290,53 @@ KeyAddResult AddSpendingKeyToWallet::operator()(const libzcash::SaplingExtendedS } } -std::optional LookupUnifiedAddress::operator()(const libzcash::SaplingPaymentAddress& saplingAddr) const { +// UFVKForReceiver :: (CWallet&, Receiver) -> std::optional + +std::optional UFVKForReceiver::operator()(const libzcash::SaplingPaymentAddress& saplingAddr) const { auto ufvkPair = wallet.GetUFVKMetadataForReceiver(saplingAddr); if (ufvkPair.has_value()) { auto ufvkid = ufvkPair.value().first; auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); - if (!(ufvk.has_value() && ufvk.value().GetSaplingKey().has_value())) { - throw std::runtime_error("CWallet::LookupUnifiedAddress(): UFVK has no Sapling key part."); - } + // If we have UFVK metadata, `GetUnifiedFullViewingKey` should always + // return a non-nullopt value, and since we obtained that metadata by + // lookup from as Sapling address, it should have a Sapling key component. + assert(ufvk.has_value() && ufvk.value().GetSaplingKey().has_value()); + return ufvk.value(); + } else { + return std::nullopt; + } +} +std::optional UFVKForReceiver::operator()(const CScriptID& scriptId) const { + // We do not currently generate unified addresses containing P2SH components, + // so there's nothing to look up here. + return std::nullopt; +} +std::optional UFVKForReceiver::operator()(const CKeyID& keyId) const { + auto ufvkPair = wallet.GetUFVKMetadataForReceiver(keyId); + if (ufvkPair.has_value()) { + auto ufvkid = ufvkPair.value().first; + // transparent address UFVK metadata is always accompanied by the child + // index at which the address was produced + assert(ufvkPair.value().second.has_value()); + auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); + assert(ufvk.has_value() && ufvk.value().GetTransparentKey().has_value()); + return ufvk.value(); + } else { + return std::nullopt; + } +} +std::optional UFVKForReceiver::operator()(const libzcash::UnknownReceiver& receiver) const { + return std::nullopt; +} + +// UnifiedAddressForReceiver :: (CWallet&, Receiver) -> std::optional + +std::optional UnifiedAddressForReceiver::operator()(const libzcash::SaplingPaymentAddress& saplingAddr) const { + auto ufvkPair = wallet.GetUFVKMetadataForReceiver(saplingAddr); + if (ufvkPair.has_value()) { + auto ufvkid = ufvkPair.value().first; + auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); + assert(ufvk.has_value() && ufvk.value().GetSaplingKey().has_value()); diversifier_index_t j; // If the wallet is missing metadata at this UFVK id, it is probably @@ -6300,10 +6358,12 @@ std::optional LookupUnifiedAddress::operator()(const l return std::nullopt; } } -std::optional LookupUnifiedAddress::operator()(const CScriptID& scriptId) const { +std::optional UnifiedAddressForReceiver::operator()(const CScriptID& scriptId) const { + // We do not currently generate unified addresses containing P2SH components, + // so there's nothing to look up here. return std::nullopt; } -std::optional LookupUnifiedAddress::operator()(const CKeyID& keyId) const { +std::optional UnifiedAddressForReceiver::operator()(const CKeyID& keyId) const { auto ufvkPair = wallet.GetUFVKMetadataForReceiver(keyId); if (ufvkPair.has_value()) { auto ufvkid = ufvkPair.value().first; @@ -6313,7 +6373,7 @@ std::optional LookupUnifiedAddress::operator()(const C diversifier_index_t j = ufvkPair.value().second.value(); auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); if (!(ufvk.has_value() && ufvk.value().GetTransparentKey().has_value())) { - throw std::runtime_error("CWallet::LookupUnifiedAddress(): UFVK has no P2PKH key part."); + throw std::runtime_error("CWallet::UnifiedAddressForReceiver(): UFVK has no P2PKH key part."); } // If the wallet is missing metadata at this UFVK id, it is probably @@ -6333,7 +6393,7 @@ std::optional LookupUnifiedAddress::operator()(const C return std::nullopt; } } -std::optional LookupUnifiedAddress::operator()(const libzcash::UnknownReceiver& receiver) const { +std::optional UnifiedAddressForReceiver::operator()(const libzcash::UnknownReceiver& receiver) const { return std::nullopt; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b70f799f6..3f8ecad39 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1256,6 +1256,10 @@ public: */ std::optional FindAccountForSelector(const ZTXOSelector& paymentSource) const; + std::optional GenerateChangeAddressForAccount( + libzcash::AccountId accountId, + std::set changeOptions); + SpendableInputs FindSpendableInputs( ZTXOSelector paymentSource, bool allowTransparentCoinbase, @@ -1413,7 +1417,7 @@ public: //! Retrieves the UFVK derived from the wallet's mnemonic seed for the specified account. std::optional - GetUnifiedFullViewingKeyByAccount(libzcash::AccountId account); + GetUnifiedFullViewingKeyByAccount(libzcash::AccountId account) const; //! Generate a new unified address for the specified account, diversifier, and //! set of receiver types. @@ -1433,7 +1437,9 @@ public: std::optional FindUnifiedFullViewingKey(const libzcash::UnifiedAddress& addr) const; std::optional GetUnifiedAccountId(const libzcash::UFVKId& ufvkId) const; - std::optional GetUnifiedForReceiver(const libzcash::Receiver& receiver) const; + + std::optional FindUFVKByReceiver(const libzcash::Receiver& receiver) const; + std::optional FindUnifiedAddressByReceiver(const libzcash::Receiver& receiver) const; /** * Increment the next transaction order id @@ -1857,12 +1863,25 @@ public: KeyAddResult operator()(const libzcash::SaplingExtendedSpendingKey &sk) const; }; -class LookupUnifiedAddress { +class UFVKForReceiver { private: const CWallet& wallet; public: - LookupUnifiedAddress(const CWallet& wallet): wallet(wallet) {} + UFVKForReceiver(const CWallet& wallet): wallet(wallet) {} + + std::optional operator()(const libzcash::SaplingPaymentAddress& saplingAddr) const; + std::optional operator()(const CScriptID& scriptId) const; + std::optional operator()(const CKeyID& keyId) const; + std::optional operator()(const libzcash::UnknownReceiver& receiver) const; +}; + +class UnifiedAddressForReceiver { +private: + const CWallet& wallet; + +public: + UnifiedAddressForReceiver(const CWallet& wallet): wallet(wallet) {} std::optional operator()(const libzcash::SaplingPaymentAddress& saplingAddr) const; std::optional operator()(const CScriptID& scriptId) const; diff --git a/src/zcash/Address.hpp b/src/zcash/Address.hpp index f093b8cb7..7e4e2a6de 100644 --- a/src/zcash/Address.hpp +++ b/src/zcash/Address.hpp @@ -23,39 +23,6 @@ const unsigned char ZCASH_UFVK_ID_PERSONAL[BLAKE2bPersonalBytes] = namespace libzcash { -class UnknownReceiver { -public: - uint32_t typecode; - std::vector data; - - UnknownReceiver(uint32_t typecode, std::vector data) : - typecode(typecode), data(data) {} - - friend inline bool operator==(const UnknownReceiver& a, const UnknownReceiver& b) { - return a.typecode == b.typecode && a.data == b.data; - } - friend inline bool operator<(const UnknownReceiver& a, const UnknownReceiver& b) { - // We don't know for certain the preference order of unknown receivers, but it is - // _likely_ that the higher typecode has higher preference. The exact sort order - // doesn't really matter, as unknown receivers have lower preference than known - // receivers. - return (a.typecode > b.typecode || - (a.typecode == b.typecode && a.data < b.data)); - } -}; - -/** - * Receivers that can appear in a Unified Address. - * - * These types are given in order of preference (as defined in ZIP 316), so that sorting - * variants by `operator<` is equivalent to sorting by preference. - */ -typedef std::variant< - SaplingPaymentAddress, - CScriptID, - CKeyID, - UnknownReceiver> Receiver; - bool HasKnownReceiverType(const Receiver& receiver); struct ReceiverIterator { @@ -90,11 +57,6 @@ private: size_t cur; }; -/** A recipient address to which a unified address can be resolved */ -typedef std::variant< - CKeyID, - CScriptID, - libzcash::SaplingPaymentAddress> RecipientAddress; class UnifiedAddress { std::vector receivers; diff --git a/src/zcash/address/sapling.hpp b/src/zcash/address/sapling.hpp index b4cccd0f1..0d8100190 100644 --- a/src/zcash/address/sapling.hpp +++ b/src/zcash/address/sapling.hpp @@ -17,6 +17,7 @@ const size_t SerializedSaplingPaymentAddressSize = 43; const size_t SerializedSaplingFullViewingKeySize = 96; const size_t SerializedSaplingExpandedSpendingKeySize = 96; const size_t SerializedSaplingSpendingKeySize = 32; +const size_t SerializedSaplingDiversifierKeySize = 32; typedef std::array diversifier_t; @@ -133,7 +134,7 @@ public: SaplingExpandedSpendingKey expanded_spending_key() const; SaplingFullViewingKey full_viewing_key() const; - // Can derive Sapling addr from default diversifier + // Can derive Sapling addr from default diversifier SaplingPaymentAddress default_address() const; }; diff --git a/src/zcash/address/unified.cpp b/src/zcash/address/unified.cpp index 555f7f931..5f6553ffc 100644 --- a/src/zcash/address/unified.cpp +++ b/src/zcash/address/unified.cpp @@ -134,3 +134,7 @@ std::optional> ZcashdUnifiedFullV const diversifier_index_t& j) const { return FindAddress(j, {ReceiverType::P2PKH, ReceiverType::Sapling}); } + +RecipientAddress ZcashdUnifiedFullViewingKey::GetChangeAddress(const std::set& changeOptions) const { + throw std::runtime_error("TODO"); +} diff --git a/src/zcash/address/unified.h b/src/zcash/address/unified.h index cddee7cc4..95c95a2de 100644 --- a/src/zcash/address/unified.h +++ b/src/zcash/address/unified.h @@ -7,6 +7,7 @@ #include "bip44.h" #include "key_constants.h" +#include "script/script.h" #include "zip32.h" namespace libzcash { @@ -18,6 +19,21 @@ enum class ReceiverType: uint32_t { //Orchard = 0x03 }; +/** A recipient address to which a unified address can be resolved */ +typedef std::variant< + CKeyID, + CScriptID, + libzcash::SaplingPaymentAddress> RecipientAddress; + +/** + * An enumeration of the types of change that a transaction may + * produce. + */ +enum class ChangeType { + Sapling, + Transparent, +}; + /** * Test whether the specified list of receiver types contains a * shielded receiver type @@ -32,7 +48,41 @@ bool HasTransparent(const std::set& receiverTypes); class ZcashdUnifiedSpendingKey; +class UnknownReceiver { +public: + uint32_t typecode; + std::vector data; + + UnknownReceiver(uint32_t typecode, std::vector data) : + typecode(typecode), data(data) {} + + friend inline bool operator==(const UnknownReceiver& a, const UnknownReceiver& b) { + return a.typecode == b.typecode && a.data == b.data; + } + friend inline bool operator<(const UnknownReceiver& a, const UnknownReceiver& b) { + // We don't know for certain the preference order of unknown receivers, but it is + // _likely_ that the higher typecode has higher preference. The exact sort order + // doesn't really matter, as unknown receivers have lower preference than known + // receivers. + return (a.typecode > b.typecode || + (a.typecode == b.typecode && a.data < b.data)); + } +}; + +/** + * Receivers that can appear in a Unified Address. + * + * These types are given in order of preference (as defined in ZIP 316), so that sorting + * variants by `operator<` is equivalent to sorting by preference. + */ +typedef std::variant< + SaplingPaymentAddress, + CScriptID, + CKeyID, + UnknownReceiver> Receiver; + // prototypes for the classes handling ZIP-316 encoding (in Address.hpp) +// TODO: ZIP-316 encoding should probably be moved here class UnifiedAddress; class UnifiedFullViewingKey; @@ -116,6 +166,14 @@ public: */ std::optional> FindAddress(const diversifier_index_t& j) const; + /** + * Return the change address for this UFVK, given the provided + * set of receiver types for pools involved in this transaction. + * If the provided set is empty, return the change address + * corresponding to the most-preferred pool. + */ + RecipientAddress GetChangeAddress(const std::set& changeOptions) const; + friend bool operator==(const ZcashdUnifiedFullViewingKey& a, const ZcashdUnifiedFullViewingKey& b) { return a.transparentKey == b.transparentKey && a.saplingKey == b.saplingKey; diff --git a/src/zcash/address/zip32.cpp b/src/zcash/address/zip32.cpp index aadd2cb99..6eb651723 100644 --- a/src/zcash/address/zip32.cpp +++ b/src/zcash/address/zip32.cpp @@ -126,6 +126,25 @@ libzcash::SaplingPaymentAddress SaplingDiversifiableFullViewingKey::DefaultAddre } } +libzcash::SaplingPaymentAddress SaplingDiversifiableFullViewingKey::GetChangeAddress() const { + CDataStream ss_fvk(SER_NETWORK, PROTOCOL_VERSION); + ss_fvk << fvk; + CSerializeData fvk_bytes(ss_fvk.begin(), ss_fvk.end()); + + SaplingDiversifiableFullViewingKey internalDFVK; + CSerializeData fvk_bytes_ret(libzcash::SerializedSaplingFullViewingKeySize); + librustzcash_zip32_sapling_derive_internal_fvk( + reinterpret_cast(fvk_bytes.data()), + dk.begin(), + reinterpret_cast(fvk_bytes_ret.data()), + internalDFVK.dk.begin()); + + CDataStream ss_fvk_ret(fvk_bytes_ret, SER_NETWORK, PROTOCOL_VERSION); + ss_fvk_ret >> internalDFVK.fvk; + + return internalDFVK.DefaultAddress(); +} + SaplingExtendedSpendingKey SaplingExtendedSpendingKey::Master(const HDSeed& seed) { auto rawSeed = seed.RawSeed(); @@ -208,6 +227,10 @@ SaplingExtendedFullViewingKey SaplingExtendedSpendingKey::ToXFVK() const return ret; } +SaplingExtendedSpendingKey SaplingExtendedSpendingKey::DeriveInternalKey() const { + throw std::runtime_error("Not yet implemented"); +} + HDKeyPath Zip32AccountKeyPath( uint32_t bip44CoinType, libzcash::AccountId accountId, diff --git a/src/zcash/address/zip32.h b/src/zcash/address/zip32.h index d62fbb94e..eb2b87b9c 100644 --- a/src/zcash/address/zip32.h +++ b/src/zcash/address/zip32.h @@ -155,6 +155,8 @@ public: libzcash::SaplingPaymentAddress DefaultAddress() const; + libzcash::SaplingPaymentAddress GetChangeAddress() const; + ADD_SERIALIZE_METHODS; template @@ -237,11 +239,12 @@ struct SaplingExtendedSpendingKey { static std::pair ForAccount(const HDSeed& seed, uint32_t bip44CoinType, libzcash::AccountId accountId); static std::pair Legacy(const HDSeed& seed, uint32_t bip44CoinType, uint32_t addressIndex); - SaplingExtendedSpendingKey Derive(uint32_t i) const; SaplingExtendedFullViewingKey ToXFVK() const; + SaplingExtendedSpendingKey DeriveInternalKey() const; + friend bool operator==(const SaplingExtendedSpendingKey& a, const SaplingExtendedSpendingKey& b) { return a.depth == b.depth &&