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

achow101 · 2023-03-06T22:28:58Z

Instead of storing and passing around fixed strings for the purpose of an address, use an enum.

DrahtBot · 2023-03-06T22:29:01Z

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	josibake
Stale ACK	aureleoules, ryanofsky, Xekyo

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:

#bitcoin-core/gui/723 (Add warnings for non-active addresses in receive tab and address book by pinheadmz)
#bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#27224 (refactor: Remove CAddressBookData::destdata by ryanofsky)
#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#26836 (wallet: finish addressbook encapsulation by furszy)
#25620 (wallet: Introduce AddressBookManager by furszy)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#24914 (wallet: Load database records in a particular order by achow101)

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.

maflcko

nits

test/functional/wallet_labels.py

src/wallet/wallet.h

ryanofsky

This is a nice change, and I started reviewing 44a2ae1, but it looks like there are a lot of functions taking optional<AddressPurpose> values that should more properly take AddressPurpose values for simplicity and better type checking. (Basically NotifyAddressBookChanged and all the functions it calls). I think this might stem from not having separate SetAddressBook and UpdateAddressBook functions, so I want to look into this before reviewing more of the PR. (Having separate functions could help because Set function could make purpose a required parameter, while the Update function could let it be it optional).

src/interfaces/wallet.h

src/qt/addresstablemodel.cpp

src/wallet/walletdb.cpp

src/wallet/wallet.h

ryanofsky · 2023-03-13T21:57:27Z

I made some changes to this in a branch that could be adopted https://github.com/ryanofsky/bitcoin/commits/review.27217.1-edit

Tries to split commits and add a lot of comments so this is easier to review
Make enum a public wallet type instead of putting it in interfaces/ so wallet/ code does not depend on interfaces/wallet code and interfaces::Wallet::AddressPurpose becomes just wallet::AddressPurpose
Replaced std::optional<AddressPurpose> with plain AddressPurpose in many places where purpose is known or can be derived from IsMine, to simplify code and avoid bugs.
Dropped AddressPurpose::UNKNOWN to avoid defining an enum value that doesn't have any corresponding value in the wallet database.
Added AddressPurpse::REFUND enum value to be able to load old wallets that may have refund addresses without errors or warnings.
Changed assert(0) failure when loading wallets with unrecognized purpose strings with a warning.

achow101 · 2023-03-15T17:10:23Z

I made some changes to this in a branch that could be adopted https://github.com/ryanofsky/bitcoin/commits/review.27217.1-edit

I've adopted these changes.

ryanofsky

Code review ACK db521d3. Thanks for adopting suggested changes. I left some new suggestions, but they are not important and can be ignored.

I think this PR should be straightforward for others to review now, and nice because it should demystify the purpose field and wallet address book as a whole.

ryanofsky · 2023-03-15T20:06:38Z

src/interfaces/wallet.h


-    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, std::string name, std::string purpose)
-        : dest(std::move(dest)), is_mine(is_mine), name(std::move(name)), purpose(std::move(purpose))
+    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name)


In commit "wallet: Replace use of purpose strings with an enum" (db521d3)

Note for reviewers: in a few places in this PR, struct order or argument order is changed so the AddressPurpose field follows IsMine or IsChange fields. This is done because the fields hold overlapping information, and in most cases new code should rely on the other fields instead of AddressPurpose.

src/wallet/interfaces.cpp

ryanofsky · 2023-03-15T20:22:14Z

src/wallet/rpc/addresses.cpp

@@ -677,7 +677,7 @@ RPCHelpMan getaddressesbylabel()
            // std::set in O(log(N))), UniValue::__pushKV is used instead,
            // which currently is O(1).
            UniValue value(UniValue::VOBJ);
-            value.pushKV("purpose", _purpose);
+            value.pushKV("purpose", _purpose ? PurposeToString(*_purpose) : "unknown");


In commit "wallet: Replace use of purpose strings with an enum" (db521d3)

Note for reviewers: this is preserving previous behavior. Default value for CAddressBook::purpose field previously was "unknown" when a purpose was not recorded, and now it is null which is translated to "unknown"

src/wallet/rpc/addresses.cpp

src/wallet/types.h

src/wallet/wallet.cpp

src/wallet/wallet.h

test/functional/wallet_labels.py

ryanofsky

Code review ACK 0d18142. Suggested a few comment/whitespace tweaks, but this is also fine as-is.

src/wallet/transaction.h

src/wallet/wallet.h

doc/release-notes-27217.md

ryanofsky

Code review ACK a48010f. Just comment and whitespace changes since last review

aureleoules

ACK a48010f, looks good to me.

src/wallet/wallet.cpp

ryanofsky

Code review ACK 98821a7. Just suggested changes since last review, the main one being changing translateTransactionType to use a switch statement.

This had 3 reviews before, so it would be nice to have reacks and hopefully merge this soon.

murchandamus

ACK 98821a7

src/wallet/types.h

src/wallet/wallet.h

doc/release-notes-27217.md

josibake · 2023-04-11T09:14:53Z

ACK 635c50a

murchandamus · 2023-04-11T19:23:57Z

reACK 635c50a

but appears to need rebase

Move isminetype and isminefilter there this commit, add WalletPurpose type next commit.

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>

josibake · 2023-04-12T08:45:17Z

reACK 18fc71a

denavila · 2023-04-13T05:29:01Z

src/qt/walletmodel.cpp

    bool invoked = QMetaObject::invokeMethod(walletmodel, "updateAddressBook",
                              Q_ARG(QString, strAddress),
                              Q_ARG(QString, strLabel),
                              Q_ARG(bool, isMine),
-                              Q_ARG(QString, strPurpose),
+                              Q_ARG(wallet::AddressPurpose, purpose),


This change appears to trigger this assert when executing "getnewaddress" from the Console:
Assertion failed: (invoked), function NotifyAddressBookChanged, file walletmodel.cpp, line 392.

debug.log shows this error message:
GUI: QMetaMethod::invoke: Unable to handle unregistered datatype 'wallet::AddressPurpose'

Thanks, opened an issue here to follow up: bitcoin-core/gui#725.

Fixed in bitcoin-core/gui#726.

a45b544 qt: Register `wallet::AddressPurpose` type (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin/bitcoin#27217. Fixes #725. ACKs for top commit: achow101: ACK a45b544 furszy: Tested ACK a45b544 Tree-SHA512: c670f4bf56442613d3fe038b0ba21acfcd4c69aa5340072e9a77d83f5fab1bf2facd87a9e1f42d88f496d277b27b79e7090444d59a9b9e71f3b486e171daa669

a45b544 qt: Register `wallet::AddressPurpose` type (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin#27217. Fixes bitcoin#725. ACKs for top commit: achow101: ACK a45b544 furszy: Tested ACK a45b544 Tree-SHA512: c670f4bf56442613d3fe038b0ba21acfcd4c69aa5340072e9a77d83f5fab1bf2facd87a9e1f42d88f496d277b27b79e7090444d59a9b9e71f3b486e171daa669

DrahtBot added the Wallet label Mar 6, 2023

maflcko reviewed Mar 7, 2023

View reviewed changes

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

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

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

achow101 force-pushed the address-purpose-enums branch from 87f8f8b to 44a2ae1 Compare March 7, 2023 15:56

ryanofsky reviewed Mar 10, 2023

View reviewed changes

achow101 force-pushed the address-purpose-enums branch from 44a2ae1 to db521d3 Compare March 15, 2023 17:10

ryanofsky approved these changes Mar 15, 2023

View reviewed changes

achow101 force-pushed the address-purpose-enums branch 2 times, most recently from a797f99 to 0d18142 Compare March 17, 2023 21:17

ryanofsky approved these changes Mar 20, 2023

View reviewed changes

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

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

doc/release-notes-27217.md Outdated Show resolved Hide resolved

achow101 force-pushed the address-purpose-enums branch from 0d18142 to a48010f Compare March 20, 2023 16:57

ryanofsky approved these changes Mar 20, 2023

View reviewed changes

This was referenced Mar 21, 2023

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

Merged

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

Closed

aureleoules approved these changes Mar 26, 2023

View reviewed changes

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

achow101 force-pushed the address-purpose-enums branch from a48010f to 98821a7 Compare April 10, 2023 14:40

ryanofsky approved these changes Apr 10, 2023

View reviewed changes

DrahtBot requested review from aureleoules and josibake April 10, 2023 16:05

murchandamus approved these changes Apr 10, 2023

View reviewed changes

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

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

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

doc/release-notes-27217.md Outdated Show resolved Hide resolved

achow101 force-pushed the address-purpose-enums branch from 98821a7 to 635c50a Compare April 11, 2023 00:44

DrahtBot requested review from ryanofsky and murchandamus and removed request for josibake April 11, 2023 09:14

DrahtBot added the Needs rebase label Apr 11, 2023

DrahtBot removed the request for review from murchandamus April 11, 2023 19:24

ryanofsky and others added 4 commits April 11, 2023 15:52

wallet: Add wallet/types.h for simple public enum and struct types

8741522

Move isminetype and isminefilter there this commit, add WalletPurpose type next commit.

wallet: add AddressPurpose enum to replace string values

2f80005

doc: Release note for purpose string restriction

18fc71a

achow101 force-pushed the address-purpose-enums branch from 635c50a to 18fc71a Compare April 11, 2023 20:01

DrahtBot removed the Needs rebase label Apr 11, 2023

DrahtBot requested a review from murchandamus April 12, 2023 08:45

fanquake merged commit cae0608 into bitcoin:master Apr 12, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 12, 2023

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

130e81a

denavila reviewed Apr 13, 2023

View reviewed changes

fanquake mentioned this pull request Apr 13, 2023

Crash using getnewaddress in the console bitcoin-core/gui#725

Closed

hebasto mentioned this pull request Apr 13, 2023

Register wallet::AddressPurpose type bitcoin-core/gui#726

Merged

ryanofsky mentioned this pull request Jul 20, 2023

wallet: Filter-out "send" addresses from listreceivedby* #25973

Closed

bitcoin locked and limited conversation to collaborators Apr 12, 2024

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

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

Uh oh!

Conversation

achow101 commented Mar 6, 2023

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Mar 13, 2023

Uh oh!

achow101 commented Mar 15, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josibake commented Apr 11, 2023

Uh oh!

murchandamus commented Apr 11, 2023

Uh oh!

josibake commented Apr 12, 2023

Uh oh!

denavila Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 6, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading