test: update guidance for agents for writing tests and follow-up fixes for lately added tests.

AGENTS.md

@@ -131,11 +131,12 @@ appended to `test/util/data/non-backported.txt`.
 Pick the test type by what it can observe, not by where it is easiest to
 write.
 
-- A unit test (`src/test/`, `src/wallet/test/` with Boost; `src/qt/test/` with QTest) checks one function or class through the standard fixtures (`BasicTestingSetup`, `TestingSetup`) and its public surface, (not `friend` or injected private state); if the behavior can't be observed that way, extract a testable function or write a functional test.
+- A unit test (`src/test/`, `src/wallet/test/` with Boost; `src/qt/test/` with QTest) checks one function or class through the standard fixtures (`BasicTestingSetup`, `TestingSetup`) and its public surface, (not `friend` or injected private state); if the behavior can't be observed that way, extract a testable function or write a functional test. Name it after the scenario.
 - A functional test (`test/functional/`) proves a user-visible outcome over RPC or P2P — a block accepted, a lock formed, a peer banned — and is the home for anything spanning several subsystems.
-- A regression test must fail without the fix, pass with it, and observe the actual claimed behavior rather than a reachable proxy like a cache entry, seen-set size, or returned container.
-- Use one scenario (or one input table) per test case with a negative case for every special-cased path, put it in the file that already covers that scope, and add it only when catching the regression outweighs its maintenance and compile cost, naming it after the scenario.
-- Don't force a test: when a change is self-evident from the diff (typo, missing `const`, log category, renamed local) or untestable deterministically (performance, races, timing), state in the PR how you actually verified it, and claim mutation checks or "fails without the fix" only when you really ran them.
+- Don't force a test: when a change is self-evident from the diff (typo, missing `const`, log category, renamed local) or untestable deterministically (performance, races, timing), state in the PR how you actually verified it, and claim mutation checks or "fails without the fix" only when you really ran them. Add a test only when catching the regression outweighs its maintenance and compile cost.
+- If a regression test is added, it must fail without the fix, pass with it, and observe the actual claimed behavior rather than a reachable proxy like a cache entry, seen-set size, or returned container.
+- A scope is what a test file covers (a subsystem, RPC, or feature), a scenario is one setup inside it (a fixture and the sequence of steps run on it), and a case is a single check or input-table row inside a scenario.
+- If a feature or fix can be tested by adding an extra check to an existing relevant scenario, avoid creating a new scenario. If an existing fixture can be used for a new scenario, use it. Add new scenarios to the file that already covers that scope. Cover every special-cased path, including a negative case, preferably as extra checks or input-table rows in an existing scenario.
 
 ## Test Commands
 

CLAUDE.md

@@ -131,11 +131,12 @@ appended to `test/util/data/non-backported.txt`.
 Pick the test type by what it can observe, not by where it is easiest to
 write.
 
-- A unit test (`src/test/`, `src/wallet/test/` with Boost; `src/qt/test/` with QTest) checks one function or class through the standard fixtures (`BasicTestingSetup`, `TestingSetup`) and its public surface, (not `friend` or injected private state); if the behavior can't be observed that way, extract a testable function or write a functional test.
+- A unit test (`src/test/`, `src/wallet/test/` with Boost; `src/qt/test/` with QTest) checks one function or class through the standard fixtures (`BasicTestingSetup`, `TestingSetup`) and its public surface, (not `friend` or injected private state); if the behavior can't be observed that way, extract a testable function or write a functional test. Name it after the scenario.
 - A functional test (`test/functional/`) proves a user-visible outcome over RPC or P2P — a block accepted, a lock formed, a peer banned — and is the home for anything spanning several subsystems.
-- A regression test must fail without the fix, pass with it, and observe the actual claimed behavior rather than a reachable proxy like a cache entry, seen-set size, or returned container.
-- Use one scenario (or one input table) per test case with a negative case for every special-cased path, put it in the file that already covers that scope, and add it only when catching the regression outweighs its maintenance and compile cost, naming it after the scenario.
-- Don't force a test: when a change is self-evident from the diff (typo, missing `const`, log category, renamed local) or untestable deterministically (performance, races, timing), state in the PR how you actually verified it, and claim mutation checks or "fails without the fix" only when you really ran them.
+- Don't force a test: when a change is self-evident from the diff (typo, missing `const`, log category, renamed local) or untestable deterministically (performance, races, timing), state in the PR how you actually verified it, and claim mutation checks or "fails without the fix" only when you really ran them. Add a test only when catching the regression outweighs its maintenance and compile cost.
+- If a regression test is added, it must fail without the fix, pass with it, and observe the actual claimed behavior rather than a reachable proxy like a cache entry, seen-set size, or returned container.
+- A scope is what a test file covers (a subsystem, RPC, or feature), a scenario is one setup inside it (a fixture and the sequence of steps run on it), and a case is a single check or input-table row inside a scenario.
+- If a feature or fix can be tested by adding an extra check to an existing relevant scenario, avoid creating a new scenario. If an existing fixture can be used for a new scenario, use it. Add new scenarios to the file that already covers that scope. Cover every special-cased path, including a negative case, preferably as extra checks or input-table rows in an existing scenario.
 
 ## Test Commands
 

test/functional/feature_llmq_rotation.py

@@ -81,7 +81,6 @@ def run_test(self):
         llmq_type_name="llmq_test_dip0024"
 
         self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn())
-        self.test_unknown_active_membership()
 
         # Connect all nodes to node1 so that we always have the whole network connected
         # Otherwise only masternode connections will be established between nodes, which won't propagate TXs/blocks
@@ -281,52 +280,6 @@ def run_test(self):
         assert_equal(rpc_qr_info["mnListDiffAtHMinus3C"]["baseBlockHash"], genesis_blockhash)
 
         self.test_getqrinfo_base_block_hashes_limit(int(best_block_hash, 16), int(hmc_base_blockhash, 16))
-        self.test_active_rotated_membership()
-
-    def test_unknown_active_membership(self):
-        self.log.info("Unknown active membership is counted conservatively before v20")
-        self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 4070908800)
-        self.wait_for_sporks_same()
-        tip = self.nodes[0].getblockcount()
-        start_height = tip + 24 - tip % 24
-        assert start_height < self.v20_height
-        self.generate(self.nodes[0], start_height - tip - 1)
-        node = self.mninfo[0].get_node(self)
-        upcoming = node.quorum("dkginfo")["upcoming_dkgs"]
-        assert upcoming
-        assert all(d["reason"] == "pre-v20 quorum selection needs future quorum base block hash" for d in upcoming)
-        self.generate(self.nodes[0], 1)
-        info = node.quorum("dkginfo")
-        assert info["active_dkgs"] > 0
-        assert_equal(info["active_dkgs"], self.expected_active_dkgs(upcoming, start_height))
-        assert_equal(node.quorum("dkgstatus")["session"], [])
-        self.generate(self.nodes[0], 13)
-        assert_equal(node.quorum("dkginfo")["active_dkgs"], 0)
-
-    def test_active_rotated_membership(self):
-        self.log.info("Active rotated membership is counted without DKG initialization")
-        self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 4070908800)
-        self.wait_for_sporks_same()
-        node = self.mninfo[0].get_node(self)
-        tip = self.nodes[0].getblockcount()
-        start_height = tip + 24 - tip % 24
-        self.generate(self.nodes[0], start_height - tip - 1)
-        upcoming = {mn.proTxHash: node.quorum("dkginfo", mn.proTxHash)["upcoming_dkgs"] for mn in self.mninfo}
-
-        # Each index's Commit phase ends after 10 blocks, independently of the
-        # shared mining window starting 12 blocks after the cycle base.
-        for offset in (0, 1, 9, 10, 11, 12, 13):
-            height = start_height + offset
-            self.generate(self.nodes[0], height - self.nodes[0].getblockcount())
-            counts = [node.quorum("dkginfo", mn.proTxHash)["active_dkgs"] for mn in self.mninfo]
-            assert_equal(counts, [self.expected_active_dkgs(upcoming[mn.proTxHash], height) for mn in self.mninfo])
-            # All other enabled regtest types share the cycle base, so only index 1 remains at offset 10.
-            if offset == 10:
-                assert sum(counts) > 0
-            elif offset > 10:
-                assert_equal(sum(counts), 0)
-            sessions = node.quorum("dkgstatus")["session"]
-            assert all(d["status"]["quorumHeight"] < start_height for d in sessions)
 
     def test_duplicate_rotated_commitment(self, quorum_info, llmq_type):
         self.log.info("A block mining one rotated commitment twice instead of its sibling is rejected")

test/functional/rpc_quorum.py

@@ -25,8 +25,18 @@ def set_test_params(self):
         self.set_dash_llmq_test_params(1, 1)
 
     def run_test(self):
-        self.test_active_dkg_membership()
-
+        self.mine_until_mns_confirmed_for_next_dkg()
+        tip = self.nodes[0].getblockcount()
+        start_height = tip + 24 - tip % 24
+        self.generate(self.nodes[0], start_height - tip - 1)
+        node = self.mninfo[0].get_node(self)
+        other = "01" * 32
+        other_upcoming = [d for d in node.quorum("dkginfo", other)["upcoming_dkgs"] if d["blocksUntilStart"] == 1]
+        assert any(d["known"] and not d["isMember"] for d in other_upcoming)
+        self.generate(self.nodes[0], 1)
+        assert node.quorum("dkginfo")["active_dkgs"] > 0  # DKG is still disabled by spork
+        # A non-member only gets the sessions whose membership is unknown, e.g. a rotated type without a snapshot yet
+        assert_equal(node.quorum("dkginfo", other)["active_dkgs"], sum(not d["known"] for d in other_upcoming))
         self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0)
         self.wait_for_sporks_same()
         self.mine_until_mns_confirmed_for_next_dkg()
@@ -39,41 +49,5 @@ def run_test(self):
                 if member["proTxHash"] == mn.proTxHash:
                     assert_equal(member['addresses']['core_p2p'][0], f'127.0.0.1:{mn.nodePort}')
 
-    def test_active_dkg_membership(self):
-        # Keep DKG disabled so no local session record can hide a reporting gap.
-        # Membership must remain counted for the whole current window anyway.
-        self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 4070908800)
-        self.wait_for_sporks_same()
-        self.mine_until_mns_confirmed_for_next_dkg()
-        mn = self.mninfo[0]
-        node = mn.get_node(self)
-        other = "01" * 32
-        tip = self.nodes[0].getblockcount()
-        start_height = tip + 24 - tip % 24
-        self.generate(self.nodes[0], start_height - tip - 1)
-        info = node.quorum("dkginfo")
-        assert_equal(info["active_dkgs"], 0)
-        upcoming = info["upcoming_dkgs"]
-        other_upcoming = node.quorum("dkginfo", other)["upcoming_dkgs"]
-        [llmq_test] = [d for d in upcoming if d["llmqType"] == 100]
-        assert_equal(llmq_test["blocksUntilStart"], 1)
-        assert_equal(llmq_test["known"], True)
-        assert_equal(llmq_test["isMember"], True)
-        [other_llmq_test] = [d for d in other_upcoming if d["llmqType"] == 100]
-        assert_equal(other_llmq_test["known"], True)
-        assert_equal(other_llmq_test["isMember"], False)
-
-        for offset in (0, 1, 9, 10):
-            height = start_height + offset
-            self.generate(self.nodes[0], height - self.nodes[0].getblockcount())
-            info = node.quorum("dkginfo")
-            if offset < 10:
-                assert info["active_dkgs"] > 0
-            assert_equal(info["active_dkgs"], self.expected_active_dkgs(upcoming, height))
-            assert_equal(node.quorum("dkginfo", other)["active_dkgs"], self.expected_active_dkgs(other_upcoming, height))
-            assert_equal(node.quorum("dkgstatus")["session"], [])
-            assert all(d["blocksUntilStart"] > 0 for d in info["upcoming_dkgs"])
-
-
 if __name__ == '__main__':
     RPCMasternodeTest().main()

test/functional/test_framework/test_framework.py

@@ -2218,11 +2218,6 @@ def get_upcoming_dkg_predictions(self, node, llmq_type):
                                if d["llmqType"] == llmq_type]
                 for mn in self.mninfo}
 
-    def expected_active_dkgs(self, upcoming, height):
-        # 'quorum dkginfo' active_dkgs at height from earlier upcoming_dkgs entries. Every regtest
-        # LLMQ type has dkgPhaseBlocks = 2, so a session is active through Commit for 10 blocks.
-        return sum(0 <= height - d["quorumHeight"] < 10 and (not d["known"] or d["isMember"]) for d in upcoming)
-
     def verify_upcoming_dkg_predictions(self, predictions, quorum_info):
         # 'quorum info' lists members in member-index order
         members = [m["proTxHash"] for m in quorum_info["members"]]

test/functional/test_runner.py

@@ -944,8 +944,6 @@ def _get_uncovered_rpc_commands(self):
         covered_cmds = set({'generate'})
         # TODO: implement functional tests for voteraw
         covered_cmds.add('voteraw')
-        # TODO: implement functional tests for importelectrumwallet
-        covered_cmds.add('importelectrumwallet')
 
         if not os.path.isfile(coverage_ref_filename):
             raise RuntimeError("No coverage reference found")

test/functional/wallet_encryption.py

@@ -24,29 +24,6 @@ def set_test_params(self):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
-    def test_passphrase_change_permissions(self):
-        self.log.info("Passphrase changes preserve locked, mixing-only and full permissions")
-        node = self.nodes[0]
-        for mode in ("locked", "mixing", "full"):
-            node.createwallet("change-" + mode, descriptors=self.options.descriptors)
-            wallet = node.get_wallet_rpc("change-" + mode)
-            address = wallet.getnewaddress()
-            wallet.encryptwallet("old passphrase")
-            if mode != "locked":
-                wallet.walletpassphrase("old passphrase", 100, mode == "mixing")
-            wallet.walletpassphrasechange("old passphrase", "new passphrase")
-            if mode == "full":
-                signature = wallet.signmessage(address, "passphrase change")
-                assert node.verifymessage(address, signature, "passphrase change")
-            else:
-                assert_raises_rpc_error(-13, "Please enter the wallet passphrase", wallet.signmessage, address, "passphrase change")
-            if mode == "locked":
-                assert_raises_rpc_error(-13, "Please unlock wallet for mixing", wallet.coinjoin, "start")
-            else:
-                assert_equal(wallet.coinjoin("start"), "Mixing requested")
-                wallet.coinjoin("stop")
-            wallet.unloadwallet()
-
     def run_test(self):
         passphrase = "WalletPassphrase"
         passphrase2 = "SecondWalletPassphrase"
@@ -85,11 +62,16 @@ def run_test(self):
         self.nodes[0].walletpassphrase(passphrase, 999000)
         sig = self.nodes[0].signmessage(address, msg)
         assert self.nodes[0].verifymessage(address, sig, msg)
+        assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase + "wrong", 10)
         self.nodes[0].walletlock()
         assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].signmessage, address, msg)
 
-        # Test passphrase changes
+        # Test passphrase changes, a mixing-only unlock must not become a full unlock
+        self.nodes[0].walletpassphrase(passphrase, 999000, True)
         self.nodes[0].walletpassphrasechange(passphrase, passphrase2)
+        assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].signmessage, address, msg)
+        assert_equal(self.nodes[0].coinjoin("start"), "Mixing requested")
+        self.nodes[0].coinjoin("stop")
         assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase, 10)
         self.nodes[0].walletpassphrase(passphrase2, 999000)
         sig = self.nodes[0].signmessage(address, msg)
@@ -125,15 +107,6 @@ def run_test(self):
         assert self.nodes[0].verifymessage(address, sig, msg)
         self.nodes[0].walletlock()
 
-        self.log.info("Check that a wrong passphrase cannot extend a full unlock")
-        self.nodes[0].walletpassphrase(passphrase_with_nulls, 100)
-        unlocked_until = self.nodes[0].getwalletinfo()['unlocked_until']
-        assert_raises_rpc_error(-14, "wallet passphrase entered", self.nodes[0].walletpassphrase, "incorrect passphrase", 1000)
-        assert_equal(self.nodes[0].getwalletinfo()['unlocked_until'], unlocked_until)
-        self.nodes[0].walletlock()
-
-        self.test_passphrase_change_permissions()
-
 
 if __name__ == '__main__':
     WalletEncryptionTest().main()