Merge bitcoin#28554: bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC

9ac114e Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)

Pull request description:

  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.

ACKs for top commit:
  Sjors:
    re-utACK 9ac114e
  kevkevinpal:
    reACK [9ac114e](bitcoin@9ac114e)
  achow101:
    ACK 9ac114e

Tree-SHA512: eefb465c2dd654fc48267f444e1809597ec5363cdd131ea9ec812458fed1e4bffbbbb0617d74687c9f7bb16274b598d8292f5eeb7953421e5d2a8dc2cc081f2b

Author: achow101

src/rpc/mining.cpp

@@ -49,21 +49,30 @@ using node::UpdateTime;
 
 /**
  * Return average network hashes per second based on the last 'lookup' blocks,
- * or from the last difficulty change if 'lookup' is nonpositive.
- * If 'height' is nonnegative, compute the estimate at the time when a given block was found.
+ * or from the last difficulty change if 'lookup' is -1.
+ * If 'height' is -1, compute the estimate from current chain tip.
+ * If 'height' is a valid block height, compute the estimate at the time when a given block was found.
  */
 static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_chain) {
+    if (lookup < -1 || lookup == 0) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid nblocks. Must be a positive number or -1.");
+    }
+
+    if (height < -1 || height > active_chain.Height()) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block does not exist at specified height");
+    }
+
     const CBlockIndex* pb = active_chain.Tip();
 
-    if (height >= 0 && height < active_chain.Height()) {
+    if (height >= 0) {
         pb = active_chain[height];
     }
 
     if (pb == nullptr || !pb->nHeight)
         return 0;
 
     // If lookup is -1, then use blocks since last difficulty change.
-    if (lookup <= 0)
+    if (lookup == -1)
         lookup = pb->nHeight % Params().GetConsensus().DifficultyAdjustmentInterval() + 1;
 
     // If lookup is larger than chain, then set it to chain length.
@@ -97,7 +106,7 @@ static RPCHelpMan getnetworkhashps()
                 "Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n"
                 "Pass in [height] to estimate the network speed at the time when a certain block was found.\n",
                 {
-                    {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of blocks, or -1 for blocks since last difficulty change."},
+                    {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of previous blocks to calculate estimate from, or -1 for blocks since last difficulty change."},
                     {"height", RPCArg::Type::NUM, RPCArg::Default{-1}, "To estimate at the time of the given height."},
                 },
                 RPCResult{

test/functional/rpc_blockchain.py

@@ -437,7 +437,6 @@ def _test_getdifficulty(self):
 
     def _test_getnetworkhashps(self):
         self.log.info("Test getnetworkhashps")
-        hashes_per_second = self.nodes[0].getnetworkhashps()
         assert_raises_rpc_error(
             -3,
             textwrap.dedent("""
@@ -449,17 +448,46 @@ def _test_getnetworkhashps(self):
             """).strip(),
             lambda: self.nodes[0].getnetworkhashps("a", []),
         )
+        assert_raises_rpc_error(
+            -8,
+            "Block does not exist at specified height",
+            lambda: self.nodes[0].getnetworkhashps(100, self.nodes[0].getblockcount() + 1),
+        )
+        assert_raises_rpc_error(
+            -8,
+            "Block does not exist at specified height",
+            lambda: self.nodes[0].getnetworkhashps(100, -10),
+        )
+        assert_raises_rpc_error(
+            -8,
+            "Invalid nblocks. Must be a positive number or -1.",
+            lambda: self.nodes[0].getnetworkhashps(-100),
+        )
+        assert_raises_rpc_error(
+            -8,
+            "Invalid nblocks. Must be a positive number or -1.",
+            lambda: self.nodes[0].getnetworkhashps(0),
+        )
+
+        # Genesis block height estimate should return 0
+        hashes_per_second = self.nodes[0].getnetworkhashps(100, 0)
+        assert_equal(hashes_per_second, 0)
+
         # This should be 2 hashes every 10 minutes or 1/300
+        hashes_per_second = self.nodes[0].getnetworkhashps()
         assert abs(hashes_per_second * 300 - 1) < 0.0001
 
-        # Test setting the first param of getnetworkhashps to negative value returns the average network
+        # Test setting the first param of getnetworkhashps to -1 returns the average network
         # hashes per second from the last difficulty change.
         current_block_height = self.nodes[0].getmininginfo()['blocks']
         blocks_since_last_diff_change = current_block_height % DIFFICULTY_ADJUSTMENT_INTERVAL + 1
         expected_hashes_per_second_since_diff_change = self.nodes[0].getnetworkhashps(blocks_since_last_diff_change)
 
         assert_equal(self.nodes[0].getnetworkhashps(-1), expected_hashes_per_second_since_diff_change)
-        assert_equal(self.nodes[0].getnetworkhashps(-2), expected_hashes_per_second_since_diff_change)
+
+        # Ensure long lookups get truncated to chain length
+        hashes_per_second = self.nodes[0].getnetworkhashps(self.nodes[0].getblockcount() + 1000)
+        assert hashes_per_second > 0.003
 
     def _test_stopatheight(self):
         self.log.info("Test stopping at height")