fix: guard cached governance object flags with the object lock

[PastaPastaPasta]

Issue being fixed or feature implemented

gobject get can report wrong values for fCachedFunding, fCachedValid and fCachedEndorsed while the node is recomputing them. For example, a proposal that has reached the funding threshold can come back as fCachedFunding: false, and a proposal the network voted invalid can come back as fCachedValid: true. The reads are also a data race on plain bool members. That is undefined behavior, and ThreadSanitizer reports it (see below).

There are two causes:

1. The cached flags (fCachedFunding, fCachedValid, fCachedDelete, fCachedEndorsed, fDirtyCache) have no lock. Readers take no lock:

   • CGovernanceObject::GetStateJson(), used by gobject get.
   • The copy constructor, used by gobject list, gobject diff and the Qt proposal list through GetAllNewerThan().

   No single outer lock covers the writers either:

   • CGovernanceManager::CheckAndRemove() holds cs_main + cs_store.
   • SuperblockManager::IsSuperblockTriggered() holds only cs_sb. It is called from block-template creation and block validation, and also from the masternode winners RPC without cs_main.
   • The vote path writes fDirtyCache while holding the object's own cs.

2. CGovernanceObject::UpdateSentinelVariables() first resets fCachedFunding/fCachedEndorsed to false and fCachedValid to true. It then counts votes, and only after that writes the real values. A reader that runs during this window sees the defaults instead of the old or the new result.

There is also a related ordering bug. fCachedDelete was set before the lock that sets nDeletionTime. CheckAndRemove() reads GetDeletionTime() and then IsSetCachedDelete() in separate calls. If IsSuperblockTriggered() marks a trigger for deletion in between, CheckAndRemove() sees a set delete flag with a zero deletion time, computes now - 0, and erases the object immediately instead of after GOVERNANCE_DELETION_DELAY.

Block validation itself is not affected. Its IsSuperblockTriggered() call holds cs_main and cs_sb, which already serializes it against the other writers. The visible impact is on RPC/GUI output, plus undefined behavior under the C++ memory model.

Reproduction

A throwaway test case (not part of this PR) in governance_vote_processing_tests reuses its single-masternode fixture. The masternode votes YES on funding and NO on validity, so with nGovernanceMinQuorum=1 the correct state never changes: fCachedFunding=true, fCachedValid=false. One thread calls UpdateSentinelVariables() in a loop, as CheckAndRemove() / IsSuperblockTriggered() do. The main thread calls GetStateJson() for 3 seconds, as gobject get does, and counts reads that disagree with the correct state.

Test case

BOOST_AUTO_TEST_CASE(proof_cached_flags_read_during_recompute)
{
    auto& govman = *m_node.govman;
    const CGovernanceObject proposal{MakeProposal(uint256::ONE)};
    const uint256 parent_hash{proposal.GetHash()};
    govman.AddGovernanceObjectForTesting(proposal);
    auto obj = govman.FindGovernanceObject(parent_hash);
    BOOST_REQUIRE(obj != nullptr);

    CGovernanceVote funding{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)};
    SignWithVotingKey(funding, mn_voting_key);
    CGovernanceVote invalid{MakeVote(parent_hash, VOTE_SIGNAL_VALID, VOTE_OUTCOME_NO)};
    SignWithOperatorKey(invalid, mn_operator_key);
    CGovernanceException exception;
    uint256 hash_to_request;
    BOOST_REQUIRE(govman.ProcessVote(funding, exception, hash_to_request));
    BOOST_REQUIRE(govman.ProcessVote(invalid, exception, hash_to_request));

    const auto mn_list{tip_mn_list()};
    obj->UpdateSentinelVariables(mn_list);
    BOOST_REQUIRE(obj->IsSetCachedFunding());
    BOOST_REQUIRE(!obj->IsSetCachedValid());

    std::atomic<bool> stop{false};
    std::thread writer([&] {
        while (!stop) obj->UpdateSentinelVariables(mn_list);
    });

    int reads{0}, wrong_funding{0}, wrong_valid{0};
    const auto deadline{std::chrono::steady_clock::now() + 3s};
    while (std::chrono::steady_clock::now() < deadline) {
        const UniValue state{obj->GetStateJson(*m_node.chainman, mn_list, /*local_valid_key=*/"fLocalValidity")};
        ++reads;
        if (!state["fCachedFunding"].get_bool()) ++wrong_funding;
        if (state["fCachedValid"].get_bool()) ++wrong_valid;
    }
    stop = true;
    writer.join();

    BOOST_TEST_MESSAGE(strprintf("GetStateJson reads=%d fCachedFunding=false seen=%d fCachedValid=true seen=%d",
                                 reads, wrong_funding, wrong_valid));
    BOOST_CHECK_EQUAL(wrong_funding, 0);
    BOOST_CHECK_EQUAL(wrong_valid, 0);
}

Results against the unfixed tree (c14104b):

# regular debug build
GetStateJson reads=198415 fCachedFunding=false seen=76428 fCachedValid=true seen=193173
# --with-sanitizers=thread
GetStateJson reads=29726 fCachedFunding=false seen=0 fCachedValid=true seen=29726
WARNING: ThreadSanitizer: data race
  Write of size 1 ... by thread T11:
    #0 CGovernanceObject::UpdateSentinelVariables(CDeterministicMNList const&) object.cpp:996
  Previous read of size 1 ... by main thread:
    #0 CGovernanceObject::IsSetCachedValid() const object.h:196
    #1 CGovernanceObject::GetStateJson(...) const object.cpp:617

What was done?

The flags are now protected by the object's existing cs mutex, the same lock that already guards fExpired, nDeletionTime and the vote maps. Clang's thread-safety analysis enforces it.

1. refactor: require cs in CGovernanceObject::CountMatchingVotes. CountMatchingVotes() becomes a private helper with EXCLUSIVE_LOCKS_REQUIRED(cs). The public Get{Yes,No,Abstain}Count() wrappers take the lock. No behavior change. This lets one critical section count several signals.
2. refactor: give CGovernanceObjectVoteFile a copy assignment operator. CGovernanceObjectVoteFile has a copy constructor that rebuilds mapVoteIndex (list iterators) against its own list, but no copy assignment. The implicit one can't be instantiated because CGovernanceVote is not assignable. This commit adds one with the same rebuild semantics, so the copy constructor in commit 3 can copy the vote file while holding the source's lock. A unit test checks that a copy-assigned file is still usable after its source is destroyed.
3. fix: guard cached governance object flags with the object lock.
   • The five flags are GUARDED_BY(cs), and the IsSetCached*()/IsSetDirtyCache() getters take the lock, like IsSetExpired().
   • UpdateSentinelVariables() counts the votes and writes every flag inside one LOCK(cs). Each flag is assigned its computed value directly, with no reset to a default first. This means:
     • readers see either the previous result or the new one;
     • two recomputes running at the same time can't interleave flag by flag;
     • fDirtyCache = false is ordered with the vote counts under the same lock that vote writers already hold, so a vote arriving during a recompute still marks the object dirty.
   • PrepareDeletion() and UpdateSentinelVariables() share a private MarkForDeletion() helper that sets fCachedDelete and nDeletionTime together, so fCachedDelete implies a non-zero deletion time.
   • GetStateJson() reads all four flags under one lock, so gobject get returns a consistent snapshot.
   • The copy constructor copies all cs-guarded state while holding other.cs.
   • The computed values are the same as before.
4. fix: read deletion flags before deletion time in CheckAndRemove. CheckAndRemove() now reads IsSetCachedDelete()/IsSetExpired() before GetDeletionTime().
   • fCachedDelete is never cleared, and it now changes together with nDeletionTime, so a set delete flag is always paired with its deletion time.
   • fExpired is only set by SuperblockManager::Clean(). That runs from CheckAndRemove() itself and stamps the deletion time before this loop.

Locking notes

• No code holds a governance object's cs while calling the getters, UpdateSentinelVariables(), PrepareDeletion(), GetStateJson(), or the copy constructor. The external cs holders only touch the vote file.
• UpdateSentinelVariables() and GetStateJson() take no other lock while holding cs.
• The cs_main/cs_store/cs_sb → obj.cs edges already existed through GetYesCount() and the old UpdateSentinelVariables(). No new lock-order edges are introduced.

Intentionally left unchanged

fCachedLocalValidity, strLocalValidityError and fUnparsable follow the same unguarded pattern. However, GetStateJson() recomputes local validity instead of reading the cache, and I found no concurrent reader of those fields, so they are out of scope here.

How Has This Been Tested?

macOS arm64, Apple clang. Built with depends using --disable-hardening --enable-debug --enable-crash-hooks --enable-stacktraces --enable-suppress-external-warnings --without-gui --enable-tests --disable-bench --disable-fuzz-binary --enable-werror. This enables -Wthread-safety as an error.

• Each commit builds on its own.
• Thread-safety annotations are enforced. As a mutation check, I removed the LOCK(cs) in GetStateJson(). The build then fails with passing variable 'fCachedValid' by reference requires holding mutex 'cs' [-Werror,-Wthread-safety-reference].
• Reproduction above with this PR: 0 wrong reads across three runs (148887, 155774 and 184427 reads).
• ThreadSanitizer build (--with-sanitizers=thread) with this PR. The reproduction ran with a second reader added that also copy-constructs the object each iteration (the gobject list path). Result: no ThreadSanitizer warnings and 0 wrong reads. The same binary built from the unfixed tree reports the data race shown above.
• Regression test status. The reproduction is not included as a regression test. It is a timing-dependent multithreaded test that runs for several seconds, and this race can't be checked deterministically in a unit test. The CheckAndRemove() ordering fix has the same limitation; I verified it by reviewing every writer of fCachedDelete, fExpired and nDeletionTime.
• New unit test: governance_vote_sync_tests/copy_assigned_vote_file_indexes_its_own_votes.
• src/test/test_dash --run_test='governance*': *** No errors detected.
• test/functional/test_runner.py feature_governance.py feature_governance_cl.py feature_governance_objects.py: all passed.
• git diff -U0 develop | contrib/devtools/clang-format-diff.py -p1 is clean except for the LogPrint in CheckAndRemove(). That call keeps its existing layout because test/lint/lint-logs.py needs the format string's \n" on the same line as LogPrint(. lint-logs, lint-format-strings, lint-includes, lint-whitespace, lint-assertions, lint-include-guards, lint-locale-dependence and lint-circular-dependencies pass locally.

Breaking Changes

None. RPC output fields and values are unchanged.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: a84d547e-0b64-4efa-818f-84f3c3f2d661

📥 Commits

Reviewing files that changed from the base of the PR and between 6010327 and 96767a4.

📒 Files selected for processing (1)

• src/governance/governance.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

---

Walkthrough

Governance-object cache reads and vote counting now use the object mutex. Deletion state updates and deletion eligibility checks use synchronized values. CGovernanceObjectVoteFile now supports copy assignment, which rebuilds its index and in-memory vote count. A test checks the assigned copy’s vote count, hash lookup, and serialization after the source is destroyed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 96767

The reviewed synchronization and deletion paths show no actionable new failure. The change is mergeable after normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to 96767

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — src (service) was modified; 6 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in src/governance/object.cpp: The copy constructor moves copying of deletion time, cached vote flags, expiration state, current votes, and vote storage out of the initializer list and into its body under LOCK(other.cs); the other copied fields remain initialized as before.
• observed — Modified behavior in src/governance/object.cpp: GetStateJson now locks cs before reading the four cached flags directly, replacing accessor calls that read those flags without this local lock.
• observed — Modified behavior in src/governance/object.cpp: CountMatchingVotes now asserts that cs is already held instead of acquiring it internally.
• observed — Modified behavior in src/governance/object.cpp: GetYesCount, GetNoCount, and GetAbstainCount now each acquire cs around CountMatchingVotes, preserving synchronized counting after that helper stopped locking internally.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: protecting cached governance-object flags with the object lock.
Description check	✅ Passed	The description explains the data race, the locking changes, deletion ordering, and reported validation. It is directly related to the changeset.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 96767a4) · triage: normal

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Verified the supplied reviews against exact head c157ef0; no actionable in-scope defects were found. The atomic accesses and explicit copy-constructor loads remove races on the five flags, while single-store recomputation preserves threshold logic, sticky deletion, dirty-cache ordering, and serialization without exposing temporary reset values. This was static verification only: the supplied CI snapshot shows successful formatting and preliminary checks, queued ARM64 container jobs, and no project build/test results yet; the author's reported test results were not independently rerun.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff is a small, contained concurrency fix that makes five cached flags atomic and removes transient reset values without changing vote thresholds or introducing intricate critical-surface logic.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 99% left, weekly 69% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Dead cache members fCachedLocalValidity and strLocalValidityError are written but never read — Routine pre-existing cleanup outside this PR's scope. The members are declared in src/governance/object.h:155-156, copied in object.cpp:360-361, and updated in UpdateLocalValidity() at object.cpp:706; GetStateJson() recomputes validity rather than consuming them. Their removal is not required for this atomic-flag fix and does not meet the bar for an exceptional high-value follow-up.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete four-commit PR diff at 96767a4, including callers, flag writers, serialization, copy semantics, and lock ordering; no actionable in-scope defects were found. The object lock protects cached-flag recomputation and snapshots, vote-file assignment rebuilds destination-owned iterators, and flag-before-timestamp reads prevent premature deletion. Verification was static only: the supplied CI snapshot shows successful completed builds, lint, and nowallet tests, while ASan, TSan, SQLite, standard Linux tests, and the multiprocess build remain pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change coordinates governance cache locking, copy semantics and deletion ordering across several files, but the diff does not introduce a large or intricate change to a qualifying critical surface.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[PastaPastaPasta]

approved

[knst]

post-utACK