wallet: Implement independent BDB parser #26606

achow101 · 2022-11-29T21:50:53Z

Split from #26596

This PR adds BerkeleyRODatabase which is an independent implementation of a BDB file parser. It provides read only access to a BDB file, and can therefore be used as a read only database backend for wallets. This will be used for dumping legacy wallet records and migrating legacy wallets without the need for BDB itself.

Wallettool's dump command is changed to use BerkeleyRODatabase instead of BerkeleyDatabase (and CWallet itself) to demonstrate that this parser works and to test it against the existing wallettool functional tests.

DrahtBot · 2022-11-29T21:50:56Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, josibake, furszy, laanwj, theStack
Concept ACK	darosior, Sjors

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:

#29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)
#28236 (fuzz: wallet, add target for Spend by Ayush170-Future)
#25722 (refactor: Use util::Result class for wallet loading 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.

Marek777777

.

Sjors

Some review nibbles.

src/streams.h

Sjors · 2023-03-01T16:05:12Z

src/streams.h

@@ -530,6 +530,14 @@ class AutoFile
    //
    // Stream subset
    //
+    void seek(int64_t offset, int origin)


1d093bb: Note for other reviewers: yes

https://en.cppreference.com/w/c/io/fseek

Sjors · 2023-03-01T16:07:54Z

src/streams.h

+        if (!file)
+            throw std::ios_base::failure("CAutoFile::read: file handle is nullptr");
+        if (fseek(file, offset, origin) != 0)
+            throw std::ios_base::failure(feof(file) ? "CAutoFile::seek: end of file" : "CAutoFile::seek: fseek failed");


1d093bb: maybe log the exact error code.

It's kind of annoying to do that, so I'll leave it as is. The other functions do the same.

Sjors · 2023-03-01T16:32:13Z

src/wallet/migrate.h

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_MIGRATE_H
+#define BITCOIN_WALLET_MIGRATE_H


1224f45: maybe call this file bdb_ro_wallet_db.h?

The intent is to have all of the migration stuff in these files, so I will leave it as is.

src/wallet/migrate.cpp

darosior · 2023-03-22T10:29:17Z

Concept ACK. Have you looked into a fuzz target for this parser?

achow101 · 2023-03-22T18:02:01Z

Have you looked into a fuzz target for this parser?

I haven't. If someone would like to write one, I'd be happy to add it.

Sjors · 2023-06-05T15:40:32Z

Adding a fuzz target that compares this to the real thing seems useful. Given that this PR changes dumpwallet to rely on the new parser, I think we should do that sooner rather than later. Otherwise I suggest changing e88ad38 so that the read-only parser is optional, and then change the tests to always use it.

It seems a bit daunting to compare the low level DBD parsing code in this PR to the real thing. Having a fuzzer also reduces the need to be very thorough in that. I'm not sure how much confidence I have in the existing test coverage for dumpwallet.

Byteswapped databases make it easier to test opening and deserializing other endian databases.

TheCharlatan

Re-ACK d51fbab

josibake · 2024-05-17T06:58:45Z

reACK d51fbab

furszy

reACK d51fbab

laanwj

re-ACK d51fbab

theStack

ACK d51fbab

Convinced myself that this parser works reliably partly by looking at the original code, partly by extending the test framework's BDB parser to match this PR's logic (see #30125), which helped a lot in understanding the structure. Also tested with a few random signet legacy wallets that I still had around, though those were all quite small.

Btw, I couldn't manage to find or create a BDB wallet where the DELETE flag is set in a record header (would be nice to exercise that in a test, though it's hard to verify that it's really set without inspecting manually, or e.g. adding debug messages on deserialization), but the code to handle that is simple enough and looks correct.

theStack · 2024-05-17T21:12:39Z

src/wallet/test/fuzz/wallet_bdb_parser.cpp

+            error.original == "Page number mismatch" ||
+            error.original == "Bad btree level" ||
+            error.original == "Bad page size" ||
+            error.original == "File size is not a multiple of page size" ||


nit: isn't thrown in the parser (commented out), hence can be removed here

Suggested change

error.original == "File size is not a multiple of page size" ||

theStack · 2024-05-17T21:14:20Z

src/wallet/test/fuzz/wallet_bdb_parser.cpp

+            error.original == "Unexpected number of entries in outer database root page" ||
+            error.original == "Subdatabase has an unexpected name" ||
+            error.original == "Subdatabase page number has unexpected length" ||
+            error.original == "Unexpected inner database page type" ||


nit: isn't thrown anywhere, can be removed

Suggested change

error.original == "Unexpected inner database page type" ||

theStack · 2024-05-17T22:15:06Z

test/functional/tool_wallet.py

@@ -24,11 +24,14 @@ class ToolWalletTest(BitcoinTestFramework):
    def add_options(self, parser):
        self.add_wallet_options(parser)
        parser.add_argument("--bdbro", action="store_true", help="Use the BerkeleyRO internal parser when dumping a Berkeley DB wallet file")
+        parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make then byte swapped internally")


typo nit

Suggested change

parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make then byte swapped internally")

parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make them byte swapped internally")

maflcko · 2024-05-21T09:29:39Z

I wonder if it makes sense to collect the fuzz inputs to qa-assets, from testers that still have them. However, the fuzz CI does not compile with BDB, so that'll probably need to be adjusted first.

…l page sizes) This aims to complete our test framework BDB parser to reflect our read-only BDB parser in the wallet codebase. This could be useful both for making review of bitcoin#26606 easier and to also possibly improve our functional tests for the BDB parser by comparing with an alternative implementation.

hebasto · 2024-06-05T11:07:25Z

Ported to the CMake-based build system in hebasto#220.

6825523 connection: run async cleanups in LIFO not FIFO order (Ryan Ofsky) Pull request description: Run Connection class asynchronous cleanups in LIFO not FIFO order, which is more natural ordering and prevents a bitcoin wallet shutdown deadlock when the connection to the node process is broken. Also add comments better documenting cleanup order. This change fixes one of two shutdown deadlocks in the bitcoin wallet when the node process is killed in bitcoin/bitcoin#10102 as of bitcoin/bitcoin@3f12b43. The other deadlock is caused by the `ProxyServerCustom<WalletLoader>` destructor in that PR and is described in commit efe42cc from #100. The bitcoin wallet shutdown deadlocks have probably been around for some time, but were not encountered because before bitcoin/bitcoin@0b75315 from bitcoin/bitcoin#26606 there weren't any tests which killed the bitcoin node process and required the bitcoin-wallet process to shut down gracefully. Top commit has no ACKs. Tree-SHA512: cbbedcc71e4c3ebb541d95f2444151c4058fff3b6357bed5c1b2929d5311fd2a621aeed1af7f56a6159bb3c1ac8abf051acc22830f748edaa6b61ecd557f5074

…thout requiring BDB 8ce3739 test: verify wallet is still active post-migration failure (furszy) 771bc60 wallet: Use LegacyDataSPKM when loading (Ava Chow) 61d872f wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM (Ava Chow) b231f4d wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM (Ava Chow) 7461d0c wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM (Ava Chow) 517e204 Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO (Ava Chow) Pull request description: #26606 introduced `BerkeleyRODatabase` which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. `LegacyDataSPKM` is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration. ACKs for top commit: cbergqvist: ACK 8ce3739 theStack: Code-review ACK 8ce3739 furszy: Code review ACK 8ce3739 Tree-SHA512: dccea12d6c597de15e3e42f97ab483cfd069e103611200279a177e021e8e9c4e74387c4f45d2e58b3a1e7e2bdb32a1d2d2060b1f8086c03eeaa0c68579d9d54e

hebasto

4d7a3ae

The new src/wallet/test/fuzz/wallet_bdb_parser.cpp fails on Windows.

achow101 · 2024-08-31T04:23:21Z

The new src/wallet/test/fuzz/wallet_bdb_parser.cpp fails on Windows.

This looks like an inconsistency in BDB itself as the failure is on BDB loading the database file. Additionally, since this test case passes on linux, I'm not entirely sure what to do about it. This will also be resolved once BDB is removed.

…l page sizes) This aims to complete our test framework BDB parser to reflect our read-only BDB parser in the wallet codebase. This could be useful both for making review of bitcoin#26606 easier and to also possibly improve our functional tests for the BDB parser by comparing with an alternative implementation.

…s, support all page sizes) d45eb39 test: compare BDB dumps of test framework parser and wallet tool (Sebastian Falbesoner) 01ddd9f test: complete BDB parser (handle internal/overflow pages, support all page sizes) (Sebastian Falbesoner) Pull request description: This PR adds missing features to our test framework's BDB parser with the goal of hopefully being able to read all legacy wallets that are created with current and past versions of Bitcoin Core. This could be useful both for making review of #26606 easier and to also possibly improve our functional tests for the wallet BDB-ro parser by additionally validating it with an alternative implementation. The second commits introduces a test that create a legacy wallet with huge label strings (in order to create overflow pages, i.e. pages needed for key/value data than is larger than the page size) and compares the dump outputs of wallet tool and the extended test framework BDB parser. It can be exercised via `$ ./test/functional/tool_wallet.py --legacy`. BDB support has to be compiled in (obviously). For some manual tests regarding different page sizes, the following patch can be used: ```diff diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 38cca32..1bf39323d3 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -395,6 +395,7 @@ void BerkeleyDatabase::Open() DB_BTREE, // Database type nFlags, // Flags 0); + pdb_temp->set_pagesize(1<<9); /* valid BDB pagesizes are from 1<<9 (=512) to <<16 (=65536) */ if (ret != 0) { throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile)); ``` I verified that the newly introduced test passes with all valid page sizes between 512 and 65536. ACKs for top commit: achow101: ACK d45eb39 furszy: utACK d45eb39 brunoerg: code review ACK d45eb39 Tree-SHA512: 9f8ac80452545f4fcd24a17ea6f9cf91b487cfb1fcb99a0ba9153fa4e3b239daa126454e26109fdcb72eb1c76a4ee3b46fd6af21dc318ab67bd12b3ebd26cfdd

DrahtBot added the Wallet label Nov 29, 2022

achow101 mentioned this pull request Nov 29, 2022

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

achow101 force-pushed the berkeleyro branch 3 times, most recently from e18fdbf to f135c19 Compare November 30, 2022 04:09

DrahtBot mentioned this pull request Nov 30, 2022

refactor: Use util::Result class for wallet loading #25722

Draft

Marek777777 approved these changes Nov 30, 2022

View reviewed changes

This was referenced Nov 30, 2022

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

achow101 force-pushed the berkeleyro branch from f135c19 to 99c43be Compare December 2, 2022 00:48

Marek777777 suggested changes Dec 3, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 18, 2023

refactor: wallet, remove global 'ArgsManager' dependency #26889

Merged

DrahtBot added the Needs rebase label Feb 17, 2023

achow101 force-pushed the berkeleyro branch from 99c43be to 89f5eda Compare February 17, 2023 20:57

DrahtBot removed the Needs rebase label Feb 17, 2023

Sjors reviewed Mar 1, 2023

View reviewed changes

achow101 force-pushed the berkeleyro branch from 89f5eda to 33de558 Compare March 15, 2023 18:09

DrahtBot added the CI failed label May 1, 2023

achow101 force-pushed the berkeleyro branch 2 times, most recently from 2d61534 to f2dc015 Compare May 2, 2023 17:53

DrahtBot removed the CI failed label May 2, 2023

achow101 mentioned this pull request May 3, 2023

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

DrahtBot added the CI failed label Jun 7, 2023

achow101 force-pushed the berkeleyro branch 2 times, most recently from 944a020 to f108c6b Compare June 8, 2023 09:42

DrahtBot removed the CI failed label Jun 8, 2023

achow101 added 5 commits May 16, 2024 15:03

bdb: Be able to make byteswapped databases

6ace3e9

Byteswapped databases make it easier to test opening and deserializing other endian databases.

test: Test dumps of other endian BDB files

fd7b16e

test: Test dumping dbs with overflow pages

c1984f1

test: Test bdb_ro dump of wallet without reset LSNs

0b75315

wallet, test: Be able to always swap BDB endianness

d51fbab

achow101 force-pushed the berkeleyro branch from c0e0a0a to d51fbab Compare May 16, 2024 19:03

TheCharlatan approved these changes May 16, 2024

View reviewed changes

DrahtBot requested a review from josibake May 16, 2024 20:41

hebasto added the Needs CMake port label May 17, 2024

furszy reviewed May 17, 2024

View reviewed changes

laanwj approved these changes May 17, 2024

View reviewed changes

theStack approved these changes May 17, 2024

View reviewed changes

fanquake merged commit 5acdc2b into bitcoin:master May 21, 2024

furszy mentioned this pull request Jun 3, 2024

wallet: optimize migration process, batch db transactions #28574

Merged

hebasto mentioned this pull request Jun 4, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#220

Closed

hebasto removed the Needs CMake port label Jun 5, 2024

ryanofsky mentioned this pull request Jun 13, 2024

connection: run async cleanups in LIFO not FIFO order bitcoin-core/libmultiprocess#101

Merged

ryanofsky mentioned this pull request Jun 13, 2024

Multiprocess bitcoin #10102

Draft

willcl-ark mentioned this pull request Jul 3, 2024

Memory leak loading legacy wallet (BDB 4.8.30) #27283

Closed

hebasto mentioned this pull request Aug 29, 2024

ci: Add missed configuration options to "Win64 native" job #30755

Merged

hebasto reviewed Aug 29, 2024

View reviewed changes

	parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make then byte swapped internally")
	parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make them byte swapped internally")

wallet: Implement independent BDB parser #26606

wallet: Implement independent BDB parser #26606

Uh oh!

Conversation

achow101 commented Nov 29, 2022

Uh oh!

DrahtBot commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Marek777777 left a comment • edited by achow101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darosior commented Mar 22, 2023

Uh oh!

achow101 commented Mar 22, 2023

Uh oh!

Sjors commented Jun 5, 2023

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

josibake commented May 17, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment • 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!

maflcko commented May 21, 2024

Uh oh!

hebasto commented Jun 5, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 31, 2024

Uh oh!

Uh oh!

DrahtBot commented Nov 29, 2022 •

edited

Loading

Marek777777 left a comment •

edited by achow101

Loading

theStack left a comment •

edited

Loading