wallet: BerkeleyBatch Handle cursor internally #19308

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:bdb-internal-cursor changing 3 files +50 −35
  1. achow101 commented at 4:41 PM on June 17, 2020: member

    Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally.

    Split from #18971

  2. achow101 force-pushed on Jun 17, 2020
  3. DrahtBot added the label Wallet on Jun 17, 2020
  4. DrahtBot commented at 11:18 PM on June 17, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
    • #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
    • #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
    • #19137 (wallettool: Add dump and createfromdump commands by achow101)
    • #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
    • #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)

    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.

  5. laanwj added this to the "Blockers" column in a project
  6. in src/wallet/walletdb.cpp:871 in 40d31e3182 outdated 
     869 | +            bool ret = m_batch.ReadAtCursor(ssKey, ssValue, complete);
 870 | +            if (complete) {
 871 | +                m_batch.CloseCursor();
 872 |                  break;
 873 | -            else if (ret != 0)
 874 | +            }
    laanwj commented at 2:48 PM on June 18, 2020:

    Nit: coding style } else if (!ret) {

    achow101 commented at 2:57 PM on June 18, 2020:

    Fixed.

  7. laanwj commented at 2:49 PM on June 18, 2020: member

    This does limit flexibility a bit, though I guess one cursor at a time is enough for all our current and future use cases.

  8. in src/wallet/bdb.cpp:742 in 40d31e3182 outdated 
     738 | @@ -738,27 +739,32 @@ void BerkeleyDatabase::ReloadDbEnv()
 739 |      }
 740 |  }
 741 |  
 742 | -Dbc* BerkeleyBatch::GetCursor()
 743 | +bool BerkeleyBatch::CreateCursor()
    laanwj commented at 2:55 PM on June 18, 2020:

    I think StartCursor would be a better name, after all, the whole process (including details like whether something is created) is internal.

    achow101 commented at 7:43 PM on June 22, 2020:

    Renamed to StartCursor.

  9. achow101 force-pushed on Jun 18, 2020
  10. achow101 commented at 2:58 PM on June 18, 2020: member

    This does limit flexibility a bit, though I guess one cursor at a time is enough for all our current and future use cases.

    If another cursor was needed, you could use another Batch object. But we barely use cursors, so one is enough for now.

  11. laanwj commented at 3:03 PM on June 18, 2020: member

    If another cursor was needed, you could use another Batch object. But we barely use cursors, so one is enough for now.

    Ohh! I missed that it's on the batch, not on the database object.

  12. in src/wallet/walletdb.cpp:745 in cda86544ae outdated 
     744 | @@ -742,8 +745,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 745 |              if (!strErr.empty())
 746 |                  pwallet->WalletLogPrintf("%s\n", strErr);
 747 |          }
 748 | -        pcursor->close();
    ryanofsky commented at 8:49 PM on June 18, 2020:

    In commit "walletdb: Handle cursor internally" (cda86544ae9de430e4558b87945046d5612202ac)

    Minor: I think it'd be a little clearer to keep this close here as m_batch.CloseCursor() so it's more obvious cursor is closed in all cases, and so you don't need to add a new close call in a different place above.

    promag commented at 11:12 PM on June 21, 2020:

    But above, in https://github.com/bitcoin/bitcoin/pull/19308/files#diff-e506682270036e4c8e0b8159a34b652dR723, it returns so it needs to be closed there, or am I missing something?

    achow101 commented at 7:45 PM on June 22, 2020:

    I've moved it to after the catch block and removed the ones inside the loop except for the ones right before the early return.

  13. ryanofsky approved
  14. ryanofsky commented at 8:53 PM on June 18, 2020: member

    Code review ACK cda86544ae9de430e4558b87945046d5612202ac. Nice to make cursor usage safer & more generic, even if it's a slightly less flexible than before (limited to single cursor per batch). I left minor style suggestion below, but feel free to ignore

  15. meshcollider added this to the "PRs" column in a project
  16. in src/wallet/bdb.cpp:750 in cda86544ae outdated 
     751 | +    int ret = pdb->cursor(nullptr, &m_cursor, 0);
 752 |      if (ret != 0)
 753 | -        return nullptr;
 754 | -    return pcursor;
 755 | +        return false;
 756 | +    return true;
    promag commented at 10:41 PM on June 21, 2020:

    nit, just return ret == 0.

    achow101 commented at 10:39 PM on June 22, 2020:

    Done

  17. promag commented at 11:12 PM on June 21, 2020: member

    Concept ACK.

  18. walletdb: Handle cursor internally
    Instead of returning a Dbc (BDB cursor object) and having the caller
deal with the cursor, make BerkeleyBatch handle the cursor internally.

This prepares BerkeleyBatch to work with other database systems as Dbc
objects are BDB specific.
    ca24edfbc1
  19. achow101 force-pushed on Jun 22, 2020
  20. in src/wallet/bdb.cpp:754 in ca24edfbc1 
     757 |  
 758 | -int BerkeleyBatch::ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue)
 759 | +bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
 760 |  {
 761 | +    complete = false;
 762 | +    if (m_cursor == nullptr) return false;
    promag commented at 9:33 PM on June 22, 2020:

    assert(m_cursor)? There was no check for pcursor.

    achow101 commented at 10:40 PM on June 22, 2020:

    I don't think an assert is necessary.

    promag commented at 9:31 AM on June 23, 2020:

    If it's something that can't happen then an assertion is fine. Also, it means that StartCursor must be called before.

    achow101 commented at 3:41 PM on June 23, 2020:

    If I have to push again.

  21. in src/wallet/bdb.cpp:338 in ca24edfbc1 
     334 | @@ -335,7 +335,7 @@ void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
 335 |  }
 336 |  
 337 |  
 338 | -BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr)
 339 | +BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr)
    promag commented at 9:33 PM on June 22, 2020:

    nit, move these to header.

    achow101 commented at 10:42 PM on June 22, 2020:

    I'm pretty sure that won't compile.

    promag commented at 10:51 PM on June 22, 2020:

    I mean 04998ef3cb866b2371bb047a5091787f61692588.

    achow101 commented at 11:08 PM on June 22, 2020:

    Ah. Maybe if I have to push again.

  22. ryanofsky approved
  23. ryanofsky commented at 12:20 AM on June 23, 2020: member

    Code review ACK ca24edfbc1941ed0a3c9586416dae4e84794eb66. Changes since last review: StartCursor rename, moving CloseCursor calls near returns

  24. promag commented at 9:31 AM on June 23, 2020: member

    Code review ACK ca24edfbc1941ed0a3c9586416dae4e84794eb66.

  25. laanwj merged this on Jul 1, 2020
  26. laanwj closed this on Jul 1, 2020
  27. laanwj removed this from the "Blockers" column in a project
  28. MarcoFalke referenced this in commit a924f61cae on Jul 14, 2020
  29. sidhujag referenced this in commit fa48d2e308 on Jul 14, 2020
  30. meshcollider moved this from the "PRs" to the "Done" column in a project
  31. Fabcien referenced this in commit d0c7f1c3fd on Aug 30, 2021
  32. kittywhiskers referenced this in commit 7b36561143 on Mar 2, 2022
  33. kittywhiskers referenced this in commit ebf8dc6785 on Mar 3, 2022
  34. kittywhiskers referenced this in commit 86946414fb on Mar 4, 2022
  35. kittywhiskers referenced this in commit 69ba10b7f1 on Mar 5, 2022
  36. kittywhiskers referenced this in commit a276282a77 on Mar 6, 2022
  37. malbit referenced this in commit c51b219273 on Aug 7, 2022
  38. DrahtBot locked this on Aug 30, 2022
Contributors
achow101DrahtBotlaanwjryanofskypromag
Labels
Wallet
Linked (view graph)
#18971 wallet: Refactor the classes in wallet/db.{cpp/h}#19325 wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class
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-04-21 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me