From dba00530ee64674936d189bc2c3284ecae82e337 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 28 Mar 2022 11:02:28 -0600 Subject: [PATCH 1/2] Ensure that legacy imported addresses are properly restored to the wallet. The filter that restricted the restore of default Sapling addresses to the wallet, which limited the restoration to those addresses associated with the legacy key, was too restrictive; it meant that imported addresses were not restored to the wallet. That check has been inverted, such that we now restore the default address for any key that is not associated with the mnemonic seed. --- src/wallet/wallet.cpp | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e582503de..091fd7285 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1140,22 +1140,23 @@ bool CWallet::LoadCaches() // the default address for each key was automatically added to the in-memory // keystore, but not persisted. Following the addition of unified addresses, // all addresses must be written to the wallet database explicitly. - auto legacySeed = GetLegacyHDSeed(); - if (legacySeed.has_value()) { - for (const auto& [saplingIvk, keyMeta] : mapSaplingZKeyMetadata) { - // This condition only applies for keys derived from the legacy seed - if (keyMeta.seedFp == legacySeed.value().Fingerprint()) { - SaplingExtendedFullViewingKey extfvk; - if (GetSaplingFullViewingKey(saplingIvk, extfvk)) { - auto defaultAddress = extfvk.DefaultAddress(); - if (!HaveSaplingIncomingViewingKey(defaultAddress)) { - // restore the address to the keystore and persist it so that - // the database state is consistent. - if (!AddSaplingPaymentAddress(saplingIvk, defaultAddress)) { - LogPrintf("%s: Error: Failed to write legacy Sapling payment address to the wallet database.\n", - __func__); - return false; - } + auto mnemonicSeed = GetMnemonicSeed(); + for (const auto& [saplingIvk, keyMeta] : mapSaplingZKeyMetadata) { + // This condition only applies for keys derived from the legacy seed + // or from imported keys. + if (!mnemonicSeed.has_value() || keyMeta.seedFp != mnemonicSeed.value().Fingerprint()) { + SaplingExtendedFullViewingKey extfvk; + if (GetSaplingFullViewingKey(saplingIvk, extfvk)) { + // only add the association with the default address if it + // does not already exist + auto defaultAddress = extfvk.DefaultAddress(); + if (!HaveSaplingIncomingViewingKey(defaultAddress)) { + // restore the address to the keystore and persist it so that + // the database state is consistent. + if (!AddSaplingPaymentAddress(saplingIvk, defaultAddress)) { + LogPrintf("%s: Error: Failed to write legacy Sapling payment address to the wallet database.\n", + __func__); + return false; } } } @@ -2493,7 +2494,7 @@ void CWallet::AddToSpends(const uint256& wtxid) } // for Orchard, the effects of this operation are performed by - // AddNotesIfInvolvingMe and LoadUnifiedCaches + // AddNotesIfInvolvingMe and LoadCaches } void CWallet::ClearNoteWitnessCache() From e8f23e36294ac45197f495b07e89eec6ae133295 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 28 Mar 2022 18:04:49 -0600 Subject: [PATCH 2/2] Add a test verifying the default addr is added when importing a Sapling key. --- qa/rpc-tests/wallet_addresses.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/wallet_addresses.py b/qa/rpc-tests/wallet_addresses.py index 265e07609..20ce16814 100755 --- a/qa/rpc-tests/wallet_addresses.py +++ b/qa/rpc-tests/wallet_addresses.py @@ -4,7 +4,14 @@ # file COPYING or https://www.opensource.org/licenses/mit-license.php . from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, start_nodes, connect_nodes_bi, NU5_BRANCH_ID +from test_framework.util import ( + assert_equal, + connect_nodes_bi, + start_nodes, + stop_nodes, + wait_bitcoinds, + NU5_BRANCH_ID, +) from test_framework.mininode import nuparams # Test wallet address behaviour across network upgrades @@ -105,6 +112,25 @@ class WalletAddressesTest(BitcoinTestFramework): assert 'diversifier_index' in unified_obj[0]['addresses'][0] assert_equal(unified_obj[0]['addresses'][0]['receiver_types'], ['p2pkh', 'sapling', 'orchard']) + # import the key for sapling_1 into node 1 + sapling_1_key = self.nodes[0].z_exportkey(sapling_1) + self.nodes[1].z_importkey(sapling_1_key) + + # verify that we see the imported source + listed_addresses = self.list_addresses(1, ['imported', 'mnemonic_seed']) + imported_src = get_source(listed_addresses, 'imported') + assert_equal(imported_src['sapling'][0]['addresses'], [sapling_1]) + + # stop the nodes & restart to ensure that the imported address + # still shows up in listaddresses output + stop_nodes(self.nodes) + wait_bitcoinds() + self.setup_network() + + listed_addresses = self.list_addresses(1, ['imported', 'mnemonic_seed']) + imported_src = get_source(listed_addresses, 'imported') + assert_equal(imported_src['sapling'][0]['addresses'], [sapling_1]) + print("Testing height 2 (NU5)") self.nodes[0].generate(1) self.sync_all()