test: add coverage to rpc_blockchain.py #27852

kevkevinpal commented at 9:05 PM on June 10, 2023: contributor

Included a test that checks the functionality of setting the first param of getnetworkhashps to negative value returns the average network hashes per second from the last difficulty change.

DrahtBot commented at 9:05 PM on June 10, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, pablomartin4btc, jlopp, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28554 (bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC by jlopp)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jun 10, 2023

fanquake requested review from theStack on Aug 3, 2023

in test/functional/rpc_blockchain.py:441 in dc190c95fc outdated

 437 | @@ -438,6 +438,7 @@ def _test_getdifficulty(self):
 438 |      def _test_getnetworkhashps(self):
 439 |          self.log.info("Test getnetworkhashps")
 440 |          hashes_per_second = self.nodes[0].getnetworkhashps()
 441 | +        hashes_per_second_since_diff_adjustment = self.nodes[0].getnetworkhashps(-1, -1)

pablomartin4btc commented at 4:11 AM on September 21, 2023:

The second argument of getnetworkhashps (height) has already {-1} as default so you don't need to pass it I believe.

        hashes_per_second_since_diff_adjustment = self.nodes[0].getnetworkhashps(-1)

kevkevinpal commented at 5:02 AM on September 25, 2023:

updated with the cherry-picked commit 17e505a

pablomartin4btc commented at 4:52 AM on September 21, 2023: member

tACK dc190c95fcb716a92b7c4fbcc10f30445a2689fb

lgtm

in test/functional/rpc_blockchain.py:455 in dc190c95fc outdated

 451 | @@ -451,6 +452,7 @@ def _test_getnetworkhashps(self):
 452 |          )
 453 |          # This should be 2 hashes every 10 minutes or 1/300
 454 |          assert abs(hashes_per_second * 300 - 1) < 0.0001
 455 | +        assert abs(hashes_per_second_since_diff_adjustment * 300 - 1) < 0.006

ismaelsadeeq commented at 1:18 PM on September 21, 2023:

Even though there is an assumption that the blocks in node[0] will be 200 in rpc_blockchain.py, I am not a fan of the hardcoded 0.006.

Since you are testing that whenever the first parameter of getnetworkhashps is negative, it will return the average hashes per second since the last difficulty change, why not calculate the number of blocks since the last difficulty change, call the getnetworkhashps with that, and then check against the getnetworkhashps rpc call with a negative value? This approach is better than the hardcoded 0.006. Also, if the assumption of 200 blocks changes, you would not need to update the 0.006 value. Also in the below diff, I added @pablomartin4btc comment https://github.com/bitcoin/bitcoin/pull/27852/commits/dc190c95fcb716a92b7c4fbcc10f30445a2689fb#r1332416752

diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index 8ae1e661d4..45e5cff8cd 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -58,6 +58,7 @@ TIME_RANGE_STEP = 600  # ten-minute steps
 TIME_RANGE_MTP = TIME_GENESIS_BLOCK + (HEIGHT - 6) * TIME_RANGE_STEP
 TIME_RANGE_TIP = TIME_GENESIS_BLOCK + (HEIGHT - 1) * TIME_RANGE_STEP
 TIME_RANGE_END = TIME_GENESIS_BLOCK + HEIGHT * TIME_RANGE_STEP
+DIFFICULTY_ADJUSTMENT_INTERVAL = 2016
 
 
 class BlockchainTest(BitcoinTestFramework):
@@ -437,7 +438,6 @@ class BlockchainTest(BitcoinTestFramework):
     def _test_getnetworkhashps(self):
         self.log.info("Test getnetworkhashps")
         hashes_per_second = self.nodes[0].getnetworkhashps()
-        hashes_per_second_since_diff_adjustment = self.nodes[0].getnetworkhashps(-1, -1)
         assert_raises_rpc_error(
             -3,
             textwrap.dedent("""
@@ -451,7 +451,15 @@ class BlockchainTest(BitcoinTestFramework):
         )
         # This should be 2 hashes every 10 minutes or 1/300
         assert abs(hashes_per_second * 300 - 1) < 0.0001
-        assert abs(hashes_per_second_since_diff_adjustment * 300 - 1) < 0.006
+
+        # Test setting the first param of getnetworkhashps to negative value 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)
 
     def _test_stopatheight(self):
         self.log.info("Test stopping at height")

</details>

feel free to cherry-pick https://github.com/bitcoin/bitcoin/commit/17e505abfe1557a8e5c1819c9d09d27abece25b9

pablomartin4btc commented at 6:29 PM on September 21, 2023:

I do agree with this approach, avoids the hassle of updating the hardcoded value if it ever changes and adds more clarity of the context (blockchain setup in the test).

kevkevinpal commented at 5:02 AM on September 25, 2023:

updated and used the cherry-picked commit 17e505a thanks for the reviews!

in test/functional/rpc_blockchain.py:464 in dc190c95fc outdated

 451 | @@ -451,6 +452,7 @@ def _test_getnetworkhashps(self):
 452 |          )
 453 |          # This should be 2 hashes every 10 minutes or 1/300
 454 |          assert abs(hashes_per_second * 300 - 1) < 0.0001
 455 | +        assert abs(hashes_per_second_since_diff_adjustment * 300 - 1) < 0.006
 456 |  
 457 |      def _test_stopatheight(self):

ismaelsadeeq commented at 1:21 PM on September 21, 2023:

nit in commit and maybe PR title. Use the imperative mood in the subject line test: add coverage to rpc_blockchain.py see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

kevkevinpal commented at 5:01 AM on September 25, 2023:

updated the title and cherry-picked your commit 17e505a

ismaelsadeeq commented at 1:25 PM on September 21, 2023: member

Concept ACK On adding a test coverage for getnetworkhashps rpc. Left some comments about the approach

test: add coverage to rpc_blockchain.py

Included a test that checks the functionality of setting
the first param of getnetworkhashps to negative value returns
the average network hashes per second from the last difficulty change.

Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>

376dc2cfb3

kevkevinpal force-pushed on Sep 25, 2023

kevkevinpal renamed this:
~~test: added coverage to rpc_blockchain.py~~
test: add coverage to rpc_blockchain.py
on Sep 25, 2023

kevkevinpal requested review from ismaelsadeeq on Sep 25, 2023

kevkevinpal requested review from pablomartin4btc on Sep 25, 2023

ismaelsadeeq approved

ismaelsadeeq commented at 4:09 PM on September 25, 2023: member

Tested ACK 376dc2cfb32806a8aa450589effe4d384e648398

DrahtBot removed review request from pablomartin4btc on Sep 25, 2023

DrahtBot requested review from pablomartin4btc on Sep 25, 2023

pablomartin4btc approved

pablomartin4btc commented at 4:23 PM on September 25, 2023: member

tACK 376dc2cfb32806a8aa450589effe4d384e648398

jlopp commented at 4:22 PM on October 17, 2023: contributor

tACK https://github.com/bitcoin/bitcoin/commit/376dc2cfb32806a8aa450589effe4d384e648398

achow101 commented at 7:17 PM on November 2, 2023: member

ACK 376dc2cfb32806a8aa450589effe4d384e648398

achow101 merged this on Nov 2, 2023

achow101 closed this on Nov 2, 2023

bitcoin locked this on Nov 1, 2024