wallet: Implement independent BDB parser #26606
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
achow101
force-pushed
the
berkeleyro
branch
3 times, most recently
from
e18fdbf
to
f135c19
Compare
src/streams.h
Outdated
| @@ -530,6 +530,14 @@ class AutoFile | |||
| // | |||
| // Stream subset | |||
| // | |||
| void seek(int64_t offset, int origin) | |||
There was a problem hiding this comment.
1d093bb: Note for other reviewers: yes
src/streams.h
Outdated
| 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"); |
There was a problem hiding this comment.
1d093bb: maybe log the exact error code.
There was a problem hiding this comment.
It's kind of annoying to do that, so I'll leave it as is. The other functions do the same.
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #ifndef BITCOIN_WALLET_MIGRATE_H | ||
| #define BITCOIN_WALLET_MIGRATE_H |
There was a problem hiding this comment.
1224f45: maybe call this file bdb_ro_wallet_db.h?
There was a problem hiding this comment.
The intent is to have all of the migration stuff in these files, so I will leave it as is.
|
Concept ACK. 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. |
achow101
force-pushed
the
berkeleyro
branch
2 times, most recently
from
2d61534
to
f2dc015
Compare
|
Adding a fuzz target that compares this to the real thing seems useful. Given that this PR changes 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. |
achow101
force-pushed
the
berkeleyro
branch
2 times, most recently
from
944a020
to
f108c6b
Compare
|
For testing big endian wallets, you can use However, I also realized that BDB lets us set the endianness to use for a new database, so I've added a I've also added a test for the overflow page issue that was discovered earlier, and another test for the LSN reset check. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
achow101
force-pushed
the
berkeleyro
branch
2 times, most recently
from
bdaf3df
to
336c340
Compare
achow101
force-pushed
the
berkeleyro
branch
3 times, most recently
from
d8bd540
to
c132602
Compare
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.
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
New tests LGTM |
Split from #26596
This PR adds
BerkeleyRODatabasewhich 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
dumpcommand is changed to useBerkeleyRODatabaseinstead ofBerkeleyDatabase(andCWalletitself) to demonstrate that this parser works and to test it against the existing wallettool functional tests.