wallet: batch and simplify addressbook migration process #26836
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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?
|
Thanks @rserranon.
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. |
cb82845 to
0053949
Compare
|
Updated per feedback. Now we no longer access the wallet's addressbook member externally. Plus, improvements to the address book migration process:
|
0053949 to
819c0bc
Compare
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.
1f177ff to
86960cd
Compare
There was a problem hiding this comment.
Updated per feedback. Thanks achow!. Small diff.
Two changes:
|
ACK 86960cd |
|
reACK 86960cd Verified the diff, looks good. |
| 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); |
There was a problem hiding this comment.
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;| 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); |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Write address book entry to disk | ||
| auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) { |
There was a problem hiding this comment.
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.
c98fc36 wallet: migration, consolidate external wallets db writes (furszy) 7c9076a wallet: migration, consolidate main wallet db writes (furszy) 9ef20e8 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy) 34bf079 wallet: refactor ApplyMigrationData to return util::Result<void> (furszy) aacaaaa wallet: provide WalletBatch to 'RemoveTxs' (furszy) 57249ff wallet: introduce active db txn listeners (furszy) 91e065e wallet: remove post-migration signals connection (furszy) 055c053 wallet: provide WalletBatch to 'DeleteRecords' (furszy) 122d103 wallet: introduce 'SetWalletFlagWithDB' (furszy) 6052c78 wallet: decouple default descriptors creation from external signer setup (furszy) f2541d0 wallet: batch MigrateToDescriptor() db transactions (furszy) 66c9936 bench: add coverage for wallet migration process (furszy) Pull request description: Last step in a chain of PRs (#26836, #28894, #28987, #29403). #### Detailed Description: The current wallet migration process performs only individual db writes. Accessing disk to delete all legacy records, clone and clean each address book entry for every created wallet, create each new descriptor (with their corresponding master key, caches and key pool), and also clone and delete each transaction that requires to be transferred to a different wallet. This work consolidates all individual disk writes into two batch operations. One for the descriptors creation from the legacy data and a second one for the execution of the migration process itself. Efficiently dumping all the information to disk at once atomically at the end of each process. This represent a speed up and also a consistency improvement. During migration, we either want to succeed or fail. No other outcomes should be accepted. We should never leave a partially migrated wallet on disk and request the user to manually restore the previous wallet from a backup (at least not if we can avoid it). Since the speedup depends on the storage device, benchmark results can vary significantly. Locally, I have seen a 15% speedup on a USB 3.2 pendrive. #### Note for Testers: The first commit introduces a benchmark for the migration process. This one can be cherry-picked on top of master to compare results pre and post changes. Please note that the benchmark setup may take some time (~70 seconds here) due to the absence of a batching mechanism for the address generation process (`GetNewDestination()` calls). ACKs for top commit: achow101: ACK c98fc36 theStack: re-ACK c98fc36 pablomartin4btc: re-ACK c98fc36 Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6
Commits decoupled from #28574, focused on the address book cloning process
Includes:
CWallet::DelAddressBookandWallet::SetAddrBookWithDBmethods.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.