wallet: fully migrate address book entries for watchonly/solvable wallets

theStack commented at 12:57 pm on December 28, 2022: contributor

Currently migratewallet migrates the address book (i.e. labels and purposes) for watchonly and solvable wallets only in RAM, but doesn’t persist them on disk. Fix this by adding another loop for both of the special wallet types after which writes the corresponding NAME and PURPOSE entries to the database in a single batch. Also adds a corresponding test that checks if labels were migrated correctly for a watchonly wallet.

Currently `migratewallet` migrates the address book (i.e. labels and
purposes) for watchonly and solvable wallets only in RAM, but doesn't
persist them on disk. Fix this by adding another loop for both of the
special wallet types after which writes the corresponding NAME and
PURPOSE entries to the database in a single batch.

d5f4ae7fac

test: wallet: check that labels are migrated to watchonly wallet 730e14a317

DrahtBot commented at 12:57 pm on December 28, 2022: 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, furszy, aureleoules

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

DrahtBot added the label Wallet on Dec 28, 2022

maflcko commented at 1:59 pm on December 28, 2022: member

Is this for backport?

Sarkerversion2030 approved

Sarkerversion2030 commented at 10:42 pm on December 30, 2022: none

theStack commented at 6:46 pm on January 3, 2023: contributor

Is this for backport?

I’d say yes. Though the issue only affects an experimental RPC and is not as severe as e.g. a node crash or losing funds, losing labels after migration is still annoying enough for users.

achow101 commented at 8:14 pm on January 3, 2023: member

ACK 730e14a317ae45fe871c8d6f44a51936756bbbea

Craw69699 approved

in src/wallet/wallet.cpp:4009 in d5f4ae7fac outdated

4001@@ -4002,6 +4002,23 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
4002             }
4003         }
4004     }
4005+
4006+    // Persist added address book entries (labels, purpose) for watchonly and solvable wallets
4007+    auto persist_address_book = [](const CWallet& wallet) {
4008+        LOCK(wallet.cs_wallet);
4009+        WalletBatch batch{wallet.GetDatabase()};

furszy commented at 12:51 pm on January 5, 2023:

Need to call batch.TxnBegin() and batch.TxnCommit() to make this a real batch. (yes.. the WalletBatch name is misleading).

Extra note: It’s basically what have done in #25297. Batch entire processes where/when is possible. Which.. should revive it soon and see how the current wallet behaves.

furszy approved

furszy commented at 12:53 pm on January 5, 2023: member

code ACK 730e14a3, left a non-blocking nit.

in src/wallet/wallet.cpp:4015 in d5f4ae7fac outdated

4010+        for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
4011+            auto address{EncodeDestination(destination)};
4012+            auto purpose{addr_book_data.purpose};
4013+            auto label{addr_book_data.GetLabel()};
4014+            // don't bother writing default values (unknown purpose, empty label)
4015+            if (purpose != "unknown") batch.WritePurpose(address, purpose);

aureleoules commented at 12:56 pm on January 5, 2023:

Maybe unrelated but shouldn’t these purpose strings be stored in a header file such as static const char* PURPOSE_RECEIVE = "receive" instead of inlining them?

furszy commented at 1:20 pm on January 5, 2023:

yeah, I remember doing this changes in the past. Moving the possible purposes to a separate file so it can be included in the GUI model and widgets without requiring wallet dependency (we want the GUI to be as abstracted as possible from the wallet internals).

aureleoules approved

aureleoules commented at 1:00 pm on January 5, 2023: member

ACK 730e14a317ae45fe871c8d6f44a51936756bbbea

I verified that the test fails on master.

achow101 merged this on Jan 5, 2023

achow101 closed this on Jan 5, 2023

theStack deleted the branch on Jan 5, 2023

fanquake added the label Needs backport (24.x) on Jan 5, 2023

sidhujag referenced this in commit 80e21ce55f on Jan 6, 2023

fanquake referenced this in commit a59b6f584f on Jan 12, 2023

fanquake referenced this in commit bb56041487 on Jan 12, 2023

fanquake removed the label Needs backport (24.x) on Jan 12, 2023

fanquake commented at 9:58 am on January 12, 2023: member

Backported in #26878.

fanquake referenced this in commit 3b430b3dbd on Jan 12, 2023

fanquake referenced this in commit ccca112a1e on Jan 12, 2023

fanquake referenced this in commit 342abfb3f4 on Feb 22, 2023

fanquake referenced this in commit cbcdafa471 on Feb 22, 2023

glozow referenced this in commit c8c85ca16e on Feb 27, 2023

in src/wallet/wallet.cpp:4016 in 730e14a317

4011+            auto address{EncodeDestination(destination)};
4012+            auto purpose{addr_book_data.purpose};
4013+            auto label{addr_book_data.GetLabel()};
4014+            // don't bother writing default values (unknown purpose, empty label)
4015+            if (purpose != "unknown") batch.WritePurpose(address, purpose);
4016+            if (!label.empty()) batch.WriteName(address, label);

ryanofsky commented at 9:05 pm on March 13, 2023:

In commit “wallet: fully migrate address book entries for watchonly/solvable wallets” (d5f4ae7fac0bceb0c9ad939b9a4fbdb85da0bf95)

It seems like a bug to not write the label when then label is empty, because will make receiving address with empty labels labels appear to be change addresses. If the label isn’t written CAddressBookData::SetLabel will not be called:

https://github.com/bitcoin/bitcoin/blob/f50fb178c30ea4bec0b369af3d15cab08d33396f/src/wallet/wallet.h#L219-L222

so CAddressBookData::IsChange will return true

https://github.com/bitcoin/bitcoin/blob/f50fb178c30ea4bec0b369af3d15cab08d33396f/src/wallet/wallet.h#L217

Probably this line should say:

0if (!addr_book_data.IsChange()) batch.WriteName(address, addr_book_data.GetLabel());

furszy commented at 9:19 pm on March 13, 2023:

Check #26836, bug should be fixed there. Have re-written the process without the persist_address_book calls (there by, no empty labels should be skipped).

ryanofsky commented at 9:29 pm on March 13, 2023:

Thanks will check it out. Also would be good to have a test for the bug.

furszy commented at 9:41 pm on March 13, 2023:

yeah 👌🏼.

ryanofsky referenced this in commit 39b0a18b87 on Mar 13, 2023

achow101 referenced this in commit db521d3e89 on Mar 15, 2023

achow101 referenced this in commit 1426c5350d on Mar 17, 2023

achow101 referenced this in commit ea3cc035dd on Mar 17, 2023

achow101 referenced this in commit ccddce7582 on Mar 20, 2023

achow101 referenced this in commit a52f20224f on Apr 10, 2023

achow101 referenced this in commit 2d95a95507 on Apr 11, 2023

achow101 referenced this in commit e83babe3b8 on Apr 11, 2023

RandyMcMillan referenced this in commit cf35c04f8c on May 27, 2023

janus referenced this in commit d713fa6a37 on Sep 3, 2023

backpacker69 referenced this in commit 5bb6a33930 on Mar 5, 2024

bitcoin locked this on Mar 12, 2024

wallet: fully migrate address book entries for watchonly/solvable wallets #26761

Reviews