wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

achow101 · 2020-06-20T02:29:17Z

BerkeleyBatch and BerkeleyDatabase are kind of messy. The goal of this is to clean up them up so that they are logically separated.

BerkeleyBatch currently handles the creation of the BerkeleyDatabase's Db handle. This is instead moved into BerkeleyDatabase and is called by BerkeleyBatch.

Instead of having BerkeleyEnvironment track each database's usage, have BerkeleyDatabase track this usage itself with the m_refcount variable that is present in WalletDatabase.

Lastly, instead of having each BerkeleyEnvironment store the fileids of the databases open in it, have a global g_fileids to track those fileids. We were already checking fileid uniqueness globally (by checking the fileids in every environment when opening a database) so it's cleaner to do this with a global variable.

All of these changes allow us to make BerkeleyBatch and BerkeleyDatabase no longer be friend classes.

The diff of this PR is currently the same as in ##18971

Requires #19334

DrahtBot · 2020-06-20T03:33:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
Don't call lsn_reset in periodic flush #18904 (Don't call lsn_reset in periodic flush by bvbfan)

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.

ryanofsky

Code review ACK d416ae5. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids

ryanofsky

Conditional code review ACK e73d0a1 if some problems below are fixed. Just reviewed last 4 commits here

61da2fa walletdb: track database file use as m_refcount within BerkeleyDatabase (5/8)
33554ed walletdb: Move Db->open to BerkeleyDatabase::Open (6/8)
8a98a83 walletdb: Use a global g_fileids instead of m_fileids for each env (7/8)
e73d0a1 walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase (8/8)

src/wallet/bdb.cpp

ryanofsky

Code review ACK ccfd611 last 5 commits

747ca18 Combine BerkeleyEnvironment::Verify into BerkeleyDatabase::Verify (5/9)
516084a walletdb: track database file use as m_refcount within BerkeleyDatabase (6/9)
93b5f04 walletdb: Move Db->open to BerkeleyDatabase::Open (7/9)
0a01ba7 No need to check for duplicate fileids in all dbenvs (8/9)
ccfd611 walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase (9/9)

Only changes since previous review are followups from #19335 (review). This is a nice cleanup and everything here is an improvement. I do think Batch/Database refcounting interaction is too complicated though and could be made simpler later (#19335 (comment)).

Instead of having BerkeleyEnvironment track the file use count, make BerkeleyDatabase do it itself.

Instead of opening the Db handle in BerkeleyBatch, make BerkeleyDatabase do that.

Since we have .walletlock in each directory, we don't need the duplicate fileid checks across all dbenvs as it shouldn't be possible anyways.

ryanofsky

Code review ACK 74507ce. No changes since last review other than rebase

promag · 2020-07-25T00:11:33Z

src/wallet/bdb.cpp

@@ -285,8 +284,7 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)

    if (fs::exists(file_path))
    {
-        LOCK(cs_db);


Why was this removed?

It was there to guard mapFileUseCount which we've now removed.

laanwj · 2020-07-29T16:02:38Z

Code review ACK 74507ce

…d BerkeleyBatch 74507ce walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase (Andrew Chow) 00f0041 No need to check for duplicate fileids in all dbenvs (Andrew Chow) d86efab walletdb: Move Db->open to BerkeleyDatabase::Open (Andrew Chow) 4fe4b3b walletdb: track database file use as m_refcount within BerkeleyDatabase (Andrew Chow) 65fb880 Combine BerkeleyEnvironment::Verify into BerkeleyDatabase::Verify (Andrew Chow) Pull request description: `BerkeleyBatch` and `BerkeleyDatabase` are kind of messy. The goal of this is to clean up them up so that they are logically separated. `BerkeleyBatch` currently handles the creation of the `BerkeleyDatabase`'s `Db` handle. This is instead moved into `BerkeleyDatabase` and is called by `BerkeleyBatch`. Instead of having `BerkeleyEnvironment` track each database's usage, have `BerkeleyDatabase` track this usage itself with the `m_refcount` variable that is present in `WalletDatabase`. Lastly, instead of having each `BerkeleyEnvironment` store the fileids of the databases open in it, have a global `g_fileids` to track those fileids. We were already checking fileid uniqueness globally (by checking the fileids in every environment when opening a database) so it's cleaner to do this with a global variable. All of these changes allow us to make `BerkeleyBatch` and `BerkeleyDatabase` no longer be friend classes. The diff of this PR is currently the same as in #bitcoin#18971 Requires bitcoin#19334 ACKs for top commit: laanwj: Code review ACK 74507ce ryanofsky: Code review ACK 74507ce. No changes since last review other than rebase Tree-SHA512: 845d84ee1a470e2bf5d2e2e3d7738183d8ce43ddd06a0bbd57edecf5779b2f55d70728b1b57f5daab0f078650a8d60c3e19dc30b75b36e7aa952ce268399d5f6

Summary: This is a backport of [[bitcoin/bitcoin#19335 | core#19335]] [1/5] bitcoin/bitcoin@65fb880 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10034

Summary: Instead of having BerkeleyEnvironment track the file use count, make BerkeleyDatabase do it itself. This is a backport of [[bitcoin/bitcoin#19335 | core#19335]] [2/5] bitcoin/bitcoin@4fe4b3b Depends on D10034 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10035

Summary: Instead of opening the Db handle in BerkeleyBatch, make BerkeleyDatabase do that. This is a backport of [[bitcoin/bitcoin#19335 | core#19335]] [3/5] bitcoin/bitcoin@d86efab Depends on D10035 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10036

Summary: Since we have .walletlock in each directory, we don't need the duplicate fileid checks across all dbenvs as it shouldn't be possible anyways. This is a backport of [[bitcoin/bitcoin#19335 | core#19335]] [4/5] bitcoin/bitcoin@00f0041 Depends on D10036 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10037

Summary: This is a backport of [[bitcoin/bitcoin#19335 | core#19335]] [5/5] bitcoin/bitcoin@74507ce Depends on D10037 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10038

fanquake added the Wallet label Jun 20, 2020

achow101 mentioned this pull request Jun 20, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

achow101 force-pushed the bdb-cleanup-refactors branch from a2a30a5 to d69f101 Compare June 22, 2020 20:12

This was referenced Jun 23, 2020

wallet: Introduce WalletDatabase abstract class #19334

Merged

wallettool: Add dump and createfromdump commands #19137

Merged

Refactor: clean up PeriodicFlush() #19085

Merged

DrahtBot added the Needs rebase label Jul 1, 2020

achow101 force-pushed the bdb-cleanup-refactors branch 2 times, most recently from 1b155cb to 8dc24d0 Compare July 1, 2020 16:05

DrahtBot added Needs rebase and removed Needs rebase labels Jul 1, 2020

achow101 force-pushed the bdb-cleanup-refactors branch from 8dc24d0 to 288bae7 Compare July 6, 2020 18:26

DrahtBot added Needs rebase and removed Needs rebase labels Jul 6, 2020

achow101 force-pushed the bdb-cleanup-refactors branch from 288bae7 to 06120bf Compare July 9, 2020 16:12

DrahtBot removed the Needs rebase label Jul 9, 2020

DrahtBot mentioned this pull request Jul 10, 2020

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

Merged

DrahtBot added the Needs rebase label Jul 11, 2020

achow101 force-pushed the bdb-cleanup-refactors branch from 06120bf to 8fa346c Compare July 11, 2020 15:10

DrahtBot removed the Needs rebase label Jul 11, 2020

DrahtBot mentioned this pull request Jul 11, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

achow101 force-pushed the bdb-cleanup-refactors branch from 8fa346c to e73d0a1 Compare July 14, 2020 15:17

ryanofsky approved these changes Jul 15, 2020

View reviewed changes

ryanofsky reviewed Jul 16, 2020

View reviewed changes

src/wallet/bdb.cpp Show resolved Hide resolved

src/wallet/bdb.cpp Show resolved Hide resolved

src/wallet/bdb.cpp Outdated Show resolved Hide resolved

src/wallet/bdb.cpp Show resolved Hide resolved

src/wallet/bdb.cpp Show resolved Hide resolved

achow101 force-pushed the bdb-cleanup-refactors branch from e73d0a1 to ccfd611 Compare July 16, 2020 18:30

ryanofsky approved these changes Jul 16, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jul 22, 2020

achow101 force-pushed the bdb-cleanup-refactors branch from ccfd611 to df8ca1a Compare July 22, 2020 15:36

DrahtBot removed the Needs rebase label Jul 22, 2020

achow101 added 5 commits July 22, 2020 23:30

Combine BerkeleyEnvironment::Verify into BerkeleyDatabase::Verify

65fb880

walletdb: track database file use as m_refcount within BerkeleyDatabase

4fe4b3b

Instead of having BerkeleyEnvironment track the file use count, make BerkeleyDatabase do it itself.

walletdb: Move Db->open to BerkeleyDatabase::Open

d86efab

Instead of opening the Db handle in BerkeleyBatch, make BerkeleyDatabase do that.

No need to check for duplicate fileids in all dbenvs

00f0041

Since we have .walletlock in each directory, we don't need the duplicate fileid checks across all dbenvs as it shouldn't be possible anyways.

walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase

74507ce

achow101 force-pushed the bdb-cleanup-refactors branch from df8ca1a to 74507ce Compare July 23, 2020 03:36

ryanofsky approved these changes Jul 24, 2020

View reviewed changes

promag reviewed Jul 25, 2020

View reviewed changes

laanwj merged commit 8db2334 into bitcoin:master Jul 29, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

Uh oh!

achow101 commented Jun 20, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Jun 20, 2020 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

promag Jul 25, 2020

Uh oh!

achow101 Jul 25, 2020

Uh oh!

laanwj commented Jul 29, 2020

Uh oh!

Uh oh!

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

Uh oh!

Conversation

achow101 commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 29, 2020

Uh oh!

Uh oh!

achow101 commented Jun 20, 2020 •

edited

Loading

DrahtBot commented Jun 20, 2020 •

edited

Loading