Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet] Sapling keys management final round. #1884

Merged
merged 19 commits into from
Oct 9, 2020

Conversation

furszy
Copy link

@furszy furszy commented Sep 29, 2020

Decoupled from #1798, purely focused on the wallet's Sapling keys area. On four main aspects:

  1. Sapling keys management refactored to support viewing key functionality.

  2. New RPC commands implemented: exportsaplingkey, importsaplingkey, importsaplingviewingkey and exportsaplingviewingkey .

  3. RPC commands expanded adding sapling keys support capabilities: dumpwallet and importwallet .

  4. Implemented unit test coverage for: getnewshieldedaddress on an encrypted and unencrypted wallet, listsaplingaddresses, importsaplingkey, exportsaplingkey via rpc calls.

Extra data:
Further down 1798 road, we will get even more testing coverage for this features like #1798/c5d4ffe3, plus a test coverage expansion for Sapling keys in importwallet and dumpwallet . (cannot add them here because they are depending on the shielded_spend functionality that will treat in another decouplement PR round).


The list of decoupled commits from #1798 is the following one:

  • SaplingFullViewingKey -> SaplingExtendedFullViewingKey in keystore maps —> a98a0be

  • Remove default address parameter from Sapling keystore methods —> 96d30a7

  • test: Add test for CBasicKeyStore::AddSaplingFullViewingKey —> 21f683c

  • Add encoding and decoding for Sapling extended full viewing keys. —> 7da9de3

  • [Wallet] Do not add a sapling key if the sspkm is not enabled. —> 9ad9572

  • GetSpendingKeyForPaymentAddress method implemented —> f1d6646

  • RPC: Implement exportsaplingkey —> 2eb2a3a

  • wallet: AddSpendingKeyToWallet method implemented. —> e66acfd

  • adding exportsaplingkey command to the rpcwallet table —> 368c0d0

  • RPC: importsaplingkey command implemented. —> 3e4b1ba

  • dumpwallet including sapling keys —> 30336c1

  • importwallet including sapling keys —> 519b78c

  • importsaplingviewingkey RPC command implemented. —> 4323b2d

  • RPC: exportsaplingviewingkey command back ported —> a38da6a

  • Sapling logging type added. —> b301ff6

  • rpc_wallet_getnewsaplingaddress test implemented. —> 102c760

  • rpc_wallet_encrypted_wallet_sapzkeys test included. —> d461836

  • Test: sapling wallet import/export keys implemented. —> 8792587

  • importsaplingkey RPC command unit test —> 67f7993

All of these maps are created from scratch on wallet load, so we can
alter their contents without compatibility concerns. With this change,
we can use the existing maps to implement viewing key support. The
downside is that the wallet will take more space in memory, but that can
easily be improved in future by storing ExtFVK fingerprints in some of
the maps (which would improve memory usage even compared to the original
layout).

Coming from zcash@d74fdd76eb1707012beed1eba1e9ab83cf6ea5b5
Now that we store SaplingExtendedFullViewingKey internally, we have
access to the default address everywhere we require it.

Coming from zcash@b62bb98087a83d8bb9c1ff7669d34b9ca9adbaf4
Coming from zcash@7de06244e6a874cd468bad021b7537d5909c1b71
@furszy furszy self-assigned this Sep 29, 2020
@furszy furszy force-pushed the 2020_sapling_wallet_keys branch 2 times, most recently from fd0033b to de5872d Compare October 1, 2020 21:29
@Fuzzbawls Fuzzbawls added this to the 5.0.0 milestone Oct 2, 2020
@Fuzzbawls Fuzzbawls changed the title Wallet: Sapling keys management final round. [Wallet] Sapling keys management final round. Oct 2, 2020
@furszy
Copy link
Author

furszy commented Oct 2, 2020

travis job solved, ready for review.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noticed some duplicate file output in dumpwallet, but otherwise looking good

src/wallet/rpcdump.cpp Show resolved Hide resolved
@furszy
Copy link
Author

furszy commented Oct 8, 2020

updated per feedback.

Fuzzbawls
Fuzzbawls previously approved these changes Oct 8, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1b63091

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good 🥃
Just few minor points

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
src/test/librust/sapling_rpc_wallet_tests.cpp Outdated Show resolved Hide resolved
@furszy
Copy link
Author

furszy commented Oct 9, 2020

Updated per feedback.

src/wallet/rpcdump.cpp Show resolved Hide resolved
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
@furszy
Copy link
Author

furszy commented Oct 9, 2020

updated per feedback.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 9bfa1d6

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 9bfa1d6

I have noticed some unused header includes in a few unit tests, but I'm already planning to do a sweep cleanup of header includes + ordering closer to 5.0 release.

@furszy furszy merged commit ee1be56 into PIVX-Project:master Oct 9, 2020
@furszy furszy deleted the 2020_sapling_wallet_keys branch November 29, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants