feat: add attestation pool monitoring#6175
Conversation
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
MolhamHamwi
left a comment
There was a problem hiding this comment.
Technical review notes:
-
The 24h history aggregation in
_attestation_pool_snapshot()filtersminer_attest_historyonly byts_ok >= ?and then groups by hourly bucket. The existing schema index I found is(miner, ts_ok), which will not help this pool-wide query because there is nominerpredicate. On a long-running node this can become a full scan of the attestation history on every dashboard/API scrape. Consider adding an index withts_okfirst (for exampleCREATE INDEX IF NOT EXISTS idx_attest_history_ts_only ON miner_attest_history(ts_ok)) or reusing an existing ts-first index if one exists elsewhere. -
The Prometheus label escaping helper correctly handles backslashes, quotes, and newlines before writing
device_archinto a label value. That is a good defensive detail for a field populated from attestation/device metadata, and the added test withriscv"devcovers the quote case. It may be worth also asserting a backslash/newline case so future refactors do not accidentally break Prometheus exposition escaping.
I received RTC compensation for this review.
|
Addressed the attestation history review note. The schema now adds a Validation run:
GitHub checks are passing on head |
|
@Scottcjn This PR is ready for maintainer review. Validation evidence is listed in the PR body. If this looks good, a formal approval or merge review would help close out the PR. |
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Summary
feat: add attestation pool monitoring
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Non-breaking changes where applicable
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Summary
PR #6175
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
BCOS Checklist (Required For Non-Doc PRs)
BCOS-L1# SPDX-License-Identifier: MIT)What Changed
GET /api/attestation-poolfor aggregate attestation pool monitoring: active/stale/known miners, recent 24h history, fingerprint pass count, average entropy, and active miner counts by device architecture./metricsand/api/metricsso existing Prometheus scraping can monitor the pool without a separate endpoint.Fixes #2528
Testing / Evidence
PYTHONDONTWRITEBYTECODE=1 .venv-py314-validation/bin/python -m pytest -p no:cacheprovider node/tests/test_attestation_pool_monitoring.py node/tests/test_integrated_metrics_route.py node/tests/test_api_miners_rate_limit.py -q --tb=short --noconftest -o addopts=''-> 6 passedPYTHONDONTWRITEBYTECODE=1 .venv-bounty-validation/bin/python -m pytest -p no:cacheprovider node/tests/test_attestation_pool_monitoring.py -q --tb=short --noconftest -o addopts=''-> 3 passed.venv-py314-validation/bin/python -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/tests/test_attestation_pool_monitoring.py node/tests/test_integrated_metrics_route.py node/tests/test_api_miners_rate_limit.py-> passedpython3 tools/bcos_spdx_check.py --base-ref origin/main-> BCOS SPDX check: OKgit diff --check origin/main...HEAD-> passedNote: the Python 3.9 preflight venv can run the new focused test. Some existing related tests use
tempfile.TemporaryDirectory(ignore_cleanup_errors=...), so I also ran the combined focused suite under Python 3.14 where that existing test setup is supported.wallet: RTC47bc28896a1a4bf240d1fd780f4559b242bcd945