diff --git a/AGENTS.md b/AGENTS.md index 26a517af0533..b10d53d3483c 100644 --- a/AGENTS.md +++ b/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 diff --git a/CLAUDE.md b/CLAUDE.md index 26a517af0533..b10d53d3483c 100644 --- a/CLAUDE.md +++ b/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 diff --git a/test/functional/feature_llmq_rotation.py b/test/functional/feature_llmq_rotation.py index b1f1c3e4947e..748e9b14e057 100755 --- a/test/functional/feature_llmq_rotation.py +++ b/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") diff --git a/test/functional/rpc_quorum.py b/test/functional/rpc_quorum.py index 3f08ac2d9589..14e8e277f706 100755 --- a/test/functional/rpc_quorum.py +++ b/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() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index eadf1ba3c127..e96dbc09f983 100755 --- a/test/functional/test_framework/test_framework.py +++ b/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"]] diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 4d6bee430d26..6ac6b127f41d 100755 --- a/test/functional/test_runner.py +++ b/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") diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 6eec2e2efa24..098fcab8de49 100755 --- a/test/functional/wallet_encryption.py +++ b/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() diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py index b81dfb95d159..f40800bf4f77 100755 --- a/test/functional/wallet_upgradetohd.py +++ b/test/functional/wallet_upgradetohd.py @@ -45,36 +45,7 @@ def recover_non_hd(self): if not self.options.descriptors: assert 'hdchainid' not in node.getwalletinfo() - def test_failed_upgrade(self): - self.log.info("Failed encrypted HD upgrades must keep existing keys locked") - node = self.nodes[0] - node.createwallet("failed-upgrade", blank=True, descriptors=self.options.descriptors) - wallet = node.get_wallet_rpc("failed-upgrade") - key = node.get_deterministic_priv_key() - wallet.importprivkey(key.key) - passphrase = "upgrade passphrase" - wallet.encryptwallet(passphrase) - mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - invalid_inputs = [("invalid mnemonic", "", "invalid mnemonic")] - if not self.options.descriptors: - invalid_inputs.append((mnemonic, "x" * 257, "Mnemonic passphrase is too long")) - for invalid_mnemonic, invalid_passphrase, error in invalid_inputs: - assert_raises_rpc_error(-1, error, wallet.upgradetohd, invalid_mnemonic, invalid_passphrase, passphrase) - assert_raises_rpc_error(-13, "Please enter the wallet passphrase", wallet.signmessage, key.address, "failed upgrade") - wallet.walletpassphrase(passphrase, 100) - sig = wallet.signmessage(key.address, "explicit unlock") - assert node.verifymessage(key.address, sig, "explicit unlock") - wallet.walletlock() - wallet.upgradetohd(mnemonic, "", passphrase, False) - assert_raises_rpc_error(-13, "Please enter the wallet passphrase", wallet.signmessage, key.address, "successful upgrade") - wallet.walletpassphrase(passphrase, 100) - sig = wallet.signmessage(key.address, "imported key after upgrade") - assert node.verifymessage(key.address, sig, "imported key after upgrade") - wallet.walletlock() - wallet.unloadwallet() - def run_test(self): - self.test_failed_upgrade() node = self.nodes[0] node.backupwallet(os.path.join(node.datadir, "non_hd.bak")) @@ -238,6 +209,7 @@ def run_test(self): # Null characters are allowed in wallet passphrases since v23 walletpass = "111\0pass222" node.encryptwallet(walletpass) + assert_raises_rpc_error(-1, "invalid mnemonic", node.upgradetohd, "invalid mnemonic", "", walletpass) assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.rescanblockchain) assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic[0]) assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic[0], "", "111")