wallet: Implement independent BDB parser by achow101 · Pull Request #26606 · bitcoin/bitcoin

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

1d093bbf97b8d4ec5413ca560ac79e56f23d7f1d: Note for other reviewers: yes

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

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

src/streams.h

1d093bbf97b8d4ec5413ca560ac79e56f23d7f1d: 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

1224f45a92972199799d1ea33700eee567dc66fe: 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 e88ad38ac2f3e43de54a332965b13eb7cc27b91f 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.

sedited

Re-ACK c0e0a0affea3e3cee9a8910de4bae55a9bdd24cf

src/wallet/migrate.cpp

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.

sedited

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

    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.

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

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

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.

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

sedited approved these changes May 16, 2024

View reviewed changes

theStack reviewed May 16, 2024

View reviewed changes

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

theStack mentioned this pull request May 16, 2024

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

Merged

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

sedited approved these changes May 16, 2024

View reviewed changes

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

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

This was referenced Jun 13, 2024

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

Merged

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")

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!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sedited 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

DrahtBot commented Nov 29, 2022 •

edited

Loading

Marek777777 left a comment •

edited by achow101

Loading

theStack left a comment •

edited

Loading