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.

…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.

BerkeleyRODatabase is intended for use after BDB is removed, so it needs to be able to read all of the records from a BDB file. Thus an independent deserializer for BDB data files is implemented in it. This deserializer is targeted towards the data files that Bitcoin Core creates so it does not fully support all of BDB's features (e.g. other database types, encryption, etc.).

Implements MakeBerkeleyRODatabase and adds DatabaseFormat::BERKELEY_RO so that MakeDatabase can use BerkeleyRO as the backend database.

…lets In order to ease the transition to not having BDB, make the dump tool use DatabaseFormmat::BERKELEY_RO when -withinternalbdb is set.

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.

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

Open

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

Open

39 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

DrahtBot requested a review from theStack May 16, 2024 11:08

theStack mentioned this pull request May 16, 2024

test: improve BDB parser (handle internal/overflow pages, support all page sizes) #30125

Open

achow101 and others added 12 commits May 16, 2024 15:03

Implement handling of other endianness in BerkeleyRODatabase

6e50bee

Add MakeBerkeleyRODatabase

dd57713

Implements MakeBerkeleyRODatabase and adds DatabaseFormat::BERKELEY_RO so that MakeDatabase can use BerkeleyRO as the backend database.

wallettool: Optionally use BERKELEY_RO as format when dumping BDB wal…

70cfbfd

…lets In order to ease the transition to not having BDB, make the dump tool use DatabaseFormmat::BERKELEY_RO when -withinternalbdb is set.

tests: Add BerkeleyRO to db prefix tests

3568dce

Berkeley RO Database fuzz test

4d7a3ae

Error if LSNs are not reset

d987890

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
16 checks passed

furszy mentioned this pull request Jun 3, 2024

wallet: optimize migration process, batch db transactions #28574

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Implement independent BDB parser #26606

wallet: Implement independent BDB parser #26606

achow101 commented Nov 29, 2022

DrahtBot commented Nov 29, 2022 •

edited

Marek777777 left a comment •

edited by achow101

Sjors left a comment

Sjors Mar 1, 2023

Sjors Mar 1, 2023

achow101 Mar 15, 2023

Sjors Mar 1, 2023

achow101 Mar 15, 2023

darosior commented Mar 22, 2023

achow101 commented Mar 22, 2023

Sjors commented Jun 5, 2023

TheCharlatan left a comment

josibake commented May 17, 2024

furszy left a comment

laanwj left a comment

theStack left a comment •

edited

theStack May 17, 2024

theStack May 17, 2024

theStack May 17, 2024

maflcko commented May 21, 2024

hebasto commented Jun 5, 2024

	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

Conversation

achow101 commented Nov 29, 2022

DrahtBot commented Nov 29, 2022 • edited

Code Coverage

Reviews

Conflicts

Marek777777 left a comment • edited by achow101

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Mar 22, 2023

achow101 commented Mar 22, 2023

Sjors commented Jun 5, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

josibake commented May 17, 2024

furszy left a comment

Choose a reason for hiding this comment

laanwj left a comment

Choose a reason for hiding this comment

theStack left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented May 21, 2024

hebasto commented Jun 5, 2024

DrahtBot commented Nov 29, 2022 •

edited

Marek777777 left a comment •

edited by achow101

theStack left a comment •

edited