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

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

   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, theStack

   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:

   • #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. 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.
6. 
   theStack commented at 9:34 pm on December 12, 2022: contributor

   Concept ACK
7. achow101 force-pushed on Dec 13, 2022
8. achow101 force-pushed on Dec 13, 2022
9. 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.
10. achow101 force-pushed on Dec 13, 2022
11. 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
12. 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.
13. achow101 force-pushed on Dec 14, 2022
14. 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.
15. 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
16. furszy approved
17. 
    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.
18. achow101 force-pushed on Dec 16, 2022
19. wallet: Have cursor users use DatabaseCursor directly

    Instead of having the DatabaseBatch manage the cursor, having the
    consumer handle it directly

    d79e8dcf29
20. 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
21. achow101 force-pushed on Dec 16, 2022
22. furszy approved
23. 
    furszy commented at 6:46 pm on December 16, 2022: member

    diff ACK 4aebd83
24. theStack approved
25. 
    theStack commented at 2:43 am on December 18, 2022: contributor

    Code-review ACK 4aebd832a405090c2608e4b60bb4f34501bcea61
26. 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.
27. fanquake merged this on Jan 23, 2023
28. fanquake closed this on Jan 23, 2023

    ---

29. sidhujag referenced this in commit 3c878bbb45 on Jan 24, 2023
30. bitcoin locked this on Jan 23, 2024

Contributors
achow101 DrahtBot theStack furszy kiminuo

Labels
Wallet