refactor: Remove CAddressBookData::destdata #27224

ryanofsky · 2023-03-07T21:51:45Z

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 (review)

DrahtBot · 2023-03-07T21:51:47Z

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.

maflcko · 2023-03-08T10:47:12Z

src/wallet/walletdb.cpp

+            if (strKey.compare("used") == 0) {
+                data.SetUsed(true);
+            } else if (strKey.compare(0, 2, "rr") == 0) {
+                data.SetReceiveRequest(strKey.substr(2), std::move(strValue));


src/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] data.SetReceiveRequest(strKey.substr(2), std::move(strValue));

re: #27224 (comment)

Thanks, fixed now

ryanofsky

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

ryanofsky · 2023-03-08T19:49:38Z

src/wallet/walletdb.cpp

+            if (strKey.compare("used") == 0) {
+                data.SetUsed(true);
+            } else if (strKey.compare(0, 2, "rr") == 0) {
+                data.SetReceiveRequest(strKey.substr(2), std::move(strValue));


re: #27224 (comment)

Thanks, fixed now

src/wallet/wallet.h

ryanofsky

Updated 6388b30 -> fe367f2 (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.

src/wallet/wallet.h

achow101 · 2023-03-23T18:09:22Z

ACK fe367f2

josibake

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

src/wallet/bdb.cpp

ryanofsky

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

src/wallet/bdb.cpp

josibake

ACK 0ad6a53

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

josibake · 2023-03-28T08:37:40Z

src/wallet/bdb.h

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


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>?

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

gotcha, thanks for the explanation!

src/wallet/walletdb.cpp

furszy

Code review 0ad6a53, left few points.

src/wallet/wallet.cpp

furszy · 2023-03-28T12:23:29Z

src/wallet/wallet.cpp

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


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

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

furszy · 2023-03-28T12:32:56Z

src/wallet/wallet.cpp

@@ -2821,65 +2817,46 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old

 bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)


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.

re: #27224 (comment)

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

furszy · 2023-03-28T12:35:30Z

src/wallet/wallet.cpp

-void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
-{
-    m_address_book[dest].destdata.insert(std::make_pair(key, value));


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

re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

furszy · 2023-03-28T12:37:28Z

src/wallet/walletdb.cpp

+            CAddressBookData& data{pwallet->m_address_book[DecodeDestination(strAddress)]};
+            if (strKey.compare("used") == 0) {
+                data.previously_spent = true;
+            } else if (strKey.compare(0, 2, "rr") == 0) {
+                data.receive_requests[strKey.substr(2)] = std::move(strValue);


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).

re: #27224 (comment)

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 (furszy@2195b1e)

furszy · 2023-03-28T12:55:35Z

src/wallet/bdb.cpp

+    // Transaction argument to cursor is only needed when using the cursor to
+    // write to the database. Read-only cursors do not need a txn pointer.
+    int ret = database.m_db->cursor(batch ? batch->txn() : nullptr, &m_cursor, 0);


this also fixes abdef47 (from #26644)

furszy · 2023-03-28T12:59:21Z

src/wallet/test/wallet_tests.cpp

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


Could

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

re: #27224 (comment)

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

Thanks, added this check

furszy · 2023-03-28T13:04:35Z

src/wallet/test/wallet_tests.cpp

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


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

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

re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

furszy · 2023-03-28T13:07:18Z

src/wallet/test/wallet_tests.cpp

+            BOOST_CHECK(wallet->m_address_book[PKHash()].previously_spent);
+            BOOST_CHECK(wallet->m_address_book[ScriptHash()].previously_spent);


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

re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

furszy · 2023-03-28T13:09:39Z

src/wallet/test/wallet_tests.cpp

+            BOOST_CHECK(!wallet->m_address_book[PKHash()].previously_spent);
+            BOOST_CHECK(!wallet->m_address_book[ScriptHash()].previously_spent);


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

re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

ryanofsky · 2023-03-28T14:03:39Z

@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)?

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 · 2023-03-28T17:17:46Z

@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 furszy@2195b1e. 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

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

ryanofsky · 2023-04-05T19:53:18Z

src/wallet/bdb.h

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


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

ryanofsky · 2023-04-06T04:18:06Z

src/wallet/test/wallet_tests.cpp

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


re: #27224 (comment)

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

Thanks, added this check

src/wallet/wallet.cpp

ryanofsky · 2023-04-06T18:37:48Z

src/wallet/walletdb.cpp

+            CAddressBookData& data{pwallet->m_address_book[DecodeDestination(strAddress)]};
+            if (strKey.compare("used") == 0) {
+                data.previously_spent = true;
+            } else if (strKey.compare(0, 2, "rr") == 0) {
+                data.receive_requests[strKey.substr(2)] = std::move(strValue);


re: #27224 (comment)

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 (furszy@2195b1e)

src/wallet/walletdb.cpp

ryanofsky · 2023-04-06T18:41:32Z

src/wallet/test/wallet_tests.cpp

+            BOOST_CHECK(wallet->m_address_book[PKHash()].previously_spent);
+            BOOST_CHECK(wallet->m_address_book[ScriptHash()].previously_spent);


re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

ryanofsky · 2023-04-06T18:41:49Z

src/wallet/test/wallet_tests.cpp

+            BOOST_CHECK(!wallet->m_address_book[PKHash()].previously_spent);
+            BOOST_CHECK(!wallet->m_address_book[ScriptHash()].previously_spent);


re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

ryanofsky · 2023-04-06T18:56:38Z

src/wallet/wallet.cpp

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


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

ryanofsky · 2023-04-06T19:10:12Z

src/wallet/wallet.cpp

@@ -2821,65 +2817,46 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old

 bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)


re: #27224 (comment)

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

ryanofsky · 2023-04-06T19:11:23Z

src/wallet/wallet.cpp

-void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
-{
-    m_address_book[dest].destdata.insert(std::make_pair(key, value));


re: #27224 (comment)

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

Thanks applied your suggested change (furszy@2195b1e)

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

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 bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

ryanofsky · 2023-04-12T15:48:25Z

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

furszy

Code ACK a5986e8

furszy · 2023-04-13T20:19:46Z

src/wallet/wallet.cpp

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


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.

achow101 · 2023-05-01T12:09:29Z

ACK a5986e8

pinheadmz · 2023-05-01T15:29:22Z

src/wallet/wallet.h

+    /**
+     * Map containing data about previously generated receive requests
+     * requesting funds to be sent to this address. Only present for IsMine
+     * addresses. Map keys are decimal numbers uniquely identifying each


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?

re: #27224 (comment)

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 (comment). 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 2877c80 (branch). I think the cleanup probably isn't that valuable standalone, but it might be useful as part of a larger change.

…itcoin#27224 (comment)

a5986e8 refactor: Remove CAddressBookData::destdata (Ryan Ofsky) 5938ad0 wallet: Add DatabaseBatch::ErasePrefix method (Ryan Ofsky) Pull request description: 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 bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs --- This PR is a rebased copy of bitcoin#18608. For some reason that PR is locked and couldn't be reopened or commented on. This PR is an alternative to bitcoin#27215 with differences described in bitcoin#27215 (review) ACKs for top commit: achow101: ACK a5986e8 furszy: Code ACK a5986e8 Tree-SHA512: 6bd3e402f1f60263fbd433760bdc29d04175ddaf8307207c4a67d59f6cffa732e176ba57886e02926f7a1615dce0ed9cda36c8cbc6430aa8e5b56934c23f3fe7

…itcoin#27224 (comment)

DrahtBot added the Refactoring label Mar 7, 2023

This was referenced Mar 8, 2023

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

Merged

wallet: Turn destdata entries into CAddressBookData fields #27215

Closed

wallet: batch and simplify addressbook migration process #26836

Merged

maflcko reviewed Mar 8, 2023

View reviewed changes

ryanofsky force-pushed the pr/nodest branch from 2a44c4e to 6388b30 Compare March 8, 2023 19:53

ryanofsky commented Mar 8, 2023

View reviewed changes

This was referenced Mar 9, 2023

Permission to comment on closed PRs #27234

Closed

refactor: Remove CAddressBookData::destdata #18608

Closed

achow101 reviewed Mar 15, 2023

View reviewed changes

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

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

ryanofsky force-pushed the pr/nodest branch from 6388b30 to fe367f2 Compare March 20, 2023 17:55

ryanofsky commented Mar 20, 2023

View reviewed changes

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

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

DrahtBot mentioned this pull request Mar 21, 2023

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

Merged

fanquake requested a review from josibake March 24, 2023 10:46

DrahtBot mentioned this pull request Mar 24, 2023

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

Closed

josibake reviewed Mar 27, 2023

View reviewed changes

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

ryanofsky force-pushed the pr/nodest branch from fe367f2 to 0ad6a53 Compare March 27, 2023 19:01

ryanofsky commented Mar 27, 2023

View reviewed changes

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

josibake reviewed Mar 28, 2023

View reviewed changes

DrahtBot requested a review from achow101 March 28, 2023 08:43

furszy reviewed Mar 28, 2023

View reviewed changes

ryanofsky force-pushed the pr/nodest branch from 0ad6a53 to d343235 Compare April 6, 2023 20:06

ryanofsky commented Apr 6, 2023

View reviewed changes

ryanofsky and others added 2 commits April 12, 2023 05:30

wallet: Add DatabaseBatch::ErasePrefix method

5938ad0

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

DrahtBot added the Needs rebase label Apr 12, 2023

ryanofsky force-pushed the pr/nodest branch from d343235 to a5986e8 Compare April 12, 2023 15:42

DrahtBot removed the Needs rebase label Apr 12, 2023

furszy reviewed Apr 13, 2023

View reviewed changes

DrahtBot requested a review from josibake April 13, 2023 20:22

DrahtBot removed the request for review from achow101 May 1, 2023 12:09

achow101 merged commit 5325a61 into bitcoin:master May 1, 2023

pinheadmz reviewed May 1, 2023

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2023

Store wallet request ids as integers instead of strings as suggested b…

b24d751

…itcoin#27224 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2023

Store wallet request ids as integers instead of strings as suggested b…

2877c80

…itcoin#27224 (comment)

bitcoin locked and limited conversation to collaborators Apr 30, 2024

		@@ -2821,65 +2817,46 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old

		bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)

		BOOST_CHECK(wallet->m_address_book[PKHash()].previously_spent);
		BOOST_CHECK(wallet->m_address_book[ScriptHash()].previously_spent);

		BOOST_CHECK(!wallet->m_address_book[PKHash()].previously_spent);
		BOOST_CHECK(!wallet->m_address_book[ScriptHash()].previously_spent);

refactor: Remove CAddressBookData::destdata #27224

refactor: Remove CAddressBookData::destdata #27224

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Mar 23, 2023

Uh oh!

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

Uh oh!

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

josibake Mar 28, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Mar 28, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

ryanofsky commented Mar 7, 2023 •

edited

Loading

DrahtBot commented Mar 7, 2023 •

edited

Loading

josibake left a comment •

edited

Loading

josibake Mar 28, 2023 •

edited

Loading

furszy Mar 28, 2023 •

edited

Loading

furszy Mar 28, 2023 •

edited

Loading

furszy commented Mar 28, 2023 •

edited

Loading