wallet: BerkeleyBatch Handle cursor internally

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

achow101 force-pushed on Jun 17, 2020

DrahtBot added the label Wallet on Jun 17, 2020

DrahtBot commented at 11:18 pm on June 17, 2020: member

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

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.

laanwj added this to the "Blockers" column in a project

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.

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.

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.

achow101 force-pushed on Jun 18, 2020

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.

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.

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.

ryanofsky approved

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

meshcollider added this to the "PRs" column in a project

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

promag commented at 11:12 pm on June 21, 2020: member

Concept ACK.

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

achow101 force-pushed on Jun 22, 2020

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.

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.

ryanofsky approved

ryanofsky commented at 0:20 am on June 23, 2020: member

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

promag commented at 9:31 am on June 23, 2020: member

Code review ACK ca24edfbc1941ed0a3c9586416dae4e84794eb66.

laanwj merged this on Jul 1, 2020

laanwj closed this on Jul 1, 2020

laanwj removed this from the "Blockers" column in a project

MarcoFalke referenced this in commit a924f61cae on Jul 14, 2020

sidhujag referenced this in commit fa48d2e308 on Jul 14, 2020

meshcollider moved this from the "PRs" to the "Done" column in a project

Fabcien referenced this in commit d0c7f1c3fd on Aug 30, 2021

kittywhiskers referenced this in commit 7b36561143 on Mar 2, 2022

kittywhiskers referenced this in commit ebf8dc6785 on Mar 3, 2022

kittywhiskers referenced this in commit 86946414fb on Mar 4, 2022

kittywhiskers referenced this in commit 69ba10b7f1 on Mar 5, 2022

kittywhiskers referenced this in commit a276282a77 on Mar 6, 2022

malbit referenced this in commit c51b219273 on Aug 7, 2022

DrahtBot locked this on Aug 30, 2022

wallet: BerkeleyBatch Handle cursor internally #19308

Conflicts