Rationale: When bitcoin-cli help verifychain is called, the user doesn't get any documentation about the checklevel argument, leading to issues like #18995.
This PR addresses that issue and adds documentation for what each level does, and that each level includes the checks of the previous levels.
Concept ACK. Though I think it would be fine to refer to the -checklevel command line documentation instead of duplicating the text here (which has a chance of becoming stale if the meanings change).
It would also be possible to make a list of the docs static const std::vector<std::string> FOO{"level 1verifies block validity", "bar"}; and then call Join(FOO, ", ") in the command line help and Join(FOO, ".\n") in the RPC help.
Now using a list of the docs as suggested.
7 | @@ -8,8 +8,11 @@ 8 | 9 | #include <memory> 10 | #include <string> 11 | +#include <vector> 12 | #include <util/system.h> 13 | 14 | +static const std::vector<std::string> CHECKLEVEL_DOC{"level 0 reads the blocks from disk", "level 1 verifies block validity", "level 2 verifies undo data", "level 3 checks disconnection of tip blocks", "level 4 tries to reconnect the blocks", "each level includes the checks of the previous levels"};
I think it would be slightly easier to read if each was on their own line (like before).
static const std::vector<std::string> CHECKLEVEL_DOC{
"level 0 reads the blocks from disk",
...
"each level includes the checks of the previous levels",
};
Also, a better place to put this would be in validation.h (just the declaration) and then define it in validation.cpp.
init.h seem an odd place to put RPC documentation.
I've changed it so that each is in their own line but wasn't quite sure how to just declare in the .h and define in .cpp without the compiler telling me I was re-declaring it.
At the moment CHECKLEVEL_DOC is declared and defined in validation.h
Almost-ACK
83 | @@ -83,6 +84,15 @@ static const int DEFAULT_STOPATHEIGHT = 0; 84 | static const unsigned int MIN_BLOCKS_TO_KEEP = 288; 85 | static const signed int DEFAULT_CHECKBLOCKS = 6; 86 | static const unsigned int DEFAULT_CHECKLEVEL = 3; 87 | +static const std::vector<std::string> CHECKLEVEL_DOC = {
extern const std::vector<std::string> CHECKLEVEL_DOC;
In the header (for expensive types, not integers) this should just be the declaration. The rest should go to the cpp file.
Ah ok I should use extern
88 | + "level 0 reads the blocks from disk", 89 | + "level 1 verifies block validity", 90 | + "level 2 verifies undo data", 91 | + "level 3 checks disconnection of tip blocks", 92 | + "level 4 tries to reconnect the blocks", 93 | + "each level includes the checks of the previous levels"
"each level includes the checks of the previous levels",
Nit: Add a trailing comma for consistency and to make clang-format happy?
Addressed
ACK, but the strings could be moved to the cpp file
Rebased and the changes requested were applied.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
rebased
81 | + "level 0 reads the blocks from disk", 82 | + "level 1 verifies block validity", 83 | + "level 2 verifies undo data", 84 | + "level 3 checks disconnection of tip blocks", 85 | + "level 4 tries to reconnect the blocks", 86 | + "each level includes the checks of the previous levels,"
Should there be a comma at the end of this documentation? Perhaps: s/,"/",/
Fixed
76 | @@ -77,6 +77,14 @@ static constexpr std::chrono::hours DATABASE_WRITE_INTERVAL{1}; 77 | static constexpr std::chrono::hours DATABASE_FLUSH_INTERVAL{24}; 78 | /** Maximum age of our tip for us to be considered current for fee estimation */ 79 | static constexpr std::chrono::hours MAX_FEE_ESTIMATION_TIP_AGE{3}; 80 | +extern const std::vector<std::string> CHECKLEVEL_DOC = {
No need for extern here in the .cpp file, extern in the .h file should be enough, if I'm not mistaken.
const std::vector<std::string> CHECKLEVEL_DOC{
nit: no need for the =
addressed
removed
Is validation.h/cpp the best place to put CHECKLEVEL_DOC?
Is validation.h/cpp the best place to put CHECKLEVEL_DOC?
checklevel is a validation setting, so where else would you put the documentation?
150 | @@ -150,6 +151,8 @@ extern bool fPruneMode; 151 | /** Number of MiB of block files that we're trying to stay below. */ 152 | extern uint64_t nPruneTarget; 153 | 154 | +extern const std::vector<std::string> CHECKLEVEL_DOC;
A doxygen comment at line 153 might be nice.
Doxygen comment added
Is validation.h/cpp the best place to put CHECKLEVEL_DOC?
checklevel is a validation setting, so where else would you put the documentation?
Yup, bad grep on my part.
ACK after fixing the nits
1094 | @@ -1095,7 +1095,8 @@ static UniValue verifychain(const JSONRPCRequest& request) 1095 | RPCHelpMan{"verifychain", 1096 | "\nVerifies blockchain database.\n", 1097 | { 1098 | - {"checklevel", RPCArg::Type::NUM, /* default */ strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL), "How thorough the block verification is."}, 1099 | + {"checklevel", RPCArg::Type::NUM, /* default */ strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL), 1100 | + strprintf("How thorough the block verification is:\n %s", Join(CHECKLEVEL_DOC, ".\n"))},
suggestion:
- strprintf("How thorough the block verification is:\n %s", Join(CHECKLEVEL_DOC, ".\n"))},
+ strprintf("How thorough the block verification is:\n - %s", Join(CHECKLEVEL_DOC, "\n- "))},
which would improve the output from
1. checklevel (numeric, optional, default=3, range=0-4) How thorough the block verification is:
level 0 reads the blocks from disk.
level 1 verifies block validity.
level 2 verifies undo data.
level 3 checks disconnection of tip blocks.
level 4 tries to reconnect the blocks.
each level includes the checks of the previous levels
2. nblocks (numeric, optional, default=6, 0=all) The number of blocks to check.
to
1. checklevel (numeric, optional, default=3, range=0-4) How thorough the block verification is:
- level 0 reads the blocks from disk
- level 1 verifies block validity
- level 2 verifies undo data
- level 3 checks disconnection of tip blocks
- level 4 tries to reconnect the blocks
- each level includes the checks of the previous levels
2. nblocks (numeric, optional, default=6, 0=all) The number of blocks to check.
Change applied :)
Thanks for fixing this issue.
ACK 292ed3c3810c4ca613c3edc5cb711c9843628fcc modulo suggestion below, happy to re-ACK if you take the suggestion. Verified the bitcoind checklevel help is unchanged and the RPC verifychain help is augmented.
$ bitcoind -regtest -help-debug | grep -A6 checklevel
-checklevel=<n>
How thorough the block verification of -checkblocks is: level 0 reads
the blocks from disk, level 1 verifies block validity, level 2
verifies undo data, level 3 checks disconnection of tip blocks,
level 4 tries to reconnect the blocks, each level includes the
checks of the previous levels (0-4, default: 3)
ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b git diff 292ed3c 501e6ab shows only change since last review is the verifychain RPCHelpMan edit; rebuild and retested manually anyway
ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b 🚝
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b 🚝
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjXMgv+L0HtUWgGV37oAs6lbrPfM8FnucZRCR88E9knckuvVA/uBsRso96E7b68
jxmnCi7Ay7EGYeFVj3PCO/NvW1xMcwuU0GOQkCnfaPVGwDPZW4bJzxJz3fmIXJFL
aoPy8C7erguDYzG5c8Y1Hx7LepCzaqmV/+0oQhp6AthIcRj2L/TKefXmj+RnJj4q
WhfZTc6228eDi4ymZpjsJET/veYHllV7NoIQFF8lL7YHO6jvEleFRA4iWGAMy+SB
7TKmqL8G7+Ye9WJPHobmaAgaBc+wuerwvYI7rw/kcBjm/YfmrSg+2s+kdYzDei4A
xr1XZR+vLKYlEZrnRShpagAkIUkiS4ovlhk64YSSm5CKxum/lKp0e5rJW0n+ClVg
4eJ84l1wS2Wn+/F2f3yq+5mKZYTNl5lS6CxEUu/a0UhE3iw102YbY9P9AN0pk6iL
n4yQoVITjwsh9DGoUkymbhpWjyBWaYkgJ08oODWRXBxwtQERoGxqDZXz1P7Y6CKq
p+a2uevd
=ae1b
-----END PGP SIGNATURE-----
Timestamp of file with hash 401273e51463f483d4eab16695a4f04b81f42ac918c61babfed48abdc8e8baa6 -
</details>
Posthumous ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b Thanks for factoring the documentatin out.