wallet: Replace use of purpose strings with an enum

achow101 commented at 10:28 pm on March 6, 2023: member

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

DrahtBot commented at 10:29 pm on March 6, 2023: contributor

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.

DrahtBot added the label Wallet on Mar 6, 2023

in test/functional/wallet_labels.py:76 in 87f8f8be65 outdated

70@@ -71,6 +71,10 @@ def run_test(self):
71         node = self.nodes[0]
72         assert_equal(len(node.listunspent()), 0)
73 
74+        self.log.info("Checking listlabels' invalid parameters")
75+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be one of 'send', or 'receive'.", node.listlabels, "notavalidpurpose");
76+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be one of 'send', or 'receive'.", node.listlabels, "unknown");

maflcko commented at 9:11 am on March 7, 2023:

nit:

0test/functional/wallet_labels.py:76:132: E703 statement ends with a semicolon

achow101 commented at 3:56 pm on March 7, 2023:

Fixed

in src/wallet/wallet.h:227 in 87f8f8be65 outdated

223+        case interfaces::Wallet::AddressPurpose::RECEIVE:
224+            return "receive";
225+        case interfaces::Wallet::AddressPurpose::SEND:
226+            return "send";
227+        // no default case so the compiler will warn when a new enum as added
228+        }

maflcko commented at 9:14 am on March 7, 2023:

0        } // no default case so the compiler will warn when a new enum is added
1

nit: typo

achow101 commented at 3:56 pm on March 7, 2023:

Done

in src/wallet/wallet.h:239 in 87f8f8be65 outdated

235+        } else if (s == "receive") {
236+            return interfaces::Wallet::AddressPurpose::RECEIVE;
237+        } else if (s == "send") {
238+            return interfaces::Wallet::AddressPurpose::SEND;
239+        } else {
240+            // Any other string is a programming error

maflcko commented at 9:15 am on March 7, 2023:

0            // Any other string is a programming error or disk corruption

nit: This could also happen on disk corruption, no?

achow101 commented at 3:57 pm on March 7, 2023:

Done

maflcko commented at 9:15 am on March 7, 2023: member

nits

achow101 force-pushed on Mar 7, 2023

in src/interfaces/wallet.h:61 in 44a2ae100b outdated

56@@ -57,6 +57,12 @@ using WalletValueMap = std::map<std::string, std::string>;
57 class Wallet
58 {
59 public:
60+    enum class AddressPurpose : uint8_t {
61+        UNKNOWN = 0,

ryanofsky commented at 7:12 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Let’s get rid of this unknown enum value. It looks like it was never written to disk and was purely an in-memory value. There are only two WalletBatch::WritePurpose calls in the codebase and if you check their callers you can see they are always passing hardcoded “send” and “receive” or "" values. This goes back to #2539 when purpose rows were first added to the database.

Having this UNKNOWN value would just be confusing and could be a footgun because you couldn’t write a function taking an AddressPurpose argument and have the compiler guarantee it was passed a known value.

in src/interfaces/wallet.h:63 in 44a2ae100b outdated

56@@ -57,6 +57,12 @@ using WalletValueMap = std::map<std::string, std::string>;
57 class Wallet
58 {
59 public:
60+    enum class AddressPurpose : uint8_t {
61+        UNKNOWN = 0,
62+        RECEIVE = 1,
63+        SEND = 2,

ryanofsky commented at 7:20 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

In #2539 there was also a “refund” purpose, so it might make sense to add a REFUND enum value to allow those wallets to be loaded without triggering an assert(false) error in PurposeFromString.

in src/qt/addresstablemodel.cpp:63 in 44a2ae100b outdated

61+    if (!purpose.has_value() || purpose.value() == interfaces::Wallet::AddressPurpose::UNKNOWN) { // if purpose not set, guess
62+        addressType = (isMine ? AddressTableEntry::Receiving : AddressTableEntry::Sending);
63+    } else if (purpose.value() == interfaces::Wallet::AddressPurpose::SEND) {
64         addressType = AddressTableEntry::Sending;
65-    else if (strPurpose == "receive")
66+    } else if (purpose == interfaces::Wallet::AddressPurpose::RECEIVE) {

ryanofsky commented at 7:28 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Code style comment: It is a little inconsistent to call .value() in one case but not the other. Personally, I think it would be clearest to write this as if (purpose == SEND) ... else if (purpose == RECEIVE) else ... like the original code, instead of being overly verbose and paranoid about null values, which are already handled sanely by operator==

in src/wallet/walletdb.cpp:352 in 44a2ae100b outdated

346@@ -347,7 +347,9 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
347         } else if (strType == DBKeys::PURPOSE) {
348             std::string strAddress;
349             ssKey >> strAddress;
350-            ssValue >> pwallet->m_address_book[DecodeDestination(strAddress)].purpose;
351+            std::string purpose_str;
352+            ssValue >> purpose_str;
353+            pwallet->m_address_book[DecodeDestination(strAddress)].SetPurposeFromString(purpose_str);

ryanofsky commented at 7:49 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

It seems like it would be better to do something like log a warning or return an error message if an unrecognized purpose is set here, rather than crashing with an assert.

in src/wallet/wallet.h:210 in 44a2ae100b outdated

206@@ -206,10 +207,47 @@ class CAddressBookData
207 private:
208     bool m_change{true};
209     std::string m_label;
210-public:
211     std::string purpose;

ryanofsky commented at 7:51 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Should this older purpose field be dropped?

in src/wallet/wallet.h:212 in 44a2ae100b outdated

206@@ -206,10 +207,47 @@ class CAddressBookData
207 private:
208     bool m_change{true};
209     std::string m_label;
210-public:
211     std::string purpose;
212+    //! The purpose of this address
213+    interfaces::Wallet::AddressPurpose m_purpose{interfaces::Wallet::AddressPurpose::UNKNOWN};

ryanofsky commented at 7:59 pm on March 10, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

It seems a little misleading to set default address book entry purpose to UNKNOWN, because the purpose of addresses with default entries are known: they are just change addresses used to hold avoid_reuse bookkeeping info.

So I think it would probably be better to change type of m_purpose field to optional<AddressPurpose> and leave it unset, when m_change is true, rather making the purpose seem unknown.

Alternately, could be more ambitious and add an AddressPurpose::CHANGE enum value and drop bool m_change entirely.

EDIT: I forgot to mention the other case when purpose will be is UNKNOWN here. It will be UNKNOWN not just for change addresses with avoid_reuse info, but also for addresses from old wallet databases that do not contain “purpose” rows. In this latter case, UNKNOWN values would actually be descriptive and not misleading. However, I still think the two suggested alternatives above that drop UNKNOWN would be preferable, because UNKNOWN is misleading in some cases, even if it is not misleading in every case.

ryanofsky commented at 8:03 pm on March 10, 2023: contributor

This is a nice change, and I started reviewing 44a2ae100b600bc9c9f4c956223fab3b1948ea8d, 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).

ryanofsky commented at 9:57 pm on March 13, 2023: contributor

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 force-pushed on Mar 15, 2023

achow101 commented at 5:10 pm on March 15, 2023: member

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.

in src/interfaces/wallet.h:359 in db521d3e89 outdated

357     std::string name;
358-    std::string purpose;
359 
360-    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, std::string name, std::string purpose)
361-        : dest(std::move(dest)), is_mine(is_mine), name(std::move(name)), purpose(std::move(purpose))
362+    WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name)

ryanofsky commented at 8:06 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

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.

in src/wallet/interfaces.cpp:209 in db521d3e89 outdated

206-            *is_mine = m_wallet->IsMine(dest);
207+            *is_mine = *dest_is_mine;
208         }
209         if (purpose) {
210-            *purpose = entry->purpose;
211+            *purpose = entry->purpose ? *entry->purpose : *dest_is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND;

ryanofsky commented at 8:16 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

Would maybe add a comment like “// In very old wallets, address purpose may not recorded, so derive it from IsMine value”

achow101 commented at 9:10 pm on March 17, 2023:

Done

in src/wallet/interfaces.cpp:221 in db521d3e89 outdated

219-        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) {
220+        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) {
221             if (is_change) return;
222-            result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose);
223+            isminetype is_mine = m_wallet->IsMine(dest);
224+            result.emplace_back(dest, is_mine, purpose ? *purpose : is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND, label);

ryanofsky commented at 8:18 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

Could add the same comment here (“In very old wallets, address purpose may not recorded, so derive it from IsMine value”)

achow101 commented at 9:10 pm on March 17, 2023:

Done

in src/wallet/rpc/addresses.cpp:680 in db521d3e89 outdated

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

ryanofsky commented at 8:22 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

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"

in src/wallet/rpc/addresses.cpp:728 in db521d3e89 outdated

728+        std::string purpose_str = request.params[0].get_str();
729+        if (!purpose_str.empty()) {
730+            if (!IsValidPurposeString(purpose_str)) {
731+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid 'purpose' argument, must be one of 'send', or 'receive'.");
732+            }
733+            purpose = PurposeFromString(purpose_str);

ryanofsky commented at 8:25 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

If not updating the code above, would be good to add assert(purpose) or a similar check here, because that condition would indicate a bug.

in src/wallet/rpc/addresses.cpp:728 in db521d3e89 outdated

725+    std::optional<AddressPurpose> purpose;
726     if (!request.params[0].isNull()) {
727-        purpose = request.params[0].get_str();
728+        std::string purpose_str = request.params[0].get_str();
729+        if (!purpose_str.empty()) {
730+            if (!IsValidPurposeString(purpose_str)) {

ryanofsky commented at 8:35 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

I think it might be a little better to drop the IsValidPurposeString function and just write:

0purpose = PurposeFromString(purpose_str);
1if (!purpose) {
2    throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid purpose argument, must a known purpose string, typically 'send' or 'receive'.");
3}

since underlying code should work for any past purpose string like “refund”, or future ones.

achow101 commented at 9:11 pm on March 17, 2023:

Done

in src/wallet/types.h:60 in db521d3e89 outdated

56@@ -57,7 +57,7 @@ using isminefilter = std::underlying_type<isminetype>::type;
57  * interfaces and saved for new addresses. It is basically redundant with an
58  * address's IsMine() result.
59  */
60-enum class AddressPurpose : uint8_t {

ryanofsky commented at 8:46 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

This change should be moved to previous commit “wallet: add AddressPurpose enum to replace string values” (2932d7be3cdf3de79b2767735a91180192d4efce), instead of being here. Alternately, could also keep the fixed width type, though IMO it is not helpful.

achow101 commented at 9:11 pm on March 17, 2023:

Done

in src/wallet/wallet.cpp:2423 in db521d3e89 outdated

2421     }
2422     NotifyAddressBookChanged(address, strName, is_mine,
2423-                             strPurpose, (fUpdated ? CT_UPDATED : CT_NEW));
2424-    if (!strPurpose.empty() && !batch.WritePurpose(EncodeDestination(address), strPurpose))
2425+                             purpose ? *purpose : is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND,
2426+                             (fUpdated ? CT_UPDATED : CT_NEW));

ryanofsky commented at 8:47 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

Could add the same comment suggested previously here above NotifyAddressBookChanged (“In very old wallets, address purpose may not recorded, so derive it from IsMine value”)

achow101 commented at 9:11 pm on March 17, 2023:

Done

in src/wallet/wallet.h:228 in db521d3e89 outdated

231 
232+    /**
233+     * Additional address metadata map that can currently hold two types of keys:
234+     *
235+     *   "used" keys with values always set to "p" if present. This is set on
236+     *       IsMine addresses that have already been spend from if the

ryanofsky commented at 8:51 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

s/spend/spent/

achow101 commented at 9:11 pm on March 17, 2023:

Done

in test/functional/wallet_labels.py:76 in db521d3e89 outdated

70@@ -71,6 +71,10 @@ def run_test(self):
71         node = self.nodes[0]
72         assert_equal(len(node.listunspent()), 0)
73 
74+        self.log.info("Checking listlabels' invalid parameters")
75+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be one of 'send', or 'receive'.", node.listlabels, "notavalidpurpose")
76+        assert_raises_rpc_error(-8, "Invalid 'purpose' argument, must be one of 'send', or 'receive'.", node.listlabels, "unknown")

ryanofsky commented at 9:06 pm on March 15, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (db521d3e89b23046f73410a875bb9660178e3842)

I think the PR might benefit from a release note, or at least a summary of behavior changes in the PR description like, ‘Wallet loading code will now issue warnings if addresses have unrecognized purpose strings other than “send” “receive” and “refund” and listlabels RPC will raise an error if an unrecognized purpose is requested.’

achow101 commented at 9:11 pm on March 17, 2023:

Done

ryanofsky approved

ryanofsky commented at 9:07 pm on March 15, 2023: contributor

Code review ACK db521d3e89b23046f73410a875bb9660178e3842. 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.

achow101 force-pushed on Mar 17, 2023

in src/wallet/transaction.h:136 in f00f5dad29 outdated

131+
132+
133 typedef std::map<std::string, std::string> mapValue_t;
134 
135+
136+

ryanofsky commented at 3:32 pm on March 20, 2023:

In commit “wallet: Add wallet/types.h for simple public enum and struct types” (f00f5dad29d81b9700dd84ec08a6e72f915988a0)

Looks like there might be an extra 3 lines of space instead of 2 (but fine to keep if that’s intentional)

achow101 commented at 4:57 pm on March 20, 2023:

Fixed

in src/wallet/wallet.h:212 in ea3cc035dd outdated

214-public:
215-    std::string purpose;
216+    /**
217+     * Address label for sending and receiving addresses, most commonly set to
218+     * "", which is the default label. Change addresses do not have labels, and
219+     * wallet code uses lack of a label to identify change address for fee

ryanofsky commented at 3:41 pm on March 20, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (ea3cc035ddf8f15fd18407231c3ff9edde8891c1)

s/address/addresses/

achow101 commented at 4:57 pm on March 20, 2023:

Done

in doc/release-notes-27217.md:4 in 0d18142f59 outdated

0@@ -0,0 +1,6 @@
1+Wallet
2+------
3+
4+* Purposes strings are restricted to the currently known values of "send", "receive", and "refund".

ryanofsky commented at 3:44 pm on March 20, 2023:

In commit “doc: Release note for purpose string restriction” (0d18142f595b43070bd07b608d43c9ed9b37cdcc)

Maybe s/restricted/now restricted/ to be clear this is new behavior not background information.

achow101 commented at 4:57 pm on March 20, 2023:

Done

ryanofsky approved

ryanofsky commented at 3:47 pm on March 20, 2023: contributor

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

achow101 force-pushed on Mar 20, 2023

ryanofsky approved

ryanofsky commented at 6:03 pm on March 20, 2023: contributor

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

in src/wallet/wallet.cpp:2421 in a48010f1fd outdated

2421     }
2422+    // In very old wallets, address purpose may not be recorded so we derive it from IsMine
2423     NotifyAddressBookChanged(address, strName, is_mine,
2424-                             strPurpose, (fUpdated ? CT_UPDATED : CT_NEW));
2425-    if (!strPurpose.empty() && !batch.WritePurpose(EncodeDestination(address), strPurpose))
2426+                             purpose ? *purpose : is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND,

aureleoules commented at 11:59 pm on March 25, 2023:

Could use purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND) instead. Same for other instances.

achow101 commented at 2:40 pm on April 10, 2023:

Changed here and elsewhere

aureleoules approved

aureleoules commented at 0:11 am on March 26, 2023: member

ACK a48010f1fd6d8a430b8234789ac3c91ec9d06972, looks good to me.

in src/qt/addresstablemodel.cpp:64 in a48010f1fd outdated

65+    } else if (purpose == wallet::AddressPurpose::RECEIVE) {
66         addressType = AddressTableEntry::Receiving;
67-    else if (strPurpose == "unknown" || strPurpose == "") // if purpose not set, guess
68+    } else {
69         addressType = (isMine ? AddressTableEntry::Receiving : AddressTableEntry::Sending);
70+    }

josibake commented at 3:50 pm on March 27, 2023:

IIRC, don’t we get all sorts of compiler goodness if we use switch with an enum, like compiler warnings for unhandled cases? Would be nice, in the event we ever add a new case to the enum (although seems unlikely for this specific use-case).

I always prefer using switch with enums, but feel free to ignore if you don’t feel like retouching/don’t share the same opinion.

josibake commented at 4:09 pm on March 27, 2023:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch-enum , super nice and still warns for unhandled cases even when you have a default.

ryanofsky commented at 2:22 pm on April 7, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (ccddce7582a51fd2857ce4d142076167631e9cfe)

re: #27217 (review)

I think I agree with the suggestion to use a switch statement here. Also:

It looks like behavior of this code has been changed unintentionally to no longer return hidden in the “refund” case.
The “if purpose not set, guess” logic is no longer here needed because it is implemented at the lower layer

So this could be written as:

0/* Determine address type from address purpose */
1static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose purpose)
2{
3    // "refund" addresses aren't shown, and change addresses aren't returned by getAddresses at all.
4    switch (purpose) {
5        case wallet::AddressPurpose::RECEIVE: return AddressTableEntry::Receiving;
6        case wallet::AddressPurpose::SEND:    return AddressTableEntry::Sending;
7        case wallet::AddressPurpose::REFUND:  return AddressTableEntry::Hidden;
8    }
9}

achow101 commented at 2:40 pm on April 10, 2023:

Done as suggested

josibake commented at 3:54 pm on March 27, 2023: member

Code review ACK https://github.com/bitcoin/bitcoin/pull/27217/commits/a48010f1fd6d8a430b8234789ac3c91ec9d06972

Absolutely love this. Death to strings.

Looks good from a code review standpoint, also did some light testing with the GUI and RPC calls, planning on doing more.

in src/wallet/wallet.h:211 in ccddce7582 outdated

213-    std::string m_label;
214-public:
215-    std::string purpose;
216+    /**
217+     * Address label for sending and receiving addresses, most commonly set to
218+     * "", which is the default label. Change addresses do not have labels, and

ryanofsky commented at 2:39 pm on April 7, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (ccddce7582a51fd2857ce4d142076167631e9cfe)

It would be good to make it more explicit that value is null for change addresses. Could say:

0/**
1 * Address label which is always nullopt for change addresses. For sending
2 * and receiving addresses, it will be set to an arbitrary label string
3 * provided by the user, or to "", which is the default label. The presence
4 * or absence of a label is used to distinguish change addresses from
5 * non-change addresses by wallet transaction listing and fee bumping code.
6 */
7std::optional<std::string> label;

achow101 commented at 2:41 pm on April 10, 2023:

Done as suggested

in src/wallet/wallet.h:227 in ccddce7582 outdated

230+    std::optional<AddressPurpose> purpose;
231 
232+    /**
233+     * Additional address metadata map that can currently hold two types of keys:
234+     *
235+     *   "used" keys with values always set to "p" if present. This is set on

ryanofsky commented at 2:57 pm on April 7, 2023:

In commit “wallet: Replace use of purpose strings with an enum” (ccddce7582a51fd2857ce4d142076167631e9cfe)

To be more accurate, comment should say with values set to "1" or "p" if present. Value was inadvertently changed from "p" to "1" in f5ba424cd44619d9b9be88b8593d69a7ba96db26 from #21353

achow101 commented at 2:41 pm on April 10, 2023:

Done as suggested

ryanofsky commented at 3:16 pm on April 7, 2023: contributor

~This PR has 3 acks so maybe it is ready to be merged.~ I think this PR basically looks good as is but it would be a little better if it avoided changing behavior for refund addresses in translateTransactionType (see comment below)

achow101 force-pushed on Apr 10, 2023

ryanofsky approved

ryanofsky commented at 4:05 pm on April 10, 2023: contributor

Code review ACK 98821a7aca88ac9401786e848899866b71446460. 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.

DrahtBot requested review from aureleoules on Apr 10, 2023

DrahtBot requested review from josibake on Apr 10, 2023

in src/wallet/types.h:54 in af47fff680 outdated

47@@ -48,6 +48,20 @@ enum isminetype : unsigned int {
48 };
49 /** used for bitflags of isminetype */
50 using isminefilter = std::underlying_type<isminetype>::type;
51+
52+/**
53+ * Address purpose field that has been been stored with wallet sending and
54+ * receiving addreses since BIP70 payment protocol support was added in

murchandamus commented at 5:53 pm on April 10, 2023:

Nit, typo:

0 * receiving addresses since BIP70 payment protocol support was added in

achow101 commented at 0:44 am on April 11, 2023:

Fixed.

in src/wallet/wallet.h:239 in a52f20224f outdated

248-    void SetLabel(const std::string& label) {
249-        m_change = false;
250-        m_label = label;
251-    }
252+    /** Accessor methods. */
253+    bool IsChange() const { return !label; }

murchandamus commented at 6:09 pm on April 10, 2023:

I thought here first that if the user does not set a label, we might treat an address as change. However, I realized that an empty string "" is truthy, while no string being set is falsy, i.e. even if the address label is set to "" which appears to be the case for any receive addresses, it’ll be truthy.

achow101 commented at 0:44 am on April 11, 2023:

I’ve made this clearer by using has_value().

in src/wallet/wallet.h:265 in a52f20224f outdated

274+        return AddressPurpose::RECEIVE;
275+    } else if (s == "send") {
276+        return AddressPurpose::SEND;
277+    } else if (s == "refund") {
278+        return AddressPurpose::REFUND;
279+    }

murchandamus commented at 6:11 pm on April 10, 2023:

Nit, since each immediately returns, this may be more readable:

0    if (s == "receive") return AddressPurpose::RECEIVE;
1    if (s == "send") return AddressPurpose::SEND;
2    if (s == "refund") return AddressPurpose::REFUND;

achow101 commented at 0:45 am on April 11, 2023:

I prefer the else if style, but have collapsed this and PurposeToString to be more readable.

in doc/release-notes-27217.md:6 in 98821a7aca outdated

0@@ -0,0 +1,6 @@
1+Wallet
2+------
3+
4+* Purposes strings are now restricted to the currently known values of "send", "receive", and "refund".
5+  Wallets that have unrecognized purpose strings will have loading warnings, and the `listlabels`
6+  RPC will raise an error if an unrecognized purpose is requested.

murchandamus commented at 6:28 pm on April 10, 2023:

Perhaps mention that the “purpose string” appears in the context of addresses, at least for me that would not necessarily have been obvious just from this release note without reviewing the PR.

achow101 commented at 0:45 am on April 11, 2023:

Done.

murchandamus approved

murchandamus commented at 6:28 pm on April 10, 2023: contributor

ACK 98821a7aca88ac9401786e848899866b71446460

achow101 force-pushed on Apr 11, 2023

josibake commented at 9:14 am on April 11, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27217/commits/635c50a57b3f72f83f1ee681f6a4b7766b8988f6

DrahtBot removed review request from josibake on Apr 11, 2023

DrahtBot requested review from ryanofsky on Apr 11, 2023

DrahtBot requested review from murchandamus on Apr 11, 2023

DrahtBot added the label Needs rebase on Apr 11, 2023

murchandamus commented at 7:23 pm on April 11, 2023: contributor

reACK https://github.com/bitcoin/bitcoin/commit/635c50a57b3f72f83f1ee681f6a4b7766b8988f6

but appears to need rebase

DrahtBot removed review request from murchandamus on Apr 11, 2023

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

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

8741522e6c

wallet: add AddressPurpose enum to replace string values 2f80005136

wallet: Replace use of purpose strings with an enum

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
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

e83babe3b8

doc: Release note for purpose string restriction 18fc71a3ad

achow101 force-pushed on Apr 11, 2023

DrahtBot removed the label Needs rebase on Apr 11, 2023

josibake commented at 8:45 am on April 12, 2023: member

reACK https://github.com/bitcoin/bitcoin/commit/18fc71a3adee5de0f74ba6ff18f5ed31ba79a646

DrahtBot requested review from murchandamus on Apr 12, 2023

fanquake merged this on Apr 12, 2023

fanquake closed this on Apr 12, 2023

sidhujag referenced this in commit 130e81ac10 on Apr 12, 2023

in src/qt/walletmodel.cpp:390 in e83babe3b8 outdated

389     bool invoked = QMetaObject::invokeMethod(walletmodel, "updateAddressBook",
390                               Q_ARG(QString, strAddress),
391                               Q_ARG(QString, strLabel),
392                               Q_ARG(bool, isMine),
393-                              Q_ARG(QString, strPurpose),
394+                              Q_ARG(wallet::AddressPurpose, purpose),

denavila commented at 5:29 am on April 13, 2023:

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’

fanquake commented at 9:17 am on April 13, 2023:

Thanks, opened an issue here to follow up: https://github.com/bitcoin-core/gui/issues/725.

hebasto commented at 11:05 am on April 13, 2023:

Fixed in https://github.com/bitcoin-core/gui/pull/726.

hebasto referenced this in commit 19764dc143 on Apr 13, 2023

sidhujag referenced this in commit 58b94f3a10 on Apr 13, 2023

bitcoin locked this on Apr 12, 2024

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

Reviews

Conflicts