test: Add functional test for bitcoin-chainstate #32145

TheCharlatan commented at 9:42 AM on March 26, 2025: contributor

While the bitcoin-chainstate utility is not shipped in a release, it is the only current utility directly using the bitcoin kernel library. Adding a simple test for it is useful for checking that the library is actually usable. The test is also useful in future to demonstrate that the bitcoin-chainstate binary using the API for the kernel library introduced in #30595 actually works and offers similar features.

DrahtBot commented at 9:42 AM on March 26, 2025: contributor

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/32145.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, maflcko, kevkevinpal

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)
#30860 (test: autogenerate bash completion by BrandonOdiwuor)

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 added the label Tests on Mar 26, 2025

maflcko commented at 9:55 AM on March 26, 2025: member

Needs rebase, as the base is too old?

test: Fix docstring for cmake migration 3f9c716e7f

TheCharlatan force-pushed on Mar 26, 2025

DrahtBot added the label CI failed on Mar 26, 2025

TheCharlatan commented at 10:57 AM on March 26, 2025: contributor

Needs rebase, as the base is too old?

Thanks, wrote this some time ago and forgot to rebase.

in test/functional/tool_bitcoin_chainstate.py:37 in 76449b9cdb outdated

  32 | +            raise AssertionError(f'Expected stderr output {expected_stderr} does not partially match stderr:\n{stderr}')
  33 | +
  34 | +    def run_test(self):
  35 | +        node = self.nodes[0]
  36 | +        datadir = node.cli.datadir
  37 | +        # This might trigger a warning that there is not enough disk space, since this node runs on mainnet. Ignore it.

laanwj commented at 11:57 AM on March 26, 2025:

FWIW #32057 went with -prune=550 to avoid this condition, i don't know if that's possible here but would prefer to be consistent.

in test/functional/tool_bitcoin_chainstate.py:42 in 76449b9cdb outdated

  37 | +        # This might trigger a warning that there is not enough disk space, since this node runs on mainnet. Ignore it.
  38 | +        node.stop_node(check_stderr=False)
  39 | +
  40 | +        self.log.info(f"Testing bitcoin-chainstate {self.get_binaries().chainstate_argv()} with datadir: {datadir}")
  41 | +        block_one = "010000006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000982051fd1e4ba744bbbe680e1fee14677ba1a3c3540bf7b1cdb606e857233e0e61bc6649ffff001d01e362990101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0704ffff001d0104ffffffff0100f2052a0100000043410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac00000000"
  42 | +        self.add_block(datadir, block_one + "\n", "Block has not yet been rejected")

laanwj commented at 12:13 PM on March 26, 2025:

It might make sense to factor out the add of "\n" to add_block?

DrahtBot removed the label CI failed on Mar 26, 2025

test: Add functional test for bitcoin-chainstate

Adds basic coverage for successfully validating a mainnet block as well
as some duplicate and invalid data.

ca55613fd1

TheCharlatan force-pushed on Mar 26, 2025

TheCharlatan commented at 2:08 PM on March 26, 2025: contributor

Thank you for the review @laanwj,

Updated 76449b9cdb367eed05cfdf5fe550fc41e477a7fe -> ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 (BitcoinChainstateFunctionalTest_0 -> BitcoinChainstateFunctionalTest_1, compare)

Addressed @laanwj's comment, use pruning instead of introducing new functionality to avoid the disk space warning. Dropped the commit for passing in argument to ignore the stderr output.
Addressed @laanwj's comment, avoid some code duplicating by having to add "\n" for arguments passed to add_block.

laanwj approved

laanwj commented at 2:47 PM on March 26, 2025: member

Code review ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0

in test/functional/tool_bitcoin_chainstate.py:34 in ca55613fd1

  29 | +        stdout, stderr = proc.communicate(input=input + "\n", timeout=5)
  30 | +        self.log.debug("STDOUT: {0}".format(stdout.strip("\n")))
  31 | +        self.log.info("STDERR: {0}".format(stderr.strip("\n")))
  32 | +
  33 | +        if expected_stderr not in stderr:
  34 | +            raise AssertionError(f"Expected stderr output {expected_stderr} does not partially match stderr:\n{stderr}")

maflcko commented at 3:20 PM on March 26, 2025:

could just use assert_equal?

TheCharlatan commented at 8:25 PM on March 26, 2025:

I used assert_equal first, but then saw that partial matches use a similar approach here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/test_node.py#L523.

maflcko approved

maflcko commented at 3:21 PM on March 26, 2025: member

ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 🎭

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 🎭
QChzeK5vs9igTYzIEqEksUtAvYIchTYkSzNBQRrlsSse8lfj94JWklqOD42JDPoGNvO5iqQVgmiYMx2735hABA==

</details>

in test/functional/tool_bitcoin_chainstate.py:46 in ca55613fd1

  41 | +        self.log.info(f"Testing bitcoin-chainstate {self.get_binaries().chainstate_argv()} with datadir: {datadir}")
  42 | +        block_one = "010000006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000982051fd1e4ba744bbbe680e1fee14677ba1a3c3540bf7b1cdb606e857233e0e61bc6649ffff001d01e362990101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0704ffff001d0104ffffffff0100f2052a0100000043410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac00000000"
  43 | +        self.add_block(datadir, block_one, "Block has not yet been rejected")
  44 | +        self.add_block(datadir, block_one, "duplicate")
  45 | +        self.add_block(datadir, "00", "Block decode failed")
  46 | +        self.add_block(datadir, "", "Empty line found")

kevkevinpal commented at 4:28 PM on March 26, 2025:

not something that may block this PR but I see other error messages in bitcoin-chainstate.cpp

maybe can be addressed in a followup PR

grep -nri "std::cerr" ./src/bitcoin-chainstate.cpp

(manually removed the ones that are already in this PR)

53:        std::cerr
   54             << "Usage: " << argv[0] << " DATADIR" << std::endl
   55             << "Display DATADIR information, and process hex-encoded blocks on standard input." << std::endl
   56             << std::endl
   57             << "IMPORTANT: THIS EXECUTABLE IS EXPERIMENTAL, FOR TESTING ONLY, AND EXPECTED TO" << std::endl
   58             << "           BREAK IN FUTURE VERSIONS. DO NOT USE ON YOUR ACTUAL DATADIR." << std::endl;
   59         return 1;
   60     }
100:            std::cerr << "Error flushing block data to disk: " << message.original << std::endl;
104:            std::cerr << "Error: " << message.original << std::endl;
134:        std::cerr << "Failed to load Chain state from your datadir." << std::endl;
139:            std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
147:            std::cerr << "Failed to connect best block (" << state.ToString() << ")" << std::endl;
222:            std::cerr << "inconclusive" << std::endl;
231:            std::cerr << "the block header may be on a too-little-work chain" << std::endl;
234:            std::cerr << "invalid by consensus rules (excluding any below reasons)" << std::endl;
237:            std::cerr << "this block was cached as being invalid and we didn't store the reason why" << std::endl;
240:            std::cerr << "invalid proof of work or time too old" << std::endl;
243:            std::cerr << "the block's data didn't match the data committed to by the PoW" << std::endl;
246:            std::cerr << "We don't have the previous block the checked one is built on" << std::endl;
249:            std::cerr << "A block this one builds on is invalid" << std::endl;
252:            std::cerr << "block timestamp was > 2 hours in the future (or our clock is bad)" << std::endl;

TheCharlatan commented at 8:17 PM on March 26, 2025:

The tests by design only capture the very basic functionality. Confirming the output of the rest seems a bit overkill.

kevkevinpal commented at 4:29 PM on March 26, 2025: contributor

ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0

looks good to me I think we can expand on the test, but not blocking this change

maflcko commented at 11:38 AM on March 27, 2025: member

This is only adding a test case, so should be rfm with three acks?

ryanofsky assigned ryanofsky on Mar 27, 2025

ryanofsky merged this on Mar 27, 2025

ryanofsky closed this on Mar 27, 2025