Merge #6479: feat: enable coverage linter for functional tests

2e509b96c4 fix: add a workaround for RPC getmerkleblocks, debug, coinjoinsalt, voteraw (Konstantin Akimov)
f0decc8790 feat: add unit test for ClearDiscouraged (Konstantin Akimov)
865b24ea00 feat: hide cleardiscouraged RPC so far as it no intent to use by regular users (Konstantin Akimov)
1f5fa7e7cf feat: enable linter coverage for functional tests (Konstantin Akimov)
59ddac5656 feat: hide deprecated RPC from help and add TODOes to remove them (Konstantin Akimov)
05732aceaf feat: implement functional tests for RPC getblockheaders (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash-issues/issues/63

  ## What was done?
  Add functional tests for `getblockheaders`
  Hide RPC `cleardiscouraged` (as it is used only for functional tests) and RPC `getpoolinfo` (deprecated long time ago)
  Add a workaround to ignore these RPCs `getmerkleblocks`, `voteraw`, `debug`, `coinjoinsalt` at the moment
  Enables linter for coverage

  ## How Has This Been Tested?
  Run locally with `test/functional/test_runner.py -j20 --previous-releases --coverage --extended`
  Enabled in CI

  ## Breaking Changes
  N/A if hidding `cleardiscouraged` is not a breaking change.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    LGTM, utACK 2e509b96c4
  PastaPastaPasta:
    utACK 2e509b96c4

Tree-SHA512: bb31465d9a71ef824533d9310393d89293c87c7407ec3e37697f6d36dc6c010381a6e0408f9598354e610d51ef662485d8a653cc0e198842e2198ac1c30c90f1

ci/test/00_setup_env_native_qt5.sh

@@ -9,8 +9,7 @@ export LC_ALL=C.UTF-8
 export CONTAINER_NAME=ci_native_qt5
 export PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libdbus-1-dev libharfbuzz-dev"
 export DEP_OPTS="NO_UPNP=1 DEBUG=1"
-# TODO: we have few rpcs that aren't covered by any test, re-enable the line below once it's fixed
+export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_pruning,feature_dbcrash"  # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
-# export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_pruning,feature_dbcrash"  # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
 export RUN_UNIT_TESTS_SEQUENTIAL="true"
 export RUN_UNIT_TESTS="false"
 export GOAL="install"

src/rpc/coinjoin.cpp

@@ -365,6 +365,7 @@ static RPCHelpMan coinjoinsalt_set()
 }
 #endif // ENABLE_WALLET
 
+// TODO: remove it completely
 static RPCHelpMan getpoolinfo()
 {
     return RPCHelpMan{"getpoolinfo",
@@ -469,7 +470,6 @@ void RegisterCoinJoinRPCCommands(CRPCTable &t)
 static const CRPCCommand commands[] =
 { //  category               actor (function)
   //  ---------------------  -----------------------
-    { "dash",                &getpoolinfo,            },
     { "dash",                &getcoinjoininfo,        },
 #ifdef ENABLE_WALLET
     { "dash",                &coinjoin,               },
@@ -480,6 +480,8 @@ static const CRPCCommand commands[] =
     { "dash",                &coinjoinsalt_generate,  },
     { "dash",                &coinjoinsalt_get,       },
     { "dash",                &coinjoinsalt_set,       },
+
+    { "hidden",              &getpoolinfo,            },
 #endif // ENABLE_WALLET
 };
 // clang-format on

src/rpc/masternode.cpp

@@ -134,6 +134,7 @@ static UniValue GetNextMasternodeForPayment(const CChain& active_chain, CDetermi
     return obj;
 }
 
+// TODO: drop it
 static RPCHelpMan masternode_winner()
 {
     return RPCHelpMan{"masternode winner",
@@ -154,6 +155,7 @@ static RPCHelpMan masternode_winner()
     };
 }
 
+// TODO: drop it
 static RPCHelpMan masternode_current()
 {
     return RPCHelpMan{"masternode current",
@@ -227,7 +229,7 @@ static RPCHelpMan masternode_status()
     }
 
     UniValue mnObj(UniValue::VOBJ);
-    // keep compatibility with legacy status for now (might get deprecated/removed later)
+    // keep compatibility with legacy status for now (TODO: get deprecated/removed later)
     mnObj.pushKV("outpoint", node.mn_activeman->GetOutPoint().ToStringShort());
     mnObj.pushKV("service", node.mn_activeman->GetService().ToStringAddrPort());
     auto dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash());
@@ -750,8 +752,8 @@ static const CRPCCommand commands[] =
     { "dash",               &masternode_status,        },
     { "dash",               &masternode_payments,      },
     { "dash",               &masternode_winners,       },
-    { "dash",               &masternode_current,       },
+    { "hidden",             &masternode_current,       },
-    { "dash",               &masternode_winner,        },
+    { "hidden",             &masternode_winner,        },
 };
 // clang-format on
     for (const auto& command : commands) {

src/rpc/net.cpp

@@ -241,7 +241,7 @@ static RPCHelpMan getpeerinfo()
         obj.pushKV("masternode", stats.m_masternode_connection);
         if (fStateStats) {
             if (IsDeprecatedRPCEnabled("banscore")) {
-                // banscore is deprecated in v21 for removal in v22
+                // TODO: banscore is deprecated in v21 for removal in v22, maybe impossible due to usages in p2p_quorum_data.py
                 obj.pushKV("banscore", statestats.m_misbehavior_score);
             }
             obj.pushKV("startingheight", statestats.m_starting_height);
@@ -1126,10 +1126,10 @@ static const CRPCCommand commands[] =
     { "network",             &setban,                  },
     { "network",             &listbanned,              },
     { "network",             &clearbanned,             },
-    { "network",             &cleardiscouraged,        },
     { "network",             &setnetworkactive,        },
     { "network",             &getnodeaddresses,        },
 
+    { "hidden",              &cleardiscouraged,        },
     { "hidden",              &addconnection,           },
     { "hidden",              &addpeeraddress,          },
     { "hidden",              &sendmsgtopeer            },

src/test/denialofservice_tests.cpp

@@ -450,6 +450,8 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     peerLogic->Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
     BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
     BOOST_CHECK(banman->IsDiscouraged(addr));
+    banman->ClearDiscouraged();
+    BOOST_CHECK(!banman->IsDiscouraged(addr));
 
     peerLogic->FinalizeNode(dummyNode);
 }

test/functional/rpc_blockchain.py

@@ -9,6 +9,7 @@ Test the following RPCs:
     - getchaintxstats
     - gettxoutsetinfo
     - getblockheader
+    - getblockheaders
     - getdifficulty
     - getnetworkhashps
     - waitforblockheight
@@ -76,7 +77,8 @@ class BlockchainTest(BitcoinTestFramework):
         self._test_getblockchaininfo()
         self._test_getchaintxstats()
         self._test_gettxoutsetinfo()
-        self._test_getblockheader()
+        self._test_getblockheader(rpc_name='getblockheader')
+        self._test_getblockheader(rpc_name='getblockheaders')
         self._test_getdifficulty()
         self._test_getnetworkhashps()
         self._test_stopatheight()
@@ -319,17 +321,20 @@ class BlockchainTest(BitcoinTestFramework):
         # Unknown hash_type raises an error
         assert_raises_rpc_error(-8, "'foo hash' is not a valid hash_type", node.gettxoutsetinfo, "foo hash")
 
-    def _test_getblockheader(self):
+    def _test_getblockheader(self, rpc_name):
-        self.log.info("Test getblockheader")
+        self.log.info(f"Test {rpc_name}")
         node = self.nodes[0]
-
+        rpc_call = getattr(node, rpc_name)
-        assert_raises_rpc_error(-8, "hash must be of length 64 (not 8, for 'nonsense')", node.getblockheader, "nonsense")
+        assert_raises_rpc_error(-8, "hash must be of length 64 (not 8, for 'nonsense')", rpc_call, "nonsense")
-        assert_raises_rpc_error(-8, "hash must be hexadecimal string (not 'ZZZ7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844')", node.getblockheader, "ZZZ7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844")
+        assert_raises_rpc_error(-8, "hash must be hexadecimal string (not 'ZZZ7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844')", rpc_call, "ZZZ7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844")
-        assert_raises_rpc_error(-5, "Block not found", node.getblockheader, "0cf7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844")
+        assert_raises_rpc_error(-5, "Block not found", rpc_call, "0cf7bb8b1697ea987f3b223ba7819250cae33efacb068d23dc24859824a77844")
 
         besthash = node.getbestblockhash()
         secondbesthash = node.getblockhash(HEIGHT - 1)
-        header = node.getblockheader(blockhash=besthash)
+        header = rpc_call(blockhash=besthash)
+        if rpc_name == 'getblockheaders':
+            assert_equal(len(header), 1)
+            header = header[0]
 
         assert_equal(header['hash'], besthash)
         assert_equal(header['height'], HEIGHT)
@@ -349,13 +354,17 @@ class BlockchainTest(BitcoinTestFramework):
         assert isinstance(header['difficulty'], Decimal)
 
         # Test with verbose=False, which should return the header as hex.
-        header_hex = node.getblockheader(blockhash=besthash, verbose=False)
+        header_hex = rpc_call(blockhash=besthash, verbose=False)
+        if rpc_name == 'getblockheaders':
+            header_hex = header_hex[0]
+
         assert_is_hex_string(header_hex)
 
         header = from_hex(CBlockHeader(), header_hex)
         header.calc_sha256()
         assert_equal(header.hash, besthash)
 
+        if rpc_name == 'getblockheader':
             assert 'previousblockhash' not in node.getblockheader(node.getblockhash(0))
             assert 'nextblockhash' not in node.getblockheader(node.getbestblockhash())
 

test/functional/test_runner.py

@@ -892,6 +892,14 @@ class RPCCoverage():
         # Consider RPC generate covered, because it is overloaded in
         # test_framework/test_node.py and not seen by the coverage check.
         covered_cmds = set({'generate'})
+        # TODO: implement functional tests for coinjoinsalt
+        covered_cmds.add('coinjoinsalt')
+        # TODO: implement functional tests for voteraw
+        covered_cmds.add('voteraw')
+        # TODO: implement functional tests for getmerkleblocks
+        covered_cmds.add('getmerkleblocks')
+        # TODO: drop it with v23+: remove `debug` in favour of `logging`
+        covered_cmds.add('debug')
 
         if not os.path.isfile(coverage_ref_filename):
             raise RuntimeError("No coverage reference found")