refactor: clean up governance inv tests after request cache removal#7449
refactor: clean up governance inv tests after request cache removal#7449thepastaclaw wants to merge 4 commits into
Conversation
|
✅ Review complete (commit 1f33a3a) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe governance implementation now owns a local 60-second reliable propagation interval, while the public header no longer includes Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/governance_inv_tests.cpp (1)
183-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated misbehavior-score check.
The
BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(peer->GetId(), stats)); BOOST_CHECK_EQUAL(stats.m_misbehavior_score, ...)pair repeats ~10 times through this test body. A small helper (e.g.AssertMisbehaviorScore(peer, expected)) would reduce duplication without changing test semantics.♻️ Example helper
void AssertMisbehaviorScore(PeerManager& peerman, const CNode& peer, int expected) { CNodeStateStats stats; BOOST_REQUIRE(peerman.GetNodeStateStats(peer.GetId(), stats)); BOOST_CHECK_EQUAL(stats.m_misbehavior_score, expected); }Otherwise, the pointer-based peer refactor (
std::unique_ptr<CNode>viaMakeGovernanceInvPeer,*peer/peer->throughout) is consistent and mirrors the existing pattern used insrc/test/net_tests.cpp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/governance_inv_tests.cpp` around lines 183 - 321, Extract the repeated GetNodeStateStats and misbehavior-score assertions from the governance inventory test into a helper such as AssertMisbehaviorScore, accepting the peer manager, peer, and expected score. Replace each repeated assertion pair in the test body with this helper while preserving the existing expected scores and BOOST_REQUIRE/BOOST_CHECK semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 183-321: Extract the repeated GetNodeStateStats and
misbehavior-score assertions from the governance inventory test into a helper
such as AssertMisbehaviorScore, accepting the peer manager, peer, and expected
score. Replace each repeated assertion pair in the test body with this helper
while preserving the existing expected scores and BOOST_REQUIRE/BOOST_CHECK
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e022b3fe-f7f9-4c9e-a994-7fc0ca7822a5
📒 Files selected for processing (3)
src/governance/governance.cppsrc/governance/governance.hsrc/test/governance_inv_tests.cpp
💤 Files with no reviewable changes (1)
- src/governance/governance.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers codex/general=gpt-5.6-sol(completed), sonnet5/general=claude-sonnet-5(completed), codex/dash-core-commit-history=gpt-5.6-sol(completed), sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).
Clean, narrow governance test-cleanup PR: it removes stale scheduler/thread/mn_metaman fixture setup left over from deleted tests, reuses an existing MakeGovernanceInvPeer helper in one test, adds a wire-valid dummy BLS signature to satisfy the network deserializer's signature-size check on unsigned test votes, and relocates RELIABLE_PROPAGATION_TIME next to its two remaining call sites. I independently verified every load-bearing claim against the current source (vote hash computation excludes vchSig, ProcessVote's metaman assert is unreachable without a parent object, RELIABLE_PROPAGATION_TIME has exactly two uses) and found no defects.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=verifier-sonnet5-7449-1783865327=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
Posting note: review_poster could not post an inline/top-level review cleanly; posting exact-SHA body-only review from the same verifier output.
Verified Findings
No active findings.
thepastaclaw
force-pushed
the
fix/governance-inv-review-cleanup
branch
from
8c48976 to
4bd2246
Compare
|
Addressed the repeated misbehavior-score assertions in 84ac0b0 by extracting |
…est_limited The test built its CNode inline, duplicating the setup MakeGovernanceInvPeer already performs for every other test in this file. Reuse the helper. The helper derives the peer address from the node id, so the peer's address changes from 0x01020305 to 0x01020306. Nothing depends on the literal value: the netfulfilledman request keys are derived from peer->addr in the test itself, so they stay consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…move The CheckAndRemove tests were removed from this file, leaving their setup behind: - <scheduler.h> and <thread> were only used by the deleted net_governance_schedule_drives_check_and_remove test. - The mn_metaman validity check and LoadCache(false) were only needed because CheckAndRemove asserts metaman.IsValid(). No remaining test reaches a path that touches the meta manager: the vote tests have no parent object, so CGovernanceManager::ProcessVote returns on the orphan branch, and the object test pre-seeds the object, so ProcessObject returns on the "already seen" branch before CheckOrphanVotes. Drop the setup and <masternode/meta.h> with it. - The fixture comments still described CheckAndRemove's preconditions. - The mock-time comment claimed the anchor made "SetMockTime advances" deterministic, but nothing advances the clock anymore. It is anchored so the object and vote timestamps built from GetTime() are deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/governance-inv-review-cleanup
branch
from
84ac0b0 to
c42a54d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7449 at c42a54d is a small, mechanical test/refactor cleanup: reuse the MakeGovernanceInvPeer helper, drop stale CheckAndRemove-era fixture setup, relocate RELIABLE_PROPAGATION_TIME beside its only two remaining uses in governance.cpp, and extract an AssertMisbehaviorScore helper. The cumulative diff matches the live PR description and four-commit stack, with no consensus or Dash-subsystem impact. The sole reported finding was dropped because the live PR body contains no obsolete signature-change claim.
Source: reviewers gpt-5.6-sol and claude-sonnet-5 (general and dash-core-commit-history); final verifier claude-sonnet-5.
Note: The verified verdict is APPROVE; this is posted as a COMMENT because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| // Safety margin on the MAX_TIME_FUTURE_DEVIATION limit for creation times. A trigger whose | ||
| // timestamp lands within this margin of the limit can still be rejected as too-far-in-the-future by | ||
| // peers whose clocks lag ours, so it is queued for an additional relay (MasternodeRateUpdate) and | ||
| // only relayed once its timestamp is this far inside the accepted window (CheckPostponedObjects). |
There was a problem hiding this comment.
@thepastaclaw IMO it's too long description. I got tired when I read it.
Compare to original // How long a requested governance inv hash remains in the request cache.
Can you make something clearer / shorter, please?
There was a problem hiding this comment.
Addressed in d3d9829. I reduced the explanation to one direct line describing the future-time margin and why lagging-clock peers need it.
| peer.SetCommonVersion(PROTOCOL_VERSION); | ||
| m_node.peerman->InitializeNode(peer, NODE_NETWORK); | ||
| peer.fSuccessfullyConnected = true; | ||
| auto peer{MakeGovernanceInvPeer(/*id=*/1)}; |
There was a problem hiding this comment.
@thepastaclaw can you refactor MakeGovernanceInvPeer to return CNode instead unique_ptr?
There was a problem hiding this comment.
I checked this against CNode itself. CNode explicitly deletes copy construction and assignment in src/net.h, has no move constructor, and owns non-movable mutex and atomic members, so returning it by value is not supported. The unique_ptr helper also matches the existing make_unique factories in src/test/net_tests.cpp, so I kept the current return type.
There was a problem hiding this comment.
to minimize diff, do
auto peer_ptr{MakeGovernanceInvPeer(/*id=*/1)};
const auto& peer{*peer_ptr};
There was a problem hiding this comment.
Because 50 lines diff is easier to review than 150. To be honest that's the only reason.
Also 'git blame' is more relevant generally but for this particular new file it is not relevant because file is new and have very little history.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior reconciliation: the prior verified review at c42a54d found zero findings, so no prior findings exist to carry forward, resolve, or defer. New findings in the latest delta: d3d9829 only rewords the RELIABLE_PROPAGATION_TIME comment introduced earlier by e6aaee6 in this same unmerged stack; the content is accurate, but the separate commit is avoidable history churn and should be squashed into its introducing commit. Additional cumulative findings: none; the full five-commit stack is a narrow, behavior-preserving cleanup, and the targeted governance_inv_tests suite passes.
Source: reviewers codex/general=gpt-5.6-sol, sonnet5/general=claude-sonnet-5, opus/general=claude-opus-4-8, codex/dash-core-commit-history=gpt-5.6-sol, sonnet5/dash-core-commit-history=claude-sonnet-5, opus/dash-core-commit-history=claude-opus-4-8; final verifier sonnet5=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (reasoning high, orchestration-only, not reviewer/verifier).
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/governance/governance.cpp`:
- [SUGGESTION] src/governance/governance.cpp:33-36: Squash the comment reword into the commit that introduced the comment
Commit d3d9829c only rewrites the RELIABLE_PROPAGATION_TIME comment that e6aaee6b948 added earlier in this same unmerged PR stack—one insertion, four deletions, and no code change. Keeping it separate creates an artificial review-fix seam in permanent history. Fold d3d9829c into e6aaee6b948 so the constant lands once, in its final location, with its final comment.
… uses The comment above the constant still described it as the lifetime of an entry in the governance inv request cache, but that cache is gone. The constant itself is still live: it is a margin on the MAX_TIME_FUTURE_DEVIATION limit for object creation times, used to decide when an object needs an additional relay (governance.cpp) and when that relay is due (CheckPostponedObjects). Both uses are in governance.cpp and both pair it with MAX_TIME_FUTURE_DEVIATION, which already lives in that file's anonymous namespace. Nothing outside governance.cpp referenced it, so move it there beside the constant it is always used with and give it a comment that matches what it does. That also drops the last use of <chrono> in governance.h. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The GetNodeStateStats + m_misbehavior_score assertion pair repeated ~20 times across the governance inventory tests. Extract a small AssertMisbehaviorScore(peerman, peer, expected) helper that preserves the existing BOOST_REQUIRE/BOOST_CHECK semantics and expected scores, and drop the now-unused local CNodeStateStats declarations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/governance-inv-review-cleanup
branch
from
d3d9829 to
1f33a3a
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified against source at exact head 1f33a3a. The prior history-hygiene finding is FIXED: rewritten commit e21257c now relocates RELIABLE_PROPAGATION_TIME and introduces its final comment together, with no separate reword-only commit. The tree is identical to d3d9829, but the current four-commit stack is cleaner; all four current reviewer lanes found no new latest-delta or cumulative issues, and a fresh test_dash run passed all three governance_inv_tests cases.
Source: reviewers codex/general=gpt-5.6-sol, sonnet5/general=claude-sonnet-5, codex/dash-core-commit-history=gpt-5.6-sol, sonnet5/dash-core-commit-history=claude-sonnet-5; final verifier sonnet5=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (reasoning high, orchestration-only, not reviewer/verifier).
Note: GitHub does not allow me to approve my own PR, so the verified clean review is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
3 participants
What was done?
Follow-up to #7442 and knst's review:
MakeGovernanceInvPeerhelper inper_object_vote_sync_is_fulfilled_request_limitedCheckAndRemovetests were deletedAssertMisbehaviorScorehelper to remove repetitive state-stat assertionsRELIABLE_PROPAGATION_TIMEbeside its only remaining uses ingovernance.cppand document its current additional-relay roleReview context: #7442 (review)
How was this tested?
test_dash --run_test=governance_inv_tests,net_tests,ratecheck_tests,miner_testslint-whitespace.pylint-include-guards.pylint-includes.pylint-circular-dependencies.pygit diff --checkThe mandatory independent pre-PR code-review gate returned
shipwith no findings.