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

theStack · 2022-12-28T12:57:31Z

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.

…lets 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.

DrahtBot · 2022-12-28T12:57:33Z

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.

maflcko · 2022-12-28T13:59:17Z

Is this for backport?

Sarkerversion2030

theStack · 2023-01-03T18:46:18Z

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 · 2023-01-03T20:14:19Z

ACK 730e14a

furszy

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

furszy · 2023-01-05T12:51:51Z

src/wallet/wallet.cpp

+    // Persist added address book entries (labels, purpose) for watchonly and solvable wallets
+    auto persist_address_book = [](const CWallet& wallet) {
+        LOCK(wallet.cs_wallet);
+        WalletBatch batch{wallet.GetDatabase()};


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.

aureleoules

ACK 730e14a

I verified that the test fails on master.

aureleoules · 2023-01-05T12:56:41Z

src/wallet/wallet.cpp

+            auto purpose{addr_book_data.purpose};
+            auto label{addr_book_data.GetLabel()};
+            // don't bother writing default values (unknown purpose, empty label)
+            if (purpose != "unknown") batch.WritePurpose(address, purpose);


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?

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).

…atchonly/solvable wallets 730e14a test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) d5f4ae7 wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: 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. ACKs for top commit: achow101: ACK 730e14a furszy: code ACK 730e14a, left a non-blocking nit. aureleoules: ACK 730e14a Tree-SHA512: 159487e11e858924ef762e0190ccaea185bdff239e3d2280c8d63c4ac2649ec71714dc4d53dec644f03488f91c3b4bbbbf3434dad23bc0fcecb6657f353ea766

…lets 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. Github-Pull: bitcoin#26761 Rebased-From: d5f4ae7

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

fanquake · 2023-01-12T09:58:59Z

Backported in #26878.

…lets 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. Github-Pull: bitcoin#26761 Rebased-From: d5f4ae7

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

…lets 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. Github-Pull: bitcoin#26761 Rebased-From: d5f4ae7

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow) 50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) 648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov) 29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov) 5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov) a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) 64e7db6 Zero out wallet master key upon lock (John Moffett) b7e242e Correctly limit overview transaction list (John Moffett) cff6718 depends: fix systemtap download URL (fanquake) 7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke) 07397cd addrdb: Only call Serialize() once (Martin Zumsande) 91f83db hash: add HashedSourceWriter (Martin Zumsande) 5c824ac For feebump, ignore abandoned descendant spends (John Moffett) 428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow) cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) 342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: Backports: * #26595 * #26675 * #26679 * #26761 * #26837 * #26909 * #26924 * #26944 * bitcoin-core/gui#704 * #27053 * #27080 ACKs for top commit: instagibbs: ACK 784a754 achow101: ACK 784a754 hebasto: ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows: Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43

ryanofsky · 2023-03-13T21:05:04Z

src/wallet/wallet.cpp

+            auto label{addr_book_data.GetLabel()};
+            // don't bother writing default values (unknown purpose, empty label)
+            if (purpose != "unknown") batch.WritePurpose(address, purpose);
+            if (!label.empty()) batch.WriteName(address, label);


In commit "wallet: fully migrate address book entries for watchonly/solvable wallets" (d5f4ae7)

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:

bitcoin/src/wallet/wallet.h

Lines 219 to 222 in f50fb17

void SetLabel(const std::string& label) {

m_change = false;

m_label = label;

}

so CAddressBookData::IsChange will return true

bitcoin/src/wallet/wallet.h

Line 217 in f50fb17

bool IsChange() const { return m_change; }

Probably this line should say:

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

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).

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

yeah 👌🏼.

Instead of storing and passing around fixed strings for the purpose of an address, use an enum. This also rationalizes the CAddressBookData struct, documenting all fields and making them public, and simplifying the representation to avoid bugs like bitcoin#26761 (comment) and make it not possible to invalid address data like change addresses with labels. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

Instead of storing and passing around fixed strings for the purpose of an address, use an enum. This also rationalizes the CAddressBookData struct, documenting all fields and making them public, and simplifying the representation to avoid bugs like bitcoin/bitcoin#26761 (comment) and make it not possible to invalid address data like change addresses with labels. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

theStack added 2 commits December 28, 2022 13:44

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

730e14a

DrahtBot added the Wallet label Dec 28, 2022

Sarkerversion2030 approved these changes Dec 30, 2022

View reviewed changes

Craw69699 approved these changes Jan 5, 2023

View reviewed changes

furszy approved these changes Jan 5, 2023

View reviewed changes

aureleoules approved these changes Jan 5, 2023

View reviewed changes

achow101 merged commit b4fb0a3 into bitcoin:master Jan 5, 2023

theStack deleted the 202212-migratewallet_persist_addressbook branch January 5, 2023 18:01

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

aureleoules mentioned this pull request Jan 9, 2023

wallet: Use defined purposes instead of inlining #26858

Closed

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023

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

bb56041

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

fanquake mentioned this pull request Jan 12, 2023

[24.x] Backports #26878

Merged

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

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023

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

ccca112

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2023

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

cbcdafa

Github-Pull: bitcoin#26761 Rebased-From: 730e14a

ryanofsky reviewed Mar 13, 2023

View reviewed changes

furszy mentioned this pull request Apr 12, 2023

wallet: batch and simplify addressbook migration process #26836

Merged

bitcoin locked and limited conversation to collaborators Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Dec 28, 2022

DrahtBot commented Dec 28, 2022 •

edited

Loading

maflcko commented Dec 28, 2022

Sarkerversion2030 left a comment •

edited by maflcko

Loading

theStack commented Jan 3, 2023

achow101 commented Jan 3, 2023

furszy left a comment

furszy Jan 5, 2023 •

edited

Loading

aureleoules left a comment

aureleoules Jan 5, 2023

furszy Jan 5, 2023 •

edited

Loading

fanquake commented Jan 12, 2023

ryanofsky Mar 13, 2023

furszy Mar 13, 2023 •

edited

Loading

ryanofsky Mar 13, 2023

furszy Mar 13, 2023

	void SetLabel(const std::string& label) {
	m_change = false;
	m_label = label;
	}

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

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

Conversation

theStack commented Dec 28, 2022

DrahtBot commented Dec 28, 2022 • edited Loading

Reviews

maflcko commented Dec 28, 2022

Sarkerversion2030 left a comment • edited by maflcko Loading

Choose a reason for hiding this comment

theStack commented Jan 3, 2023

achow101 commented Jan 3, 2023

furszy left a comment

Choose a reason for hiding this comment

furszy Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

aureleoules Jan 5, 2023

Choose a reason for hiding this comment

furszy Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

fanquake commented Jan 12, 2023

ryanofsky Mar 13, 2023

Choose a reason for hiding this comment

furszy Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

ryanofsky Mar 13, 2023

Choose a reason for hiding this comment

furszy Mar 13, 2023

Choose a reason for hiding this comment

DrahtBot commented Dec 28, 2022 •

edited

Loading

Sarkerversion2030 left a comment •

edited by maflcko

Loading

furszy Jan 5, 2023 •

edited

Loading

furszy Jan 5, 2023 •

edited

Loading

furszy Mar 13, 2023 •

edited

Loading