wallet: batch erase procedures and improve ‘EraseRecords’ performance #29403

pull furszy wants to merge 3 commits into bitcoin:master from furszy:2024_wallet_simplify_EraseRecords changing 6 files +96 −48
  1. furszy commented at 1:42 pm on February 7, 2024: member

    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 06216b344dea6ad6c385fda0b37808ff9ae5273b.

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

  2. DrahtBot commented at 1:42 pm on February 7, 2024: contributor

    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.

  3. DrahtBot added the label Wallet on Feb 7, 2024
  4. in src/wallet/wallet.cpp:2413 in da103ecf4a outdated 
    2409+            return false;
2410+        }
2411+
2412+        // Delete purpose and name
2413+        if (!batch.ErasePurpose(dest) || !batch.EraseName(dest)) {
2414+            WalletLogPrintf("%s error erasing purpose/name\n", __func__);
    maflcko commented at 1:50 pm on February 7, 2024:
    nit, feel free to ignore: Any reason to add __func__ to this log, when none of the other logs have it here? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.
    furszy commented at 2:04 pm on February 7, 2024:

    Any reason to add __func__ to this log, when none of the other logs have it here?

    Not really. It is just a bad habit. I pulled 9a4e1f5 from #26836 because it is required for this changes. Will change it there.

  5. furszy force-pushed on Feb 7, 2024
  6. furszy commented at 2:16 pm on February 7, 2024: member
    Updated per feedback. Thanks Marko.
  7. furszy force-pushed on Feb 7, 2024
  8. DrahtBot added the label CI failed on Feb 7, 2024
  9. DrahtBot commented at 2:18 pm on February 7, 2024: contributor

    🚧 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

  10. furszy commented at 3:21 pm on February 7, 2024: member
    Green CI. Ready.
  11. DrahtBot removed the label CI failed on Feb 7, 2024
  12. in src/wallet/wallet.cpp:2406 in 2d770f6ae8 outdated 
    2402+            WalletLogPrintf("Error: cannot erase address book entry data\n");
2403+            return false;
2404+        }
2405+
2406+        // Delete purpose and name
2407+        if (!batch.ErasePurpose(dest) || !batch.EraseName(dest)) {
    achow101 commented at 7:43 pm on February 7, 2024:

    In 2d770f6ae88d9ee6f1775e011f241274059fe275 “wallet: clean redundancies in DelAddressBook”

    I’d prefer to have these occur separately so that a distinct error can be logged for each, that way we can discover which operation failed.

    furszy commented at 9:35 pm on February 7, 2024:

    I’d prefer to have these occur separately so that a distinct error can be logged for each, that way we can discover which operation failed.

    Sure. Also decoupled it in #26836.

  13. in src/wallet/bdb.cpp:890 in 0dba0b6de0 outdated 
    886@@ -887,7 +887,7 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
887 
888 bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix)
889 {
890-    if (!TxnBegin()) return false;
891+    assert(activeTxn); // because this is a read-write operation, the current db config requires a txn context.
    achow101 commented at 7:46 pm on February 7, 2024:

    In 0dba0b6de03c0c79bb09296b913729ec8eb75aca “wallet: bdb batch ‘ErasePrefix’, do not create txn internally”

    Instead of asserting, I would prefer if this did if (!Assume(activeTxn)) return false so we didn’t cause nodes to crash if a transaction were missing for whatever reason. Assume should still catch this for developers.

    furszy commented at 10:36 pm on February 7, 2024:

    Instead of asserting, I would prefer if this did if (!Assume(activeTxn)) return false so we didn’t cause nodes to crash if a transaction were missing for whatever reason. Assume should still catch this for developers.

    That would be risky in terms of consistency. BerkeleyBatch::ErasePrefix traverses the entire db, reading entries one by one, erasing the ones that match the key prefix. If we don’t execute this function within a txn context, it’s possible that certain records are removed while others remain due to an internal failure during the procedure.

    Currently, the only function that uses ErasePrefix() is WalletBatch::EraseAddressData, which is exclusively called from CWallet::DelAddressBook, which is modified in the first two commits (thats one of the reasons behind taking those commits from #26836). This prevents any crashing risk.

    achow101 commented at 10:52 pm on February 7, 2024:
    Sure, that’s why I suggest using Assume, and return false if there is no activeTxn. The callers of this should already be checking the return value to know whether the records were actually erased from the database.
    furszy commented at 2:13 pm on February 8, 2024:

    Sure, that’s why I suggest using Assume, and return false if there is no activeTxn. The callers of this should already be checking the return value to know whether the records were actually erased from the database.

    ah, I completely skipped the return false last night.. sorry. All good, pushed.

  14. achow101 commented at 7:46 pm on February 7, 2024: member
    ACK 69c5bb5c3ad3da508541a3ef6c29a0bad21f2fc0
  15. furszy force-pushed on Feb 7, 2024
  16. furszy commented at 10:38 pm on February 7, 2024: member
    Updated per feedback. Thanks achow!
  17. furszy force-pushed on Feb 8, 2024
  18. furszy commented at 2:16 pm on February 8, 2024: member
    Updated per feedback. Thanks achow. Small diff, only changed assert for Assume.
  19. furszy force-pushed on Feb 8, 2024
  20. furszy force-pushed on Feb 8, 2024
  21. furszy commented at 5:20 pm on February 8, 2024: member
    Rebased after #26836 merge. Ready to go.
  22. in src/wallet/test/db_tests.cpp:254 in 5235257077 outdated 
    249+        BOOST_CHECK(batch->Write(make_key(value, key), value));
250+
251+        // Erase the ones with the same prefix and verify result
252+        BOOST_CHECK(batch->TxnBegin());
253+        BOOST_CHECK(batch->ErasePrefix(DataStream() << key));
254+        BOOST_CHECK(batch->TxnCommit());
    achow101 commented at 7:45 pm on February 8, 2024:

    In 523525707742fc620039cd67e8f1437f7f613a01 “wallet: bdb batch ‘ErasePrefix’, do not create txn internally”

    nit: Could use RunWithinTxn().

    furszy commented at 9:04 pm on February 8, 2024:

    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.

  23. in src/wallet/test/wallet_tests.cpp:452 in 5235257077 outdated 
    446@@ -447,7 +447,9 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
447             BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
448             WalletBatch batch{wallet->GetDatabase()};
449             BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false));
450+            BOOST_CHECK(batch.TxnBegin()); // bdb requirement (more info at BerkeleyBatch::ErasePrefix)
451             BOOST_CHECK(batch.EraseAddressData(ScriptHash()));
452+            BOOST_CHECK(batch.TxnCommit());
    achow101 commented at 7:45 pm on February 8, 2024:

    In 523525707742fc620039cd67e8f1437f7f613a01 “wallet: bdb batch ‘ErasePrefix’, do not create txn internally”

    nit: Could use RunWithinTxn().

    furszy commented at 12:13 pm on February 9, 2024:
    sure
  24. in src/wallet/bdb.cpp:892 in 5235257077 outdated 
    886@@ -887,7 +887,11 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
887 
888 bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix)
889 {
890-    if (!TxnBegin()) return false;
891+    // Because this function erases records one by one, ensure that it is executed within a txn context.
892+    // Otherwise, consistency is at risk; it's possible that certain records are removed while others
893+    // remain due to an internal failure during the procedure.
    achow101 commented at 7:45 pm on February 8, 2024:

    In 523525707742fc620039cd67e8f1437f7f613a01 “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.

    furszy commented at 8:55 pm on February 8, 2024:

    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 https://github.com/furszy/bitcoin-core/commit/4e7b5c02837b6b3bc3d1ee81a52e801996f763b7), I didn’t see it as worth pushing it.

    ryanofsky commented at 7:31 pm on February 9, 2024:

    re: #29403 (review)

    In commit “wallet: bdb batch ‘ErasePrefix’, do not create txn internally” (96eb527fad0effe082b087c0898843feccc2e213)

    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 commented at 8:16 pm on February 9, 2024:

    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.

  25. in src/wallet/walletdb.cpp:1339 in 757d6516db outdated 
    1335@@ -1337,6 +1336,12 @@ bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, con
1336     return true;
1337 }
1338 
1339+bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
    achow101 commented at 7:46 pm on February 8, 2024:

    In 757d6516db56e2732ea77624e66025446cb816a4 “wallet: simplify EraseRecords by using ‘ErasePrefix’”

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

    furszy commented at 9:35 pm on February 8, 2024:

    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.

  26. in src/wallet/walletdb.cpp:1317 in 757d6516db outdated 
    1313@@ -1314,9 +1314,8 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
1314     return DBErrors::LOAD_OK;
1315 }
1316 
1317-bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
1318+static bool RunWithinTxn(WalletBatch& batch, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
    achow101 commented at 7:47 pm on February 8, 2024:

    In 757d6516db56e2732ea77624e66025446cb816a4 “wallet: simplify EraseRecords by using ‘ErasePrefix’”

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

    furszy commented at 9:37 pm on February 8, 2024:

    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.

  27. in src/wallet/walletdb.cpp:1317 in a8a24cb209 outdated 
    1313@@ -1314,6 +1314,29 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
1314     return DBErrors::LOAD_OK;
1315 }
1316 
1317+bool RunWithinTxn(WalletDatabase& database, const std::string& process_desc, const std::function<bool(WalletBatch&)>& func)
    ryanofsky commented at 9:04 pm on February 8, 2024:

    In commit “wallet: db, introduce ‘RunWithinTxn()’ helper function” (a8a24cb20974cb8eaec8cb438f6b22489719d605)

    Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object

    furszy commented at 12:13 pm on February 9, 2024:
    Sure!, done.
  28. ryanofsky approved
  29. ryanofsky commented at 9:05 pm on February 8, 2024: contributor
    Light code review ACK 757d6516db56e2732ea77624e66025446cb816a4, nice simplification of EraseRecords
  30. DrahtBot requested review from achow101 on Feb 8, 2024
  31. furszy force-pushed on Feb 9, 2024
  32. furszy commented at 12:12 pm on February 9, 2024: member
    Updated per feedback. Thanks both!
  33. achow101 commented at 6:41 pm on February 9, 2024: member
    ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249
  34. DrahtBot removed review request from achow101 on Feb 9, 2024
  35. DrahtBot requested review from ryanofsky on Feb 9, 2024
  36. in src/wallet/walletdb.cpp:1243 in b6dcfca8f7 outdated 
    1322+        return false;
1323+    }
1324+
1325+    // Run procedure, errors are logged internally
1326+    if (!func(batch)) {
1327+        batch.TxnAbort();
    ryanofsky commented at 7:10 pm on February 9, 2024:

    In commit “wallet: db, introduce ‘RunWithinTxn()’ helper function” (b6dcfca8f77f32cb1f41230fb71bab6666252041)

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

    furszy commented at 8:24 pm on February 9, 2024:
    Yeah, using util::Interrupted to skip the failure log in the future sounds great. Error logging added.
  37. in src/wallet/walletdb.h:308 in b6dcfca8f7 outdated 
    303+ * atomically committed to the db at the end of the process. And, in case of a
304+ * failure during execution, all performed changes are rolled back.
305+ *
306+ * @param database The db connection instance to perform the transaction on.
307+ * @param process_desc A description of the process being executed, used for logging purposes in the event of a failure.
308+ * @param func The function to be executed within the db txn context. Returns a boolean indicating success or failure.
    ryanofsky commented at 7:15 pm on February 9, 2024:

    In commit “wallet: db, introduce ‘RunWithinTxn()’ helper function” (b6dcfca8f77f32cb1f41230fb71bab6666252041)

    Would be useful to add this is determines whether to commit or roll back the txn

    furszy commented at 8:24 pm on February 9, 2024:
    Done
  38. ryanofsky approved
  39. ryanofsky commented at 7:44 pm on February 9, 2024: contributor
    Code review ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249. Looks pretty good, but would suggest cleaning up some things
  40. furszy force-pushed on Feb 9, 2024
  41. furszy commented at 8:29 pm on February 9, 2024: member
    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.
  42. DrahtBot added the label Needs rebase on Feb 12, 2024
  43. wallet: db, introduce 'RunWithinTxn()' helper function 
    '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.
    cf4d72a75e
  44. wallet: bdb batch 'ErasePrefix', do not create txn internally 
    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.
    33757814ce
  45. wallet: simplify EraseRecords by using 'ErasePrefix' 77331aa2a1
  46. furszy force-pushed on Feb 12, 2024
  47. DrahtBot removed the label Needs rebase on Feb 12, 2024
  48. achow101 commented at 11:26 pm on February 12, 2024: member
    reACK 77331aa2a198b708372a5c6cdf331faf7e2968ef
  49. DrahtBot requested review from ryanofsky on Feb 12, 2024
  50. in src/wallet/walletdb.cpp:1235 in 77331aa2a1 
    1229@@ -1230,9 +1230,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1230     return result;
1231 }
1232 
1233-bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
1234+static bool RunWithinTxn(WalletBatch& batch, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
1235 {
1236-    WalletBatch batch(database);
    josibake commented at 10:17 am on February 13, 2024:

    in “wallet: simplify EraseRecords by using ‘ErasePrefix’” (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):

    nit: these changes should be in the first commit

  51. in src/wallet/walletdb.cpp:1261 in 77331aa2a1 
    1252@@ -1254,6 +1253,12 @@ bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const
1253     return true;
1254 }
1255 
1256+bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
1257+{
1258+    WalletBatch batch(database);
1259+    return RunWithinTxn(batch, process_desc, func);
1260+}
1261+
    josibake commented at 10:17 am on February 13, 2024:

    in “wallet: simplify EraseRecords by using ‘ErasePrefix’” (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):

    nit: these changes should be in the first commit

  52. josibake commented at 10:21 am on February 13, 2024: member

    code review ACK https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef

    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.

  53. achow101 merged this on Feb 13, 2024
  54. achow101 closed this on Feb 13, 2024
  55. furszy deleted the branch on Feb 13, 2024


furszy DrahtBot maflcko achow101 ryanofsky josibake


ryanofsky

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me