wallet: Refactor database cursor into its own object with proper return codes #26690

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:walletdb-cursor-unstupidfy changing 11 files +178 −134
  1. achow101 commented at 8:07 PM on December 12, 2022: member

    Instead of having database cursors be tied to a particular DatabaseBatch object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors.

    Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the Next function (formerly ReadAtCursor) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read.

    Extracted from #24914

  2. Move SafeDbt out of BerkeleyBatch 69efbc011b
  3. DrahtBot commented at 8:07 PM on December 12, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, theStack

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26715 (Introduce MockableDatabase for wallet unit tests by achow101)
    • #26705 (clang-tidy: Check headers and fixes them by hebasto)
    • #26644 (wallet: bugfix, 'wallet_load_ckey' unit test fails with bdb by furszy)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #25296 (Add DataStream without ser-type and ser-version and use it where possible by MarcoFalke)
    • #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.

  4. DrahtBot added the label Wallet on Dec 12, 2022
  5. achow101 cross-referenced this on Dec 12, 2022 from issue wallet: Load database records in a particular order by achow101
  6. in src/wallet/bdb.cpp:666 in b11345f730 outdated 
     664 | -        return false;
 665 | -    int ret = pdb->cursor(nullptr, &m_cursor, 0);
 666 | -    return ret == 0;
 667 | +    assert(database.m_db.get());
 668 | +    int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
 669 | +    assert(ret == 0);
    theStack commented at 9:34 PM on December 12, 2022:

    Being annoying once again, but should probably replace this with an exception? (see discussion at https://github.com/bitcoin/bitcoin/pull/24914/commits/2c7b214cbc06ac07311add940925683c779e49b4#r893736353 which was marked as resolved, but was actually never done)

    furszy commented at 9:39 PM on December 12, 2022:

    link is broken, ref #24914 (review)

    achow101 commented at 8:12 PM on December 13, 2022:

    Done now.

  7. theStack commented at 9:34 PM on December 12, 2022: contributor

    Concept ACK

  8. DrahtBot cross-referenced this on Dec 12, 2022 from issue wallet: bugfix, 'wallet_load_ckey' unit test fails with bdb by furszy
  9. DrahtBot cross-referenced this on Dec 13, 2022 from issue Add DataStream without ser-type and ser-version and use it where possible by maflcko
  10. achow101 force-pushed on Dec 13, 2022
  11. achow101 force-pushed on Dec 13, 2022
  12. in src/wallet/test/wallet_tests.cpp:891 in 1660af4892 outdated 
     887 | @@ -882,9 +888,7 @@ class FailBatch : public DatabaseBatch
 888 |      void Flush() override {}
 889 |      void Close() override {}
 890 |  
 891 | -    bool StartCursor() override { return true; }
 892 | -    bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return false; }
 893 | -    void CloseCursor() override {}
 894 | +    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<FailCursor>(m_pass); }
    furszy commented at 9:14 PM on December 13, 2022:

    In 1660af48:

    FailCursor has an empty constructor. This commit shouldn't be compiling alone.

    achow101 commented at 11:33 PM on December 13, 2022:

    Fixed.

  13. achow101 force-pushed on Dec 13, 2022
  14. wallet: Introduce DatabaseCursor RAII class for managing cursor
    Instead of having DatabaseBatch deal with opening and closing database
cursors, have a separate RAII class that deals with those.

For now, DatabaseBatch manages DatabaseCursor, but this will change
later.
    7a198bba0a
  15. in src/wallet/bdb.cpp:667 in 58f2eafef4 outdated 
     668 | +    if (!database.m_db.get()) {
 669 | +        throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
 670 | +    }
 671 | +    assert(database.m_db.get());
 672 | +    int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
 673 | +    assert(ret == 0);
    theStack commented at 2:06 PM on December 14, 2022:

    The assert is still there, making the code below unreachable.

    achow101 commented at 5:41 PM on December 14, 2022:

    Oops, fixed.

  16. achow101 force-pushed on Dec 14, 2022
  17. in src/wallet/walletdb.cpp:881 in f2c83f4bc1 outdated 
     878 | @@ -878,7 +879,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 879 |      } catch (...) {
 880 |          result = DBErrors::CORRUPT;
 881 |      }
 882 | -    m_batch->CloseCursor();
    furszy commented at 11:45 PM on December 15, 2022:

    In f2c83f4b:

    In case of read completion or an exception, the cursor will stay open up to the next return statement if we don't call cursor.reset() here.

    achow101 commented at 5:36 PM on December 16, 2022:

    The cursor is within the try scope, so when we get here, it will already be out of scope and destroyed.

    furszy commented at 6:46 PM on December 16, 2022:

    true, I missed that. nvm then.

  18. in src/wallet/walletdb.cpp:1006 in f2c83f4bc1 outdated 
    1003 | +            bool ret = cursor->Next(ssKey, ssValue, complete);
1004 |              if (complete) {
1005 |                  break;
1006 |              } else if (!ret) {
1007 | -                m_batch->CloseCursor();
1008 | +                cursor.reset();
    furszy commented at 11:47 PM on December 15, 2022:

    In f2c83f4b:

    nit: as the cursor will be cleaned on the destructor, and we are returning here, this reset call is not needed.

    achow101 commented at 5:35 PM on December 16, 2022:

    Done

  19. furszy approved
  20. furszy commented at 11:58 PM on December 15, 2022: member

    Code review ACK bfef559b (follow-up to the reviews made to #24914) Only few non-blocking nits.

  21. DrahtBot cross-referenced this on Dec 16, 2022 from issue clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers by hebasto
  22. DrahtBot cross-referenced this on Dec 16, 2022 from issue net: Use serialization parameters for CAddress serialization by maflcko
  23. achow101 force-pushed on Dec 16, 2022
  24. wallet: Have cursor users use DatabaseCursor directly
    Instead of having the DatabaseBatch manage the cursor, having the
consumer handle it directly
    d79e8dcf29
  25. db: Change DatabaseCursor::Next to return status enum
    Next()'s result is a tri-state - failed, more to go, complete. Replace
the way that this is returned with an enum with values FAIL, MORE, and
DONE rather than with two booleans.
    4aebd832a4
  26. achow101 force-pushed on Dec 16, 2022
  27. achow101 cross-referenced this on Dec 16, 2022 from issue Introduce `MockableDatabase` for wallet unit tests by achow101
  28. furszy approved
  29. furszy commented at 6:46 PM on December 16, 2022: member

    diff ACK 4aebd83

  30. theStack approved
  31. theStack commented at 2:43 AM on December 18, 2022: contributor

    Code-review ACK 4aebd832a405090c2608e4b60bb4f34501bcea61

  32. in src/wallet/sqlite.cpp:497 in 4aebd832a4 
     495 | -void SQLiteBatch::CloseCursor()
 496 | +SQLiteCursor::~SQLiteCursor()
 497 |  {
 498 |      sqlite3_reset(m_cursor_stmt);
 499 | -    m_cursor_init = false;
 500 | +    int res = sqlite3_finalize(m_cursor_stmt);
    kiminuo commented at 9:22 PM on December 29, 2022:

    L496: Why is it necessary to reset the cursor before sqlite3_finalize is called?

    achow101 commented at 4:34 PM on January 3, 2023:

    It probably isn't necessary, but also is not harmful.

  33. DrahtBot cross-referenced this on Jan 11, 2023 from issue refactor: Split util/system into exception, shell, and fs-specific files by Empact
  34. DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: Use util::Result class for wallet loading by ryanofsky
  35. fanquake merged this on Jan 23, 2023
  36. fanquake closed this on Jan 23, 2023
  37. sidhujag referenced this in commit 3c878bbb45 on Jan 24, 2023
  38. bitcoin locked this on Jan 23, 2024
Contributors
achow101DrahtBottheStackfurszykiminuo
Labels
Wallet
Linked (view graph)
#24914 wallet: Load database records in a particular order#26715 Introduce `MockableDatabase` for wallet unit tests
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: 2026-05-19 06:53 UTC