This probably should’ve been included in #12653 when -blocksdir was introduced. Credit TheCharlatan for noticing that it’s missing.
This guards against 2 processes running with separate datadirs but the same blocksdir. I didn’t add walletdir as I assume sqlite has us covered there.
It’s not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31674.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3, maflcko |
| Concept ACK | davidgumberg, hebasto, BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
A subsequent commit will add a .lock file to this dir at startup, meaning that
the blocksdir is never empty by the time the xor key is being read/written.
Ignore all hidden files when determining if this is the first run.
No functional change. This is in preparation for adding additional directory
locks on startup.
This guards against 2 processes running with separate datadirs but the same
blocksdir.
It's not likely to happen currently, but may be more relevant in the future
with applications using the kernel.
Note that the kernel does not currently do any dir locking, but it should.
1147- // Only use random fresh key when the boolean option is set and on the
1148- // very first start of the program.
1149- FastRandomContext{}.fillrand(xor_key);
1150- }
1151-
1152+ const fs::path xor_feature_path{opts.blocks_dir / ".feature_xor"};
.) as an alternative.
Ideally the xor key would be written at the time of blocksdir creation. But I had a go at that and it means that non-bitcoind (kernel, tests, etc) binaries wouldn’t get a key written.
So yeah, filtering it out here is probably the most straightforward. Blah.
1152+ const std::string path = fs::PathToString(entry.path().filename());
1153+ if (!entry.is_regular_file() || !path.starts_with('.')) {
1154+ first_run = false;
1155+ break;
1156+ }
1157+ }
style nit: Could use ranges, if you feel fancy :sweat_smile:
0bool first_run = std::ranges::all_of(
1 fs::directory_iterator(opts.blocks_dir),
2 [](const auto& entry) {
3 const std::string path = fs::PathToString(entry.path().filename());
4 return entry.is_regular_file() && path.starts_with('.');
5 }
6);
1085@@ -1086,7 +1086,8 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
1086 }
1087 static bool LockDirectories(bool probeOnly)
1088 {
1089- return LockDirectory(gArgs.GetDataDirNet(), probeOnly);
1090+ return LockDirectory(gArgs.GetDataDirNet(), probeOnly) && \
\?
36 self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)
37
38- self.log.info("Check that cookie and PID file are not deleted when attempting to start a second bitcoind using the same datadir")
39+ self.log.info("Check that we can't start a second bitcoind instance using the same blocksdir")
40+ expected_msg = f"Error: Cannot obtain a lock on directory {blocksdir}. {self.config['environment']['CLIENT_NAME']} is probably already running."
41+ self.nodes[1].assert_start_raises_init_error(extra_args=[f'-blocksdir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)
blocksdir takes is actually the profiledir which contains the blocks dir, not the blocks dir itself. So passing blocksdir=datadir here means “use my own datadir but node0’s blocksdir”, which is what this test intends to cover. Unless I’m missing something?
lgtm, but I left a question in the test commit and didn’t check it further.
review ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🏼
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🏼
38rJpj3CDdDpna05UotXDyfLiqqhCsEUjIzBw/yCLmpTSLtB4f5whzkBtAgcYsMdq7aROp1S/UHZ45R1B4v6ODQ==
This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
Once you figure that out, you can use a previous release to demonstrate that this works. See e.g. mempool_compatibility.py.
This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
Once you figure that out, you can use a previous release to demonstrate that this works. See e.g.
mempool_compatibility.py.
The compatibility problem was actually caught by the feature_unsupported_utxo_db previous release check. So passing here indicates that we’re good with old blockstores (and thankfully that we have a working check for that :)
It’s not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
I think if we want directory locking in the kernel library, we should do that here before we introduce and ossify new assumptions on when exactly the lock is taken. As an alternative, how about taking the lock in the BlockManager constructor, like done here https://github.com/TheCharlatan/bitcoin/tree/lock_blocksdir_raii.
Code review and light test ACK 2656a5658c14b43c32959db7235e9db55a17d4c8
Nice update. Increasing safety for blocks dir before concurrency issues arise makes sense.
Sanity checked the migration from .lockless blocks dir on signet. New .lock was created without issue.
No change since my last review. I just mis-read the test commit.
re-ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🐓
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🐓
3z/AuyZTvEwZ4aF6WePeTTDgAPquZT5sd1uLNwLP1zxYViAM+8hrg/lW1cznwZ8JdkBvGYwzk6AqgFGWTzKDfBA==