wallet: address book migration bug fixes #28038
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
addressbook records with no associated label could be treated as change. And we don't want that for external addresses.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
Member
|
ACK 7ecc29a Nice catch! |
Member
|
@achow101 do you want these backported for 25.1? |
Member
Yes |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jul 7, 2023
Github-Pull: bitcoin#28038 Rebased-From: 1b64f64
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jul 7, 2023
addressbook records with no associated label could be treated as change. And we don't want that for external addresses. Github-Pull: bitcoin#28038 Rebased-From: a277f83
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Jul 7, 2023
Github-Pull: bitcoin#28038 Rebased-From: 7ecc29a
Member
|
Added to #28047 for backporting. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 7, 2023
7ecc29a test: wallet, add coverage for addressbook migration (furszy) a277f83 wallet: migration bugfix, persist empty labels (furszy) 1b64f64 wallet: migration bugfix, clone 'send' record label to all wallets (furszy) Pull request description: 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. ACKs for top commit: achow101: ACK 7ecc29a Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7
ryanofsky
reviewed
Jul 10, 2023
| auto address{EncodeDestination(destination)}; | ||
| auto label{addr_book_data.GetLabel()}; | ||
| // don't bother writing default values (unknown purpose, empty label) | ||
| std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel()); |
Contributor
There was a problem hiding this comment.
This looks ok, but I think would have been more straightforward to write as:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8fa93b97d655..ac6520e95a29 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4059,10 +4059,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
WalletBatch batch{wallet.GetDatabase()};
for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
auto address{EncodeDestination(destination)};
- std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel());
// don't bother writing default values (unknown purpose)
if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));
- if (label) batch.WriteName(address, *label);
+ if (addr_book_data.label) batch.WriteName(address, *addr_book_data.label);
}
};
if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet);
fanquake
added a commit
that referenced
this pull request
Sep 6, 2023
494f1af depends: xcb-proto 1.15.2 (fanquake) 513ca0a test: wallet, add coverage for watch-only raw sh script migration (furszy) 6d5a510 descriptor: InferScript, do not return top-level only func as sub descriptor (furszy) 37d9cc6 test: wallet, add coverage for addressbook migration (furszy) 4b16650 wallet: migration bugfix, persist empty labels (furszy) 59b06b6 wallet: migration bugfix, clone 'send' record label to all wallets (furszy) Pull request description: Currently backports: * #28038 * #28067 2nd & 3rd commits. * #28097 ACKs for top commit: stickies-v: ACK 494f1af Tree-SHA512: cea134cfa72950d8428191adce79c0881302ca54488f64d3d4a5f9070bb2445d8074e58fa31a482481c4eabb74c852a025f53597540fc646dc20f33b21cf0a06
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
labelfield inside theCAddressBookDataclass is optional,nulloptlabels makeCAddressBookData ::IsChange()return true), we must persist empty labels during the migration process.The user might have called
setlabelwith an "" string for an external address and that must be retained during migration.