Skip to content

fix: preserve and bound orphan governance votes #7819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PastaPastaPasta wants to merge 3 commits into
base: develop
Choose a base branch
from
+287 −38
Open

fix: preserve and bound orphan governance votes #7819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81650eb
fix: look up the cache multimap entry after pruning
PastaPastaPasta Oct 7, 2026
94c969f
fix: preserve distinct orphan governance votes
PastaPastaPasta Oct 7, 2026
df7d7bb
fix: bound authenticated orphan governance backlogs
PastaPastaPasta Oct 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions src/cachemultimap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,22 +86,17 @@ class CacheMultiMap

bool Insert(const K& key, const V& value)
{
map_it mit = mapIndex.find(key);
if(mit == mapIndex.end()) {
mit = mapIndex.emplace(key, it_map_t()).first;
}
it_map_t& mapIt = mit->second;

if(mapIt.count(value) > 0) {
if (map_cit mit = mapIndex.find(key); mit != mapIndex.end() && mit->second.count(value) > 0) {
// Don't insert duplicates
return false;
}

// Pruning may erase this key's entry in mapIndex, so look it up only afterwards
if(listItems.size() == nMaxSize) {
PruneLast();
}
listItems.push_front(item_t(key, value));
mapIt.emplace(value, listItems.begin());
mapIndex[key].emplace(value, listItems.begin());
return true;
}

Expand Down Expand Up @@ -137,6 +132,18 @@ class CacheMultiMap
return true;
}

bool GetAll(const K& key, const V& lower, const V& upper, std::vector<V>& vecValues) const
{
const map_cit mit = mapIndex.find(key);
if (mit == mapIndex.end() || upper < lower) return false;
const it_map_t& mapIt = mit->second;
const auto range_end = mapIt.upper_bound(upper);
for (auto it = mapIt.lower_bound(lower); it != range_end; ++it) {
vecValues.push_back(it->second->value);
}
return true;
}

void GetKeys(std::vector<K>& vecKeys)
{
for(map_cit it = mapIndex.begin(); it != mapIndex.end(); ++it) {
Expand Down
40 changes: 37 additions & 3 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <univalue.h>

#include <limits>
#include <ranges>

const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-17";
Expand All @@ -37,6 +38,39 @@ constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h};
// Margin below MAX_TIME_FUTURE_DEVIATION so lagging-clock peers don't reject near-limit objects.
constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{60};

// The encoding chooses which key to verify: IsValidForUnknownParent accepts only compact
// signatures by the voting key and BLS signatures by the operator key.
bool UsesVotingKey(const CGovernanceVote& vote)
{
return vote.GetSignal() == VOTE_SIGNAL_FUNDING && vote.GetSignatureSize() == CGovernanceVote::COMPACT_SIG_SIZE;
}

bool InsertOrphanVote(CacheMultiMap<uint256, governance::OrphanVote>& cache, const CDeterministicMNList& tip_mn_list,
const governance::OrphanVote& orphan_vote)
{
auto lower{orphan_vote};
auto upper{orphan_vote};
lower.vote.SetTime(std::numeric_limits<int64_t>::min());
lower.vote.SetSignature({});
upper.vote.SetTime(std::numeric_limits<int64_t>::max());
upper.vote.SetSignature(std::vector<unsigned char>(CGovernanceVote::BLS_SIG_SIZE));
std::vector<governance::OrphanVote> variants;
const auto& parent = orphan_vote.vote.GetParentHash();
cache.GetAll(parent, lower, upper, variants);
const bool use_voting_key{UsesVotingKey(orphan_vote.vote)};
for (const auto& cached : variants) {
const bool cached_uses_voting_key{UsesVotingKey(cached.vote)};
// Only current-key authentication may suppress a new vote.
if (cached.expiration < Now<NodeSeconds>() || !cached.vote.IsValid(tip_mn_list, cached_uses_voting_key)) {
cache.Erase(parent, cached);
} else if (cached_uses_voting_key == use_voting_key) {
if (cached.vote.GetTimestamp() >= orphan_vote.vote.GetTimestamp()) return false;
cache.Erase(parent, cached);
}
}
return cache.Insert(parent, orphan_vote);
}

bool IsSyncableObject(const std::shared_ptr<CGovernanceObject>& govobj)
{
const auto& obj = *Assert(govobj);
Expand Down Expand Up @@ -820,9 +854,8 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
AssertLockNotHeld(cs_store);
hashToRequest = uint256{};

const auto tip_mn_list{m_dmnman.GetListAtChainTip()};

LOCK(cs_store);
const auto tip_mn_list{m_dmnman.GetListAtChainTip()};
uint256 nHashVote = vote.GetHash();
uint256 nHashGovobj = vote.GetParentHash();

Expand All @@ -846,7 +879,8 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
// No penalty: the vote is signed by a masternode, it just arrived before its parent object,
// which routinely happens during governance sync. Misbehaviour scores never decay.
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING);
if (cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
if (InsertOrphanVote(cmmapOrphanVotes, tip_mn_list,
governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
hashToRequest = nHashGovobj; // Caller should request this object
}
LogPrint(BCLog::GOBJECT, "%s\n", msg);
Expand Down
2 changes: 2 additions & 0 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ struct OrphanVote {

inline bool operator<(const OrphanVote& lhs, const OrphanVote& rhs)
{
// Equal signed fields may arrive authenticated by different voting/operator keys.
if (lhs.vote == rhs.vote) return lhs.vote.GetSignatureSize() < rhs.vote.GetSignatureSize();
return lhs.vote < rhs.vote;
}
} // namespace governance
Expand Down
31 changes: 4 additions & 27 deletions src/governance/vote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include <timedata.h>
#include <util/string.h>

#include <tuple>

static_assert(CGovernanceVote::COMPACT_SIG_SIZE == CPubKey::COMPACT_SIGNATURE_SIZE);
static_assert(CGovernanceVote::BLS_SIG_SIZE == CBLSSignature::SerSize);

Expand Down Expand Up @@ -262,31 +264,6 @@ bool operator==(const CGovernanceVote& vote1, const CGovernanceVote& vote2)

bool operator<(const CGovernanceVote& vote1, const CGovernanceVote& vote2)
{
bool fResult = (vote1.masternodeOutpoint < vote2.masternodeOutpoint);
if (!fResult) {
return false;
}
fResult = (vote1.masternodeOutpoint == vote2.masternodeOutpoint);

fResult = fResult && (vote1.nParentHash < vote2.nParentHash);
if (!fResult) {
return false;
}
fResult = (vote1.nParentHash == vote2.nParentHash);

fResult = fResult && (vote1.nVoteOutcome < vote2.nVoteOutcome);
if (!fResult) {
return false;
}
fResult = (vote1.nVoteOutcome == vote2.nVoteOutcome);

fResult = fResult && (vote1.nVoteSignal == vote2.nVoteSignal);
if (!fResult) {
return false;
}
fResult = (vote1.nVoteSignal == vote2.nVoteSignal);

fResult = fResult && (vote1.nTime < vote2.nTime);

return fResult;
return std::tie(vote1.masternodeOutpoint, vote1.nParentHash, vote1.nVoteOutcome, vote1.nVoteSignal, vote1.nTime) <
std::tie(vote2.masternodeOutpoint, vote2.nParentHash, vote2.nVoteOutcome, vote2.nVoteSignal, vote2.nTime);
Comment on lines +267 to +268

@thepastaclaw thepastaclaw Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Resolved at 9720cdad; see the replies below.

🔴 Blocking: Repair cache eviction before allowing distinct votes for an existing parent

The repaired comparator exposes a use-after-free in the governance orphan cache. CacheMultiMap::Insert binds mapIt to the destination parent's inner map before calling PruneLast() (src/cachemultimap.h:89–104). If the cache is full and its oldest entry is that parent's only vote, pruning erases the empty outer-map entry (lines 217–220), destroying the map referenced by mapIt. The subsequent mapIt.emplace(...) accesses freed memory and can crash or corrupt the node. For example, a capacity-two cache containing one vote for parent A followed by one for parent B reaches this path when inserting a distinct vote for A. Before this PR, the broken comparator treated the new vote as a duplicate and returned before pruning, so this path was unreachable through governance vote insertion. The production cache permits 1,000,000 entries, and ProcessVote accepts authenticated votes for arbitrary unknown parent hashes without a per-signer orphan quota; authentication does not eliminate the saturation case. Reacquire or create the destination map after pruning, and add a small-capacity cache regression covering eviction of the destination parent's sole entry.

source: gpt-6.1-sol (phase2-reviewer: general)

@PastaPastaPasta PastaPastaPasta Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 9720cda. CacheMultiMap::Insert now does the duplicate check without creating an index entry, prunes, and only then looks up or creates the key's inner map (mapIndex[key]). That way a PruneLast() that erases the same key can no longer leave a dangling reference.

The new cachemultimap_insert_evicts_sole_value_of_same_key case uses a capacity-2 cache holding (1,10) then (2,20) and inserts (1,11). On the previous header it aborts with a memory access violation (exit 201). With the fix it passes, and the new item is indexed and erasable. test_dash --run_test=cachemultimap_tests,governance* passes (31 cases), and dashd builds.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved (re-reviewed at 9720cdad): Your change in 9720cda removes the inner-map reference across PruneLast() and looks up the destination only afterwards, so eviction of that same key no longer leaves a dangling reference. I also inspected the capacity-two regression and confirmed that it covers the reported insertion sequence and replacement indexing/erasure; I did not rerun the reported builds or tests in this static verification lane.

Comment on lines +267 to +268

@thepastaclaw thepastaclaw Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Resolved at 2339cf7a; see the replies below.

🟡 Suggestion: Bound same-signer orphan backlogs before replay

The repaired ordering retains timestamp variants from the same signer for the same parent, signal, and outcome. A registered masternode can submit many authenticated variants with increasing positive timestamps below the accepted upper bound; the unknown-parent path has no per-signer limit, only the shared 1,000,000-entry cap. When that parent arrives, CheckOrphanVotes materializes the entire parent's backlog and disables rate checks while holding cs_store; AddGovernanceObjectInternal also holds cs_main. Because these variants are returned in increasing timestamp order, they pass the obsolete-vote check and each accepted vote performs updates, notifications, and relay work. NotifyGovernanceVote additionally copies the tip masternode list for each accepted variant. Before this change, only one orphan per parent reached replay, so this concentrated batch is newly exposed. Signature memos are copied with votes, so same-key replay does not necessarily repeat cryptographic verification, and global cache saturation was already possible using distinct parents; neither is the basis of this finding. Add a bounded same-signer backlog policy that preserves distinct voters and the alternate-key scenario covered by the new regression, with coverage for excessive timestamp variants.

source: glm-5.3-flash (phase1-reviewer: general)

@PastaPastaPasta PastaPastaPasta Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the authenticated backlog path and added a bounded admission policy. The orphan cache now keeps only the newest currently authenticated candidate per parent, collateral, signal, outcome, and signing role. Voting/operator roles and distinct voters remain independent, including equal-time alternate-role votes. A stale-key high timestamp cannot suppress a current-key candidate because the incumbent is revalidated before comparison.

Normal intake uses a targeted range lookup in the existing cache index, rather than scanning or copying the parent backlog. Serialized caches are reauthenticated and compacted before use once the deterministic tip keys are available, preserving the retained arrival order for global eviction. Signature requirements, vote hashes, and serialization remain unchanged.

The new chain-backed regressions check bounded cache cardinality, actual accepted notifications, and final current votes/tallies for excess timestamp variants, persisted backlogs, alternate roles, and key rotation. At the previous published head the notification/equal-time regressions fail with four assertions; all 38 final cache/governance cases pass, along with the local production builds and targeted lints. A four-record regression also checks retained arrival order and fails when the rebuild direction is reverted. The one-time loaded-cache pass still authenticates every unexpired persisted entry while holding the cache lock; its cost was not benchmarked. Independent review found no blocking issue. No real-network load threshold or sanitizer claim is made.

🤖 Posted autonomously by Codex on behalf of pasta.

@thepastaclaw thepastaclaw Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved (re-reviewed at 2339cf7a): Your admission helper now retains only the newest currently authenticated candidate per parent, collateral, signal, outcome, and signing role, revalidating incumbents before timestamp comparison. I also confirmed that deserialized backlogs are compacted before intake or replay, that rebuilding preserves retained arrival order, and that the added regressions observe accepted notifications and final votes; this verification was static, not an independent test or performance run.

}
1 change: 1 addition & 0 deletions src/governance/vote.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class CGovernanceVote
}

void SetSignature(const std::vector<unsigned char>& vchSigIn) { vchSig = vchSigIn; }
size_t GetSignatureSize() const { return vchSig.size(); }

bool CheckSignature(const CKeyID& keyID) const;
bool CheckSignature(const CBLSPublicKey& pubKey) const;
Expand Down
39 changes: 39 additions & 0 deletions src/test/cachemultimap_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,43 @@ BOOST_AUTO_TEST_CASE(cachemultimap_test)
BOOST_CHECK(Compare(cmmapTest1, mapTest4));
}

BOOST_AUTO_TEST_CASE(cachemultimap_insert_evicts_sole_value_of_same_key)
{
CacheMultiMap<int, int> cmmap(2);
BOOST_CHECK(cmmap.Insert(1, 10));
BOOST_CHECK(cmmap.Insert(2, 20));

// The cache is full and its oldest item is key 1's only value
BOOST_CHECK(cmmap.Insert(1, 11));

BOOST_CHECK_EQUAL(cmmap.GetSize(), 2U);
std::vector<int> vecVals;
BOOST_CHECK(cmmap.GetAll(1, vecVals));
BOOST_CHECK(vecVals == std::vector<int>{11});
BOOST_CHECK(cmmap.HasKey(2));

// The index still tracks the new item, so erasing it empties the cache
cmmap.Erase(1);
cmmap.Erase(2);
BOOST_CHECK_EQUAL(cmmap.GetSize(), 0U);
BOOST_CHECK(!cmmap.HasKey(1));
}

BOOST_AUTO_TEST_CASE(cachemultimap_get_all_in_value_range)
{
CacheMultiMap<int, int> cmmap(5);
for (const int value : {1, 2, 3, 4})
BOOST_REQUIRE(cmmap.Insert(1, value));
BOOST_REQUIRE(cmmap.Insert(2, 2));
std::vector<int> values;
BOOST_CHECK(cmmap.GetAll(1, 2, 3, values));
BOOST_CHECK(values == (std::vector<int>{2, 3}));
values.clear();
BOOST_CHECK(cmmap.GetAll(1, 5, 6, values));
BOOST_CHECK(values.empty());
BOOST_CHECK(!cmmap.GetAll(3, 1, 4, values));
BOOST_CHECK(!cmmap.GetAll(1, 3, 2, values));
BOOST_CHECK(values.empty());
}

BOOST_AUTO_TEST_SUITE_END()
Loading
Loading