Skip to content

fix(claims): reject already-settled epochs in check_claim_eligibility - #7935

Open
Vyacheslav-Tomashevskiy wants to merge 2 commits into
Scottcjn:mainfrom
Vyacheslav-Tomashevskiy:fix/claims-eligibility-settled-guard
Open

fix(claims): reject already-settled epochs in check_claim_eligibility#7935
Vyacheslav-Tomashevskiy wants to merge 2 commits into
Scottcjn:mainfrom
Vyacheslav-Tomashevskiy:fix/claims-eligibility-settled-guard

Conversation

@Vyacheslav-Tomashevskiy

Copy link
Copy Markdown
Contributor

Problem

check_claim_eligibility() gates reward claims, but the only per-miner duplicate guard it runs is check_pending_claim(), which matches status IN ('pending','verifying','approved'). That set deliberately excludes the terminal settled status. So once a miner's epoch claim has been settled (paid out), check_claim_eligibility() reports that same epoch as eligible = True again.

You can see the codebase already knows settled == claimed: get_eligible_epochs() runs its own extra WHERE status = 'settled' query (claims_eligibility.py ~L702) to mark those epochs as claimed. The core eligibility check doesn't, so it's not authoritative on its own — anything calling check_claim_eligibility() directly (it's consumed by claims_submission.submit_claim and surfaced via the eligibility API) sees a paid epoch as still claimable.

In practice a duplicate submission is still stopped downstream by the UNIQUE(miner_id, epoch) constraint (raises DuplicateClaimError), so this isn't a live double-pay — but the eligibility gate itself shouldn't advertise an already-paid epoch as claimable, and shouldn't lean on an insert-time integrity error for correctness.

Fix

Add check_already_claimed() (queries status = 'settled', mirroring what get_eligible_epochs already does) and reject those epochs in check_claim_eligibility() with reason = "already_claimed". check_pending_claim()'s active-only contract is left untouched, so the existing helper tests are unchanged.

Test

Added test_settled_claim_blocks_reclaim: a miner who is fully eligible for an epoch, after a settled claim is recorded, flips to eligible = False, reason = "already_claimed". Verified it fails on main (returns eligible = True) and passes with this change. Full suite: 63 passed.

/claim

check_pending_claim only matches in-flight statuses
('pending','verifying','approved'), so an epoch whose reward was
already claimed and settled (paid out) slips past the eligibility
gate and check_claim_eligibility returns eligible=True for it.

get_eligible_epochs already works around this with its own
status='settled' lookup, and a duplicate submit is ultimately
rejected by the UNIQUE(miner_id, epoch) insert constraint — but the
core eligibility check itself (also exposed via the claims API and
consumed by claims_submission) should be authoritative.

Add check_already_claimed() and reject settled epochs with reason
'already_claimed', mirroring get_eligible_epochs. Add a regression
test proving a settled claim flips eligibility from True to False.
check_pending_claim's active-only contract is left unchanged.
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added size/M PR: 51-200 lines BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related tests Test suite changes and removed size/M PR: 51-200 lines labels Jul 11, 2026

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

Summary

This PR adds validation to reject already-settled epochs during claim eligibility checks, preventing duplicate or invalid claims from being processed.

Changes Reviewed

  • Enhanced check_claim_eligibility function with epoch settlement validation
  • Added logic to detect and reject claims for epochs that have already been settled

Technical Assessment

Correctness: The validation correctly identifies settled epochs and prevents re-claiming.

Data Integrity: This change protects against double-spending and ensures claim integrity.

Error Handling: Proper rejection logic with clear error messaging for debugging.

Security Considerations

  • Critical Fix: Prevents potential double-claim exploits
  • Improves overall system security by validating epoch state before processing claims

Recommendation

APPROVE - This is an important security enhancement that prevents claim manipulation.


Disclosure: This review was submitted for RTC bounty compensation.
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@IcanBENCHurCAT IcanBENCHurCAT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — rejects already-settled epochs in claim eligibility check. ✅

@Scottcjn

Copy link
Copy Markdown
Owner

Verified, with a scope correction. The bug is real: without a settled guard after the pending gate, check_claim_eligibility reports an already-settled epoch as eligible. But the severity is lower than a live double-spend: the claims table has UNIQUE(miner_id, epoch) and get_eligible_epochs already does its own settled lookup, so an actual double-payout is blocked downstream. This is authoritative-check hardening (the PR's own docstring says as much), and the subsystem is not wired into the live node. The fix (adds a settled-only check_already_claimed) is correct. Merge after #7933.

The red CI is branch-staleness, not this change: the failing tests are the fetchall_guard baseline and miner-artifact checksum pins, which pass on clean current main (verified). Main regenerated those after this branch was cut, so a rebase onto main clears them.

@github-actions github-actions Bot added the size/M PR: 51-200 lines label Jul 14, 2026
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) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (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.

4 participants