diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index bc15e1424c00..adae225859d7 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -345,18 +345,25 @@ void CGovernanceManager::AddGovernanceObjectInternal(CGovernanceObject& insert_o LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- Before trigger block, GetDataAsPlainString = %s, nObjectType = %d\n", Assert(govobj)->GetDataAsPlainString(), std23::to_underlying(govobj->GetObjectType())); + // Count the attempt before AddTrigger can reject it: MasternodeRateCheck allows any + // masternode without a rate-buffer entry, so a key whose triggers all fail AddTrigger + // would otherwise never be throttled. + MasternodeRateUpdate(*govobj); + if (govobj->GetObjectType() == GovernanceObject::TRIGGER && !m_superblocks.AddTrigger(govobj, nCachedBlockHeight)) { LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- undo adding invalid trigger object: hash = %s\n", nHash.ToString()); govobj->PrepareDeletion(GetTime().count()); return; } + // Only objects we keep may be announced. Scheduling this before the AddTrigger + // check would make us re-announce, and serve on GETDATA, a trigger we just + // undid and marked for deletion. + ScheduleTriggerRelay(*govobj); + LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- %s new, received from peer %s\n", strHash, peer_str); RelayObject(*govobj); - // Update the rate buffer - MasternodeRateUpdate(*govobj); - m_mn_sync.BumpAssetLastTime("CGovernanceManager::AddGovernanceObject"); // WE MIGHT HAVE PENDING/ORPHAN VOTES FOR THIS OBJECT @@ -701,15 +708,23 @@ void CGovernanceManager::MasternodeRateUpdate(const CGovernanceObject& govobj) const COutPoint& masternodeOutpoint = govobj.GetMasternodeOutpoint(); auto it = mapLastMasternodeObject.try_emplace(masternodeOutpoint, /*fStatusOKIn=*/true).first; - int64_t nTimestamp = govobj.GetCreationTime(); - it->second.triggerBuffer.AddTimestamp(nTimestamp); + it->second.triggerBuffer.AddTimestamp(govobj.GetCreationTime()); + it->second.fStatusOK = true; +} + +void CGovernanceManager::ScheduleTriggerRelay(const CGovernanceObject& govobj) +{ + AssertLockHeld(cs_store); - if (nTimestamp > GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) { - // schedule additional relay for the object + if (govobj.GetObjectType() != GovernanceObject::TRIGGER) return; + + // A trigger created this close to the future-deviation limit is still too new for + // peers with a lagging clock to accept, so re-announce it once it has aged past + // RELIABLE_PROPAGATION_TIME (see CheckPostponedObjects). + if (govobj.GetCreationTime() > + GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) { setAdditionalRelayObjects.insert(govobj.GetHash()); } - - it->second.fStatusOK = true; } bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus) diff --git a/src/governance/governance.h b/src/governance/governance.h index 33e7996def9a..4395afcb1193 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -404,6 +404,11 @@ class CGovernanceManager : public GovernanceStore void MasternodeRateUpdate(const CGovernanceObject& govobj) EXCLUSIVE_LOCKS_REQUIRED(cs_store); + /** Queue a deferred re-announcement for a trigger that is too new to propagate + * reliably yet. Only call this for triggers we are keeping. */ + void ScheduleTriggerRelay(const CGovernanceObject& govobj) + EXCLUSIVE_LOCKS_REQUIRED(cs_store); + bool MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus, bool fForce, bool& fRateCheckBypassed) EXCLUSIVE_LOCKS_REQUIRED(cs_store); diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index 3317312d477a..cd104b6253ac 100644 --- a/src/test/governance_vote_processing_tests.cpp +++ b/src/test/governance_vote_processing_tests.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include @@ -60,6 +61,20 @@ void SignWithOperatorKey(CGovernanceVote& vote, const CBLSSecretKey& key) key.Sign(vote.GetSignatureHash(), /*specificLegacyScheme=*/false).ToByteVector(/*specificLegacyScheme=*/false)); } +//! A trigger that is signed by the fixture masternode and passes IsValidLocally, but whose payload +//! is not a superblock, so SuperblockManager::AddTrigger rejects it after it was inserted. +CGovernanceObject MakeFailedTrigger(const COutPoint& mn_collateral, const CBLSSecretKey& operator_key, + int64_t creation_time, int salt) +{ + const std::string data{strprintf(R"({"type":2,"salt":%d})", salt)}; + CGovernanceObject govobj{uint256{}, /*revision=*/1, creation_time, uint256{}, HexStr(data)}; + BOOST_REQUIRE(govobj.GetObjectType() == GovernanceObject::TRIGGER); + govobj.SetMasternodeOutpoint(mn_collateral); + govobj.SetSignature(operator_key.Sign(govobj.GetSignatureHash(), /*specificLegacyScheme=*/false) + .ToByteVector(/*specificLegacyScheme=*/false)); + return govobj; +} + // A chain with one registered masternode whose voting (ECDSA) and operator (BLS) keys are known // to the test, so votes can be signed for real and CGovernanceVote::CheckSignature is actually // reached. Without a populated masternode list every CGovernanceVote::IsValid() call would @@ -473,4 +488,50 @@ BOOST_AUTO_TEST_CASE(legacy_invalid_vote_cache_is_discarded) BOOST_CHECK_EQUAL(saved_invalid_votes.GetSize(), 0U); } +// MasternodeRateCheck allows a masternode that has no rate-buffer entry yet, and only accepted +// triggers used to create one. A key whose triggers all fail AddTrigger was therefore never +// throttled: every one cost a BLS verification and a mapErasedGovernanceObjects entry. +BOOST_AUTO_TEST_CASE(failed_triggers_advance_the_masternode_rate_limit) +{ + const int64_t now{GetTime()}; + constexpr int attempts{12}; + int stored{0}; + for (int i = 0; i < attempts; ++i) { + CGovernanceObject govobj{MakeFailedTrigger(mn_collateral, mn_operator_key, now + i, /*salt=*/i)}; + const uint256 hash{govobj.GetHash()}; + // Rate rejection is not misbehaviour, so ProcessObject reports success either way. + WITH_LOCK(::cs_main, BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj))); + if (m_node.govman->HaveObjectForHash(hash)) { + ++stored; + BOOST_CHECK(Assert(m_node.govman->FindGovernanceObject(hash))->IsSetCachedDelete()); + } + } + + // The rate is only computed over a full buffer, so the attempt that would fill it is the + // first one rated, and these timestamps are far too close together to pass. + BOOST_CHECK_EQUAL(stored, RATE_BUFFER_SIZE - 1); + BOOST_CHECK(m_node.govman->FetchRelayInventory().empty()); +} + +// Rate accounting now runs before AddTrigger, but scheduling the deferred re-announcement of a +// trigger created close to the future-deviation limit must not: a trigger that AddTrigger rejected +// would be announced, and served on GETDATA, once it aged past RELIABLE_PROPAGATION_TIME. +BOOST_AUTO_TEST_CASE(failed_trigger_is_not_scheduled_for_deferred_relay) +{ + const int64_t now{GetTime()}; + // Inside (now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME, now + MAX_TIME_FUTURE_DEVIATION], + // i.e. 1h - 60s < offset <= 1h: accepted by MasternodeRateCheck, too new to propagate reliably. + CGovernanceObject govobj{MakeFailedTrigger(mn_collateral, mn_operator_key, now + 3550, /*salt=*/0)}; + const uint256 hash{govobj.GetHash()}; + WITH_LOCK(::cs_main, BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj))); + BOOST_REQUIRE(m_node.govman->HaveObjectForHash(hash)); + BOOST_CHECK(!m_node.govman->HaveSyncableObjectForHash(hash)); + BOOST_CHECK(m_node.govman->FetchRelayInventory().empty()); + + // Age it past RELIABLE_PROPAGATION_TIME and run the deferred-relay pass. + SetMockTime(std::chrono::seconds{now + 120}); + m_node.govman->UpdatedBlockTip(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip())); + BOOST_CHECK(m_node.govman->FetchRelayInventory().empty()); +} + BOOST_AUTO_TEST_SUITE_END()