wallet: Move BerkeleyBatch static functions to BerkeleyDatabase #19324

achow101 · 2020-06-18T21:02:38Z

The BerkeleyBatch class has 4 static functions that operate on BerkeleyDatabase or BerkeleyEnvironment. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from BerkeleyBatch into BerkeleyDatabase and make them member functions instead of static.

BerkeleyBatch::VerifyEnvironment and BerkeleyBatch::VerifyDatabaseFile are combined into a single BerkeleyDatabase::Verify function that operates on that BerkeleyDatabase object.

BerkeleyBatch::Rewrite and BerkeleyBatch::PeriodicFlush both took a BerkeleyDatabase as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.

Part of #18971

ryanofsky

Code review almost-ACK 893ec59. Suggested some tweaks

src/wallet/bdb.cpp

src/wallet/wallettool.cpp

src/wallet/bdb.cpp

ryanofsky · 2020-06-18T22:04:24Z

src/wallet/wallettool.cpp

@@ -112,7 +112,7 @@ static bool SalvageWallet(const fs::path& path)
    // Initialize the environment before recovery
    bilingual_str error_string;
    try {
-        WalletBatch::VerifyEnvironment(path, error_string);
+        database->Verify(error_string);


In commit "walletdb: Combine VerifyDatabaseFile and VerifyEnvironment" (ae1a9d9)

It doesn't seem like there is a point to calling Verify here in SalvageWallet when ExecuteWalletToolFunc already called it immediately before calling this. It seems like this SalvageWallet function could just be dropped and ExecuteWalletToolFunc just call RecoverDatabaseFile directly.

Since the database is closed before we reach this function, we need to reopen it before doing the recovery function. We could remove this and only close the database for info but that seems a bit like a hack.

Maybe something to think about for a followup.

In commit "walletdb: Combine VerifyDatabaseFile and VerifyEnvironment" (8f1bcf8)

Since the database is closed before we reach this function, we need to reopen it before doing the recovery function. We could remove this and only close the database for info but that seems a bit like a hack.

Maybe something to think about for a followup.

It seems like the unintended change in behavior described #19324 (comment) is back.

Instead of salvage command just recovering wallet data, now it tries to verify before recovering. Maybe this change is good because verifying before recovering is good for some reason. Maybe it is bad because it prevents salvage command from working some cases. Or maybe it is ok but slows the salvage operation down unnecessarily. I can't tell if it's good or bad but it seems unintended and not mentioned in the PR description or any commit message and ignored in reviews.

It looks like previous behavior could be restored by deleting the SalvageWallet function and just calling RecoverDatabaseFile function instead (previously suggested #19324 (comment))

Hmm. Seems like there was a rebase error at some point. I'll make a followup that handles this and a few other related suggestions.

src/wallet/wallettool.cpp

DrahtBot · 2020-06-19T02:31:44Z

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: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
wallet: Introduce WalletDatabase abstract class #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
Refactor: clean up PeriodicFlush() #19085 (Refactor: clean up PeriodicFlush() by jnewbery)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
refactor: Remove CAddressBookData::destdata #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.

promag · 2020-06-20T10:33:42Z

Code review ACK 4693b11.

No strong opinion whether wallet tool info and salvage should verify.

ryanofsky

Code review ACK 4693b11. VerifyNotInUse rename is wonky but other than that this is an improvement. Main difference since last review is reverting wallet tool behavior change

src/wallet/bdb.cpp

src/wallet/wallettool.cpp

maflcko

Concept ACK

src/wallet/wallet.cpp

maflcko

ACK 514ed76 📁

Show signature and timestamp

Signature:

Timestamp of file with hash 1639c2f3437a4a8f4fe84a0eaa5966db2964f9df9c75f3885e2308bb1c24d313 -

maflcko · 2020-06-30T22:58:56Z

src/wallet/wallettool.cpp

-        if (!WalletBatch::VerifyEnvironment(path, error)) {
-            tfm::format(std::cerr, "%s\nError loading %s. Is wallet being used by other process?\n", error.original, name);
-            return false;
-        }


in commit 17924a5

This change seems unrelated. Maybe put it into a preceding commit and explain what it does? I presume it removes a redundant check that is done as part of LoadWallet?

How is it unrelated? This function is being removed from WalletBatch.

I was wrong. VerifyEnvironment is called via LoadWallet -> LoadWalletInternal -> CWallet::Verify, which is ~~different from~~ (edit: this was wrong) the LoadWallet call below.

So why is it ok to remove this call to VerifyEnvironment?

VerifyEnvironment would just call BerkeleyEnvironment::Open. A couple of other things also call that in their normal operation. In particular, CWallet::LoadWallet creates a WalletBatch which creates a BerkeleyBatch which calls BerkeleyEnvironment::Open in its constructor.

In the cases where we need to open the environment and aren't using the higher level stuff, a direct call to BerkeleyEnvironment::Open is needed which is why it is added to SalvageWallet.

With "unrelated" I meant "can be split up into its own atomic and logical commit without depending on other changes in this pull request".

See the following commit on current master, which compiles fine and runs all tests fine as well:

$ git show commit 43d120ec8a7a999a48ba31e2621efec06ca668f6 (HEAD) Author: MarcoFalke <falke.marco@gmail.com> Date: Thu Jul 2 07:53:05 2020 -0400 wallet: Remove redundant WalletBatch::VerifyEnvironment It is redundant because LoadWallet will call CWallet::LoadWallet -> LoadWalletInternal -> CWallet::Verify, which calls VerifyEnvironment For SalvageWallet it is redundant with the newly added call to Open diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index d42950ee42..e6e62332c0 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -20,6 +20,11 @@ bool RecoverDatabaseFile(const fs::path& file_path) std::string filename; std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename); + if (!env->Open(true /* retry */)) { + tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory()); + return false; + } + // Recovery procedure: // move wallet file to walletfilename.timestamp.bak // Call Salvage with fAggressive=true to diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 77ed6beb5d..430a866a7c 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -140,11 +140,6 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) tfm::format(std::cerr, "Error: no wallet file at %s\n", name); return false; } - bilingual_str error; - if (!WalletBatch::VerifyEnvironment(path, error)) { - tfm::format(std::cerr, "%s\nError loading %s. Is wallet being used by other process?\n", error.original, name); - return false; - } if (command == "info") { std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path); diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 524e1593ba..18f0beb598 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -71,8 +71,7 @@ class ToolWalletTest(BitcoinTestFramework): self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create') self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') self.assert_raises_tool_error( - 'Error initializing wallet database environment "{}"!\nError loading wallet.dat. Is wallet being used by other process?' - .format(os.path.join(self.nodes[0].datadir, self.chain, 'wallets')), + 'Error loading wallet.dat. Is wallet being used by another process?', '-wallet=wallet.dat', 'info', )

Maybe if I need to push again.

maflcko · 2020-06-30T22:59:47Z

src/wallet/salvage.cpp

+    if (!env->Open(true /* retry */)) {
+        tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory());
+        return false;
+    }


Same commit

This also seems unrelated. Is it related to the change in ExecuteWalletToolFunc?

See https://github.com/bitcoin/bitcoin/pull/19324/files#r448951064

maflcko · 2020-06-30T23:06:08Z

src/wallet/salvage.cpp

@@ -20,6 +20,11 @@ bool RecoverDatabaseFile(const fs::path& file_path)
    std::string filename;
    std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);

+    if (!env->Open(true /* retry */)) {
+        tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory());


in commit 17924a5:

Any reason to write to stderr when all other failures are written to the debug log? Since this is a pure utility function, I'd prefer if this returned the error string to the caller in the future, so that the caller can decide how to display the error

It was to largely retain the previous behavior of this error going to stderr. I think that cleaning up this function to return error strings and remove the LogPrintfs in a followup.

Combine these two functions into a single Verify function that is a member of WalletDatabase. Additionally, these are no longer static.

Make PeriodicFlush a non-static member of WalletDatabase instead of WalletBatch.

Make Rewrite actually a member of BerkeleyDatabase instead of a static function in BerkeleyBatch

achow101 · 2020-07-01T16:37:51Z

Apparently the python linter on master fails here, so I've pushed a change to fix that.

maflcko · 2020-07-02T00:15:46Z

re-ACK d8e9ca6 only change is test fixup 🤞

Show signature and timestamp

Signature:

Timestamp of file with hash 7e2f395c7afce74dc863a67c21bbf3c3b75edbd4fb9a35f91c5cc23e1674a2a6 -

maflcko · 2020-07-02T16:36:57Z

@promag or @ryanofsky Mind to re-ACK?

promag

Code review ACK d8e9ca6, good stuff.

Sjors · 2020-07-06T18:12:33Z

There's a stray VerifyEnvironment and VerifyDatabaseFile in walletdb.h. Maybe fix those in #19325?

…Batch abstract class b82f0ca walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow) eac9200 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow) Pull request description: In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`. The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`. Part of #18971 Requires #19308 and #19324 ACKs for top commit: Sjors: re-utACK b82f0ca MarcoFalke: ACK b82f0ca 🌘 meshcollider: LGTM, utACK b82f0ca Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a

…atabaseBatch abstract class b82f0ca walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow) eac9200 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow) Pull request description: In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`. The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`. Part of bitcoin#18971 Requires bitcoin#19308 and bitcoin#19324 ACKs for top commit: Sjors: re-utACK b82f0ca MarcoFalke: ACK b82f0ca 🌘 meshcollider: LGTM, utACK b82f0ca Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a

…ous declarations 0e279fe walletdb: Remove unused static functions from walletdb.h (Andrew Chow) 9f536d4 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow) 06e263a Call RecoverDatabaseFile directly from wallettool (Andrew Chow) Pull request description: Followup to #19324 addressing some comments. Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in bitcoin/bitcoin#19324 (comment) Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in bitcoin/bitcoin#19324 (comment) Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in bitcoin/bitcoin#19324 (comment) ACKs for top commit: meshcollider: Code review ACK 0e279fe ryanofsky: Code review ACK 0e279fe, just dropped last commit Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d

…extraneous declarations 0e279fe walletdb: Remove unused static functions from walletdb.h (Andrew Chow) 9f536d4 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow) 06e263a Call RecoverDatabaseFile directly from wallettool (Andrew Chow) Pull request description: Followup to bitcoin#19324 addressing some comments. Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in bitcoin#19324 (comment) Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in bitcoin#19324 (comment) Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in bitcoin#19324 (comment) ACKs for top commit: meshcollider: Code review ACK 0e279fe ryanofsky: Code review ACK 0e279fe, just dropped last commit Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d

Summary: ``` The BerkeleyBatch class has 4 static functions that operate on BerkeleyDatabase or BerkeleyEnvironment. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from BerkeleyBatch into BerkeleyDatabase and make them member functions instead of static. BerkeleyBatch::VerifyEnvironment and BerkeleyBatch::VerifyDatabaseFile are combined into a single BerkeleyDatabase::Verify function that operates on that BerkeleyDatabase object. BerkeleyBatch::Rewrite and BerkeleyBatch::PeriodicFlush both took a BerkeleyDatabase as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument. ``` Backport of core [[bitcoin/bitcoin#19324 | PR19324]]. Depends on D8616. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8617

Summary: VerifyEnvironment and VerifyDatabaseFile were removed in D8617/[[bitcoin/bitcoin#19324 | PR19324]], but their declarations weren't. Remove those. This is a backport of [[bitcoin/bitcoin#19457 | core#19457]] [3/3] bitcoin/bitcoin@0e279fe Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9079

This was referenced Jun 18, 2020

wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325

Merged

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

Closed

DrahtBot added the Wallet label Jun 18, 2020

ryanofsky reviewed Jun 18, 2020

View reviewed changes

achow101 force-pushed the bdb-batch-rm-statics branch from 893ec59 to 4693b11 Compare June 18, 2020 23:51

ryanofsky approved these changes Jun 23, 2020

View reviewed changes

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

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

achow101 force-pushed the bdb-batch-rm-statics branch from 4693b11 to 192218c Compare June 23, 2020 16:05

DrahtBot mentioned this pull request Jun 23, 2020

Refactor: clean up PeriodicFlush() #19085

Merged

maflcko reviewed Jun 24, 2020

View reviewed changes

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

achow101 force-pushed the bdb-batch-rm-statics branch from 192218c to 514ed76 Compare June 24, 2020 17:48

maflcko reviewed Jun 30, 2020

View reviewed changes

achow101 added 3 commits July 1, 2020 12:32

walletdb: Combine VerifyDatabaseFile and VerifyEnvironment

8f1bcf8

Combine these two functions into a single Verify function that is a member of WalletDatabase. Additionally, these are no longer static.

walletdb: Move PeriodicFlush into WalletDatabase

91d1091

Make PeriodicFlush a non-static member of WalletDatabase instead of WalletBatch.

walletdb: Move Rewrite into BerkeleyDatabase

d8e9ca6

Make Rewrite actually a member of BerkeleyDatabase instead of a static function in BerkeleyBatch

achow101 force-pushed the bdb-batch-rm-statics branch from 514ed76 to d8e9ca6 Compare July 1, 2020 16:36

promag reviewed Jul 5, 2020

View reviewed changes

maflcko merged commit 171f4a5 into bitcoin:master Jul 5, 2020

achow101 mentioned this pull request Jul 6, 2020

wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

Merged

ryanofsky mentioned this pull request Sep 1, 2020

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Merged

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

wallet: Move BerkeleyBatch static functions to BerkeleyDatabase #19324

wallet: Move BerkeleyBatch static functions to BerkeleyDatabase #19324

Uh oh!

Conversation

achow101 commented Jun 18, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

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

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 1, 2020

Uh oh!

maflcko commented Jul 2, 2020

Uh oh!

maflcko commented Jul 2, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jul 6, 2020

Uh oh!

Uh oh!

DrahtBot commented Jun 19, 2020 •

edited

Loading

promag commented Jun 20, 2020 •

edited

Loading

maflcko Jul 2, 2020 •

edited

Loading

achow101 Jul 1, 2020 •

edited

Loading