Integrate Orchard into Unified Address types #5569

str4d · 2022-02-19T15:30:59Z

This PR enables z_getnewaccount to produce accounts that support Orchard-containing UAs, but does not add support for generating those UAs.

Closes #5255.
Closes #5394.

Co-authored-by: Jack Grigg <jack@z.cash>

str4d · 2022-02-21T17:10:00Z

Force-pushed to add a few missing pieces to the "add Orchard to UAs" and "add Orchard to UFVKs" commits.

src/gtest/test_keys.cpp

daira · 2022-02-21T17:38:46Z

src/gtest/test_keys.cpp

+                CDataStream ss(data, SER_NETWORK, PROTOCOL_VERSION);
+                auto key = libzcash::OrchardFullViewingKey::Read(ss);
+                EXPECT_EQ(key, orchardKey);
+            }
            // Enable the following after Orchard keys are supported.


Should this still be disabled? If it should, add a reference to the relevant issue.

This test is checking that constructing a UFVK from the same seed as the test vectors gives the same receivers, but the final overall check cannot be enabled without us having a way to construct a UFVK manually with an unknown component, because the test vectors chose to include an unknown component that by definition we cannot derive. Adding that API is out of scope for this PR.

We should modify the test vectors to also generate the UA that contains only the known receiver types.

Why not do this check only for the UAs without unknown components?

src/gtest/test_keys.cpp

src/keystore.cpp

str4d · 2022-02-21T18:23:52Z

src/wallet/wallet.cpp

@@ -576,7 +576,8 @@ std::optional<libzcash::ZcashdUnifiedSpendingKey>
            throw std::runtime_error("CWallet::GenerateUnifiedSpendingKeyForAccount(): Failed to add Sapling change address to the wallet.");
        };

-        // TODO ORCHARD: Add Orchard component to the wallet
+        // Add Orchard spending key to the wallet
+        orchardWallet.AddSpendingKey(usk.value().GetOrchardKey());


To clarify, this change means that z_getnewaccount will start storing the Orchard component in memory, but this PR is not exposing this fact in z_getnewaccount or allowing Orchard-containing UAs to be derived. Those changes will be made in a subsequent PR.

nuttycom

Flushing comments.

src/keystore.cpp

nuttycom

Flushing more comments.

src/rust/include/rust/orchard/keys.h

src/rust/src/orchard_keys_ffi.rs

src/keystore.cpp

daira · 2022-02-21T23:32:56Z

src/wallet/gtest/test_wallet.cpp

+    auto ufvkpair = wallet.GenerateNewUnifiedSpendingKey();
+    auto ufvk = ufvkpair.first;
+    auto account = ufvkpair.second;
+    uaResult = wallet.GenerateUnifiedAddress(account, {ReceiverType::P2PKH, ReceiverType::Sapling});


Can this test Orchard yet?

Not yet; that will be in a successor PR that @str4d is working on.

Co-authored-by: Jack Grigg <jack@z.cash>

Orchard spending keys now live exclusively on the Rust side, and will be serialized there as part of the Orchard wallet serialization.

The only place we were using the returned Unified Spending Key was in a test, where we immediately converted it to a UFVK. Co-authored-by: Kris Nuttycombe <kris@nutty.land>

str4d · 2022-02-22T21:13:20Z

Force-pushed to fix review comments, and add a missing handling of the Orchard component of a type conversion that was causing tests to fail. I'll push a separate commit to make the change to how UFVKs are looked up from Sapling addresses (to have it match Orchard).

This ensures we have the same semantics for detecting Orchard and Sapling receivers in UAs, even if the UA they are part of was derived outside of the zcashd wallet.

nuttycom

Two minor issues.

src/rust/src/unified_keys_ffi.rs

nuttycom · 2022-02-23T15:38:21Z

src/wallet/gtest/test_wallet.cpp

+    auto ufvkpair = wallet.GenerateNewUnifiedSpendingKey();
+    auto ufvk = ufvkpair.first;
+    auto account = ufvkpair.second;
+    uaResult = wallet.GenerateUnifiedAddress(account, {ReceiverType::P2PKH, ReceiverType::Sapling});


Not yet; that will be in a successor PR that @str4d is working on.

src/zcash/Address.cpp

src/zcash/address/orchard.hpp

nuttycom

utACK 3179e45 After discussion with @str4d, I've opened the following issues to to track work that shouldn't block this PR: #5583, #5584

daira

utACK

Add various bits of documentation

3e3ee1b

str4d added A-wallet Area: Wallet A-orchard Area: Orchard protocol labels Feb 19, 2022

str4d changed the base branch from master to feature/wallet_orchard February 19, 2022 15:31

str4d force-pushed the feature/wallet_orchard-ua_integration branch 2 times, most recently from 8c3cb7d to 1fa9482 Compare February 21, 2022 15:15

Add Orchard components to unified address

1a1522a

Co-authored-by: Jack Grigg <jack@z.cash>

str4d force-pushed the feature/wallet_orchard-ua_integration branch from 1a41c57 to 454db83 Compare February 21, 2022 17:09

daira reviewed Feb 21, 2022

View reviewed changes

src/gtest/test_keys.cpp Show resolved Hide resolved

str4d force-pushed the feature/wallet_orchard-ua_integration branch from 454db83 to e9810d7 Compare February 21, 2022 17:34

daira reviewed Feb 21, 2022

View reviewed changes

src/gtest/test_keys.cpp Outdated Show resolved Hide resolved

daira reviewed Feb 21, 2022

View reviewed changes

src/keystore.cpp Outdated Show resolved Hide resolved

str4d changed the title ~~Integrate Orchard into Unified Addresses~~ Integrate Orchard into Unified Address types Feb 21, 2022

daira reviewed Feb 21, 2022

View reviewed changes

src/keystore.cpp Show resolved Hide resolved

str4d marked this pull request as ready for review February 21, 2022 18:20

str4d commented Feb 21, 2022

View reviewed changes

nuttycom reviewed Feb 21, 2022

View reviewed changes

src/keystore.cpp Show resolved Hide resolved

src/keystore.cpp Show resolved Hide resolved

nuttycom reviewed Feb 21, 2022

View reviewed changes

src/rust/include/rust/orchard/keys.h Show resolved Hide resolved

src/rust/src/orchard_keys_ffi.rs Outdated Show resolved Hide resolved

src/keystore.cpp Show resolved Hide resolved

daira reviewed Feb 21, 2022

View reviewed changes

r3ld3v added this to the Core Sprint 2022-04 milestone Feb 22, 2022

nuttycom marked this pull request as draft February 22, 2022 16:04

nuttycom and others added 5 commits February 22, 2022 21:08

Add Orchard components to unified full viewing keys

258f0fc

Co-authored-by: Jack Grigg <jack@z.cash>

Add Orchard components to unified spending keys

225c78f

Co-authored-by: Jack Grigg <jack@z.cash>

Remove OrchardSpendingKey serialization code

3c84ed6

Orchard spending keys now live exclusively on the Rust side, and will be serialized there as part of the Orchard wallet serialization.

Return UFVK from CWallet::GenerateNewUnifiedSpendingKey

c2a47b8

The only place we were using the returned Unified Spending Key was in a test, where we immediately converted it to a UFVK. Co-authored-by: Kris Nuttycombe <kris@nutty.land>

Rename to ZcashdUnifiedSpendingKey::GetSaplingKey for consistency

d099e46

str4d force-pushed the feature/wallet_orchard-ua_integration branch from e9810d7 to d099e46 Compare February 22, 2022 21:11

r3ld3v modified the milestones: Core Sprint 2022-04, Core Sprint 2022-06 Feb 23, 2022

Trial-decrypt candidate Sapling receivers with all wallet UFVKs

3179e45

This ensures we have the same semantics for detecting Orchard and Sapling receivers in UAs, even if the UA they are part of was derived outside of the zcashd wallet.

str4d marked this pull request as ready for review February 23, 2022 09:45

r3ld3v requested a review from ebfull February 23, 2022 15:29

nuttycom requested changes Feb 23, 2022

View reviewed changes

This was referenced Feb 23, 2022

Clean up null pointer handling in Rust FFI #5583

Open

Skip unnecessary serialization & parsing when crossing the FFI with Orchard raw addresses. #5584

Open

nuttycom approved these changes Feb 23, 2022

View reviewed changes

ebfull approved these changes Feb 23, 2022

View reviewed changes

daira approved these changes Feb 23, 2022

View reviewed changes

nuttycom merged commit 49065be into zcash:feature/wallet_orchard Feb 23, 2022

str4d deleted the feature/wallet_orchard-ua_integration branch February 23, 2022 18:21

This was referenced Feb 24, 2022

Add the ability to generate Orchard spending keys in the wallet #5255

Closed

Add Orchard key support to ZcashdUnified* types #5394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Orchard into Unified Address types #5569

Integrate Orchard into Unified Address types #5569

str4d commented Feb 19, 2022 •

edited

str4d commented Feb 21, 2022

daira Feb 21, 2022

str4d Feb 21, 2022

nuttycom Feb 21, 2022

daira Feb 23, 2022

str4d Feb 21, 2022

nuttycom left a comment

nuttycom left a comment

daira Feb 21, 2022

nuttycom Feb 23, 2022

str4d commented Feb 22, 2022

nuttycom left a comment

nuttycom Feb 23, 2022

nuttycom left a comment •

edited by str4d

daira left a comment

Integrate Orchard into Unified Address types #5569

Integrate Orchard into Unified Address types #5569

Conversation

str4d commented Feb 19, 2022 • edited

str4d commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Feb 22, 2022

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment • edited by str4d

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d commented Feb 19, 2022 •

edited

nuttycom left a comment •

edited by str4d