When writing some scripts that iterated over many blocks to generate hashrate estimates I realized that my script was going out of range of the current chain tip height but was not encountering any errors.
I believe that passing an invalid block height to this function but receiving the hashrate estimate for the chain tip instead should be considered unexpected behavior.
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.
| Type | Reviewers |
|---|---|
| ACK | Sjors, kevkevinpal, achow101 |
| Concept ACK | luke-jr |
| Stale ACK | maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
447@@ -448,6 +448,11 @@ def _test_getnetworkhashps(self):
448 """).strip(),
449 lambda: self.nodes[0].getnetworkhashps("a", []),
450 )
451+ assert_raises_rpc_error(
452+ -8,
453+ textwrap.dedent("Block does not exist at specified height").strip(),
454+ lambda: self.nodes[0].getnetworkhashps(100, 99999999),
99999999 we can use self.nodes[0].getblockcount() + 1 which might be better
58@@ -59,6 +59,10 @@ static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_ch
59 pb = active_chain[height];
60 }
61
62+ if (height > active_chain.Height()) {
nit: Could move this early return earlier?
Maybe also remove the && height < active_chain.Height() check as well, then?
447@@ -448,6 +448,11 @@ def _test_getnetworkhashps(self):
448 """).strip(),
449 lambda: self.nodes[0].getnetworkhashps("a", []),
450 )
451+ assert_raises_rpc_error(
452+ -8,
453+ textwrap.dedent("Block does not exist at specified height").strip(),
0 "Block does not exist at specified height",
nit: Should be identical?
utACK
nit: Remove pb == nullptr check below, as it is no longer possible.
74 // If lookup is -1, then use blocks since last difficulty change.
75- if (lookup <= 0)
76+ if (lookup == -1)
77 lookup = pb->nHeight % Params().GetConsensus().DifficultyAdjustmentInterval() + 1;
78
79 // If lookup is larger than chain, then set it to chain length.
CHECK_NONFATAL(pb) if you remove the nullptr check. Also, does this change the result for the genesis block, because you skip !pb->nHeight?
If pb->nHeight == 0 then lookup should get set to 0, then minTime and maxTime will both be the genesis block time, thus it will return 0.
If removing the nullptr check doesn’t actually simplify the logic then let’s add it back.
52@@ -53,17 +53,22 @@ using node::UpdateTime;
53 * If 'height' is nonnegative, compute the estimate at the time when a given block was found.
54 */
55 static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_chain) {
56+ if (height > active_chain.Height()) {
57+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Block does not exist at specified height");
58+ }
59+
60+ if (lookup < -1 || lookup == 0) {
height, no?
105@@ -97,7 +106,7 @@ static RPCHelpMan getnetworkhashps()
106 "Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n"
107 "Pass in [height] to estimate the network speed at the time when a certain block was found.\n",
108 {
109- {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of blocks, or -1 for blocks since last difficulty change."},
110+ {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of trailing blocks, or -1 for blocks since last difficulty change."},
0 {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of previous blocks to calculate estimate from, or -1 for blocks since last difficulty change."},
Not sure what’s going on with the windows build; it failed on a wallet address test with a socket timeout. Seems unrelated to my PR.
Looks like it’s a known issue. https://github.com/bitcoin/bitcoin/pull/28509
Thanks for rebasing.
re-utACK 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658
