wallet: address book migration bug fixes #28038

1. 
   furszy commented at 7:28 pm on July 6, 2023: member

   Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it.

   Bug 1: Non-Cloning of External ‘Send’ Records The external ‘send’ records were not being correctly cloned to all wallets.

   Bug 2: Persistence of Empty Labels As address book entries without associated db label records can be treated as change (the label field inside the CAddressBookData class is optional, nullopt labels make CAddressBookData ::IsChange() return true), we must persist empty labels during the migration process. The user might have called setlabel with an "" string for an external address and that must be retained during migration.

2. wallet: migration bugfix, clone 'send' record label to all wallets 1b64f6498c
3. wallet: migration bugfix, persist empty labels

   addressbook records with no associated label could be
   treated as change. And we don't want that for external
   addresses.

   a277f8357a
4. test: wallet, add coverage for addressbook migration 7ecc29a0b7
5. 
   DrahtBot commented at 7:28 pm on July 6, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101

   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:

   • #26836 (wallet: fix bug, simplify and add coverage for addressbook migration by furszy)

   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.

6. DrahtBot added the label Wallet on Jul 6, 2023
7. 
   achow101 commented at 8:49 pm on July 6, 2023: member

   ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9

   Nice catch!

8. 
   fanquake commented at 2:25 pm on July 7, 2023: member

   @achow101 do you want these backported for 25.1?
9. 
   achow101 commented at 4:20 pm on July 7, 2023: member

   @achow101 do you want these backported for 25.1?

   Yes

10. achow101 added the label Needs backport (25.x) on Jul 7, 2023
11. fanquake merged this on Jul 7, 2023
12. fanquake closed this on Jul 7, 2023

    ---

13. fanquake referenced this in commit 59b06b696a on Jul 7, 2023
14. fanquake referenced this in commit 4b16650c10 on Jul 7, 2023
15. fanquake referenced this in commit 37d9cc657c on Jul 7, 2023
16. fanquake removed the label Needs backport (25.x) on Jul 7, 2023
17. 
    fanquake commented at 4:35 pm on July 7, 2023: member

    Added to #28047 for backporting.
18. sidhujag referenced this in commit 25cbdad9bd on Jul 7, 2023
19. in src/wallet/wallet.cpp:4062 in a277f8357a outdated

    4058@@ -4059,10 +4059,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
    4059         WalletBatch batch{wallet.GetDatabase()};
    4060         for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
    4061             auto address{EncodeDestination(destination)};
    4062-            auto label{addr_book_data.GetLabel()};
    4063-            // don't bother writing default values (unknown purpose, empty label)
    4064+            std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel());
    

    ---

    ---

    

    ryanofsky commented at 5:35 pm on July 10, 2023:

    This looks ok, but I think would have been more straightforward to write as:

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

20. 
    ryanofsky commented at 5:54 pm on July 10, 2023: contributor

    Code review ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9
21. furszy deleted the branch on Jul 20, 2023
22. fanquake referenced this in commit ecc74cd4f3 on Sep 6, 2023
23. bitcoin locked this on Jul 19, 2024

Contributors
furszy DrahtBot achow101 fanquake ryanofsky

Labels
Wallet