From 8080df4278f3cf312fda9858d00456ef8aa1c4c9 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 4 Jul 2026 09:51:45 -0500 Subject: [PATCH 1/4] test: expose governance inv request cache internals for unit tests Adds a tiny const accessor RequestedHashCacheSizeForTesting() that returns the size of m_requested_hash_time so unit tests can observe ConfirmInventoryRequest / CheckAndRemove expiration behavior without scraping debug logs or asserting against the inserted-bool that the public API does not surface. Also moves RELIABLE_PROPAGATION_TIME from an anonymous-namespace constant in governance.cpp to governance::RELIABLE_PROPAGATION_TIME in governance.h so tests can advance mocktime by the production timeout instead of mirroring its value. Co-Authored-By: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Fable 5 --- src/governance/governance.cpp | 9 ++++++++- src/governance/governance.h | 6 ++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 05d0d103fd7e..ec4419bce7b5 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -32,7 +32,7 @@ namespace { constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{10min}; constexpr std::chrono::seconds GOVERNANCE_ORPHAN_EXPIRATION_TIME{10min}; constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h}; -constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{1min}; +using governance::RELIABLE_PROPAGATION_TIME; class ScopedLockBool { @@ -608,6 +608,13 @@ bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv) return true; } +size_t CGovernanceManager::RequestedHashCacheSizeForTesting() const +{ + AssertLockNotHeld(cs_store); + LOCK(cs_store); + return m_requested_hash_time.size(); +} + std::vector CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp, const CBloomFilter& filter) const { LOCK(cs_store); diff --git a/src/governance/governance.h b/src/governance/governance.h index 07a3bc4f606e..dff378c20d47 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -39,6 +39,8 @@ class UniValue; namespace governance { class SuperblockManager; +// How long a requested governance inv hash remains in the request cache. +inline constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{60}; } // namespace governance using vote_time_pair_t = std::pair; @@ -297,6 +299,10 @@ class CGovernanceManager : public GovernanceStore */ bool ConfirmInventoryRequest(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); + /** Test-only accessor: number of inv hashes currently tracked by + * ConfirmInventoryRequest pending expiration in CheckAndRemove. */ + size_t RequestedHashCacheSizeForTesting() const + EXCLUSIVE_LOCKS_REQUIRED(!cs_store); bool ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_store, !cs_relay); void RelayObject(const CGovernanceObject& obj) From 89ed5061eaa0889bc5f14a5356ac465ab54678b5 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 4 Jul 2026 09:51:56 -0500 Subject: [PATCH 2/4] test: add governance_inv_tests for inv request cache expiration Covers the request-cache half of CGovernanceManager previously exercised by test/functional/p2p_governance_invs.py: - ConfirmInventoryRequest records the inv hash - a duplicate before governance::RELIABLE_PROPAGATION_TIME does not re-insert - CheckAndRemove before the timeout leaves the entry intact - after mocktime advances past the timeout, CheckAndRemove evicts the entry and a new inv for the same hash is accepted again Runs both MSG_GOVERNANCE_OBJECT and MSG_GOVERNANCE_OBJECT_VOTE cases, plus an end-to-end case that feeds a real INV message through PeerManager::ProcessMessage and the NetGovernance handler into ConfirmInventoryRequest. TestingSetup does not run the init.cpp/AppInit startup path, so the fixture installs the same NetGovernance handler init.cpp registers; startup registration itself stays outside this unit test. The fixture also advances CMasternodeSync past the blockchain stage and loads the metaman cache so neither ConfirmInventoryRequest nor CheckAndRemove short-circuits. Co-Authored-By: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Fable 5 --- src/Makefile.test.include | 1 + src/test/governance_inv_tests.cpp | 181 ++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 src/test/governance_inv_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 2885c5fccb25..8dc73896ae96 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -120,6 +120,7 @@ BITCOIN_TESTS =\ test/flatfile_tests.cpp \ test/fs_tests.cpp \ test/getarg_tests.cpp \ + test/governance_inv_tests.cpp \ test/governance_superblock_tests.cpp \ test/governance_validators_tests.cpp \ test/coinjoin_inouts_tests.cpp \ diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp new file mode 100644 index 000000000000..5099aac4fb94 --- /dev/null +++ b/src/test/governance_inv_tests.cpp @@ -0,0 +1,181 @@ +// Copyright (c) 2026 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include +#include + +using namespace std::chrono_literals; + +namespace { +struct GovernanceInvSetup : public TestingSetup { + GovernanceInvSetup() : TestingSetup{CBaseChainParams::MAIN} + { + // ConfirmInventoryRequest and CheckAndRemove short-circuit on + // !IsBlockchainSynced(); CheckAndRemove also asserts metaman.IsValid(). + // NetGovernance::AlreadyHave gates on m_gov_manager.IsValid(). + BOOST_REQUIRE(m_node.mn_sync); + m_node.mn_sync->SwitchToNextAsset(); + BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); + + BOOST_REQUIRE(m_node.mn_metaman); + BOOST_REQUIRE(m_node.mn_metaman->LoadCache(/*load_cache=*/false)); + + BOOST_REQUIRE(m_node.govman); + // Match runtime preconditions: NetGovernance::AlreadyHave claims we + // already have the inv when governance isn't loaded (e.g. + // -disablegovernance), so ConfirmInventoryRequest would never run. + BOOST_REQUIRE(m_node.govman->LoadCache(/*load_cache=*/false)); + + BOOST_REQUIRE(m_node.netfulfilledman); + // Loaded here for the later test that advances GOVERNANCE -> FINISHED; + // the sync notifier asserts netfulfilledman.IsValid(). + BOOST_REQUIRE(m_node.netfulfilledman->LoadCache(/*load_cache=*/false)); + BOOST_REQUIRE(m_node.connman); + BOOST_REQUIRE(m_node.peerman); + + // Intentional unit-test boundary: TestingSetup does not run the + // init.cpp/AppInit startup path that registers the Dash-specific + // handlers, so the INV branch in PeerManagerImpl::AlreadyHave would not + // route MSG_GOVERNANCE_OBJECT[_VOTE] anywhere. Install the same + // NetGovernance handler init.cpp registers so a real INV reaches + // CGovernanceManager::ConfirmInventoryRequest; the startup registration + // itself stays outside this unit test. + m_node.peerman->AddExtraHandler(std::make_unique( + m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, + *m_node.netfulfilledman, *m_node.connman)); + + // Anchor the mocked clock so SetMockTime advances are deterministic. + SetMockTime(1'700'000'000s); + } +}; + +// Replaces the per-type loop in test/functional/p2p_governance_invs.py: an +// inv hash is recorded by ConfirmInventoryRequest, deduplicated while valid, +// and purged by CheckAndRemove only after the reliable propagation timeout. +void CheckInvExpirationCycle(CGovernanceManager& govman, const CInv& inv) +{ + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 0U); + + // First inv is recorded. + BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); + + // Duplicate inv before expiry does not re-insert. + BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); + + // Cleanup before the reliable propagation timeout must not expire the entry. + govman.CheckAndRemove(); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); + + // Still recorded -> another inv for the same hash is treated as a duplicate. + BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); + + // Advance past the reliable propagation timeout and clean: the entry is evicted. + SetMockTime(GetTime() + governance::RELIABLE_PROPAGATION_TIME + 1s); + govman.CheckAndRemove(); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 0U); + + // After eviction the same inv must be accepted and recorded again. + BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); + BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); +} +} // namespace + +BOOST_FIXTURE_TEST_SUITE(governance_inv_tests, GovernanceInvSetup) + +BOOST_AUTO_TEST_CASE(object_inv_request_expiration) +{ + CheckInvExpirationCycle(*m_node.govman, CInv{MSG_GOVERNANCE_OBJECT, uint256S("01")}); +} + +BOOST_AUTO_TEST_CASE(vote_inv_request_expiration) +{ + CheckInvExpirationCycle(*m_node.govman, CInv{MSG_GOVERNANCE_OBJECT_VOTE, uint256S("02")}); +} + +// Replaces the end-to-end check the old functional test performed via real P2P: +// a governance INV delivered to PeerManager::ProcessMessage must reach +// CGovernanceManager::ConfirmInventoryRequest through PeerManagerImpl::AlreadyHave +// and the registered NetGovernance handler. Exercising the full inbound INV path +// keeps the wiring from regressing if PeerManager's dispatch ever changes. +BOOST_AUTO_TEST_CASE(peerman_inv_routes_to_governance_request_cache) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x01020304); + CNode peer{/*id=*/0, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::INBOUND, + /*inbound_onion=*/false}; + peer.nVersion = PROTOCOL_VERSION; + peer.SetCommonVersion(PROTOCOL_VERSION); + m_node.peerman->InitializeNode(peer, NODE_NETWORK); + peer.fSuccessfullyConnected = true; + + auto make_inv_stream = [](const CInv& inv) { + CDataStream s{SER_NETWORK, PROTOCOL_VERSION}; + s << std::vector{inv}; + return s; + }; + + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 0U); + + const std::atomic interrupt_dummy{false}; + + // Object INV: PeerManager -> AlreadyHave -> NetGovernance -> ConfirmInventoryRequest. + { + const CInv inv{MSG_GOVERNANCE_OBJECT, uint256S("06")}; + auto stream = make_inv_stream(inv); + m_node.peerman->ProcessMessage(peer, NetMsgType::INV, stream, + /*time_received=*/std::chrono::microseconds{0}, + interrupt_dummy); + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); + + // Duplicate INV with the same hash must not grow the cache. + auto dup_stream = make_inv_stream(inv); + m_node.peerman->ProcessMessage(peer, NetMsgType::INV, dup_stream, + std::chrono::microseconds{0}, interrupt_dummy); + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); + } + + // Vote INV travels the same path and adds a separate entry. + { + const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, uint256S("07")}; + auto stream = make_inv_stream(vote_inv); + m_node.peerman->ProcessMessage(peer, NetMsgType::INV, stream, + std::chrono::microseconds{0}, interrupt_dummy); + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 2U); + } + + m_node.peerman->FinalizeNode(peer); +} + +BOOST_AUTO_TEST_SUITE_END() From 291f40e59f953776644c5030ada9a72b570da8cd Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 4 Jul 2026 09:52:12 -0500 Subject: [PATCH 3/4] test: cover NetGovernance::Schedule periodic CheckAndRemove path Final piece of the functional-test conversion: NetGovernance::Schedule registers a 5-minute periodic task that calls CGovernanceManager:: CheckAndRemove. The previous Python test exercised this implicitly via node.mockscheduler; the new test pins it directly. Constructs a dedicated CScheduler so the assertion is independent of m_node.scheduler's other tasks, pre-loads an inv that has already passed governance::RELIABLE_PROPAGATION_TIME, calls Schedule, then MockForwards 5 minutes so the periodic cleanup is due. A stop marker queued behind the due tasks makes the check deterministic: the worker thread runs the cleanup, then the marker stops the scheduler and the test joins before asserting the entry was evicted. Advances mn_sync through GOVERNANCE->FINISHED (the periodic callback gates on IsSynced) and loads the netfulfilledman cache so the SyncFinished notifier passes its IsValid() assert. Co-Authored-By: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Fable 5 --- src/test/governance_inv_tests.cpp | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 5099aac4fb94..9a5c06561199 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -22,6 +23,7 @@ #include #include +#include #include using namespace std::chrono_literals; @@ -178,4 +180,40 @@ BOOST_AUTO_TEST_CASE(peerman_inv_routes_to_governance_request_cache) m_node.peerman->FinalizeNode(peer); } +// Pins the periodic-cleanup wiring the deleted functional test exercised via +// node.mockscheduler: NetGovernance::Schedule queues a task that calls +// CGovernanceManager::CheckAndRemove, so an expired inv request is purged +// without any manual CheckAndRemove call. +BOOST_AUTO_TEST_CASE(net_governance_schedule_drives_check_and_remove) +{ + // NetGovernance::Schedule's periodic callback short-circuits on + // !m_node_sync.IsSynced(); advance from GOVERNANCE to FINISHED. + m_node.mn_sync->SwitchToNextAsset(); + BOOST_REQUIRE(m_node.mn_sync->IsSynced()); + + // Pre-load an entry that has already passed the reliable propagation timeout so + // the very next CheckAndRemove evicts it. + const CInv inv{MSG_GOVERNANCE_OBJECT, uint256S("05")}; + BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(inv)); + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); + SetMockTime(GetTime() + governance::RELIABLE_PROPAGATION_TIME + 1s); + + // Drive a dedicated scheduler so the assertion is independent of + // m_node.scheduler's existing workload. + CScheduler scheduler; + NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, + *m_node.netfulfilledman, *m_node.connman); + net_gov.Schedule(scheduler); + std::thread worker([&] { scheduler.serviceQueue(); }); + + // First periodic fire is at +5min; bump the clock so the queue is ready. + scheduler.MockForward(std::chrono::minutes{5}); + + // Queue a stop marker after the mocked-forward tasks; due cleanup runs first. + scheduler.scheduleFromNow([&scheduler] { scheduler.stop(); }, 1ms); + worker.join(); + + BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 0U); +} + BOOST_AUTO_TEST_SUITE_END() From c82e3a0896308000b8e32d01f7e7760ca8ef8f1d Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 4 Jul 2026 09:52:21 -0500 Subject: [PATCH 4/4] test: drop p2p_governance_invs.py functional test Coverage moved to src/test/governance_inv_tests.cpp (BOOST suite governance_inv_tests). The C++ tests assert directly against the m_requested_hash_time cache via RequestedHashCacheSizeForTesting() instead of scraping CGovernanceManager debug logs through a real P2P peer + mockscheduler, so they are functionally equivalent without the dashd boot, regtest network setup, or scheduler round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Fable 5 --- test/functional/p2p_governance_invs.py | 62 -------------------------- test/functional/test_runner.py | 1 - 2 files changed, 63 deletions(-) delete mode 100755 test/functional/p2p_governance_invs.py diff --git a/test/functional/p2p_governance_invs.py b/test/functional/p2p_governance_invs.py deleted file mode 100755 index 4679d18cc25d..000000000000 --- a/test/functional/p2p_governance_invs.py +++ /dev/null @@ -1,62 +0,0 @@ -#!/usr/bin/env python3 -# Copyright (c) 2024 The Dash Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. - -""" -Test inv expiration for governance objects/votes -""" - -from test_framework.messages import ( - CInv, - msg_inv, - MSG_GOVERNANCE_OBJECT, - MSG_GOVERNANCE_OBJECT_VOTE, -) -from test_framework.p2p import P2PInterface -from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import force_finish_mnsync - -RELIABLE_PROPAGATION_TIME = 60 # src/governance/governance.cpp -DATA_CLEANUP_TIME = 5 * 60 # src/init.cpp -MSG_INV_ADDED = 'CGovernanceManager::ConfirmInventoryRequest added {} inv hash to m_requested_hash_time' - -class GovernanceInvsTest(BitcoinTestFramework): - def set_test_params(self): - self.num_nodes = 1 - - def run_test(self): - node = self.nodes[0] - force_finish_mnsync(node) - inv = msg_inv([CInv(MSG_GOVERNANCE_OBJECT, 1)]) - self.test_request_expiration(inv, "object") - inv = msg_inv([CInv(MSG_GOVERNANCE_OBJECT_VOTE, 2)]) - self.test_request_expiration(inv, "vote") - - def test_request_expiration(self, inv, name): - msg = MSG_INV_ADDED.format(name) - node = self.nodes[0] - peer = node.add_p2p_connection(P2PInterface()) - self.log.info(f"Send dummy governance {name} inv and make sure it's added to requested map") - with node.assert_debug_log([msg]): - peer.send_message(inv) - self.log.info(f"Send dummy governance {name} inv again and make sure it's not added because we know about it already") - with node.assert_debug_log([], [msg]): - peer.send_message(inv) - self.log.info("Force internal cleanup") - with node.assert_debug_log(['UpdateCachesAndClean']): - node.mockscheduler(DATA_CLEANUP_TIME + 1) - self.log.info(f"Send dummy governance {name} inv again and make sure it's not added because we still know about it") - with node.assert_debug_log([], [msg]): - peer.send_message(inv) - self.log.info(f"Bump mocktime, force internal cleanup, send dummy governance {name} inv again and make sure it's accepted again") - self.bump_mocktime(RELIABLE_PROPAGATION_TIME + 1, nodes=[node]) - with node.assert_debug_log(['UpdateCachesAndClean']): - node.mockscheduler(DATA_CLEANUP_TIME + 1) - with node.assert_debug_log([msg]): - peer.send_message(inv) - node.disconnect_p2ps() - - -if __name__ == '__main__': - GovernanceInvsTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b398e6e55e47..a3c9c9a15119 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -344,7 +344,6 @@ 'feature_cltv.py', 'feature_new_quorum_type_activation.py', 'feature_governance_objects.py', - 'p2p_governance_invs.py', 'p2p_govsync_bloom.py', 'rpc_uptime.py', 'feature_discover.py',