fix(net): throttle per-object governance vote sync requests - #7414
Conversation
|
Review skipped (commit 4bbf58db): PR head moved to 5ad1ef1 before reviewer launch. |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK 9e97a979ce3be7d33767b5edf4da37e79ca0c48c
![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
📥 Commits
Reviewing files that changed from the base of the PR and between 82d1492 and 9e97a979ce3be7d33767b5edf4da37e79ca0c48c.
📒 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.
|
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 8e5b54a26f79db561563f289ac5c65d85034d3ef: 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 d8b9867182e7207d87c5d8dcaa07dda4b7f6bdd2. 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.
d8b9867 to
de0b70f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
|
Backport prerequisite from dashpay#7387: expose RequestedHashCacheSizeForTesting() and governance::RELIABLE_PROPAGATION_TIME so the v23.1.x unit-test migration of governance inv cache coverage can observe ConfirmInventoryRequest / CheckAndRemove expiration without scraping logs. The full dashpay#7387 merge is intentionally not cherry-picked: v23.1.8 retains p2p_governance_invs.py and only needs the unit-test fixture subset required by later dashpay#7414/dashpay#7442 coverage.
Backport prerequisite for dashpay#7414/dashpay#7442: add the minimal governance inventory unit-test fixture and NetGovernance::Schedule CheckAndRemove coverage that dashpay#7387 introduced, without removing p2p_governance_invs.py from v23.1.x. Covers ConfirmInventoryRequest request-cache expiration and the periodic Schedule cleanup path used by later governance throttle/authorization tests.
…c requests 5ad1ef1 fix: throttle governance vote sync requests (PastaClaw) Pull request description: ## Issue being fixed or feature implemented - Per-object `MNGOVERNANCESYNC` vote requests were not recorded in `NetFulfilledRequestManager`, unlike full governance sync requests. - A peer could repeatedly ask for votes for the same governance object and make the node rescan/build the same vote inventory again. This is a low-cost CPU/lock-work concern, not a crash/OOM primitive. ## What was done? - Add a fulfilled-request key for per-object vote sync requests: `MNGOVERNANCESYNC-votes-<object hash>`. - Return early and score repeat requests from the same peer/address. - Add unit coverage asserting the per-object request is registered as fulfilled. ## How Has This Been Tested? - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - Not run locally: `src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited` because this fresh worktree has no configured build/test binary. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5ad1ef1 Tree-SHA512: 4d72826ddf3a5e1834281fb53a0c7af724ac97eba7a1a0cc73936641015215511c3cc9c409812cc8168be0523d53f2395d6e871f42ab0fcb36ed67404ab63a9e
Narrow exception from Dash dashpay#7387 for the v23.1.8 release history. The full dashpay#7387 merge removes p2p_governance_invs.py after migrating coverage to unit tests. v23.1.x still ships that functional test, so only the prerequisite fixture/coverage subset is included here: - expose RequestedHashCacheSizeForTesting() and governance::RELIABLE_PROPAGATION_TIME for unit observation of ConfirmInventoryRequest / CheckAndRemove - add governance_inv_tests coverage for request-cache expiration and NetGovernance::Schedule periodic cleanup Later dashpay#7414/dashpay#7442 build on these fixtures. p2p_governance_invs.py is retained and updated with dashpay#7442. Co-Authored-By: Claude <noreply@anthropic.com>
…c requests 5ad1ef1 fix: throttle governance vote sync requests (PastaClaw) Pull request description: ## Issue being fixed or feature implemented - Per-object `MNGOVERNANCESYNC` vote requests were not recorded in `NetFulfilledRequestManager`, unlike full governance sync requests. - A peer could repeatedly ask for votes for the same governance object and make the node rescan/build the same vote inventory again. This is a low-cost CPU/lock-work concern, not a crash/OOM primitive. ## What was done? - Add a fulfilled-request key for per-object vote sync requests: `MNGOVERNANCESYNC-votes-<object hash>`. - Return early and score repeat requests from the same peer/address. - Add unit coverage asserting the per-object request is registered as fulfilled. ## How Has This Been Tested? - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - Not run locally: `src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited` because this fresh worktree has no configured build/test binary. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5ad1ef1 Tree-SHA512: 4d72826ddf3a5e1834281fb53a0c7af724ac97eba7a1a0cc73936641015215511c3cc9c409812cc8168be0523d53f2395d6e871f42ab0fcb36ed67404ab63a9e
…it tests Backport of dashpay#7387 (upstream merge da52293, cherry-picked with -m1). Included because dashpay#7414/dashpay#7442/dashpay#7450 modify src/test/governance_inv_tests.cpp, which this PR introduces; it also replaces the p2p_governance_invs.py functional test with equivalent unit-test coverage. v23.1.x adaptations: (1) governance.h on this branch has no 'namespace governance' block at the top - added one containing only the RELIABLE_PROPAGATION_TIME constant this PR introduces (develop's SuperblockManager forward declaration does not exist here and was not brought along). (2) ProcessVoteAndRelay keeps this branch's 'override' specifier. (3) Makefile.test.include entry added without develop-only governance_superblock_tests.cpp. All other hunks unchanged from upstream. (cherry picked from commit da52293e8d6ba51fca27a557f6efd83a439a4b70)
…c requests 5ad1ef1 fix: throttle governance vote sync requests (PastaClaw) Pull request description: ## Issue being fixed or feature implemented - Per-object `MNGOVERNANCESYNC` vote requests were not recorded in `NetFulfilledRequestManager`, unlike full governance sync requests. - A peer could repeatedly ask for votes for the same governance object and make the node rescan/build the same vote inventory again. This is a low-cost CPU/lock-work concern, not a crash/OOM primitive. ## What was done? - Add a fulfilled-request key for per-object vote sync requests: `MNGOVERNANCESYNC-votes-<object hash>`. - Return early and score repeat requests from the same peer/address. - Add unit coverage asserting the per-object request is registered as fulfilled. ## How Has This Been Tested? - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - Not run locally: `src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited` because this fresh worktree has no configured build/test binary. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5ad1ef1 Tree-SHA512: 4d72826ddf3a5e1834281fb53a0c7af724ac97eba7a1a0cc73936641015215511c3cc9c409812cc8168be0523d53f2395d6e871f42ab0fcb36ed67404ab63a9e (cherry picked from commit c168228)
…c requests 5ad1ef1 fix: throttle governance vote sync requests (PastaClaw) Pull request description: ## Issue being fixed or feature implemented - Per-object `MNGOVERNANCESYNC` vote requests were not recorded in `NetFulfilledRequestManager`, unlike full governance sync requests. - A peer could repeatedly ask for votes for the same governance object and make the node rescan/build the same vote inventory again. This is a low-cost CPU/lock-work concern, not a crash/OOM primitive. ## What was done? - Add a fulfilled-request key for per-object vote sync requests: `MNGOVERNANCESYNC-votes-<object hash>`. - Return early and score repeat requests from the same peer/address. - Add unit coverage asserting the per-object request is registered as fulfilled. ## How Has This Been Tested? - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - Not run locally: `src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited` because this fresh worktree has no configured build/test binary. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5ad1ef1 Tree-SHA512: 4d72826ddf3a5e1834281fb53a0c7af724ac97eba7a1a0cc73936641015215511c3cc9c409812cc8168be0523d53f2395d6e871f42ab0fcb36ed67404ab63a9e (cherry picked from commit c168228)
…c requests 5ad1ef1 fix: throttle governance vote sync requests (PastaClaw) Pull request description: ## Issue being fixed or feature implemented - Per-object `MNGOVERNANCESYNC` vote requests were not recorded in `NetFulfilledRequestManager`, unlike full governance sync requests. - A peer could repeatedly ask for votes for the same governance object and make the node rescan/build the same vote inventory again. This is a low-cost CPU/lock-work concern, not a crash/OOM primitive. ## What was done? - Add a fulfilled-request key for per-object vote sync requests: `MNGOVERNANCESYNC-votes-<object hash>`. - Return early and score repeat requests from the same peer/address. - Add unit coverage asserting the per-object request is registered as fulfilled. ## How Has This Been Tested? - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` - Not run locally: `src/test/test_dash --run_test=governance_inv_tests/per_object_vote_sync_is_fulfilled_request_limited` because this fresh worktree has no configured build/test binary. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 5ad1ef1 Tree-SHA512: 4d72826ddf3a5e1834281fb53a0c7af724ac97eba7a1a0cc73936641015215511c3cc9c409812cc8168be0523d53f2395d6e871f42ab0fcb36ed67404ab63a9e (cherry picked from commit c168228)
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
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: