Add unified addresses to the zcashd wallet. by nuttycom · Pull Request #5395 · zcash/zcash

nuttycom · 2021-11-19T21:23:09Z

Add method for new account generation; i.e. generate a new Unified Spending Key for the next available account #
Persist the metadata & UFVK corresponding to the generated USK to the walletdb
When a new UFVK is generated, or when a UFVK is read from the walletdb, add its Sapling components to the keystore so that note detection can find the correct UFVK.
Add Unified Address generation for a particular account & set of receivers.
When a new UA is generated, add an association between its diversifier and the source account and set of receivers to the walletdb and keystore so that we can look up/regenerate the original UA.
Make it possible to look up the source UA from a receiver on which a payment was received.

str4d

Flushing review comments.

str4d · 2021-12-15T18:02:47Z

In the PR that connects this logic to the z_getaddressforaccount RPC method, the latter's help text needs to document that "unsupported" receiver types for this account (e.g. if the account was generated before some future shielded protocol was added to zcashd) will be ignored, i.e. the returned UA will contain the largest subset of the specified receiver types that it can support. (This behaviour might change in future, if e.g. we decide to automatically add future shielded pools to existing accounts, but doing so is compatible with this documentation.)

str4d

Flushing comments.

daira

The ordering of items for the ID is blocking. Otherwise utACK.

str4d

In addition to my comments, I notice that zcash-gtest failed on three of seven CI workers, which seems statistically significant (but the error logs have been garbage-collected from Tekton).

str4d

utACK c532257bbdee1c2d8780457a0dd526d2b8b59683 modulo the return value issue below (which is technically fine as-is because it will always return true AFAICT).

ZcashdUnifiedFullViewingKey::FindAddress was previously not checking that the transparent child index did not overflow in unified address generation. GenerateUnifiedAddress has been modified to search from the last known diversifier index if no diversifier is specified, and boundary conditions for transparent addresses are now correctly handled.

…onstruction.

Co-authored-by: str4d <jack@electriccoin.co>

str4d

re-utACK eb53abb

daira · 2022-01-05T14:07:04Z

+
+        diversifier_index_t j;
+        // If the wallet is missing metadata at this UFVK id, it is probably
+        // corrupt and the node should shut down.


Does a std::out_of_range exception here actually shut the node down in all calling contexts?

We normally minimize the use of exceptions because of the difficulty of answering questions like that (and because we've had past cases in which an exception was caught and the node proceeded with inconsistent state).

daira · 2022-01-05T14:07:19Z

+        }
+
+        // If the wallet is missing metadata at this UFVK id, it is probably
+        // corrupt and the node should shut down.


Does a std::out_of_range exception here actually shut the node down in all calling contexts?

daira · 2022-01-05T14:09:35Z

            ZCASH_LEGACY_ACCOUNT).value();

-    std::optional<std::pair<CExtKey, HDKeyPath>> extKey = std::nullopt;
+    std::optional<std::pair<CKey, HDKeyPath>> extKey = std::nullopt;


The variable is still called extKey even though it no longer is. (I'm assuming that ext stood for extended rather than external.)

daira · 2022-01-05T19:22:35Z

+     * Returns `true` if this is a new entry or if the operation would not
+     * alter the existing set of receiver types at this index, `false`
+     * otherwise.


Why is it useful to return true in those two cases?

daira · 2022-01-05T19:23:36Z

+    }
+
+    /**
+     * Search the for the maximum diversifier that has already been used to


Suggested change

* Search the for the maximum diversifier that has already been used to

* Search for the the maximum diversifier that has already been used to

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 2 times, most recently from d77c489 to a47ae76 Compare November 25, 2021 02:02

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 2 times, most recently from 12c8dcb to de6ac67 Compare December 3, 2021 20:26

r3ld3v added the S-committed Status: Planned work in a sprint label Dec 7, 2021

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 2 times, most recently from 316e02d to 66837af Compare December 9, 2021 01:13

r3ld3v added this to the Core Sprint 2021-48 milestone Dec 13, 2021

nuttycom marked this pull request as ready for review December 13, 2021 22:13

zcash deleted a comment from mdr0id Dec 13, 2021

str4d force-pushed the feature/wallet_orchard branch from de1d41b to c908a3b Compare December 14, 2021 00:35

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 2 times, most recently from 20e1614 to c6289cd Compare December 14, 2021 04:18

Add functions for generating BIP-44 and ZIP-32 keypaths

48446b5

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 2 times, most recently from 7e859f4 to bbe3d87 Compare December 14, 2021 22:44

mdr0id added the safe-to-build Used to send PR to prod CI environment label Dec 15, 2021

str4d reviewed Dec 15, 2021

View reviewed changes

Comment thread src/chainparams.h Outdated

str4d reviewed Dec 15, 2021

View reviewed changes

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch from 6cd46ad to aaf48b0 Compare December 15, 2021 20:31

nuttycom added 3 commits December 15, 2021 13:53

Compute key id for UFVKs.

4966bf3

Add ZcashdUnifiedKeyMetadata and libzcash::ReceiverType

24ff7b3

Add unified key components to the transparent & Sapling wallet parts.

6d36921

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch from aaf48b0 to 7adc6ef Compare December 15, 2021 20:59

str4d reviewed Dec 16, 2021

View reviewed changes

Comment thread src/rust/include/librustzcash.h Outdated

Comment thread src/zcash/Address.cpp Outdated

nuttycom mentioned this pull request Dec 16, 2021

Remove the InvalidEncoding type from key & address variants. #5428

Closed

r3ld3v modified the milestones: Core Sprint 2021-48, Core Sprint 2021-50 Dec 20, 2021

r3ld3v added S-committed Status: Planned work in a sprint and removed S-committed Status: Planned work in a sprint labels Dec 20, 2021

r3ld3v modified the milestones: Core Sprint 2021-50, Core Sprint 2021-52 Jan 3, 2022

daira reviewed Jan 3, 2022

View reviewed changes

Comment thread src/keystore.cpp Outdated

daira approved these changes Jan 3, 2022

View reviewed changes

ebfull reviewed Jan 3, 2022

View reviewed changes

Comment thread src/zcash/address/unified.cpp Outdated

str4d requested changes Jan 3, 2022

View reviewed changes

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch 4 times, most recently from f9e1894 to c532257 Compare January 4, 2022 00:36

nuttycom mentioned this pull request Jan 4, 2022

Make address serialization respect encoding order. #5450

Merged

nuttycom added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jan 4, 2022

str4d approved these changes Jan 4, 2022

View reviewed changes

Comment thread src/wallet/wallet.cpp Outdated

Comment thread src/wallet/wallet.cpp Outdated

Comment thread src/wallet/wallet.h Outdated

Comment thread src/zcash/address/zip32.h Outdated

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch from 93a0c19 to dda30ba Compare January 4, 2022 20:09

nuttycom and others added 4 commits January 4, 2022 16:29

Use Bip44TransparentAccountKeyPath() for Bip44AccountChains keypath c…

8d6d178

…onstruction.

Improve documentation of UFVK/UA metadata storage semantics.

ad54591

Apply suggestions from code review

eb53abb

Co-authored-by: str4d <jack@electriccoin.co>

nuttycom force-pushed the feature/wallet_orchard-unified_addrs branch from dda30ba to eb53abb Compare January 4, 2022 23:30

str4d approved these changes Jan 5, 2022

View reviewed changes

str4d merged commit 1d7a29e into zcash:feature/wallet_unified_addresses Jan 5, 2022

nuttycom deleted the feature/wallet_orchard-unified_addrs branch January 5, 2022 02:37

daira reviewed Jan 5, 2022

View reviewed changes

nuttycom mentioned this pull request Jan 6, 2022

Add Unified Address key generation and storage to the wallet #5179

Closed

str4d mentioned this pull request Feb 14, 2022

Add Unified Address support to the zcashd wallet #5419

Merged

	* Search the for the maximum diversifier that has already been used to
	* Search for the the maximum diversifier that has already been used to

Conversation

nuttycom commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

daira Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

daira Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

daira Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

daira Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

daira Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nuttycom commented Nov 19, 2021 •

edited

Loading