Skip to content

fix: prevent failed-fingerprint attestations from earning rewards#5560

Merged
Scottcjn merged 6 commits into
Scottcjn:mainfrom
iamdinhthuan:fix-failed-fingerprint-zero-reward
May 18, 2026
Merged

fix: prevent failed-fingerprint attestations from earning rewards#5560
Scottcjn merged 6 commits into
Scottcjn:mainfrom
iamdinhthuan:fix-failed-fingerprint-zero-reward

Conversation

@iamdinhthuan
Copy link
Copy Markdown

Summary

  • set failed-fingerprint epoch enrollment weight to zero instead of a positive sentinel
  • cover the missing-fingerprint attestation reward bypass with a regression test
  • keep explicit epoch enrollment aligned with the zero-reward failed-fingerprint policy

Verification

  • python3 -m pytest node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_settlement_integrity.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py -q
  • python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py
  • git diff --check

Safety

@iamdinhthuan iamdinhthuan requested a review from Scottcjn as a code owner May 17, 2026 15:23
@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 labels May 17, 2026
@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!

@iamdinhthuan
Copy link
Copy Markdown
Author

Status update after the CI rerun:

  • Added missing test-only dependencies to tests/requirements.txt so the CI job gets past collection (aiohttp, flask-cors, matplotlib, seaborn).
  • Focused verification for this PR still passes locally:
python3 -m pytest node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_settlement_integrity.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py -q
python3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py
git diff --check

Observed locally on the current PR branch: 23 passed, py_compile passed, and git diff --check clean.

The remaining test CI failure is now in the broad pytest tests/ step after collection succeeds, with failures in unrelated areas such as tests/security_audit/test_security_findings_2867.py, tests/test_beacon_*, tests/test_cpu_architecture_detection.py, tests/test_issue2310_package_validation.py, tests/test_setup_miner_downloads.py, and tests/test_wallet_review_holds.py. I have not patched those into this PR because they are outside the failed-fingerprint reward bypass change.

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

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

Reviewed PR #5560 at head c511c77427e50deacb0acd14329cc241f7681517.

This looks good to me. The patch changes failed-fingerprint enrollment from the previous minimum non-zero unit to FAILED_FINGERPRINT_WEIGHT_UNITS = 0, and applies that in both auto-enroll after /attest/submit and explicit /epoch/enroll paths while preserving the first-enrollment-wins guard.

Validation run:

git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py tests/requirements.txt
uv run --no-project --python /opt/homebrew/bin/python3.11 --with pytest --with flask --with requests --with pynacl --with psutil --with prometheus-client python -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py
uv run --no-project --python /opt/homebrew/bin/python3.11 --with pytest --with flask --with requests --with pynacl --with psutil --with prometheus-client python -B -m pytest -q -o addopts='' --noconftest node/tests/test_attest_missing_fingerprint_reward_bypass.py
# 1 passed
uv run --no-project --python /opt/homebrew/bin/python3.11 --with pytest --with flask --with requests --with pynacl --with psutil --with prometheus-client python -B -m pytest -q -o addopts='' --noconftest node/tests/test_epoch_weight_fixedpoint.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py
# 19 passed

The new regression proves a missing/failed fingerprint attestation is still accepted and recorded with fingerprint_passed=false, but finalizing the epoch leaves the miner at amount_i64 == 0 and balance_rtc == 0.0. I did not find a blocker in the reward-bypass fix.

Copy link
Copy Markdown

@kekehanshujun kekehanshujun left a comment

Choose a reason for hiding this comment

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

I found a merge-gate issue in the new test coverage. node/tests/test_attest_missing_fingerprint_reward_bypass.py is a newly added Python file without the required SPDX header, so the repository BCOS/SPDX check will reject this PR even though the focused pytest path is useful. Please add the license header and rerun the advertised py_compile/pytest checks. I would also keep the broad tests/requirements.txt additions separate if possible, but the missing header is the blocker here.

@iamdinhthuan
Copy link
Copy Markdown
Author

Update after requested SPDX fix:

  • Pushed commit 574e682 adding the missing # SPDX-License-Identifier: MIT header to node/tests/test_attest_missing_fingerprint_reward_bypass.py.
  • Re-ran the focused checks:
    • python3 tools/bcos_spdx_check.py --base-ref origin/main -> BCOS SPDX check: OK
    • python3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py -> passed
    • python3 -B -m pytest -q node/tests/test_attest_missing_fingerprint_reward_bypass.py --noconftest -> 1 passed
    • git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py tests/requirements.txt -> passed

This addresses the missing-header merge-gate blocker.

Copy link
Copy Markdown

@ZacharyZhang-NY ZacharyZhang-NY left a comment

Choose a reason for hiding this comment

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

Blocking zero-weight side effect found at commit 574e682. This PR now stores failed-fingerprint enrollments with weight 0, but vrf_is_selected() still builds all_miners from every epoch_enroll row. In the selection loop, a zero-weight row can satisfy cumulative >= threshold when threshold is 0 and return False before a later positive-weight miner is evaluated. Manual probe: epoch_enroll rows (failed, 0) then (good, 1), slot 144, vrf_is_selected('good', 144) returned False even though good was the only positive-weight miner; vrf_is_selected('failed', 144) also returned False. Please filter all_miners to positive normalized weights before computing total_weight/threshold and before iterating selection. Validation run: diff check passed; py_compile passed; new missing-fingerprint regression passed with 1 passed; the expanded author test group produced 3 passed and 20 Windows SQLite temp-file cleanup failures.

@iamdinhthuan
Copy link
Copy Markdown
Author

Addressed the zero-weight VRF selection blocker in commit a6e70ed.

What changed:

  • vrf_is_selected() now removes non-positive normalized weights from the candidate list before computing total_weight, threshold, and cumulative selection.
  • Added a regression where a zero-weight failed-fingerprint row precedes the only positive miner; the failed miner remains ineligible and the positive miner is selected.

Verification:

  • RED first: PYTHONPATH=node python3 -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py::test_vrf_selection_ignores_zero_weight_enrollments --noconftest --tb=short failed with good-miner returning False.
  • PYTHONPATH=node python3 -B -m pytest -q node/tests/test_attest_missing_fingerprint_reward_bypass.py --noconftest --tb=short -> 1 passed.
  • PYTHONPATH=node python3 -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py --noconftest --tb=short -> 20 passed.
  • python3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py
  • python3 tools/bcos_spdx_check.py --base-ref origin/main
  • git diff --check HEAD^..HEAD

Note: I kept the missing-fingerprint import-heavy test as a separate pytest command from the epoch-weight module because importing the integrated node twice under different module names in one process triggers duplicate Prometheus metric registration; both scoped commands pass independently.

Copy link
Copy Markdown

@ZacharyZhang-NY ZacharyZhang-NY left a comment

Choose a reason for hiding this comment

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

Follow-up review on current head a6e70ed.

The previous zero-weight selection blocker is resolved in the product code. A manual probe with epoch_enroll rows (failed-fingerprint, 0) followed by (good-miner, 1) now returns:

failed_selected=False
good_selected=True

Requesting changes for the new regression test portability failure on Windows. The focused test added for this exact fix fails after the assertions during TemporaryDirectory cleanup because the SQLite DB file remains locked:

PYTHONPATH=node python -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py::test_vrf_selection_ignores_zero_weight_enrollments --noconftest --tb=short

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ...\vrf_zero_weight.db
1 failed

The module loader already uses TemporaryDirectory(ignore_cleanup_errors=True) for import-time DB cleanup; the added VRF regression should use the same Windows-safe cleanup pattern or otherwise ensure every SQLite handle/background user is released before the temp directory is deleted. That will keep the regression usable for local reviewers on Windows while preserving the fixed zero-weight selection behavior.

Validation run:

git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py tests/requirements.txt
python -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py
PYTHONPATH=node python -B -m pytest -q node/tests/test_attest_missing_fingerprint_reward_bypass.py --noconftest --tb=short
python tools/bcos_spdx_check.py --base-ref origin/main

Result: diff check clean, py_compile clean, missing-fingerprint focused test 1 passed, BCOS SPDX check OK. The broader focused group still hits the known Windows SQLite temp-file cleanup failures in other tests.

@iamdinhthuan
Copy link
Copy Markdown
Author

Addressed the Windows cleanup issue in commit 462883a.

What changed:

  • The new VRF zero-weight regression now uses TemporaryDirectory(ignore_cleanup_errors=True), matching the existing module-loader cleanup pattern in the same test file.
  • Product behavior from a6e70ed is unchanged: zero-weight enrollments are excluded from VRF candidate selection, so the failed miner is ineligible and the positive-weight miner remains selectable.

Verification on head 462883a:

  • PYTHONPATH=node python3 -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py::test_vrf_selection_ignores_zero_weight_enrollments --noconftest --tb=short -> 1 passed.
  • PYTHONPATH=node python3 -B -m pytest -q node/tests/test_attest_missing_fingerprint_reward_bypass.py --noconftest --tb=short -> 1 passed.
  • PYTHONPATH=node python3 -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py --noconftest --tb=short -> 20 passed.
  • python3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py
  • python3 tools/bcos_spdx_check.py --base-ref origin/main
  • git diff --check HEAD^..HEAD

Copy link
Copy Markdown

@ZacharyZhang-NY ZacharyZhang-NY left a comment

Choose a reason for hiding this comment

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

Follow-up review on current head 462883a.

Approved. The Windows cleanup blocker from my previous review is fixed: the new VRF zero-weight regression now uses TemporaryDirectory(ignore_cleanup_errors=True), matching the existing module import cleanup pattern, and the exact focused test now passes on Windows.

The product behavior is still correct. Failed-fingerprint enrollment uses FAILED_FINGERPRINT_WEIGHT_UNITS = 0, and vrf_is_selected() filters normalized weights <= 0 before building the candidate list, so a failed-fingerprint zero-weight row stays ineligible while the positive-weight miner remains selectable.

Validation run:

git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py tests/requirements.txt
python -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attest_missing_fingerprint_reward_bypass.py node/tests/test_epoch_weight_fixedpoint.py
python tools/bcos_spdx_check.py --base-ref origin/main
PYTHONPATH=node python -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py::test_vrf_selection_ignores_zero_weight_enrollments --noconftest --tb=short
PYTHONPATH=node python -B -m pytest -q node/tests/test_attest_missing_fingerprint_reward_bypass.py --noconftest --tb=short
PYTHONPATH=node python -B -m pytest -q node/tests/test_epoch_weight_fixedpoint.py node/tests/test_attestation_overwrite_reward_loss.py node/tests/test_rip309_fingerprint_rotation.py --noconftest --tb=short

Result: diff check clean, py_compile clean, BCOS SPDX check OK, focused VRF regression 1 passed, focused failed-fingerprint bypass test 1 passed. The expanded Windows group still reports 16 PermissionError cleanup failures in unchanged SQLite temp-file tests under test_attestation_overwrite_reward_loss.py and test_rip309_fingerprint_rotation.py; the PR-added regression now clears its Windows cleanup path.

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

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 on this PR. 🚀

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Scottcjn Scottcjn merged commit a4ef155 into Scottcjn:main May 18, 2026
10 of 11 checks passed
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.

7 participants