Fixes a block of code which seems to be incorrectly performing two existence checks instead of catching and logging errors. fs::remove returns false only if the file being removed does not exist, so it is redundant with the fs::exists check. If an error does occur when trying to remove an existing file, fs::remove will throw. See https://en.cppreference.com/w/cpp/filesystem/remove.
Also see https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L326-L332 for a similar pattern.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | MarcoFalke, jamesob, achow101 |
| Concept ACK | brunoerg, luke-jr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Approach ACK, thanks for fixing this. Will give a thorough look in the next day or two.
4863 | - bool removed = fs::remove(base_blockhash_path); 4864 | - if (!removed) { 4865 | - LogPrintf("[snapshot] failed to remove file %s\n", 4866 | - fs::PathToString(base_blockhash_path)); 4867 | + try { 4868 | + bool existed = fs::remove(base_blockhash_path);
nit:
const bool existed{fs::remove(base_blockhash_path)};
Concept ACK
lgtm ACK 0e21b56a44d53cec9080edb04410a692717f1ddc
No idea what the link in OP should point to, but I presume it is https://github.com/bitcoin/bitcoin/blob/fff80cd2488899a322f9e673930a00eb9ab5b165/src/init.cpp#L326-L332
ACK https://github.com/bitcoin/bitcoin/pull/26828/commits/0e21b56a44d53cec9080edb04410a692717f1ddc
Built locally. Makes sense, thank you for the cleanup!
ACK 0e21b56a44d53cec9080edb04410a692717f1ddc