fix(net): throttle per-object governance vote sync requests#7414
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e97a979ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| m_peer_manager->PeerMisbehaving(peer.GetId(), 20); | ||
| return; | ||
| } | ||
| m_netfulfilledman.AddFulfilledRequest(peer.addr, vote_sync_request); |
There was a problem hiding this comment.
With any nonzero random nProp that does not correspond to a governance object, this line still records a fulfilled request before GetSyncableVoteInvs() can return empty. Because the request key embeds the peer-controlled 256-bit hash and CNetFulfilledRequestManager stores every unique key until expiry, a single peer can send unlimited distinct hashes without hitting the repeat-request penalty and grow mapFulfilledRequests for the full expiry window. Gate the insertion on the object being known/syncable, or use a bounded per-peer throttle key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
consider refactoring.
Now both if / else branches has m_netfulfilledman.HasFulfilledRequest() call.
Instead, call it externally once for both.
See my implementation: 2db9352
|
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:
WalkthroughThis change centralizes governance object syncability checks, updates Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetGovernance
participant CGovernanceManager
participant NetFulfilledManager
Peer->>NetGovernance: MNGOVERNANCESYNC
NetGovernance->>CGovernanceManager: check syncable object / fetch eligibility
NetGovernance->>NetFulfilledManager: check fulfilled request key
alt already fulfilled
NetGovernance->>Peer: PeerMisbehaving(20)
else not fulfilled
NetGovernance->>NetFulfilledManager: AddFulfilledRequest(...)
end
Estimated code review effort: 4 (Complex) | ~45 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)
220-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers the "not yet fulfilled" path.
The test verifies that a first-time per-object request correctly gets recorded via
AddFulfilledRequest, but doesn't assert that a second identical request triggersPeerMisbehaving(peer.GetId(), 20)and returns early without re-processing vote invs — which is the actual throttling behavior this PR intends to fix. Consider extending the test to callProcessMessagea second time with the sameobject_hashand verify the misbehavior score is bumped (e.g., viapeerman's exposed banscore, if available in test scaffolding).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd41badb-02e0-47d8-b8ae-d33dce338021
📒 Files selected for processing (2)
src/governance/net_governance.cppsrc/test/governance_inv_tests.cpp
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
| object_hash.ToString())}; | ||
| BOOST_CHECK(!m_node.netfulfilledman->HasFulfilledRequest(peer.addr, vote_sync_request)); | ||
|
|
||
| CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; | ||
| stream << object_hash << CBloomFilter{}; | ||
|
|
||
| NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, | ||
| *m_node.netfulfilledman, *m_node.connman); | ||
| net_gov.ProcessMessage(peer, NetMsgType::MNGOVERNANCESYNC, stream); | ||
|
|
||
| BOOST_CHECK(m_node.netfulfilledman->HasFulfilledRequest(peer.addr, vote_sync_request)); |
There was a problem hiding this comment.
IMO this unit-test is very specific and not reliable for any refactoring because it tests very specific current implemntation rather than behaviour. It is hard dependency of netfulfilledman member but it test also high-level behaviour yet it's not a test of netfulfilledman.
Dunno, seems as having this regression test is worse than don't have it at all
There was a problem hiding this comment.
moreover, the test already has bugs - CI failed
| m_peer_manager->PeerMisbehaving(peer.GetId(), 20); | ||
| return; | ||
| } | ||
| m_netfulfilledman.AddFulfilledRequest(peer.addr, vote_sync_request); |
There was a problem hiding this comment.
consider refactoring.
Now both if / else branches has m_netfulfilledman.HasFulfilledRequest() call.
Instead, call it externally once for both.
See my implementation: 2db9352
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
The production change matches the PR intent, but the new unit test does not actually exercise the added fulfilled-request throttling path. The fixture leaves masternode sync at GOVERNANCE, and NetGovernance::ProcessMessage() ignores MNGOVERNANCESYNC until sync is fully finished, so the final assertion fails instead of validating the new behavior.
🔴 1 blocking
🤖 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.
- [BLOCKING] src/test/governance_inv_tests.cpp:248-250: Advance masternode sync before testing vote-sync throttling
The fixture constructor calls `SwitchToNextAsset()` only once, which moves sync from BLOCKCHAIN to GOVERNANCE. That makes `IsBlockchainSynced()` true, but `IsSynced()` remains false. `NetGovernance::ProcessMessage()` returns immediately for `MNGOVERNANCESYNC` while `m_node_sync.IsSynced()` is false, so this test never reaches the new per-object fulfilled-request code and the following `HasFulfilledRequest()` check fails. Advance from GOVERNANCE to FINISHED before invoking `ProcessMessage()`, as the existing schedule test in this file already does for synced-only governance behavior.
Suggested replacement:
m_node.mn_sync->SwitchToNextAsset();
BOOST_REQUIRE(m_node.mn_sync->IsSynced());
NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
*m_node.netfulfilledman, *m_node.connman);
net_gov.ProcessMessage(peer, NetMsgType::MNGOVERNANCESYNC, stream);
Note: posted as a COMMENT review because GitHub does not allow approving/requesting changes on my own PR; the verified blocking finding is preserved above.
| object_hash.ToString())}; | ||
| BOOST_CHECK(!m_node.netfulfilledman->HasFulfilledRequest(peer.addr, vote_sync_request)); | ||
|
|
||
| CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; | ||
| stream << object_hash << CBloomFilter{}; | ||
|
|
||
| NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, | ||
| *m_node.netfulfilledman, *m_node.connman); | ||
| net_gov.ProcessMessage(peer, NetMsgType::MNGOVERNANCESYNC, stream); | ||
|
|
||
| BOOST_CHECK(m_node.netfulfilledman->HasFulfilledRequest(peer.addr, vote_sync_request)); |
There was a problem hiding this comment.
moreover, the test already has bugs - CI failed
thepastaclaw
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25d3e4a04d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (m_netfulfilledman.HasFulfilledRequest(peer.addr, vote_sync_request)) { | ||
| LogPrint(BCLog::GOBJECT, "MNGOVERNANCESYNC -- peer already asked me for votes for %s\n", | ||
| nProp.ToString()); | ||
| m_peer_manager->PeerMisbehaving(peer.GetId(), 20); |
There was a problem hiding this comment.
This duplicate penalty now applies to every nonzero MNGOVERNANCESYNC, but the same message is also emitted by the orphan-vote recovery path below (ProcessVote sets hashToRequest, then line 212 sends MNGOVERNANCESYNC(hashToRequest, empty filter) to ask the announcing peer for the missing parent object). If several orphan votes for the same new object arrive before the object does, the requester can legitimately repeat that same hash; each repeat is scored +20 here and reaches the 100 discouragement threshold after five duplicates, so normal propagation can get peers discouraged for following the existing orphan-recovery flow. Please distinguish/exempt the empty-filter object fetches or throttle only the filtered vote-sync requests.
Useful? React with 👍 / 👎.
|
Addressed the review feedback in
Local validation:
@coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
25d3e4a to
8e5b54a
Compare
|
Handled the latest feedback and force-pushed the cleaned-up two-commit stack (
Local validation after rebuilding: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed the latest review feedback in
Validation: git diff --check upstream/develop..HEAD
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
make -C src -j6 test/test_dash
src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited
src/test/test_dash --run_test=governance_inv_tests
src/test/test_dash --run_test=bloom_tests@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/7414/head:pr7414 2>&1 | tail -5 || true
git checkout pr7414 2>&1 | tail -5 || true
sed -n '1,200p' src/governance/net_governance.cpp | grep -n "MNGOVERNANCESYNC" -A 60 | head -120Length of output: 6654 🏁 Script executed: #!/bin/bash
set -euo pipefail
grep -n "BOOST_AUTO_TEST_CASE\|HasFulfilledRequest\|AddFulfilledRequest\|misbehav\|filter" src/test/governance_inv_tests.cpp | head -80Length of output: 2181 The refactor and test coverage look solid:
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb9d4ccedc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Nonzero govsync with an empty filter is used to retry missing-object | ||
| // fetches for orphan votes. Only full sync and known-object filtered | ||
| // vote sync are fulfilled-request limited. | ||
| const bool track_request{full_sync || (!filter.IsEmpty() && m_gov_manager.HaveObjectForHash(nProp))}; |
There was a problem hiding this comment.
When the requested object is only in mapPostponedObjects (for example, a proposal waiting for collateral confirmations), HaveObjectForHash() makes this condition true, but CGovernanceManager::GetSyncableVoteInvs() only looks in mapObjects and returns no vote inventory for postponed objects (governance.cpp:622-624). The first filtered vote request is therefore cached as fulfilled even though no votes were served, so a retry from the same address after CheckPostponedObjects() promotes the object can be blocked/scored until the fulfilled-request entry expires. Please key this throttle on the same “syncable object” predicate used by the vote-inv path, not on postponed objects.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
Carried-forward prior finding prior-1 is STILL VALID at 8e5b54a: the second commit still repairs test setup introduced by the first commit. New findings in the latest delta: none beyond that carried-forward commit-stack hygiene issue. CodeRabbit had no concrete inline findings, so there are no reactions.
🟡 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/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:224-227: Squash the test setup repair into the throttling commit
Commit 8e5b54a26f7 adds these lines to advance masternode sync from GOVERNANCE to FINISHED before calling `NetGovernance::ProcessMessage()`, but commit d3a52812ee5 already introduced this test without that setup. The fixture only advances sync to GOVERNANCE, and `ProcessMessage()` returns before handling `MNGOVERNANCESYNC` unless `IsSynced()` is true, so the first commit leaves the new unit test broken until the second commit repairs it. Squash this setup fix into d3a52812ee5, or rewrite the first commit so each commit in the final stack is self-contained and testable.
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
Incremental + cumulative review for bb9d4ccedc after prior review 8e5b54a26f7. CodeRabbit had no concrete inline findings, so there are no reactions.
Prior finding reconciliation:
prior-1STILL VALID: the current stack still introducesper_object_vote_sync_is_fulfilled_request_limitedind3a52812ee5and only adds the required masternode-sync setup in8e5b54a26f7.
Carried-forward prior findings:
- [SUGGESTION] src/test/governance_inv_tests.cpp:225-228: Squash the test setup repair into the throttling commit
Commitd3a52812ee5introduced the governance sync test without advancing masternode sync to FINISHED, butNetGovernance::ProcessMessage()returns before handlingMNGOVERNANCESYNCunlessIsSynced()is true. These lines were added later in8e5b54a26f7, so the first commit remains a known-broken bisect point if the stack lands as-is. Squash this setup intod3a52812ee5, or rewrite the stack so the test is introduced only once it is self-contained.
New findings in latest delta:
- [SUGGESTION] src/governance/net_governance.cpp:90-93: Throttle empty-filter requests for known objects
The empty-filter exemption is only needed for missing-object retries, but it also applies whennPropis a locally known governance object. Those requests still fall through toGetSyncableVoteInvs(nProp, filter), which locks governance state and iterates the object's votes; with a default empty filter,CBloomFilter::contains()returns true immediately, so no inventory is sent but the vote file is still rescanned on every request. Limit unknown-object retries without letting known-object empty-filter requests bypass fulfilled-request tracking. - [SUGGESTION] src/governance/net_governance.cpp:90-107: Fold the govsync correction into the original fix
Commitbb9d4ccedcdchanges the behavior introduced byd3a52812ee5: the original throttling commit fulfilled every nonzeroMNGOVERNANCESYNChash, while this latest commit narrows tracking to full sync or known-object filtered vote sync. That is a correction to the same logical change, not a separate final behavior. Rewrite the stack sod3a52812ee5contains the final throttling semantics and the matching test coverage is introduced in a self-contained commit.
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.
Carried-forward prior findings:
- [SUGGESTION] src/test/governance_inv_tests.cpp:225-228: Squash the test setup repair into the throttling commit
Commit d3a52812ee5 introduced `per_object_vote_sync_is_fulfilled_request_limited` without advancing masternode sync to FINISHED, but `NetGovernance::ProcessMessage()` returns before handling `MNGOVERNANCESYNC` unless `IsSynced()` is true. These lines were added later in 8e5b54a26f7, so the first commit remains a known-broken bisect point if the stack lands as-is. Squash this setup into d3a52812ee5, or rewrite the stack so the test is introduced only once it is self-contained.
New findings in latest delta:
- [SUGGESTION] src/governance/net_governance.cpp:90-93: Throttle empty-filter requests for known objects
The empty-filter exemption is only needed for missing-object retries, but it also applies when `nProp` is a locally known governance object. Those requests still fall through to `GetSyncableVoteInvs(nProp, filter)`, which locks governance state and iterates the object's votes; with a default empty filter, `CBloomFilter::contains()` returns true immediately, so no inventory is sent but the vote file is still rescanned on every request. Limit unknown-object retries without letting known-object empty-filter requests bypass fulfilled-request tracking.
- [SUGGESTION] src/governance/net_governance.cpp:90-107: Fold the govsync correction into the original fix
Commit bb9d4ccedcd changes the behavior introduced by d3a52812ee5: the original throttling commit fulfilled every nonzero `MNGOVERNANCESYNC` hash, while this latest commit narrows tracking to full sync or known-object filtered vote sync. That is a correction to the same logical change, not a separate final behavior. Rewrite the stack so d3a52812ee5 contains the final throttling semantics and the matching test coverage is introduced in a self-contained commit.
bb9d4cc to
d8b9867
Compare
|
Addressed the latest review feedback in |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b9867182
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed: extra usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5 (Claude/opus verifier failed: extra usage quota)
Carried-forward prior findings: none remain valid at d8b9867. prior-1 and prior-3 were resolved by rewriting the branch to a single self-contained commit, and prior-2 was resolved because known-object requests are now tracked even when the peer sends an empty filter. New findings in the latest delta: none; CodeRabbit supplied no actionable inline findings.
Note: GitHub does not allow me to approve my own PR, so this clean result is posted as a COMMENT while preserving the exact-SHA review record.
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
d8b9867 to
de0b70f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5.
Prior Finding Reconciliation: prior fd245702 remains clean/no active findings; current-head inspection did not uncover evidence that changes that conclusion. Carried-forward prior findings: none. New findings in latest delta: none; the object_fetch early return matches the empty-filter object-fetch semantics and the added assertions cover the absence of vote-sync status messages. CodeRabbit had no actionable inline findings to validate or react to, and the targeted unit test could not be run because src/test/test_dash is not built in this worktree.
review_poster failed (1); posted exact-SHA body-only review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/governance/net_governance.cpp`:
- Around line 129-131: The MNGOVERNANCESYNC handling in net_governance.cpp
returns early for object_fetch requests before relaying any object inventory,
which leaves empty-filter retries stalled. Update the object_fetch branch in the
governing sync flow so an empty filter still serves the object-fetch response
instead of returning immediately, and ensure the logic in the
MNGOVERNANCESYNC/object_fetch path only skips the normal relay work when
appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65ba6ce4-0ace-44be-9da3-6e9b73274040
📒 Files selected for processing (4)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/test/governance_inv_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/governance/governance.h
- src/governance/governance.cpp
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
f70dbd2 to
8b0ec1d
Compare
|
Addressed the latest CodeRabbit object-fetch feedback in
Validation: git diff --check
COMMIT_RANGE=HEAD^..HEAD test/lint/lint-whitespace.py
make -C src test/test_dash-governance_inv_tests.o test/test_dash -j4
src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited
src/test/test_dash --run_test=governance_inv_tests@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/7414/head:pr7414 2>&1 | tail -5 || true
git checkout pr7414 2>&1 | tail -5 || true
git log --oneline -3
echo "=== net_governance.cpp MNGOVERNANCESYNC block ==="
rg -n -C 40 'MNGOVERNANCESYNC|object_fetch|track_request|HaveSyncableObjectForHash|SYNCSTATUSCOUNT|PeerMisbehaving|MSG_GOVERNANCE_OBJECT' src/governance/net_governance.cpp
echo "=== test file relevant sections ==="
rg -n -C 15 'object_fetch|MSG_GOVERNANCE_OBJECT|inv|SYNCSTATUSCOUNT' src/test/governance_inv_tests.cpp | head -200Length of output: 23384 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 20 'per_object_vote_sync_is_fulfilled_request_limited' src/test/governance_inv_tests.cpp | sed -n '1,400p'Length of output: 2175 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '254,470p' src/test/governance_inv_tests.cppLength of output: 9274 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -A10 'bool CGovernanceManager::HaveObjectForHash' src/governance/governance.cpp
rg -n -A10 'bool CGovernanceManager::HaveSyncableObjectForHash' src/governance/governance.cpp
rg -n -A6 'void CGovernanceManager::AddGovernanceObjectForTesting' src/governance/governance.cppLength of output: 1405 Everything checks out. The implementation and test coverage in
This looks good to me — the object-fetch response now correctly serves the requesting peer without introducing throttling side effects. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b0ec1d4c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
8b0ec1d to
0eb6d88
Compare
|
Addressed the latest current-head Codex feedback in
Validation: git diff --check HEAD^..HEAD
COMMIT_RANGE=HEAD^..HEAD test/lint/lint-whitespace.py
make -C src test/test_dash-governance_inv_tests.o test/test_dash -j4
src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited
src/test/test_dash --run_test=governance_inv_tests@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/7414/head:pr7414 2>&1 | tail -5 || true
git checkout pr7414 2>&1 | tail -5 || true
git log --oneline -3
echo "=== net_governance.cpp MNGOVERNANCESYNC block ==="
rg -n -C 45 'MNGOVERNANCESYNC|object_fetch|track_request|HaveSyncableObjectForHash|SYNCSTATUSCOUNT|PeerMisbehaving|MSG_GOVERNANCE_OBJECT|PeerPushInventory|PushInventory' src/governance/net_governance.cpp
echo "=== test file relevant section ==="
rg -n -C 15 'object_fetch|MSG_GOVERNANCE_OBJECT|SYNCSTATUSCOUNT|CountQueuedInventory' src/test/governance_inv_tests.cppLength of output: 27712
This looks good to me — the object-fetch response now correctly serves the object to the peer on every retry without introducing throttling side effects, addressing the Codex feedback. ✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
Carried-forward prior findings: none; I read the prior f70dbd2 review artifact and revalidated the clean state at current head 8b0ec1d. New findings in the latest delta: none; the object-fetch response now advertises the available governance object without marking the per-object vote-sync fulfilled key, and the duplicate suppression follows the existing peer inventory-known filter behavior. CodeRabbit had no actionable inline findings, and static diff checks passed; the unit test binary is not built in this worktree, so I did not run the new Boost test.
Note: posted as a COMMENT review because GitHub does not allow approving/requesting changes on my own PR via the normal review event. Fallback reason: review_poster_failed_rc_1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb6d88697
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (m_gov_manager.HaveObjectForHash(nProp)) { | ||
| CNetMsgMaker msgMaker(peer.GetCommonVersion()); | ||
| m_connman.PushMessage(&peer, msgMaker.Make(NetMsgType::INV, | ||
| std::vector<CInv>{CInv{MSG_GOVERNANCE_OBJECT, nProp}})); |
There was a problem hiding this comment.
When an empty-filter fetch names an object we still have but have already marked deleted or expired, HaveObjectForHash() still returns true, so this new direct INV advertises it even though GetSyncableObjectInvs()/GetSyncableVoteInvs() deliberately suppress non-syncable objects. The requester can then GETDATA it via SerializeObjectForHash(), causing us to serve stale governance objects until cleanup; gate this response on the same syncable-object predicate, with any separate explicit handling needed for postponed objects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4bbf58db309. Empty-filter object-fetch replies now use HaveSyncableObjectForHash() before sending the direct MSG_GOVERNANCE_OBJECT INV, so postponed/deleted/expired objects are not advertised through this retry path. I also extended governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited to assert postponed empty-filter retries queue no object INV while remaining unfulfilled/unpenalized.
Validation:
git diff --check HEAD^..HEAD && git diff --checkCOMMIT_RANGE=HEAD^..HEAD test/lint/lint-whitespace.py./autogen.sh && ./configure --without-guimake -C src test/test_dash-governance_inv_tests.o test/test_dash -j4src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limitedsrc/test/test_dash --run_test=governance_inv_tests
|
CI triage note for the old The failing test was No branch change is appropriate for that failure; the PR branch is proper as-is for this unrelated CI result. |
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
0eb6d88 to
4bbf58d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bbf58db30
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/governance-vote-sync-throttle
branch
from
4bbf58d to
5ad1ef1
Compare
|
Addressed the latest Codex object-fetch feedback in
Validation: git diff --check HEAD^..HEAD
COMMIT_RANGE=HEAD^..HEAD test/lint/lint-whitespace.py
env CCACHE_DISABLE=1 make -C src test/test_dash-governance_inv_tests.o test/test_dash -j4
src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited
src/test/test_dash --run_test=governance_inv_tests@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/7414/head:pr7414 2>&1 | tail -5 || true
git checkout pr7414 2>&1 | tail -5 || true
git log --oneline -3
echo "=== governance.h HaveObjectForFetch/HaveSyncableObjectForHash ==="
rg -n "HaveObjectForFetch|HaveSyncableObjectForHash|HaveObjectForHash" src/governance/governance.h
echo "=== governance.cpp implementations ==="
rg -n -B3 -A15 "bool CGovernanceManager::HaveObjectForFetch|bool CGovernanceManager::HaveSyncableObjectForHash|bool CGovernanceManager::HaveObjectForHash" src/governance/governance.cpp
echo "=== net_governance.cpp MNGOVERNANCESYNC block ==="
rg -n -C 50 "MNGOVERNANCESYNC" src/governance/net_governance.cpp | head -140
echo "=== test file postponed empty-filter section ==="
rg -n -C 20 "postponed.*empty|HaveObjectForFetch|object_fetch" src/test/governance_inv_tests.cpp | head -200Length of output: 15639 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '230,305p' src/test/governance_inv_tests.cppLength of output: 3834 🏁 Script executed: #!/bin/bash
set -euo pipefail
grep -n "postponed" src/test/governance_inv_tests.cpp
echo "---"
grep -n "CBloomFilter{}" src/test/governance_inv_tests.cpp
echo "---AddPostponedObject in governance.h/cpp---"
rg -n "AddPostponedObject" src/governance/governance.h src/governance/governance.cppLength of output: 2601 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '353,376p' src/test/governance_inv_tests.cppLength of output: 1892
This closes the gap where postponed objects weren't being served on retry. Looks good to me. ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
4 participants
Issue being fixed or feature implemented
MNGOVERNANCESYNCvote requests were not recorded inNetFulfilledRequestManager, unlike full governance sync requests.What was done?
MNGOVERNANCESYNC-votes-<object hash>.How Has This Been Tested?
git diff --check upstream/develop..HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pysrc/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limitedbecause this fresh worktree has no configured build/test binary.Breaking Changes
None.
Checklist: