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

theStack · 2024-08-14T15:34:52Z

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.

DrahtBot · 2024-08-14T15:34:55Z

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.

TheCharlatan · 2024-08-14T15:53:36Z

Concept ACK

tdb3

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

tdb3 · 2024-08-15T00:09:03Z

test/functional/test_framework/util.py

@@ -311,6 +311,13 @@ def sha256sum_file(filename):
    return h.digest()


+def util_xor(data, key, *, offset):


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

tdb3 · 2024-08-15T01:14:10Z

test/functional/feature_blocksxor.py

+        self.start_node(0, extra_args=['-blocksxor=0'])
+        # checklevel=2 -> verify block validity + undo data
+        # nblocks=0    -> verify all blocks
+        node.verifychain(checklevel=2, nblocks=0)


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

maflcko

ACK faa1b9b

maflcko · 2024-08-15T05:49:13Z

test/functional/test_framework/util.py

@@ -508,6 +515,12 @@ def check_node_connections(*, node, num_in, num_out):
    assert_equal(info["connections_out"], num_out)


+def read_xor_key(*, node):


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?

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.

glozow · 2024-08-15T11:48:19Z

test/functional/feature_blocksxor.py

+        self.log.info("Check that restarting with 'blocksxor=0' fails if XOR key is present")
+        node.assert_start_raises_init_error(['-blocksxor=0'],
+            'The blocksdir XOR-key can not be disabled when a random key was already stored!',
+            match=ErrorMatch.PARTIAL_REGEX)


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?

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

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

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)?

logs_30657_2.txt

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

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

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

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.

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: fjahr@ef00b8e 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.

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 (comment) may be a good try.

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

Tested ACK faa1b9b

glozow · 2024-08-16T08:56:29Z

ACK faa1b9b

e1d5dd7 test: check xor.dat recreated when missing (tdb3) d161096 test: add null block xor key (tdb3) 1ad999b refactor: lift NUM_XOR_BYTES (tdb3) d839958 refactor: move read_xor_key() to TestNode (tdb3) d43948c refactor: use unlink rather than os.remove (tdb3) c8176f7 test: add blocks_key_path (tdb3) Pull request description: Builds on PR #30657. Refactors `read_xor_key()` from `util.py` to `test_node.py` (comment #30657 (comment)) Adds a check that `xor.dat` is created when missing (comment #30657 (comment)) Help states: ``` -blocksxor Whether an XOR-key applies to blocksdir *.dat files. The created XOR-key will be zeros for an existing blocksdir or when `-blocksxor=0` is set, and random for a freshly initialized blocksdir. (default: 1) ``` ACKs for top commit: maflcko: ACK e1d5dd7 achow101: ACK e1d5dd7 theStack: re-ACK e1d5dd7 brunoerg: reACK e1d5dd7 Tree-SHA512: 325912ef646ec88e0a58e1ece263a2b04cbc06497e8fe5fcd603e509e80c6bcf84b09dd52dfac60e23013f07fc2b2f6db851ed0598649c3593f452c4a1424bd9

test: refactor: move read_xor_key/util_xor helpers to util module

6b3676b

DrahtBot added the Tests label Aug 14, 2024

theStack changed the title ~~test: add functional test for XORed block/undo files (-blockxor option)~~ test: add functional test for XORed block/undo files (-blocksxor option) Aug 14, 2024

theStack force-pushed the 202408-add_blocksxor_test branch from 86b80ce to fb2ea1e Compare August 14, 2024 15:35

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

faa1b9b

theStack force-pushed the 202408-add_blocksxor_test branch from fb2ea1e to faa1b9b Compare August 14, 2024 15:38

tdb3 reviewed Aug 15, 2024

View reviewed changes

maflcko approved these changes Aug 15, 2024

View reviewed changes

DrahtBot requested review from tdb3 and TheCharlatan August 15, 2024 05:53

maflcko added this to the 28.0 milestone Aug 15, 2024

glozow reviewed Aug 15, 2024

View reviewed changes

ismaelsadeeq approved these changes Aug 15, 2024

View reviewed changes

glozow merged commit 6474132 into bitcoin:master Aug 16, 2024
16 checks passed

tdb3 mentioned this pull request Aug 17, 2024

test: XORed blocks test follow up #30669

Merged

theStack deleted the 202408-add_blocksxor_test branch August 20, 2024 13:28

		@@ -311,6 +311,13 @@ def sha256sum_file(filename):
		return h.digest()


		def util_xor(data, key, *, offset):

		@@ -508,6 +515,12 @@ def check_node_connections(*, node, num_in, num_out):
		assert_equal(info["connections_out"], num_out)


		def read_xor_key(*, node):

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

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

Uh oh!

Conversation

theStack commented Aug 14, 2024

Uh oh!

DrahtBot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

TheCharlatan commented Aug 14, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Aug 16, 2024

Uh oh!

Uh oh!

Uh oh!

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

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

DrahtBot commented Aug 14, 2024 •

edited

Loading

maflcko Aug 15, 2024 •

edited

Loading

glozow Aug 15, 2024 •

edited

Loading

glozow Aug 16, 2024 •

edited

Loading