Skip to content

fix(rewards): import hmac for admin endpoints#6669

Merged
Scottcjn merged 2 commits into
Scottcjn:mainfrom
keon0711:codex/rewards-admin-hmac-import
May 31, 2026
Merged

fix(rewards): import hmac for admin endpoints#6669
Scottcjn merged 2 commits into
Scottcjn:mainfrom
keon0711:codex/rewards-admin-hmac-import

Conversation

@keon0711
Copy link
Copy Markdown
Contributor

@keon0711 keon0711 commented May 31, 2026

Summary

  • move hmac to module scope in node/rewards_implementation_rip200.py
  • add regression coverage for the RIP-200 admin wallet endpoints
  • verify wrong admin keys still return 401 instead of crashing

Fixes #6668.

Why

/rewards/settle imported hmac inside its route function, but sibling routes registered by register_rewards_rip200() also call hmac.compare_digest(). Those routes hit NameError at runtime when RC_ADMIN_KEY is configured:

  • GET /wallet/balance
  • GET /wallet/balances/all
  • GET /lottery/eligibility
  • GET /consensus/round_robin_status

Validation

python3 -m py_compile node/rewards_implementation_rip200.py tests/test_rewards_admin_hmac_import.py
uv run --no-project --with pytest --with flask python -m pytest -q tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
8 passed in 0.12s
git diff --check

Payout/miner id: keon0711
Disclosure: submitted as a RustChain bug bounty candidate; no payout is asserted unless maintainers accept it.

@keon0711 keon0711 requested a review from Scottcjn as a code owner May 31, 2026 04:54
@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 May 31, 2026
@keon0711
Copy link
Copy Markdown
Contributor Author

keon0711 commented May 31, 2026

CI note after the first run:

  • The PR-specific checks other than the broad test job are green, including BCOS gates.
  • The broad test job is currently red, but current main is already red on the same workflow. Latest main CI run for 43e6d4be is also failure: https://github.com/Scottcjn/Rustchain/actions/runs/26694271523
  • Focused validation for this patch remains green:
python3 -m py_compile node/rewards_implementation_rip200.py tests/test_rewards_admin_hmac_import.py
uv run --no-project --with pytest --with flask python -m pytest -q tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
8 passed in 0.12s
git diff --check

The failing broad job is reporting unrelated baseline failures such as test_api_miners_requires_auth, bridge lock ledger expectations, installer checksum drift, and monitor payload shape assertions. I moved the new regression test into tests/ so the broad CI target can collect it on the next run.

@keon0711 keon0711 force-pushed the codex/rewards-admin-hmac-import branch from 7e58837 to 4220071 Compare May 31, 2026 04:58
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 and in this PR.

Two technical observations:

  • Moving to module scope is the right fix for the admin balance routes because those closures can call without depending on the unrelated route importing it first. That keeps privileged endpoint authentication deterministic across route order and request history.
  • The new tests cover both a successful admin-key path and a wrong-key 401 path for , plus , which is a good regression shape for the prior NameError/500 class: it proves the module-level import is available on both guarded balance endpoints and that auth failures remain controlled responses.

One non-blocking note: the test module relies on , so it remains tied to the existing dynamic-load test harness. That is fine if the suite already injects this module name, but worth keeping consistent if these tests are ever run standalone.

I received RTC compensation for this review.

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 node/rewards_implementation_rip200.py and tests/test_rewards_admin_hmac_import.py in this PR.

Two technical observations:

  • Moving hmac to module scope is the right fix for the admin balance routes because those closures can call hmac.compare_digest without depending on the unrelated /rewards/settle route importing it first. That keeps privileged endpoint authentication deterministic across route order and request history.
  • The new tests cover both a successful admin-key path and a wrong-key 401 path for /wallet/balance, plus /wallet/balances/all, which is a good regression shape for the prior NameError/500 class: it proves the module-level import is available on both guarded balance endpoints and that auth failures remain controlled responses.

One non-blocking note: the test module relies on sys.modules["rewards_mod"], so it remains tied to the existing dynamic-load test harness. That is fine if the suite already injects this module name, but worth keeping consistent if these tests are ever run standalone.

I received RTC compensation for this review.

@keon0711
Copy link
Copy Markdown
Contributor Author

Follow-up after the non-blocking review note:

I updated tests/test_rewards_admin_hmac_import.py so it imports rewards_implementation_rip200 directly from node/, instead of relying on the root tests/conftest.py injection of sys.modules["rewards_mod"]. That makes the new regression test runnable as a focused standalone file as well as under the broader root test harness.

Validation after the update:

python3 -m py_compile node/rewards_implementation_rip200.py tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
uv run --no-project --with pytest --with flask python -m pytest -q tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
8 passed in 0.06s
git diff --check

No payout is asserted; this just removes the remaining test-harness concern and keeps the focused validation green.

@keon0711
Copy link
Copy Markdown
Contributor Author

CI follow-up after commit 161f47c1:

  • All PR-specific/non-broad checks are green: label, BCOS tier gate, BCOS trust scans, size-label, P2P/UTXO/RIP-309 checks.
  • The broad test job is still red, but the failure list is the same repo-wide baseline class as before and does not include tests/test_rewards_admin_hmac_import.py or node/tests/test_rewards_settle_endpoint.py.
  • The broad failures are unrelated suites such as tests/test_api.py::test_api_miners_requires_auth, beacon atlas auth expectations, bridge lock ledger expectations, installer checksum drift, monitor payload shape drift, and tx-handler auth/redaction expectations.

Focused validation for this PR remains:

python3 -m py_compile node/rewards_implementation_rip200.py tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
uv run --no-project --with pytest --with flask python -m pytest -q tests/test_rewards_admin_hmac_import.py node/tests/test_rewards_settle_endpoint.py
8 passed in 0.06s
git diff --check

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.

[BUG] RIP-200 admin reward endpoints crash because hmac is only imported inside settle route

3 participants