refactor: Remove CAddressBookData::destdata

ryanofsky commented at 9:51 pm on March 7, 2023: contributor

This is cleanup that doesn’t change external behavior. Benefits of the cleanup are:

Removes awkward StringMap intermediate representation for wallet address metadata.
Simplifies CWallet, deals with used address and received request serialization in walletdb.cpp instead of higher level wallet code
Adds test coverage and documentation

This PR doesn’t change externally observable behavior. Internally, the only change in behavior is that EraseDestData deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single lookup and scan instead of multiple lookups.

Motivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs

This PR is a rebased copy of #18608. For some reason that PR is locked and couldn’t be reopened or commented on.

This PR is an alternative to #27215 with differences described in #27215#pullrequestreview-1329028143

DrahtBot commented at 9:51 pm on March 7, 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	furszy, achow101
Stale ACK	josibake

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)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26836 (wallet: finish addressbook encapsulation by furszy)
#26644 (wallet: bugfix, ‘wallet_load_ckey’ unit test fails with bdb by furszy)
#25991 (Wallet: Add foreign_outputs metadata to support CoinJoin transactions by luke-jr)
#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 Refactoring on Mar 7, 2023

in src/wallet/walletdb.cpp:616 in 2a44c4e0c7 outdated

608@@ -609,7 +609,12 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
609             ssKey >> strAddress;
610             ssKey >> strKey;
611             ssValue >> strValue;
612-            pwallet->LoadDestData(DecodeDestination(strAddress), strKey, strValue);
613+            CAddressBookData& data = pwallet->m_address_book[DecodeDestination(strAddress)];
614+            if (strKey.compare("used") == 0) {
615+                data.SetUsed(true);
616+            } else if (strKey.compare(0, 2, "rr") == 0) {
617+                data.SetReceiveRequest(strKey.substr(2), std::move(strValue));

maflcko commented at 10:47 am on March 8, 2023:

0
1src/wallet/walletdb.cpp:616:58: error: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg,-warnings-as-errors]
2                data.SetReceiveRequest(strKey.substr(2), std::move(strValue));

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

re: #27224 (review)

Thanks, fixed now

ryanofsky force-pushed on Mar 8, 2023

ryanofsky commented at 8:23 pm on March 8, 2023: contributor

Updated 2a44c4e0c7a20a1e1964ecb25fe148759fbb0e2f -> 6388b30274971f0488214d283f28bfc948995a1b (pr/nodest.15 -> pr/nodest.16, compare) to fix clang-tidy std::move warning https://cirrus-ci.com/task/4799988845248512?logs=ci#L4742

in src/wallet/wallet.h:227 in 6388b30274 outdated

225         m_label = label;
226     }
227+    const std::map<std::string, std::string>& GetReceiveRequests() const { return m_receive_requests; }
228+    bool SetReceiveRequest(std::string id, std::string value)
229+    {
230+        if (value.empty()) return m_receive_requests.erase(id);

achow101 commented at 5:00 pm on March 15, 2023:

I strongly dislike using the empty string to indicate deletion. I think it would be better to have a separate RemoveReceiveRequest.

ryanofsky commented at 7:23 pm on March 15, 2023:

re: #27224 (review)

I strongly dislike using the empty string to indicate deletion. I think it would be better to have a separate RemoveReceiveRequest.

You are right. This is shitty and I will add a separate method to avoid continuing this pattern.

ryanofsky commented at 5:52 pm on March 20, 2023:

re: #27224 (comment)

This is done now

in src/wallet/wallet.h:210 in 6388b30274 outdated

204@@ -205,21 +205,29 @@ class CAddressBookData
205 {
206 private:
207     bool m_change{true};
208+    bool m_used{false};
209     std::string m_label;
210+    std::map<std::string, std::string> m_receive_requests;

achow101 commented at 5:03 pm on March 15, 2023:

I’m not convinced that this needs to be a map. Receive requests are only created by the GUI, and the GUI only creates them for a newly retrieved address, so it shouldn’t be possible to have multiple receive requests.

Additionally, receive requests are identified by an int which is being (un)stringified. This could just be an int to avoid that constant conversion in the GUI.

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

re: #27224 (review)

I’m not convinced that this needs to be a map. Receive requests are only created by the GUI, and the GUI only creates them for a newly retrieved address, so it shouldn’t be possible to have multiple receive requests.

Additionally, receive requests are identified by an int which is being (un)stringified. This could just be an int to avoid that constant conversion in the GUI.

You could definitely be right about these things, but I need to look into GUI code more to have an opinion. This PR avoids changing any GUI code, and keeps the behavior of interfaces::Wallet methods the same as before from the perspective of the GUI.

If the definitions or behavior of interfaces::Wallet methods should change as you say, I think it would make sense to implement these changes as a separate PR, because making those changes here would not really simplify much (would basically replace string with int and find() with has_value()), and would require understanding GUI code to review, unlike the current changes.

ryanofsky commented at 5:02 pm on March 20, 2023:

re: #27224 (comment)

Instead of extending the PR to modify GUI code, I’ve kept changes here internal to the wallet and added a note in setAddressReceiveRequest about the wallet/gui interface and say how it could be improved in the future.

ryanofsky force-pushed on Mar 20, 2023

ryanofsky commented at 5:56 pm on March 20, 2023: contributor

Updated 6388b30274971f0488214d283f28bfc948995a1b -> fe367f2e4e5fb95546fa26491d43346e0bc11adb (pr/nodest.16 -> pr/nodest.17, compare) adding suggested erase method, also adding more comments and removing some unnecessary changes to existing code after the rebase.

achow101 commented at 6:09 pm on March 23, 2023: member

ACK fe367f2e4e5fb95546fa26491d43346e0bc11adb

fanquake requested review from josibake on Mar 24, 2023

in src/wallet/bdb.cpp:817 in fe367f2e4e outdated

809@@ -808,6 +810,22 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
810     return ret == 0;
811 }
812 
813+bool BerkeleyBatch::ErasePrefix(Span<std::byte> prefix)
814+{
815+    if (!TxnBegin()) return false;
816+    auto cursor = std::make_unique<BerkeleyCursor>(m_database, this);
817+    Dbt prefix_key(prefix.data(), prefix.size()), prefix_value;

josibake commented at 11:48 am on March 27, 2023:

This line was really confusing for me at first. Perhaps something like this could make it more clear?

0    Dbt prefix_key(prefix.data(), prefix.size()), prefix_value{};

ryanofsky commented at 6:43 pm on March 27, 2023:

re: #27224 (review)

Thanks, switched to brace initialization like you suggested in many places now. Should be clearer and safer

josibake commented at 11:50 am on March 27, 2023: member

Concept ACK

I read through the motivating examples you listed and I think this change makes a lot of sense. Also great to see more test coverage!

Still reading and testing to make sure I fully understand the change but left a small suggestion in the meanwhile

ryanofsky force-pushed on Mar 27, 2023

ryanofsky commented at 7:02 pm on March 27, 2023: contributor

Updated fe367f2e4e5fb95546fa26491d43346e0bc11adb -> 0ad6a533b93162ec808de092cbe5edc31a27a390 (pr/nodest.17 -> pr/nodest.18, compare) using more brace initialization as suggested

in src/wallet/bdb.h:209 in 0ad6a533b9 outdated

205@@ -205,6 +206,7 @@ class BerkeleyBatch : public DatabaseBatch
206     bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override;
207     bool EraseKey(DataStream&& key) override;
208     bool HasKey(DataStream&& key) override;
209+    bool ErasePrefix(Span<std::byte> prefix) override;

josibake commented at 8:37 am on March 28, 2023:

Perhaps there’s some nuance that I’m missing, but it seems strange to have this be Span<std::byte> when the rest of the methods take DataStream&&. I changed the ErasePrefix method to instead take DataStream&& as an argument and was able to compile and pass the unit and functional tests. Seems like it would be preferable to match the other methods unless you have a reason to prefer Span<std::byte>?

ryanofsky commented at 7:53 pm on April 5, 2023:

re: https://github.com/bitcoin/bitcoin/pull/27224/files#r1150232826

you have a reason to prefer Span<std::byte>?

Well it should actually take a span of const bytes, so I fixed this to add const.

Passing a span makes more sense than passing a datastream because the function just needs a read-only array of bytes, it doesn’t need a read/write vector or a stream object or anything more complicated.

The other functions are using CDataStream just because they are called by older code which might be counting on them to call memory_cleanse, and would require extra work to convert to spans, though it was tried previously in #19320

josibake commented at 9:05 am on April 7, 2023:

gotcha, thanks for the explanation!

in src/wallet/walletdb.cpp:1124 in 0ad6a533b9 outdated

1109 
1110+bool WalletBatch::EraseAddressData(const CTxDestination& dest)
1111+{
1112+    DataStream prefix;
1113+    prefix << DBKeys::DESTDATA << EncodeDestination(dest);
1114+    return m_batch->ErasePrefix(prefix);

josibake commented at 8:41 am on March 28, 2023:

as part of changing ErasePrefix to accept a DataStream&&, I also changed this line:

0    return m_batch->ErasePrefix(DataStream(prefix));

to make this an rvalue. there is likely a cleaner way to do this, if you end up going with DataStream

ryanofsky commented at 6:38 pm on April 6, 2023:

re: #27224 (review)

to make this an rvalue. there is likely a cleaner way to do this, if you end up going with DataStream

Thanks, responded to earlier comment

josibake commented at 8:43 am on March 28, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27224/commits/0ad6a533b93162ec808de092cbe5edc31a27a390

I left a suggestion about using DataStream&& instead of Span<std::byte>, but not blocking

DrahtBot requested review from achow101 on Mar 28, 2023

in src/wallet/wallet.cpp:2843 in 0ad6a533b9 outdated

2865-            if (!data.first.compare(0, prefix.size(), prefix)) {
2866-                values.emplace_back(data.second);
2867-            }
2868+    for (const auto& dest : m_address_book) {
2869+        for (const auto& request : dest.second.receive_requests) {
2870+            values.emplace_back(request.second);

furszy commented at 12:19 pm on March 28, 2023:

nit: could use structured bindings.

0for (const auto& [dest, entry] : m_address_book) {
1     for (const auto& [id, request] : entry.receive_requests) {
2         values.emplace_back(request);
3     }
4}

ryanofsky commented at 6:35 pm on April 6, 2023:

re: #27224 (review)

nit: could use structured bindings.

Thanks, applied suggestion

in src/wallet/wallet.cpp:2441 in 0ad6a533b9 outdated

2438@@ -2439,11 +2439,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
2439             return false;
2440         }
2441         // Delete destdata tuples associated with address

furszy commented at 12:23 pm on March 28, 2023:

No longer the case. Same for the comment that is above this one.

ryanofsky commented at 6:56 pm on April 6, 2023:

No longer the case. Same for the comment that is above this one.

Thanks, I basically just replaced “destdata” with “address data” in these comments, but you can let me know if the issue was something else

in src/wallet/wallet.cpp:2818 in 0ad6a533b9 outdated

2816@@ -2821,65 +2817,46 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old
2817 
2818 bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)

furszy commented at 12:32 pm on March 28, 2023:

Not really for this PR, but.. the used arg is never false.

SetAddressUsed is only called from SetSpentKeyState which is only called from AddToWallet, which provides used=true.

Which.. means that the address is not reverted to a “not used” state if the tx gets abandoned/discarded.

Could also work on it on a follow-up. Probably after #26836 so changes don’t end up conflicting that much.

ryanofsky commented at 7:10 pm on April 6, 2023:

re: #27224 (review)

Sure, I think it would make sense to clear this flag when a transaction is abandoned. I think this could be a separate issue, and it should be pretty easy to implement anytime IMO

in src/wallet/wallet.cpp:2845 in 0ad6a533b9 outdated

2833-    return batch.WriteDestData(EncodeDestination(dest), key, value);
2834-}
2835-
2836-void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
2837-{
2838-    m_address_book[dest].destdata.insert(std::make_pair(key, value));

furszy commented at 12:35 pm on March 28, 2023:

This method was removed from the cpp but not from the header.

ryanofsky commented at 7:11 pm on April 6, 2023:

re: #27224 (review)

This method was removed from the cpp but not from the header.

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

in src/wallet/walletdb.cpp:616 in 0ad6a533b9 outdated

608@@ -609,7 +609,12 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue,
609             ssKey >> strAddress;
610             ssKey >> strKey;
611             ssValue >> strValue;
612-            pwallet->LoadDestData(DecodeDestination(strAddress), strKey, strValue);
613+            CAddressBookData& data{pwallet->m_address_book[DecodeDestination(strAddress)]};
614+            if (strKey.compare("used") == 0) {
615+                data.previously_spent = true;
616+            } else if (strKey.compare(0, 2, "rr") == 0) {
617+                data.receive_requests[strKey.substr(2)] = std::move(strValue);

furszy commented at 12:37 pm on March 28, 2023:

Would be good to keep the encapsulation and don’t access m_address_book from outside of the wallet class (otherwise we are reverting #25337 and, future, #26836).

ryanofsky commented at 6:37 pm on April 6, 2023:

re: #27224 (review)

Would be good to keep the encapsulation and don’t access m_address_book from outside of the wallet class (otherwise we are reverting #25337 and, future, #26836).

Thanks, applied your change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

in src/wallet/bdb.cpp:675 in 0ad6a533b9 outdated

663         throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
664     }
665-    int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
666+    // Transaction argument to cursor is only needed when using the cursor to
667+    // write to the database. Read-only cursors do not need a txn pointer.
668+    int ret = database.m_db->cursor(batch ? batch->txn() : nullptr, &m_cursor, 0);

furszy commented at 12:55 pm on March 28, 2023:

this also fixes abdef47f206dec114300b2d852f5c297b03179e3 (from #26644)

in src/wallet/test/wallet_tests.cpp:450 in 0ad6a533b9 outdated

457+    DatabaseStatus status;
458+    bilingual_str error;
459+    std::vector<bilingual_str> warnings;
460+    auto database{MakeWalletDatabase(name, options, status, error)};
461+    auto wallet{std::make_shared<CWallet>(chain.get(), "", std::move(database))};
462+    wallet->LoadWallet();

furszy commented at 12:59 pm on March 28, 2023:

Could

0BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::::LOAD_OK);

ryanofsky commented at 4:18 am on April 6, 2023:

re: #27224 (review)

0BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::::LOAD_OK);

Thanks, added this check

in src/wallet/test/wallet_tests.cpp:459 in 0ad6a533b9 outdated

466+BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
467+{
468+    for (DatabaseFormat format : DATABASE_FORMATS) {
469+        const std::string name{strprintf("receive-requests-%i", format)};
470+        TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
471+            BOOST_CHECK(!wallet->m_address_book[PKHash()].previously_spent);

furszy commented at 1:04 pm on March 28, 2023:

Same as above, would be good to not access m_address_book directly from outside of the wallet class. The map [] operator usage in a random place of the sources could easily screw things up.

Could change it to

0BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));

ryanofsky commented at 6:40 pm on April 6, 2023:

re: #27224 (review)

0BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

in src/wallet/test/wallet_tests.cpp:471 in 0ad6a533b9 outdated

478+            BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "1", "val_rr11"));
479+            BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, ScriptHash(), "2", "val_rr20"));
480+        });
481+        TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
482+            BOOST_CHECK(wallet->m_address_book[PKHash()].previously_spent);
483+            BOOST_CHECK(wallet->m_address_book[ScriptHash()].previously_spent);

furszy commented at 1:07 pm on March 28, 2023:

0BOOST_CHECK(wallet->IsAddressUsed(PKHash()));
1BOOST_CHECK(wallet->IsAddressUsed(ScriptHash()));

ryanofsky commented at 6:41 pm on April 6, 2023:

re: #27224 (review)

0BOOST_CHECK(wallet->IsAddressUsed(PKHash()));
1BOOST_CHECK(wallet->IsAddressUsed(ScriptHash()));

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

in src/wallet/test/wallet_tests.cpp:481 in 0ad6a533b9 outdated

488+            BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false));
489+            BOOST_CHECK(batch.EraseAddressData(ScriptHash()));
490+        });
491+        TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
492+            BOOST_CHECK(!wallet->m_address_book[PKHash()].previously_spent);
493+            BOOST_CHECK(!wallet->m_address_book[ScriptHash()].previously_spent);

furszy commented at 1:09 pm on March 28, 2023:

0BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
1BOOST_CHECK(!wallet->IsAddressUsed(ScriptHash()));

ryanofsky commented at 6:41 pm on April 6, 2023:

re: #27224 (review)

0BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
1BOOST_CHECK(!wallet->IsAddressUsed(ScriptHash()));

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

furszy commented at 1:13 pm on March 28, 2023: member

Code review 0ad6a533, left few points.

ryanofsky commented at 2:03 pm on March 28, 2023: contributor

@furszy, thanks for the thorough review. I will start implementing your suggestions, but do you think you could help me with your comments about encapsulation like #27224 (review)?

The comments are just a little vague from my perspective so it’d be great if you could provide specific code suggestions and I will apply or implement them. For background, I’ve been a little detached from the address encapsulation attempt, and personally like m_address_book.find(dest) / m_addressbook[dest] lookups and for loops a little better than calling FindAddress/FindOrInsertAddress/ForAddresses wrapper functions. But I don’t have a strong opinion and am happy to use wrappers, I just think you would be better at suggesting with names / parameters since you have been designing the API and know more about the goals.

furszy commented at 5:17 pm on March 28, 2023: member

@furszy, thanks for the thorough review. I will start implementing your suggestions, but do you think you could help me with your comments about encapsulation like #27224 (comment)?

For background, I’ve been a little detached from the address encapsulation attempt, and personally like m_address_book.find(dest) / m_addressbook[dest] lookups and for loops a little better than calling FindAddress/FindOrInsertAddress/ForAddresses wrapper functions. But I don’t have a strong opinion and am happy to use wrappers, I just think you would be better at suggesting with names / parameters since you have been designing the API and know more about the goals.

Yeah ok.

The main purpose of address book encapsulation is prevent people from accidentally modifying the ‘purpose’ field in an RPC command or the GUI, or from creating a new entry by misusing the map’s [] operator. Which it could lead, in the worst-case scenario, to a “send” address being relabeled as a “receive” address, resulting in a user sending funds to an address they do not own while thinking they do.

I know that it didn’t happen so far but.. it’s a big project and I think that we shouldn’t assume that everyone touching the GUI and/or the RPC commands are aware of the internal address book usage (e.g. #25979). Simpler to just forget about this possible issue by letting people access entries through an interface.

Also, the encapsulation will let us use another mutex for the address book, so commands like setlabel, getaddressbylabel etc will be able to run cs_wallet free (not such a big feature but it adds value).

The comments are just a little vague from my perspective so it’d be great if you could provide specific code suggestions and I will apply or implement them.

Sadly, the cleanest implementation would come after #24914, when we load the wallet in-order, first read the address book entry, then “dest data” etc. We are currently forced to append new address book entries while we loop over the “used” and “requests” db records because the entry could not exist yet.. which is ugly and adds more boilerplate code.

About the API naming, we still have a mixture of names there. But we could continue moving toward the usage of LoadXXX prefix for methods that are only relevant for the loading process. We are already using this term for the spkms and other members. Thoughts?.

Then, in the future, to not allow calling this LoadXXX methods from RPC related code, same as we do in the GUI, would be nice to start using the wallet interface there instead of directly access the CWallet object (not sure if there are plans for this already).

But anyway, future stuff. Here is the small commit for this https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59. Check if you like it. It’s not a big deal.

Another idea: Once we make the address book member private, we could allow direct access only from LoadWallet by making a new “loading” class that has CWallet as friend. This would remove the LoadXXX encapsulation boilerplate if that is what makes you doubt about it.

ryanofsky force-pushed on Apr 6, 2023

ryanofsky commented at 8:07 pm on April 6, 2023: contributor

Updated 0ad6a533b93162ec808de092cbe5edc31a27a390 -> d34323544d7283bf46bb154a90b8feca443b103e (pr/nodest.18 -> pr/nodest.19, compare) applying most of the suggested changes. Thanks for the reviews!

wallet: Add DatabaseBatch::ErasePrefix method

This new function is not used yet this commit, but next commit adds usages and
test coverage for both BDB and sqlite.

5938ad0bdb

refactor: Remove CAddressBookData::destdata

This is cleanup that doesn't change external behavior.

- Removes awkward `StringMap` intermediate representation
- Simplifies CWallet code, deals with used address and received request
  serialization in walletdb.cpp
- Adds test coverage and documentation
- Reduces memory usage

This PR doesn't change externally observable behavior. Internally, only change
in behavior is that EraseDestData deletes directly from database because the
`StringMap` is gone. This is more direct and efficient because it uses a single
btree lookup and scan instead of multiple lookups

Motivation for this cleanup is making changes like #18550, #18192, #13756
easier to reason about and less likely to result in unintended behavior and
bugs

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

a5986e82dd

DrahtBot added the label Needs rebase on Apr 12, 2023

ryanofsky force-pushed on Apr 12, 2023

ryanofsky commented at 3:48 pm on April 12, 2023: contributor

Rebased d34323544d7283bf46bb154a90b8feca443b103e -> a5986e82dd2b8f923d60f9e31760ef62b9010105 (pr/nodest.19 -> pr/nodest.20, compare) due to conflict with #27217

DrahtBot removed the label Needs rebase on Apr 12, 2023

in src/wallet/wallet.cpp:2874 in a5986e82dd

2904+}
2905+
2906+bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id)
2907+{
2908+    if (!batch.EraseAddressReceiveRequest(dest, id)) return false;
2909+    m_address_book[dest].receive_requests.erase(id);

furszy commented at 8:19 pm on April 13, 2023:

Would be good to check that the map contains the element prior accessing it. Otherwise the GUI could be adding a new address book entry by mistake.

furszy commented at 8:22 pm on April 13, 2023: member

Code ACK a5986e82

DrahtBot requested review from josibake on Apr 13, 2023

achow101 commented at 12:09 pm on May 1, 2023: member

ACK a5986e82dd2b8f923d60f9e31760ef62b9010105

DrahtBot removed review request from achow101 on May 1, 2023

achow101 merged this on May 1, 2023

achow101 closed this on May 1, 2023

in src/wallet/wallet.h:237 in a5986e82dd

240+    bool previously_spent{false};
241+
242+    /**
243+     * Map containing data about previously generated receive requests
244+     * requesting funds to be sent to this address. Only present for IsMine
245+     * addresses. Map keys are decimal numbers uniquely identifying each

pinheadmz commented at 3:29 pm on May 1, 2023:

I was momentarily confused by this because (IIUC) map keys are the int64_t id field of RecentRequestEntry but actually encoded into std::string by a few methods in RecentRequestsTableModel. So I’m wondering: Is there a reason we need to keep those IDs as strings? Is it like, at this point, going back to int64 would be a needless refactor?

ryanofsky commented at 4:11 pm on May 1, 2023:

re: #27224 (review)

So I’m wondering: Is there a reason we need to keep those IDs as strings? Is it like, at this point, going back to int64 would be a needless refactor?

Only reason that the CAddressBookData::receive_requests map containing the requests has string keys and string values is that the setAddressReceiveRequest API adding the requests takes string keys and string values.

I think it is best for the map types to match the API types, so wallet code can store the values it is passed without parsing them. But definitely the setAddressReceiveRequest API could be improved, and there was discussion about this in #27224 (review). Not only could the string keys be replaced by integer keys, but it might be possible to drop the keys and the map entirely if the GUI doesn’t need to store multiple requests for a single address. I’m not sure how valuable this refactoring would be, but it should be easier to do now that receive requests are stored separately from other address data.

EDIT: I was curious what replacing strings with ints would look like so I implemented it in commit 2877c80afd40313760dbe11a86e89e87dbc7f2b8 (branch). I think the cleanup probably isn’t that valuable standalone, but it might be useful as part of a larger change.

ryanofsky referenced this in commit b24d751a23 on May 1, 2023

sidhujag referenced this in commit 959e5cd153 on May 1, 2023

ryanofsky referenced this in commit 2877c80afd on May 1, 2023

bitcoin locked this on Apr 30, 2024

refactor: Remove CAddressBookData::destdata #27224

Reviews

Conflicts