bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

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

Open theStack opened this issue 1 year ago • 5 comments

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 https://github.com/bitcoin/bitcoin/pull/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:

diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 38cca32f80..1bf39323d3 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -395,6 +395,7 @@ void BerkeleyDatabase::Open()
                             DB_BTREE,                                 // Database type
                             nFlags,                                   // Flags
                             0);
+            pdb_temp->set_pagesize(1<<9); /* valid BDB pagesizes are from 1<<9 (=512) to <<16 (=65536) */
 
             if (ret != 0) {
                 throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile));

I verified that the newly introduced test passes with all valid page sizes between 512 and 65536.

theStack avatar May 16 '24 17:05 theStack

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30125.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK achow101, furszy, brunoerg
Concept ACK laanwj, BrandonOdiwuor

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:

  • #31375 (multiprocess: Add bitcoin wrapper executable 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.

DrahtBot avatar May 16 '24 17:05 DrahtBot

Code review ACK, from what i learned about how BDB databases work internally, implementation looks correct on first glance.

laanwj avatar May 16 '24 19:05 laanwj

Rebased on master (necessary since #26606 was merged and also touched the same functional test file).

theStack avatar May 21 '24 17:05 theStack

Thanks for the review! Rebased on master (as I suspect CI would be very unhappy otherwise) and addressed comments https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813809067, https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813825732 and https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813830674

theStack avatar Oct 24 '24 13:10 theStack

ACK d45eb3964f693eddcf96f1e4083cf19d327be989

achow101 avatar Nov 06 '24 21:11 achow101