Builds on PR #30657.
Refactors read_xor_key() from util.py to test_node.py (comment #30657 (review))
Adds a check that xor.dat is created when missing (comment #30657 (review))
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)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See 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.
tACK 07365d026127eb8435e7919a7793bd25d7f3ceca
59 | @@ -59,6 +60,8 @@ def run_test(self): 60 | # checklevel=2 -> verify block validity + undo data 61 | # nblocks=0 -> verify all blocks 62 | node.verifychain(checklevel=2, nblocks=0) 63 | + self.log.info("Check that xor.dat is recreated") 64 | + assert read_xor_key(node=node) == NULL_BLK_XOR_KEY
assert_equal(read_xor_key(node=node), NULL_BLK_XOR_KEY)
Thanks for reviewing. Implemented
Thanks @danielabrozzoni and @brunoerg. Pushed an update to address comments, and clarified the log message.
390 | @@ -391,6 +391,8 @@ def rpc_url(datadir, i, chain, rpchost): 391 | # The size of the blocks xor key, 392 | # from InitBlockdirXorKey::xor_key.size() 393 | NUM_XOR_BYTES = 8 394 | +# The null block key (all 0s) 395 | +NULL_BLK_XOR_KEY = bytes(NUM_XOR_BYTES)
nit, personally I prefer to be explicit about the zeroes initialization, but no big deal
NULL_BLK_XOR_KEY = bytes([0] * NUM_XOR_BYTES)
lgtm, added.
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2
Thanks for following up! If you want, you could also tackle the suggestion to move the read_xor_key helper to the TestNode class, see #30657 (review)
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2
If you want, you could also tackle the suggestion to move the
read_xor_keyhelper to theTestNodeclass, see #30657 (comment)
Thanks for the suggestion! This PR seems to be a reasonable place to perform the move, so I updated it to include this. Happy to make tweaks if you or @maflcko have any further suggestions.
468 | @@ -469,6 +469,12 @@ def blocks_path(self) -> Path: 469 | def blocks_key_path(self) -> Path: 470 | return self.blocks_path / "xor.dat" 471 | 472 | + @property 473 | + def read_xor_key(self) -> bytes: 474 | + with open(self.blocks_key_path, "rb") as xor_f: 475 | + NUM_XOR_BYTES = 8 # From InitBlocksdirXorKey::xor_key.size() 476 | + return xor_f.read(NUM_XOR_BYTES)
Not sure if there are guidelines for this (and if yes, how strict we are), but I would tend to avoid using the "property" decorator for non-trivial methods that access state outside of the class (in this case, by interacting with the file system). At least I wouldn't expect as user that accessing a class field ever involves any I/O. I think dropping the property and keeping the name as-is "read_xor_key" might be just fine?
Updated
52 | @@ -54,7 +53,7 @@ def run_test(self): 53 | match=ErrorMatch.PARTIAL_REGEX) 54 | 55 | self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity") 56 | - os.remove(node.blocks_path / 'xor.dat') 57 | + os.remove(node.blocks_key_path)
nit: could put this already into the commit that introduces the blocks_key_path property to TestNode.
Yup, that is more coherent. Updated.
Adds a convenience function to TestNode
to provide the path to the blocks xor key.
Updates util and feature_blocksxor to use it.
42 | @@ -43,6 +43,9 @@ 43 | ) 44 | 45 | BITCOIND_PROC_WAIT_TIMEOUT = 60 46 | +# The size of the blocks xor key 47 | +# from InitBlockdirXorKey::xor_key.size()
an 's' got lost on the move up
# from InitBlocksdirXorKey::xor_key.size()
Thanks. Fixed.
ACK ab9d2451c47fd30abecaea3f8d8a6e33541911d7
re-ACK 1c403af92be13cbdfa6ab1906f8cc34fabc21e63
55 | @@ -54,11 +56,13 @@ def run_test(self): 56 | match=ErrorMatch.PARTIAL_REGEX) 57 | 58 | self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity") 59 | - os.remove(node.blocks_path / 'xor.dat') 60 | + os.remove(node.blocks_key_path)
style nit, while touching this line: The argument is of type Path, so I think you can just call unlink() on it to avoid the os import?
Thanks, that's cleaner. Added.
ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b
re-ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b
reACK e1d5dd732d5dc641faf1dde316275c84b6bb224b
ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b