test: improve BDB parser (handle internal/overflow pages, support all page sizes) #30125
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30125. 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. |
|
Code review ACK, from what i learned about how BDB databases work internally, implementation looks correct on first glance. |
theStack
force-pushed
the
complete_bdb-ro_python_parser2
branch
from
d166e88 to
361c3b0
Compare
|
Rebased on master (necessary since #26606 was merged and also touched the same functional test file). |
theStack
force-pushed
the
complete_bdb-ro_python_parser2
branch
from
361c3b0 to
ad0a8ff
Compare
| # Determine pagesize first | ||
| data = f.read(PAGE_HEADER_SIZE) | ||
| pagesize = struct.unpack('I', data[20:24])[0] | ||
| assert pagesize in (512, 1024, 2048, 4096, 8192, 16384, 32768, 65536) |
There was a problem hiding this comment.
WDYT about creating a constant tuple for these values? They seem pretty generic enough.
There was a problem hiding this comment.
WDYT about creating a constant tuple for these values? They seem pretty generic enough.
are they needed anywhere else? in #31222 i suggested
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0)but honestly, enumerating all 8 possible values here seems fine
There was a problem hiding this comment.
Yeah now that I think about my comment again, I feel creating a constant is not necessary.
Instead, I like your suggestion more specially with that bit hack. If other usages of is-power-of-two are anticipated in the functional tests, then probably a function can be added like so: https://stackoverflow.com/a/600306. Not necessary in this PR though.
…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.
theStack
force-pushed
the
complete_bdb-ro_python_parser2
branch
from
ad0a8ff to
d45eb39
Compare
|
Thanks for the review! Rebased on master (as I suspect CI would be very unhappy otherwise) and addressed comments #30125 (comment), #30125 (comment) and #30125 (comment) |
|
ACK d45eb39 |
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:
I verified that the newly introduced test passes with all valid page sizes between 512 and 65536.