test: add functional test for XORed block/undo files (`-blocksxor` option) #30657

theStack commented at 3:34 PM on August 14, 2024: contributor

This PR adds a dedicated functional test for XORed block data/undo file support (bitcoind option -blocksxor, see PR #28052). In order to verify that the XOR pattern has been applied, the {blk,rev}*.dat files are rewritten un-XORed manually by the test while the node is shut down; the node is then started again with -blocksxor=0, and both the data and undo files are verified via the verifychain RPC (with checklevel=2). Note that starting bitcoind with -blocksxor=0 fails if a xor key is present already, which is also tested explicitly.

Fixes #30599.

test: refactor: move `read_xor_key`/`util_xor` helpers to util module 6b3676be3e

DrahtBot commented at 3:34 PM on August 14, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, ismaelsadeeq, glozow
Concept ACK	TheCharlatan
Approach ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Aug 14, 2024

theStack renamed this:
~~test: add functional test for XORed block/undo files (`-blockxor` option)~~
test: add functional test for XORed block/undo files (`-blocksxor` option)
on Aug 14, 2024

theStack force-pushed on Aug 14, 2024

test: add functional test for XORed block/undo files (`-blocksxor`) faa1b9b0e6

theStack force-pushed on Aug 14, 2024

TheCharlatan commented at 3:53 PM on August 14, 2024: contributor

Concept ACK

in test/functional/test_framework/util.py:314 in 6b3676be3e outdated

 310 | @@ -311,6 +311,13 @@ def sha256sum_file(filename):
 311 |      return h.digest()
 312 |  
 313 |  
 314 | +def util_xor(data, key, *, offset):

tdb3 commented at 12:09 AM on August 15, 2024:

nitty nit: Since this is being moved, could update to use a more descriptive function name (e.g. xor_bytes). Feel free to ignore.

in test/functional/feature_blocksxor.py:61 in faa1b9b0e6

  56 | +        self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity")
  57 | +        os.remove(node.blocks_path / 'xor.dat')
  58 | +        self.start_node(0, extra_args=['-blocksxor=0'])
  59 | +        # checklevel=2 -> verify block validity + undo data
  60 | +        # nblocks=0    -> verify all blocks
  61 | +        node.verifychain(checklevel=2, nblocks=0)

tdb3 commented at 1:14 AM on August 15, 2024:

The help for -blocksxor mentions The created XOR-key will be zeros for an existing blocksdir or when -blocksxor=0 is set.

After the verifychain, could test that a new xor.dat is created (consisting of all zeros) when the node is started with no xor.dat and with -blocksxor=0. Something like:

assert read_xor_key(node=node) == bytes(8)

or maybe lift NUM_XOR_BYTES out of read_xor_key to enable

assert read_xor_key(node=node) == bytes(NUM_XOR_BYTES)

or even add a constant in util.py

NULL_BLK_XOR_KEY = bytes(NUM_XOR_BYTES) # all-0 xor key

to enable

assert read_xor_key(node=node) == NULL_BLK_XOR_KEY

tdb3 commented at 1:15 AM on August 15, 2024: contributor

Approach ACK. Nice additions. Left a comment and a nit.

in test/functional/test_framework/util.py:518 in faa1b9b0e6

 514 | @@ -508,6 +515,12 @@ def check_node_connections(*, node, num_in, num_out):
 515 |      assert_equal(info["connections_out"], num_out)
 516 |  
 517 |  
 518 | +def read_xor_key(*, node):

maflcko commented at 5:49 AM on August 15, 2024:

nit: Any reason to make this a global function and pass a node to it? Seems clearer to just make it a member function on the node, like blocks_path(), no?

theStack commented at 1:52 PM on August 20, 2024:

nit: Any reason to make this a global function and pass a node to it? Seems clearer to just make it a member function on the node, like blocks_path(), no?

No strong reason. Personally I first look into util.py for helpers and tend to forget that there are also some implemented as TestNode methods, but I agree that the latter makes sense. Could be picked up in a follow-up, e.g. #30669.

maflcko approved

maflcko commented at 5:53 AM on August 15, 2024: member

ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc

DrahtBot requested review from tdb3 on Aug 15, 2024

DrahtBot requested review from TheCharlatan on Aug 15, 2024

maflcko added this to the milestone 28.0 on Aug 15, 2024

in test/functional/feature_blocksxor.py:54 in faa1b9b0e6

  49 | +                f.write(util_xor(xored_data, xor_key, offset=0))
  50 | +
  51 | +        self.log.info("Check that restarting with 'blocksxor=0' fails if XOR key is present")
  52 | +        node.assert_start_raises_init_error(['-blocksxor=0'],
  53 | +            'The blocksdir XOR-key can not be disabled when a random key was already stored!',
  54 | +            match=ErrorMatch.PARTIAL_REGEX)

glozow commented at 11:48 AM on August 15, 2024:

This fails for me locally, because somehow I have 0000000000000000 as my xor key... but it fails consistently. Unsure if that's supposed to be possible?

maflcko commented at 12:38 PM on August 15, 2024:

Maybe a leftover previous bitcoind was used? Can you try make clean, just to double check? Otherwise, the combined log could be useful.

glozow commented at 4:53 PM on August 15, 2024:

I added a if (opts.use_xor) assert(xor_key != decltype(xor_key){}); which hit, so not an old bitcoind. Attaching combined log.

 node0 2024-08-15T16:40:41.093882Z [init] [node/blockstorage.cpp:1184] [InitBlocksdirXorKey] Using obfuscation key for blocksdir *.dat files (/tmp/bitcoin_func_test__c38k83u/node0/regtest/blocks): '0000000000000000'

logs_30657.txt

maflcko commented at 5:02 PM on August 15, 2024:

I added a if (opts.use_xor) assert(xor_key != decltype(xor_key){}); which hit, so not an old bitcoind. Attaching combined log.

Can you also print/log/debug the result of fs::is_empty(opts.blocks_dir) and fs::exists(xor_key_path)?

glozow commented at 5:22 PM on August 15, 2024:

logs_30657_2.txt

Ah interesting, fs::is_empty(opts.blocks_dir) is 0

^so clearly this isn't a problem with the test

maflcko commented at 5:32 PM on August 15, 2024:

Fascinating. What is your compiler, stdlib version, OS and filesystem for /tmp/ and ./?

Does the issue happen when you create a non-/tmp/ folder? (I presume /tmp/ is a ramdisk?)

mkdir no_ramdisk_temp_prefix
./test/functional/test_runner.py --tmpdirprefix ./no_ramdisk_temp_prefix/ feature_blocksxor

maflcko commented at 5:35 PM on August 15, 2024:

Also, what is ls --all /tmp/bitcoin_func_test_fj812gkv/node0/regtest/blocks

maflcko commented at 5:37 PM on August 15, 2024:

Is it Apple https://en.wikipedia.org/wiki/.DS_Store ?

glozow commented at 5:52 PM on August 15, 2024:

It's jammy with clang 17, indeed it works with a non-/tmp folder. Nothing out of place in ls. Hmmm

maflcko commented at 6:21 PM on August 15, 2024:

Jammy doesn't have clang 17, so I guess it is installed from the nightly llvm apt and then CC=clang-17 CXX=clang++-17 ../configure (using the libstdc++-11, not libc++-17)?

Next steps to test would be:

df --human-readable /tmp/ (to confirm the filesystem type)
Try with CC=gcc CXX=g++ ./configure (using the libstdc++-11)
Try with CC=clang-17 CXX='clang++-17 -stdlib=libc++-17' ./configure (using libc++-17)
Try turning it off and on again (To kill any processes that may make /tmp/ dirty)?

glozow commented at 8:55 AM on August 16, 2024:

Filesystem      Size  Used Avail Use% Mounted on
/dev/nvme0n1p2  468G  110G  335G  25% /

It worked with gcc. ./configure didn't like libc++-17. After reboot I can't get it to fail anymore (also I don't mean to hold up this PR)

maflcko commented at 9:08 AM on August 16, 2024:

After reboot I can't get it to fail anymore (also I don't mean to hold up this PR)

Thanks for checking. This is really helpful to make sure that the feature isn't shipped to users with bugs included.

My random guess would be that there was a process running on your machine which accidentally or intentionally put files into all folders in /tmp/. However, based on your check with ls --all on the temp folder, which didn't reveal any hidden files, this may be something else, or a race (as the files were deleted again by the same process or a different one).

It would be useful, if an Apple person tried to break this feature with https://en.wikipedia.org/wiki/.DS_Store. The process would be:

Start the test, but pause Bitcoin Core once the blocksdir is created (empty)
Run Apple Finder on the blocksdir to check if a file will be created
Continue the test and see it fail (if a file was created), or pass (otherwise)

glozow commented at 2:26 PM on August 16, 2024:

I tried this on my apple machine and am able to make this test fail if I add a sleep right before the line checking fs::is_empty. No sign of DS_Store. It kind of seems more like blk0000.dat, rev0000.dat, and index/ are showing up early... I don't think I know init well enough to know exactly what order everything should happen in.

edit: ah, maybe I am not looking hard enough

Starting at macOS 10.12 16A238m, Finder will not display .DS_Store files (even with com.apple.finder AppleShowAllFiles YES set).

maflcko commented at 2:45 PM on August 16, 2024:

Thanks for checking!

I honestly don't known if this should be considered a bug in Bitcoin Core. If so, are we going to enumerate all external programs that write to the blocksdir without permission and add workarounds for them?

To me it seems like this is something that Apple should fix, or Apple users (by not using Apple, or by adjusting the Finder settings to leave Bitcoin Core alone).

maflcko commented at 2:50 PM on August 16, 2024:

I guess fs::is_empty(opts.blocks_dir) could be replaced by not fs::exists("blk0000.dat")?

glozow commented at 3:23 PM on August 16, 2024:

fwiw the sleepy version doesn't seem to use 0 as xor key when I start a node (tried a few times with mainnet, regtest) normally. I also never have problems when I use a non-tmp folder. So maybe your guess about temp directory paranormal activity is correct.

Going to merge this test since there's not a problem with it.

fjahr commented at 11:18 AM on September 8, 2024:

Found this here because I saw #30833 and it got me to look into blocksxor for the first time. I see the same failures @glozow describes (also on macos) and I can confirm the findings here about the use of the temp folders because it does not happen when the test uses a clean chain, e.g. using a clean chain like this made the test robust for me: https://github.com/fjahr/bitcoin/commit/ef00b8eab48cf7ebcc8a0539f237175b8f5a0373 I didn't find anything new beyond that so far but I thought it should be noted that it's not on @glozow 's machine alone.

maflcko commented at 6:34 AM on September 9, 2024:

made the test robust for me: fjahr@ef00b8e

Thanks for checking. Using a clean chain means that the cache won't be used, which normally lives on the storage unit where the build dir lives, so only the temp dir should be used. However, given the prior result that a non-tmp folder was generally fine, I would have expected the opposite result (Test would be less fragile by using the persisted cache on a non-tmp dir). However, I don't have a macOS to test this, so I don't know for sure what is going on. Also, if this "fixes" the test, it will just silence the symptoms, but it may be better to fix the underlying issue itself. The suggested replacement in #30657 (review) may be a good try.

fjahr commented at 4:20 PM on September 15, 2024:

I guess fs::is_empty(opts.blocks_dir) could be replaced by not fs::exists("blk0000.dat")?

I think that alone wouldn't work if the user upgrades a pruned node to v28?

Anyway, I was going to revisit this today but I simply don't see the failures anymore currently 🤷‍♂️ I will get back to it if I see them occur again.

ismaelsadeeq approved

ismaelsadeeq commented at 1:10 PM on August 15, 2024: member

Tested ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc

glozow commented at 8:56 AM on August 16, 2024: member

ACK faa1b9b0e6d

glozow merged this on Aug 16, 2024

glozow closed this on Aug 16, 2024

theStack deleted the branch on Aug 20, 2024

achow101 referenced this in commit d50f0ce248 on Aug 26, 2024

bitcoin locked this on Sep 15, 2025