This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79
This is done by renaming the first block in the blocks folder
Doing a quick grep for the error code in our functional tests leads to zero results
grep -nri "Can't read block from disk" ./test/functional/
<!--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.
https://cirrus-ci.com/task/5783582369644544?logs=ci#L3033
node0 2024-05-30T02:04:26.659353Z [httpworker.0] [rpc/request.cpp:222] [parse] [rpc] ThreadRPCServer method=listsinceblock user=__cookie__
test 2024-05-30T02:04:26.660000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_listsinceblock.py", line 45, in run_test
self.test_cant_read_block()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_listsinceblock.py", line 192, in test_cant_read_block
assert_raises_rpc_error(-32603, "Can't read block from disk",
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 148, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: No exception raised
test 2024-05-30T02:04:26.664000Z TestFramework (DEBUG): Closing down network thread
185 | + self.generate(self.nodes[2], 7, sync_fun=lambda: self.sync_all(self.nodes[2:]))[0] 186 | + 187 | + self.join_network() 188 | + 189 | + # Setting permissions to none 190 | + os.chmod(self.nodes[0].blocks_path, stat.S_IWUSR)
Seems easier to rename the file than to change its permissions?
Yes that might make more sense, I've tried to do such in f278ac9
Lets see if it passes the ci, it was passing on my machine
183 | + self.generate(self.nodes[2], 7, sync_fun=lambda: self.sync_all(self.nodes[2:]))[0] 184 | + 185 | + self.join_network() 186 | + 187 | + # Setting permissions to none 188 | + os.rename(self.nodes[0].blocks_path / "blk00000.dat", self.nodes[0].blocks_path / "blk00000.dat.copy")
nit: The comment could be updated to match the current action, e.g. # Renaming the block file to induce unsuccessful block read.
nit:
This self.nodes[0].blocks_path / "blk..." / ... is a Path so you don't need to import os to call os.rename to rename it, you can instead:
(self.nodes[0].blocks_path / "blk00000.dat").rename("blk00000.dat.copy")
Most of the things you could do to files/directories with os can also be done with Path.
(The .rename is aware of which bit of the path is the file so you don't need to give it the self.nodes[0].blocks_path for the rename to work correctly)
thanks! added in 21bc4ff
thanks! added in 21bc4ff
One thing I did notice is that if I did not add the full path in .rename the second time I call .rename it cannot find the correct file and when I check the blocks dir manually it seemed like it never changed the file name. But oddly the rpc error is still seen
I might do some more investigation why I'm seeing that, but let me know if this looks okay now
Hrm, I can't check just now but I'd guess either .rename returns a new path instance that you should use for further changes instead, or you can try call .resolve() on it after rename? (Resolves to an absolute path, maybe needed if the original changed?)
https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename
The target path may be absolute or relative. Relative paths are interpreted relative to the current working directory, not the directory of the Path object.
Maybe this?
I've added #30195 (review) with a suggestion
190 | + # listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible 191 | + assert_raises_rpc_error(-32603, "Can't read block from disk", 192 | + self.nodes[0].listsinceblock, nodes1_last_blockhash) 193 | + 194 | + # ReSetting permissions to 600 195 | + os.rename(self.nodes[0].blocks_path / "blk00000.dat.copy", self.nodes[0].blocks_path / "blk00000.dat")
nit: The comment could be updated to match the current action, e.g. # Restoring block file.
thanks! added in 21bc4ff
168 | @@ -167,6 +169,31 @@ def test_reorg(self): 169 | found = next(tx for tx in transactions if tx['txid'] == senttx) 170 | assert_equal(found['blockheight'], self.nodes[0].getblockheader(nodes2_first_blockhash)['height']) 171 | 172 | + def test_cant_read_block(self): 173 | + self.log.info('Test passing cannot read block from disk rpc error')
nit: Could phrase this a little clearer:
'Test the RPC error "Can't read block from disk"'
thanks! added in 21bc4ff
174 | + 175 | + # Split network into two 176 | + self.split_network() 177 | + 178 | + # send to nodes[0] from nodes[2] 179 | + self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
Is this send necessary for this test? The test seems to work with or without it, and listsinceblock would return an empty array (rather than throw) if there were no transactions to show, right?
Maybe I'm missing something?
thanks! added in 21bc4ff
Approach ACK
Thanks for adding test coverage.
Exercised test's ability to fail by commenting out the rename calls (failed as expected with no exception being raised).
Built and ran all unit/functional tests. All passed.
Thanks for the updates. ACK for 21bc4ff147cdd0a2fd37097feffbab8edfc93320
186 | + # listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible 187 | + assert_raises_rpc_error(-32603, "Can't read block from disk", 188 | + self.nodes[0].listsinceblock, nodes1_last_blockhash) 189 | + 190 | + # Restoring block file 191 | + (self.nodes[0].blocks_path / "blk00000.dat.copy").rename(self.nodes[0].blocks_path / "blk00000.dat")
re. this comment #30195 (review) and needing to use the full path again, I think it is that rename(...) returns a new Path instance, which is then the 'correct' one to use going forwards for further changes you want to happen on-disk (alas Python has no 'use of moved value' concept like Rust et al. that could've pointed this out).
I think the way to think of this is that a Path is just a path, not a file-handle, so you can have a Path instance that doesn't exist, because for all Python knows you might be wanting to create it -- so .rename(...) on a Path that doesn't exist changes the path instance but of-course can't change the actual filesystem because it doesn't exist.
>>> from pathlib import Path
>>> # Make a Path and rename it to add `".copy"` on the end:
>>> p = Path("README.md")
>>> p.exists()
True
>>> q = p.rename("README.md.copy") # capture the return value in `q`
>>>
>>> # We still have the Path instance called `p`...
>>> p
PosixPath('README.md')
>>> # ...but it doesn't exist anymore (becuase we just renamed it)
>>> p.exists()
False
>>> # `.rename` returned a new Path instance, which *does* exist
>>> q
PosixPath('README.md.copy')
>>> q.exists()
True
>>>
>>> # Just to prove the rename happened:
>>> [item.name for item in Path(".").iterdir() if "md" in item.name]
['README.md.copy']
>>>
>>> # Rename again to move it back to the original name...
>>> r = q.rename("README.md")
>>> r
PosixPath('README.md')
>>> r.exists()
True
>>> # Because `p` and `r` now have the same name, the original `p` also 'exists' again:
>>> p.exists(), q.exists(), r.exists()
(True, False, True)
So:
# Renaming the block file to induce unsuccessful block read
blk_dat = (self.nodes[0].blocks_path / "blk00000.dat")
assert blk_dat.exists()
blk_dat_moved = blk_dat.rename("blk00000.dat.moved")
assert not blk_dat.exists()
# listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible
assert_raises_rpc_error(-32603, "Can't read block from disk",
self.nodes[0].listsinceblock, nodes1_last_blockhash)
# Restoring block file
blk_dat_moved.rename("blk00000.dat")
assert blk_dat.exists()
Also suggest naming change to "moved" or "backup" instead of "copy" as you're not copying?
And additional asserts to check that the files exist and don't when expected (maybe overkill, but prevents unexpected pass/fails for some other reason than you renaming the file)
(Could even blk_dat_moved.rename(blk_dat.name) to avoid repeating the literal "blk00000.dat" but meh)
The exists() asserts are a nice improvement/sanity check.
I was getting this error so I opted to use the full path
error message: OSError: [Errno 18] Invalid cross-device link: '/tmp/bitcoin_func_test_lt7jywkj/node0/regtest/blocks/blk00000.dat.moved' -> 'blk00000.dat'
but I like the assertions thanks for the suggestion! updated in 9bf646c
(Could even
blk_dat_moved.rename(blk_dat.name)to avoid repeating the literal"blk00000.dat"but meh)
No, this is wrong. https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename says "The target path may be absolute or relative. Relative paths are interpreted relative to the current working directory, not the directory of the Path object."
Ah, d'oh. Full path is fine then, sorry I should've checked with a Path not in my current working dir
189 | + # listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible 190 | + assert_raises_rpc_error(-32603, "Can't read block from disk", 191 | + self.nodes[0].listsinceblock, nodes1_last_blockhash) 192 | + 193 | + # Restoring block file 194 | + blk_dat_moved.rename(self.nodes[0].blocks_path / "blk00000.dat")
nit: could also add an assert (or combine into the one above) to check that blk_dat_move.exists() before performing the blk_dat_moved.rename().
I am not sure about adding the assertions that the src file exists. os.rename already does the assertion internally (and pathlib.Path.rename calls os.rename internally as well).
Yeah, looks like we'd get a FileNotFoundError (induced with the code below, blk_dat_moved.rename() throws).
An assert to check that blk_dat_moved.exists() before blk_dat_moved.rename() would be overkill, since the FileNotFoundError would cause test failure.
# Restoring block file
blk_dat_moved.unlink()
assert not blk_dat_moved.exists()
blk_dat_moved.rename(self.nodes[0].blocks_path / "blk00000.dat")
assert blk_dat.exists()
re ACK 9bf646c4190e55a13bf3a89d1705763c7a833a06
This change adds a test to add coverage to the rpc error that emmits the message "Can't
read block from disk"
173 | + 174 | + # Split network into two 175 | + self.split_network() 176 | + 177 | + # generate on both sides 178 | + nodes1_last_blockhash = self.generate(self.nodes[1], 6, sync_fun=lambda: self.sync_all(self.nodes[:2]))[-1]
Super Nit: node1_last_blockhash
removed an assertion that didn't seem needed in 881724d
The other two seem fine as they assert the file was replaced properly which .replace would not throw an error for
Error .replace throws if file is not there
Traceback (most recent call last):
File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/kevkevin/DEVDIR/bitcoin/./test/functional/wallet_listsinceblock.py", line 43, in run_test
self.test_cant_read_block()
File "/home/kevkevin/DEVDIR/bitcoin/./test/functional/wallet_listsinceblock.py", line 185, in test_cant_read_block
blk_dat_moved = blk_dat.rename(self.nodes[0].blocks_path / "blk00000.dat.moved")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/pathlib.py", line 1175, in rename
os.rename(self, target)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/bitcoin_func_test_uge402pj/node0/regtest/blocks/randomdir/blk00000.dat' -> '/tmp/bitcoin_func_test_uge402pj/node0/regtest/blocks/blk00000.dat.moved'
re ACK for 881724d443d11f984a721ef1edd5777c24d1ed29
re-ran test (passed) and temporarily moved up blk_dat_moved.rename() to induce failure with assert_raises_rpc_error (test failed as expected).
185 | + blk_dat_moved = blk_dat.rename(self.nodes[0].blocks_path / "blk00000.dat.moved") 186 | + assert not blk_dat.exists() 187 | + 188 | + # listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible 189 | + assert_raises_rpc_error(-32603, "Can't read block from disk", 190 | + self.nodes[0].listsinceblock, nodes1_last_blockhash)
I've been trying to understand how does calling listsinceblock nodes1_last_blockhash on node0 is causing the last condition to be true here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L663
The comment above this line talks about requesting a reorg'ed block, but isn't the last blockhash on node0 part of the main chain from POV of node0?
There is a reorg happening, nodes 0 and 1 mine 6 blocks on top of the common ancestor, while 2 and 3 mine 7 blocks. When the network is rejoined, the 7 block branch reorgs the 6 block branch. However, nodes1_last_blockhash is the tip of the 6 block branch which is not in the main chain.
ACK 881724d443d11f984a721ef1edd5777c24d1ed29