-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: improve BDB parser (handle internal/overflow pages, support all page sizes) #30125
base: master
Are you sure you want to change the base?
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. ConflictsNo conflicts as of last run. |
Code review ACK, from what i learned about how BDB databases work internally, implementation looks correct on first glance. |
…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.
d166e88
to
361c3b0
Compare
Rebased on master (necessary since #26606 was merged and also touched the same functional test file). |
361c3b0
to
ad0a8ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete review and untested yet, left couple comments off the top of my head atm.
Will do another round thoroughly in the next few days.
entry['data'] = data[offset + 3:offset + 3 + e_len] | ||
record_header = data[offset:offset + 3] | ||
offset += 3 | ||
e_len, record_type = struct.unpack('HB', record_header) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small benefit to struct.pack
being less code when more than one item is packed or unpacked. Seems fine to use either in this case, up to the author. However, in most other cases where only a single item is (un)packed, the modern alternatives are less code, and clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for using struct.unpack
in this PR is to be consistent with the already existing code in the bdb module (i.e. avoiding to mix up different serialization methods), and I didn't want to introduce large refactoring changes in this PR. I strongly agree though that int.{to,from}_bytes
is preferred and support #29401.
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about creating a constant tuple for these values? They seem pretty generic enough.
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.