wallet: batch erase procedures and improve 'EraseRecords' performance #29403

furszy · 2024-02-07T13:42:56Z

Seeks to optimize and simplify WalletBatch::EraseRecords. Currently, this process opens a cursor to iterate over the entire database, searching for records that match the type prefixes, to then call the WalletBatch::Erase function for each of the matching records.
This PR rewrites this 40-line manual process into a single line; instead of performing all of those actions manually, we can simply utilize the ErasePrefix() functionality. The result is 06216b3.

Moreover, it expands the test coverage for the ErasePrefix functionality and documents the db txn requirement for BerkeleyBatch::ErasePrefix .

DrahtBot · 2024-02-07T13:42:59Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, josibake
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

src/wallet/wallet.cpp

furszy · 2024-02-07T14:16:21Z

Updated per feedback. Thanks Marko.

DrahtBot · 2024-02-07T14:18:36Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21321086502}

furszy · 2024-02-07T15:21:23Z

Green CI. Ready.

achow101

ACK 69c5bb5

src/wallet/wallet.cpp

src/wallet/bdb.cpp

furszy

Updated per feedback. Thanks achow!

src/wallet/wallet.cpp

src/wallet/bdb.cpp

furszy

Updated per feedback. Thanks achow.
Small diff, only changed assert for Assume.

furszy

Rebased after #26836 merge. Ready to go.

achow101 · 2024-02-08T19:45:10Z

src/wallet/test/db_tests.cpp

+        BOOST_CHECK(batch->TxnBegin());
+        BOOST_CHECK(batch->ErasePrefix(DataStream() << key));
+        BOOST_CHECK(batch->TxnCommit());


In 5235257 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

nit: Could use RunWithinTxn().

In 5235257 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

nit: Could use RunWithinTxn().

I did not used it because it needs walletdb.h dependency (RunWithinTxn provides access to WalletBatch). And this tests (db_tests.cpp) run on a lower level, using DatabaseBatch.

src/wallet/test/wallet_tests.cpp

achow101 · 2024-02-08T19:45:53Z

src/wallet/bdb.cpp

+    // Because this function erases records one by one, ensure that it is executed within a txn context.
+    // Otherwise, consistency is at risk; it's possible that certain records are removed while others
+    // remain due to an internal failure during the procedure.


In 5235257 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

I think it would be relevant to mention that BDB returns errors on del() if it is not in a transaction.

I think it would be relevant to mention that BDB returns errors on del() if it is not in a transaction.

Sure, we could mention that the read-write operation is currently disallowed by our current bdb configurations.

Still, JFYI, I fixed it locally by subdividing the read-write operations. However, since we currently ensure that all callers start a transaction, and this division adds an extra loop over the matching entries (see furszy@4e7b5c0), I didn't see it as worth pushing it.

re: #29403 (comment)

In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" (96eb527)

The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review thread. Would suggest just using achow's comment or combining both comments with something like "The Dbc::del() cursor delete call below would fail without an active transaction.".

re: #29403 (comment)

In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" (96eb527)

The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review thread. Would suggest just using achow's comment or combining both comments with something like "The Dbc::del() cursor delete call below would fail without an active transaction.".

I wrote it in this way to (try to) inform readers about current bdb config limitations in case anyone changes this code and doesn't understand why it fails. But, I agree that it is not immediately clear. Will change it for you line.
Hopefully no one else will change this again.

achow101 · 2024-02-08T19:46:53Z

src/wallet/walletdb.cpp

@@ -1337,6 +1336,12 @@ bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, con
    return true;
 }

+bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)


In 757d651 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This overload feels unnecessary given that there are only 2 places this function is really used.

In 757d651 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This overload feels unnecessary given that there are only 2 places this function is really used.

The idea is to use this function as a top-level wrapper for wallet functions and scope the other function for internal walletdb.cpp usage mostly/only. This approach avoids creating WalletBatch objects across the sources and makes use of the error-handling code in the WalletBatch destructor.
Functions such as DescriptorScriptPubKeyMan::TopUp, the two CWallet::SetupDescriptorScriptPubKeyMans, and the final state of the batched migration process can utilize this overload in a follow-up.

achow101 · 2024-02-08T19:47:43Z

src/wallet/walletdb.cpp

@@ -1314,9 +1314,8 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
    return DBErrors::LOAD_OK;
 }

-bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
+static bool RunWithinTxn(WalletBatch& batch, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)


In 757d651 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This could be done in the commit introducing RunWithinTxn() rather than changing it in a later commit.

This could be done in the commit introducing RunWithinTxn() rather than changing it in a later commit.

This is merely an organizational discussion, but.. I'm not a big fan of introducing a function overload in a commit that does not utilizes it. Yet, if you have a strong opinion about it, I could move this to the first commit. np.

ryanofsky

Light code review ACK 757d651, nice simplification of EraseRecords

src/wallet/walletdb.cpp

furszy

Updated per feedback. Thanks both!

furszy · 2024-02-08T20:55:08Z

src/wallet/bdb.cpp

+    // Because this function erases records one by one, ensure that it is executed within a txn context.
+    // Otherwise, consistency is at risk; it's possible that certain records are removed while others
+    // remain due to an internal failure during the procedure.


I think it would be relevant to mention that BDB returns errors on del() if it is not in a transaction.

Sure, we could mention that the read-write operation is currently disallowed by our current bdb configurations.

Still, JFYI, I fixed it locally by subdividing the read-write operations. However, since we currently ensure that all callers start a transaction, and this division adds an extra loop over the matching entries (see furszy@4e7b5c0), I didn't see it as worth pushing it.

furszy · 2024-02-08T21:04:18Z

src/wallet/test/db_tests.cpp

+        BOOST_CHECK(batch->TxnBegin());
+        BOOST_CHECK(batch->ErasePrefix(DataStream() << key));
+        BOOST_CHECK(batch->TxnCommit());


In 5235257 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

nit: Could use RunWithinTxn().

I did not used it because it needs walletdb.h dependency (RunWithinTxn provides access to WalletBatch). And this tests (db_tests.cpp) run on a lower level, using DatabaseBatch.

furszy · 2024-02-08T21:35:49Z

src/wallet/walletdb.cpp

@@ -1337,6 +1336,12 @@ bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, con
    return true;
 }

+bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)


In 757d651 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This overload feels unnecessary given that there are only 2 places this function is really used.

The idea is to use this function as a top-level wrapper for wallet functions and scope the other function for internal walletdb.cpp usage mostly/only. This approach avoids creating WalletBatch objects across the sources and makes use of the error-handling code in the WalletBatch destructor.
Functions such as DescriptorScriptPubKeyMan::TopUp, the two CWallet::SetupDescriptorScriptPubKeyMans, and the final state of the batched migration process can utilize this overload in a follow-up.

furszy · 2024-02-08T21:37:16Z

src/wallet/walletdb.cpp

@@ -1314,9 +1314,8 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
    return DBErrors::LOAD_OK;
 }

-bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
+static bool RunWithinTxn(WalletBatch& batch, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)


This could be done in the commit introducing RunWithinTxn() rather than changing it in a later commit.

This is merely an organizational discussion, but.. I'm not a big fan of introducing a function overload in a commit that does not utilizes it. Yet, if you have a strong opinion about it, I could move this to the first commit. np.

achow101 · 2024-02-09T18:41:50Z

ACK 8330145

ryanofsky

Code review ACK 8330145. Looks pretty good, but would suggest cleaning up some things

ryanofsky · 2024-02-09T19:10:32Z

src/wallet/walletdb.cpp

+
+    // Run procedure, errors are logged internally
+    if (!func(batch)) {
+        batch.TxnAbort();


In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (b6dcfca)

I think it's bad for debugging that this function inconsistently logs errors when an operation fails. For example it seems like there is no error if the new eraserecords function fails? It would be better to add LogPrint(BCLog::WALLETDB, "Error: %s failed\n", process_desc); here.

I understand that in the future there may be cases where we may want to abort without logging errors but I think a better way to handle those cases would be to accept kernel::Interrupted return values here and use the kernel::IsInterrupted() helper. (We can move these from kernel to util without breaking compatibility.)

The comment above on line 1325 "errors are logged internally" is also not accurate and impossible to enforce with an anonymous callback, so would suggest dropping that and logging unconditionally here. (Without logging, the most the comment could say would be "errors need to be logged by func callback" or "errors should be logged by func callback")

Yeah, using util::Interrupted to skip the failure log in the future sounds great. Error logging added.

src/wallet/walletdb.h

ryanofsky · 2024-02-09T19:31:25Z

src/wallet/bdb.cpp

+    // Because this function erases records one by one, ensure that it is executed within a txn context.
+    // Otherwise, consistency is at risk; it's possible that certain records are removed while others
+    // remain due to an internal failure during the procedure.


re: #29403 (comment)

In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" (96eb527)

The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review thread. Would suggest just using achow's comment or combining both comments with something like "The Dbc::del() cursor delete call below would fail without an active transaction.".

furszy

Updated per feedback. Thanks ryanofsky!. Small diff.
Per request, now RunWithinTxn logs an error when the provided function requires to abort the db txn. As well as two comments have been improved for clarity.

'RunWithinTxn()' provides a way to execute db operations within a transactional context. It avoids writing repetitive boilerplate code for starting and committing the database transaction.

Transactions are intended to be started on upper layers rather than internally by the bdb batch object. This enables us to consolidate different write operations within a procedure in the same db txn, improving consistency due to the atomic property of the transaction, as well as its performance due to the reduction of disk write operations. Important Note: This approach also ensures that the BerkeleyBatch::ErasePrefix function behaves exactly as the SQLiteBatch::ErasePrefix function, which does not create a db txn internally. Furthermore, since the `BerkeleyBatch::ErasePrefix' implementation erases records one by one (by traversing the db), this change ensures that the function is always called within an active txn context. Without this measure, there's a potential risk to consistency; certain records may be removed while others could persist due to an internal failure during the procedure.

achow101 · 2024-02-12T23:26:18Z

reACK 77331aa

josibake

code review ACK 77331aa

If you end up retouching, I think it's better to keep all of the RunWithinTxn changes in the first commit. Makes it easier to review and avoids introducing the function with one signature and then changing the signature in a later commit.

josibake · 2024-02-13T10:17:34Z

src/wallet/walletdb.cpp

+static bool RunWithinTxn(WalletBatch& batch, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
 {
-    WalletBatch batch(database);


in "wallet: simplify EraseRecords by using 'ErasePrefix'" (77331aa):

nit: these changes should be in the first commit

src/wallet/walletdb.cpp

DrahtBot added the Wallet label Feb 7, 2024

maflcko reviewed Feb 7, 2024

View reviewed changes

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

furszy mentioned this pull request Feb 7, 2024

wallet: batch and simplify addressbook migration process #26836

Merged

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from da103ec to b7b5e5b Compare February 7, 2024 14:15

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from b7b5e5b to 69c5bb5 Compare February 7, 2024 14:18

DrahtBot added the CI failed label Feb 7, 2024

DrahtBot removed the CI failed label Feb 7, 2024

achow101 reviewed Feb 7, 2024

View reviewed changes

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

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

achow101 mentioned this pull request Feb 7, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Open

40 tasks

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from 69c5bb5 to 0fd7b0d Compare February 7, 2024 22:37

furszy commented Feb 7, 2024

View reviewed changes

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

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

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from 0fd7b0d to 784034f Compare February 8, 2024 14:11

furszy commented Feb 8, 2024

View reviewed changes

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch 2 times, most recently from d6bf468 to 757d651 Compare February 8, 2024 16:16

furszy commented Feb 8, 2024

View reviewed changes

achow101 reviewed Feb 8, 2024

View reviewed changes

ryanofsky approved these changes Feb 8, 2024

View reviewed changes

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

DrahtBot requested a review from achow101 February 8, 2024 21:05

DrahtBot mentioned this pull request Feb 9, 2024

Remove the legacy wallet and BDB dependency #28710

Draft

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from 757d651 to 8330145 Compare February 9, 2024 12:11

furszy commented Feb 9, 2024

View reviewed changes

DrahtBot requested review from ryanofsky and removed request for achow101 February 9, 2024 18:41

ryanofsky approved these changes Feb 9, 2024

View reviewed changes

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from 8330145 to 2095ccd Compare February 9, 2024 20:21

furszy commented Feb 9, 2024

View reviewed changes

DrahtBot mentioned this pull request Feb 10, 2024

wallet: simplify and batch zap wallet txes process #28987

Merged

DrahtBot added the Needs rebase label Feb 12, 2024

furszy added 3 commits February 12, 2024 16:05

wallet: db, introduce 'RunWithinTxn()' helper function

cf4d72a

'RunWithinTxn()' provides a way to execute db operations within a transactional context. It avoids writing repetitive boilerplate code for starting and committing the database transaction.

wallet: simplify EraseRecords by using 'ErasePrefix'

77331aa

furszy force-pushed the 2024_wallet_simplify_EraseRecords branch from 2095ccd to 77331aa Compare February 12, 2024 19:08

DrahtBot removed the Needs rebase label Feb 12, 2024

furszy mentioned this pull request Feb 12, 2024

rpc: Drop migratewallet experimental warning #28037

Merged

DrahtBot requested a review from ryanofsky February 12, 2024 23:26

josibake reviewed Feb 13, 2024

View reviewed changes

achow101 merged commit 128b4a8 into bitcoin:master Feb 13, 2024
16 checks passed

furszy deleted the 2024_wallet_simplify_EraseRecords branch February 13, 2024 18:56

furszy mentioned this pull request Feb 13, 2024

wallet: optimize migration process, batch db transactions #28574

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

furszy commented Feb 7, 2024 •

edited

DrahtBot commented Feb 7, 2024 •

edited

furszy commented Feb 7, 2024

DrahtBot commented Feb 7, 2024

furszy commented Feb 7, 2024 •

edited

achow101 left a comment

furszy left a comment

furszy left a comment

furszy left a comment

achow101 Feb 8, 2024

furszy Feb 8, 2024

achow101 Feb 8, 2024

furszy Feb 8, 2024

ryanofsky Feb 9, 2024

furszy Feb 9, 2024

achow101 Feb 8, 2024

furszy Feb 8, 2024

achow101 Feb 8, 2024

furszy Feb 8, 2024

ryanofsky left a comment

furszy left a comment

furszy Feb 8, 2024

furszy Feb 8, 2024

furszy Feb 8, 2024

furszy Feb 8, 2024

achow101 commented Feb 9, 2024

ryanofsky left a comment

ryanofsky Feb 9, 2024 •

edited

furszy Feb 9, 2024 •

edited

ryanofsky Feb 9, 2024

furszy left a comment

achow101 commented Feb 12, 2024

josibake left a comment

josibake Feb 13, 2024

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

Conversation

furszy commented Feb 7, 2024 • edited

DrahtBot commented Feb 7, 2024 • edited

Code Coverage

Reviews

Conflicts

furszy commented Feb 7, 2024

DrahtBot commented Feb 7, 2024

furszy commented Feb 7, 2024 • edited

achow101 left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Feb 9, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 9, 2024 • edited

Choose a reason for hiding this comment

furszy Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

achow101 commented Feb 12, 2024

josibake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy commented Feb 7, 2024 •

edited

DrahtBot commented Feb 7, 2024 •

edited

furszy commented Feb 7, 2024 •

edited

ryanofsky Feb 9, 2024 •

edited

furszy Feb 9, 2024 •

edited