wallet: Migrate entire address book entries to watchonly and solvables too

achow101 commented at 10:39 pm on October 6, 2023: member

Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.

DrahtBot commented at 10:39 pm on October 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28976 (wallet: Fix migration of blank wallets by achow101)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#22341 (rpc: add path to gethdkey by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Oct 6, 2023

DrahtBot added the label Needs rebase on Oct 11, 2023

achow101 force-pushed on Oct 11, 2023

DrahtBot removed the label Needs rebase on Oct 11, 2023

DrahtBot added the label Needs rebase on Oct 23, 2023

achow101 force-pushed on Oct 23, 2023

DrahtBot removed the label Needs rebase on Oct 24, 2023

DrahtBot added the label CI failed on Oct 24, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

DrahtBot added the label CI failed on Oct 27, 2023

in test/functional/wallet_migration.py:893 in a39eff2675 outdated

888+        # Import a pubkey to the test wallet and send some funds to it
889+        reused_imported_addr = def_wallet.getnewaddress()
890+        wallet.importpubkey(def_wallet.getaddressinfo(reused_imported_addr)["pubkey"])
891+        txid = def_wallet.sendtoaddress(reused_imported_addr, 2)
892+        vout = find_vout_for_address(def_wallet, txid, reused_imported_addr)
893+        imported_utxo = {"txid": txid, "vout": vout}

maflcko commented at 11:29 am on October 27, 2023:

Could use the new helper from 2a349f9ea5de9f0157cd117289996e8640473a6d or add the missing include (see lint failure)

achow101 commented at 0:00 am on November 14, 2023:

used the new helper.

achow101 force-pushed on Nov 13, 2023

achow101 force-pushed on Nov 14, 2023

DrahtBot removed the label CI failed on Nov 14, 2023

in src/wallet/wallet.cpp:3964 in 02665016a5 outdated

3965-                        data.watchonly_wallet->m_address_book[addr_pair.first].purpose = addr_pair.second.purpose;
3966-                        if (!addr_pair.second.IsChange()) {
3967-                            data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label);
3968-                        }
3969+                        // Add to the watchonly. Copy the entire address book entry
3970+                        data.watchonly_wallet->m_address_book[addr_pair.first] = addr_pair.second;

ryanofsky commented at 3:49 pm on December 5, 2023:

In commit “wallet: Migrate entire address book entries” (02665016a5870c9fcba049b849a4086b14d08255)

This is so much more straightforward, thank you for cleaning it up. But while you are getting rid of confusing GetLabel code here, I think you should do it below too:

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -4007,10 +4007,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
 3         WalletBatch batch{wallet.GetDatabase()};
 4         for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
 5             auto address{EncodeDestination(destination)};
 6-            std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel());
 7-            // don't bother writing default values (unknown purpose)
 8             if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));
 9-            if (label) batch.WriteName(address, *label);
10+            if (addr_book_data.label) batch.WriteName(address, *addr_book_data.label);
11             for (const auto& [id, request] : addr_book_data.receive_requests) {
12                 batch.WriteAddressReceiveRequest(destination, id, request);
13             }

ryanofsky approved

ryanofsky commented at 3:52 pm on December 5, 2023: contributor

Code review ACK 02665016a5870c9fcba049b849a4086b14d08255. Nice cleanup and test!

wallet: Migrate entire address book entries 406b71abcb

achow101 force-pushed on Dec 5, 2023

ryanofsky approved

ryanofsky commented at 7:13 pm on December 5, 2023: contributor

Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review

achow101 requested review from furszy on Jan 2, 2024

achow101 requested review from Sjors on Jan 2, 2024

in test/functional/wallet_migration.py:885 in 406b71abcb

876@@ -877,6 +877,61 @@ def test_failed_migration_cleanup(self):
877             assert_equal(magic, BTREE_MAGIC)
878 
879 
880+    def test_avoidreuse(self):
881+        self.log.info("Test that avoidreuse persists after migration")
882+        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
883+
884+        wallet = self.create_legacy_wallet("avoidreuse")
885+        wallet.setwalletflag("avoid_reuse", True)

furszy commented at 5:51 pm on January 3, 2024:

tiny nit: could provide the avoid_reuse flag to createwallet() instead of calling setwalletflag separately.

in test/functional/wallet_migration.py:954 in 406b71abcb

950@@ -896,6 +951,7 @@ def run_test(self):
951         self.test_conflict_txs()
952         self.test_hybrid_pubkey()
953         self.test_failed_migration_cleanup()
954+        self.test_avoidreuse()

furszy commented at 6:45 pm on January 3, 2024:

Would be good to unload the wallets at the end of the test case.

furszy approved

furszy commented at 7:06 pm on January 3, 2024: member

Code review ACK 406b71ab

With two non-blocking points:

Firstly, made a test to verify that the transaction’s extra information is preserved during migration: c1aabd1b2a. This includes the preservation of bump fee info (‘replaces_txid’ and ‘replaced_by_txid’) and the user’s custom comments (‘comment’ and ‘comment_to’). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.

Secondly, shilling mode; with #26836, which now is part of #28574, this could have been implemented with even less code.

fanquake merged this on Jan 8, 2024

fanquake closed this on Jan 8, 2024

wallet: Migrate entire address book entries to watchonly and solvables too #28610

Code Coverage

Reviews

Conflicts