wallet: batch and simplify addressbook migration process by furszy · Pull Request #26836 · bitcoin/bitcoin

furszy · 2023-01-06T13:05:27Z

Commits decoupled from #28574, focused on the address book cloning process

Includes:

DB batch operations and flow simplification for the address book migration process.
Code improvements to CWallet::DelAddressBook and Wallet::SetAddrBookWithDB methods.

These changes will let us consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.

DrahtBot · 2023-01-06T13:05:29Z

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	achow101, josibake
Concept ACK	rserranon, hernanmarino
Approach ACK	w0xlt
Stale ACK	pablomartin4btc

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:

#29403 (wallet: batch erase procedures and improve 'EraseRecords' performance 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.

w0xlt

Approach ACK.
I will review soon.

rserranon

Concept ACK,

I understand this may become a separate commit, but why not encapsulate the other calls to wallet->m_address_book[addr_pair.first].purpose = purpose ?

src/wallet/wallet.cpp:3980:                            data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
src/wallet/wallet.cpp:3996:                            data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
src/wallet/wallet.cpp:4017:                    data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
src/wallet/wallet.cpp:4030:                    data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;

If m_address_book has been encapsulated, why not make it private or protected?

src/wallet/wallet.h

src/wallet/wallet.cpp

furszy · 2023-02-27T13:53:44Z

Thanks @rserranon.

why not encapsulate the other calls to wallet->m_address_book[addr_pair.first].purpose = purpose ?

Good catch. I did this changes prior the wallet migration PR merge. Will add those too and make the addressbook member private so we don't keep adding more in the future.

furszy · 2023-02-27T14:13:42Z

Updated per feedback. Now we no longer access the wallet's addressbook member externally.

Plus, improvements to the address book migration process:

Fixed a bug where we don't copy the "send" records to all the wallets.
Have re-written the process with no code duplication.
Batched db writes so the disk dump is done all at once.

rserranon

tAck

unit tests
functional tests
qt & bitcoin-cli manual tests

hernanmarino

tested ACK, both unit and functional tests.

pablomartin4btc

Approach ACK.
I'll perform some testing soon.

src/wallet/wallet.cpp

pablomartin4btc

tested ACK.
On the code review, went thru the different commits of the PR.
Performed manual testing using bitcoin-qt.

josibake · 2024-02-05T19:43:14Z

reACK 238993e

verified the diff, thanks for taking the suggestions!

furszy

Updated per @maflcko review in #29403 (comment). Small diff.

DrahtBot · 2024-02-07T14:17:57Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21320950165}

furszy · 2024-02-07T15:21:03Z

Green CI. Ready to go.

achow101

ACK 1f177ff9a6ab229bd6486941e46daa92ab22b622

src/wallet/wallet.cpp

josibake · 2024-02-07T20:18:14Z

reACK 1f177ff

1) Encode destination only once (instead of three). 2) Fail if the entry's linked data cannot be removed. 3) Don't remove entry from memory if db write fail. 4) Notify GUI only if removal succeeded

Same process written in a cleaner manner. Removing code duplication.

Optimizing the process performance and consistency.

Instead of doing one db transaction per removed record, we now batch all removals in a single db transaction. Speeding up the process and preventing the wallet from entering an inconsistent state when any of the intermediate writes fail.

furszy

Updated per feedback. Thanks achow!. Small diff.

Two changes:

Removed __func__ usages in the logging messages (comment_1 and comment_2).
Have divided the purpose and name erasing lines per request.

achow101 · 2024-02-07T22:26:39Z

ACK 86960cd

josibake · 2024-02-08T08:42:54Z

reACK 86960cd

Verified the diff, looks good.

ryanofsky

Post-merge code review ACK 86960cd. Nice code cleanups, and use of batches to speed things up

ryanofsky · 2024-02-08T14:30:05Z

src/wallet/wallet.cpp

+        fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
+
+        CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
+        record.SetLabel(strName);


In commit "refactor: SetAddressBookWithDB, minimize number of map lookups" (d094331)

This isn't actually minimizing the number of lookups since it is doing two lookups in the insert case. Would replace:

std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address); fUpdated = mi != m_address_book.end() && !mi->second.IsChange(); CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];

with:

auto [it, inserted] = m_address_book.emplace(std::piecewise_construct, std::forward_as_tuple(address), std::tuple{}); fUpdated = !inserted && !it->second.IsChange(); CAddressBookData& record = it->second;

ryanofsky · 2024-02-08T15:04:10Z

src/wallet/wallet.cpp

+    for (const auto& [dest, record] : m_address_book) {
+        // Ensure "receive" entries that are no longer part of the original wallet are transferred to another wallet
+        // Entries for everything else ("send") will be cloned to all wallets.
+        bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest);


In commit "refactor: wallet, simplify addressbook migration" (595bbe6)

IMO, this would be less confusing if require_transfer bool was replaced by an copy_to_all bool with the opposite meaning, because the thing require_transfer is used for in the loop below is to determining whether the address book entry is copied to all wallets or is moved from the *this wallet to one of the other two.

The name require_transfer is also misleading because the transfer is not required if dest is in not_migrated_dests.

ryanofsky · 2024-02-08T15:23:04Z

src/wallet/wallet.cpp

+    }
+
+    // Write address book entry to disk
+    auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) {


In commit "wallet: addressbook migration, batch db writes" (342c45f)

write_address_book would be a more descriptive name than func_store_addr and watchonly_wallets would be more descriptive than wallet_vec. The _vec suffix and and func_ prefix do not do anything to help a reader of the code, IMO.

DrahtBot added the Wallet label Jan 6, 2023

This was referenced Jan 7, 2023

wallet: Introduce AddressBookManager #25620

Closed

wallet: Load database records in a particular order #24914

Merged

[Draft / POC] Silent Payments #24897

Closed

w0xlt reviewed Jan 9, 2023

View reviewed changes

rserranon reviewed Feb 24, 2023

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_finish_addressbook_encapsulation branch 2 times, most recently from cb82845 to 0053949 Compare February 27, 2023 14:06

This was referenced Feb 27, 2023

wallet: Use defined purposes instead of inlining #26858

Closed

refactor: Use util::Result class for wallet loading #25722

Draft

furszy requested a review from rserranon March 6, 2023 21:47

DrahtBot mentioned this pull request Mar 6, 2023

wallet: Turn destdata entries into CAddressBookData fields #27215

Closed

rserranon approved these changes Mar 7, 2023

View reviewed changes

This was referenced Mar 7, 2023

wallet: Replace use of purpose strings with an enum #27217

Merged

refactor: Remove CAddressBookData::destdata #27224

Merged

Banin1363 mentioned this pull request Mar 11, 2023

Concept ACK, Banin1363/Bannet-#2

Open

furszy requested a review from w0xlt March 12, 2023 14:15

This was referenced Mar 13, 2023

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

Merged

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

DrahtBot mentioned this pull request Mar 21, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

hernanmarino approved these changes Mar 26, 2023

View reviewed changes

pablomartin4btc reviewed Mar 27, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 5, 2023

Add warnings for non-active addresses in receive tab and address book bitcoin-core/gui#723

Closed

pablomartin4btc approved these changes Apr 5, 2023

View reviewed changes

DrahtBot added the Needs rebase label Apr 12, 2023

furszy force-pushed the 2022_wallet_finish_addressbook_encapsulation branch from 0053949 to 819c0bc Compare April 12, 2023 14:00

DrahtBot removed the Needs rebase label Apr 12, 2023

DrahtBot requested review from hernanmarino, rserranon and w0xlt and removed request for rserranon February 5, 2024 14:30

furszy force-pushed the 2022_wallet_finish_addressbook_encapsulation branch from 3aff6d5 to 238993e Compare February 5, 2024 14:33

DrahtBot requested review from hernanmarino, rserranon and w0xlt and removed request for hernanmarino, rserranon and w0xlt February 5, 2024 19:43

furszy mentioned this pull request Feb 7, 2024

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

Merged

furszy commented Feb 7, 2024

View reviewed changes

achow101 reviewed Feb 7, 2024

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

achow101 mentioned this pull request Feb 7, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

furszy added 6 commits February 7, 2024 18:11

wallet: clean redundancies in DelAddressBook

97b0753

1) Encode destination only once (instead of three). 2) Fail if the entry's linked data cannot be removed. 3) Don't remove entry from memory if db write fail. 4) Notify GUI only if removal succeeded

refactor: SetAddrBookWithDB, signal only if write succeeded

bba4f8d

refactor: SetAddressBookWithDB, minimize number of map lookups

d094331

refactor: wallet, simplify addressbook migration

595bbe6

Same process written in a cleaner manner. Removing code duplication.

wallet: addressbook migration, batch db writes

342c45f

Optimizing the process performance and consistency.

furszy commented Feb 7, 2024

View reviewed changes

ryanofsky reviewed Feb 8, 2024

View reviewed changes

ryanofsky mentioned this pull request Feb 27, 2024

log, refactor: Allow log macros to accept context arguments #29256

Draft

Conversation

furszy commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

rserranon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy commented Feb 27, 2023

Uh oh!

furszy commented Feb 27, 2023

Uh oh!

rserranon left a comment

Choose a reason for hiding this comment

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Feb 5, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 7, 2024

Uh oh!

furszy commented Feb 7, 2024

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josibake commented Feb 7, 2024

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 7, 2024

Uh oh!

josibake commented Feb 8, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

furszy commented Jan 6, 2023 •

edited

Loading

DrahtBot commented Jan 6, 2023 •

edited

Loading

rserranon left a comment •

edited

Loading

pablomartin4btc left a comment •

edited

Loading

furszy left a comment •

edited

Loading