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/
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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
0 node0 2024-05-30T02:04:26.659353Z [httpworker.0] [rpc/request.cpp:222] [parse] [rpc] ThreadRPCServer method=listsinceblock user=__cookie__
1 test 2024-05-30T02:04:26.660000Z TestFramework (ERROR): Assertion failed
2 Traceback (most recent call last):
3 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
4 self.run_test()
5 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_listsinceblock.py", line 45, in run_test
6 self.test_cant_read_block()
7 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
8 assert_raises_rpc_error(-32603, "Can't read block from disk",
9 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
10 assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
11 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 AssertionError: No exception raised
13 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)
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")
# 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:
0 (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
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
.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?
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")
# Restoring block file.
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')
'Test the RPC error "Can't read block from disk"'
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?
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.
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.
0>>> from pathlib import Path
1>>> # Make a Path and rename it to add `".copy"` on the end:
2>>> p = Path("README.md")
3>>> p.exists()
4True
5>>> q = p.rename("README.md.copy") # capture the return value in `q`
6>>>
7>>> # We still have the Path instance called `p`...
8>>> p
9PosixPath('README.md')
10>>> # ...but it doesn't exist anymore (becuase we just renamed it)
11>>> p.exists()
12False
13>>> # `.rename` returned a new Path instance, which *does* exist
14>>> q
15PosixPath('README.md.copy')
16>>> q.exists()
17True
18>>>
19>>> # Just to prove the rename happened:
20>>> [item.name for item in Path(".").iterdir() if "md" in item.name]
21['README.md.copy']
22>>>
23>>> # Rename again to move it back to the original name...
24>>> r = q.rename("README.md")
25>>> r
26PosixPath('README.md')
27>>> r.exists()
28True
29>>> # Because `p` and `r` now have the same name, the original `p` also 'exists' again:
30>>> p.exists(), q.exists(), r.exists()
31(True, False, True)
So:
0 # Renaming the block file to induce unsuccessful block read
1 blk_dat = (self.nodes[0].blocks_path / "blk00000.dat")
2 assert blk_dat.exists()
3 blk_dat_moved = blk_dat.rename("blk00000.dat.moved")
4 assert not blk_dat.exists()
5
6 # listsinceblock(nodes1_last_blockhash) should now fail as blocks are not accessible
7 assert_raises_rpc_error(-32603, "Can't read block from disk",
8 self.nodes[0].listsinceblock, nodes1_last_blockhash)
9
10 # Restoring block file
11 blk_dat_moved.rename("blk00000.dat")
12 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)
blk_dat_moved.rename(blk_dat.name) to avoid repeating the literal "blk00000.dat" but meh)
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.”
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")
blk_dat_move.exists() before performing the blk_dat_moved.rename().
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.
0 # Restoring block file
1 blk_dat_moved.unlink()
2 assert not blk_dat_moved.exists()
3 blk_dat_moved.rename(self.nodes[0].blocks_path / "blk00000.dat")
4 assert blk_dat.exists()
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]
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
0Traceback (most recent call last):
1 File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
2 self.run_test()
3 File "/home/kevkevin/DEVDIR/bitcoin/./test/functional/wallet_listsinceblock.py", line 43, in run_test
4 self.test_cant_read_block()
5 File "/home/kevkevin/DEVDIR/bitcoin/./test/functional/wallet_listsinceblock.py", line 185, in test_cant_read_block
6 blk_dat_moved = blk_dat.rename(self.nodes[0].blocks_path / "blk00000.dat.moved")
7 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
8 File "/usr/lib64/python3.11/pathlib.py", line 1175, in rename
9 os.rename(self, target)
10FileNotFoundError: [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?
nodes1_last_blockhash is the tip of the 6 block branch which is not in the main chain.