Skip to content

fix: rate-limit public governance votes#6875

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
vicentsmith470-web:vicentsmith/governance-vote-rate-limit
Jun 5, 2026
Merged

fix: rate-limit public governance votes#6875
Scottcjn merged 1 commit into
Scottcjn:mainfrom
vicentsmith470-web:vicentsmith/governance-vote-rate-limit

Conversation

@vicentsmith470-web
Copy link
Copy Markdown
Contributor

/claim #6869

Fixes #6869

Summary

  • adds a dedicated per-client sliding-window limit for POST /governance/vote
  • returns HTTP 429 with Retry-After after 20 attempts per 60 seconds by default
  • keeps holder self-voting open; no admin gate is restored
  • uses the existing trusted client-IP resolution, including the configured proxy allowlist
  • preserves the existing fail-fast ordering: malformed wallet/public-key/signature requests are rejected before SQLite is opened
  • syncs the fetchall baseline line numbers shifted by the three response-header lines merged in [API] Add X-Total-Count header to /api/miners (issue #6565) #6866

Security behavior

The limiter runs in before_request, so repeated invalid-signature traffic is bounded before signature verification or database work. Limits are isolated per client IP and configurable with:

  • RC_GOVERNANCE_VOTE_RATE_LIMIT_MAX
  • RC_GOVERNANCE_VOTE_RATE_LIMIT_WINDOW_SECONDS

Validation

python -m pytest -q tests/test_governance_vote_rate_limit.py tests/test_governance_api.py node/tests/test_admin_rate_limit.py node/test_signature_pubkey_type_validation.py
22 passed

PATH=/usr/bin:/bin bash scripts/check_fetchall.sh
OK: no new unannotated .fetchall() calls in node/.

python -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py tests/test_governance_vote_rate_limit.py
git diff --check origin/main

Wallet: RTCf69dd944558d4e843a4a676495a97638055caea2

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related tests Test suite changes size/M PR: 51-200 lines labels Jun 5, 2026
Copy link
Copy Markdown
Contributor

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

Reviewed the governance vote rate-limit patch against #6869.

Validation I ran:

  • python3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py tests/test_governance_vote_rate_limit.py
  • python3 -B -m pytest tests/test_governance_vote_rate_limit.py -q3 passed, 1 warning in 0.05s

This satisfies the important acceptance points: the limiter runs in before_request before signature verification/database work, the tests prove per-IP isolation and 429/Retry-After, and the invalid-signature regression keeps SQLite unopened. Keeping holder self-voting open instead of restoring @admin_required is the right security boundary for #6869.

Minor non-blocking follow-up: the in-memory bucket can retain keys for clients that stop sending requests after their window expires. Given the low default vote threshold this is acceptable for this PR, but if the endpoint sees large distributed traffic later, consider a periodic stale-key prune or reusing the shared SlidingWindowRateLimiter implementation mentioned in the issue.

Review submitted for the public RustChain code-review bounty context; no payout/payment is assumed unless maintainers accept it.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great work! 👍

Copy link
Copy Markdown
Contributor

@JesusMP22 JesusMP22 left a comment

Choose a reason for hiding this comment

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

Code Review: Rate-limit public governance votes

Summary: Adds rate limiting to public governance votes, preventing spam and ensuring fair participation.

What I like:

  • Rate limiting is essential for any public voting system
  • Prevents Sybil attacks on governance decisions

Suggestions:

  1. Document the rate limit parameters (requests per window, window size)
  2. Consider different rate limits for authenticated vs unauthenticated users
  3. Add monitoring/alerting for rate limit hits to detect potential attacks
  4. Ensure rate limit responses include Retry-After headers

Security considerations:

  • ✅ Strong security improvement: prevents governance spam/Sybil attacks
  • Consider whether rate limits could be circumvented via IP rotation
  • May want to combine with proof-of-personhood for high-stakes votes

Verdict: ✅ Important security fix. Rate limiting governance votes is critical.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for contributing.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🎉

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Great work.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Nice work!

@vicentsmith470-web
Copy link
Copy Markdown
Contributor Author

Maintainer-ready status update:

  • formal APPROVED review is present
  • PR is currently clean and mergeable
  • all CI/BCOS checks are green
  • focused suite: 22 passed
  • fetchall guard, py_compile, and diff checks passed

The reviewer noted only a non-blocking future improvement for pruning inactive in-memory rate-limit keys; no contributor changes were requested. The implementation remains scoped to #6869 and preserves public signed holder voting.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks for contributing. 👍

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🙏

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks for contributing.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

LGTM! Great work.

@Scottcjn Scottcjn merged commit dfe0ace into Scottcjn:main Jun 5, 2026
12 checks passed
@vicentsmith470-web
Copy link
Copy Markdown
Contributor Author

Payment recovery note (not a duplicate claim): the 5 RTC merge-reward workflow for this PR ran at https://github.com/Scottcjn/Rustchain/actions/runs/27035854180 but the reward action exited with Input required and not supplied: node-url. No transfer for this merge was created.

The root cause is tracked in Scottcjn/rustchain-bounties#13202 and fixed in Scottcjn/rustchain-bounties#13203. Please backfill this PR's 5 RTC reward, or rerun it after the action fix merges.

Wallet: RTCf69dd944558d4e843a4a676495a97638055caea2

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This PR has been reviewed.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! The code changes look good.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rate-limit /governance/vote (DoS surface after removing the admin gate in #6781)

5 participants