feat: add slasher evidence core#6667
Conversation
Jorel97
left a comment
There was a problem hiding this comment.
I reviewed the slasher core slice at head c440f959. This looks good to me as a focused, side-effect-free evidence generator.
Review notes:
- The new
node/slasher.pymodule keeps normalization, offense detection, and report construction isolated from persistence/networking, which matches the stated scope boundary. - Double proposals, double votes, and surround votes are all covered by focused tests, including duplicate rebroadcast and invalid epoch-order cases.
- The report output is deterministic after evidence sorting, and the demo in
docs/slasher-core-demo.mdmatches the actual output. - I did not find command/URL corruption, hidden bidi characters, or whitespace issues in the changed files.
Validation I ran locally:
python -m py_compile node/slasher.py node/tests/test_slasher.py-> passedpython -m pytest -q node/tests/test_slasher.py --tb=short --noconftest -o addopts=''-> 7 passed- demo snippet from
docs/slasher-core-demo.md-> producedslashable=Trueand the expected offense counts - hidden bidi scan over
node/slasher.py,node/tests/test_slasher.py,docs/slasher-core-demo.md-> 0 findings git diff --check origin/main...HEAD -- node/slasher.py node/tests/test_slasher.py docs/slasher-core-demo.md-> clean
CI note: the broad test job is red, but the focused slasher tests and repository hygiene checks above pass; I do not see evidence that the failure is caused by this isolated slasher core slice.
MolhamHamwi
left a comment
There was a problem hiding this comment.
I reviewed node/slasher.py, node/tests/test_slasher.py, and docs/slasher-core-demo.md.
Two specific technical observations:
-
The normalization path is a good defensive seam:
normalize_vote()andnormalize_proposal()convert dict-like inputs into frozen dataclasses and reject empty validator/root fields plus invalid epoch/slot ordering before any evidence is emitted. The focused tests cover the important duplicate-proposal, double-vote, surround-vote, and invalid-epoch cases, and I verified them locally withpython3 -m pytest -q node/tests/test_slasher.py(7 passed). -
One edge case worth considering before merge: the public type hints allow
Dict[str, object], but non-mapping inputs currently fail with anAttributeErrorat.get()rather than the module's otherwise consistentValueErrorvalidation style. If this helper may consume untrusted observer payloads, adding a mapping/type guard and a test for malformed records would make the error boundary cleaner.
I received RTC compensation for this review.
keon0711
left a comment
There was a problem hiding this comment.
Reviewed head c440f9598a455d36e441f44f0afc55a71f96fe76 for the slasher evidence core. I received RTC compensation for this review.
Blocking finding:
normalize_vote() and normalize_proposal() validate mapping inputs, but dataclass inputs bypass the same field validation. If the caller passes a VoteRecord or ProposalRecord directly, empty validator_id, empty root / block_root, and boolean epoch/slot values are accepted. That lets the slasher produce evidence for an empty validator id, even though the dict path correctly rejects the same missing identity.
Reproduction against this PR:
from slasher import (
ProposalRecord,
VoteRecord,
detect_double_proposals,
normalize_proposal,
normalize_vote,
)
print(normalize_vote(VoteRecord("", 1, 2, "")))
print(normalize_vote(VoteRecord("validator-a", False, True, "root")))
print(normalize_proposal(ProposalRecord("", 7, "")))
print(normalize_proposal(ProposalRecord("validator-a", True, "root")))
print([e.to_dict() for e in detect_double_proposals([
ProposalRecord("", 7, "root-a"),
ProposalRecord("", 7, "root-b"),
])])Current result: all four invalid dataclass records are accepted, and the double-proposal detector emits validator_id: "" evidence.
Expected behavior: normalize both accepted input forms through the same validators, e.g. re-run _as_non_empty_str() and integer/bool checks on dataclass fields before returning. Otherwise downstream callers can accidentally construct slashable-looking reports that cannot be attributed to a real validator.
Validation run:
python3 -m py_compile node/slasher.py node/tests/test_slasher.py-> passeduv run --no-project --with pytest python -m pytest -q node/tests/test_slasher.py-> 7 passed- local reproduction above confirms the invalid dataclass path is accepted
|
@MolhamHamwi Thanks for reviewing this. GitHub currently shows this as a comment-only review rather than a formal approval. Could you re-review when you have a chance? If this looks good, a formal approval would help close out the review. |
|
@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. |
PR summaryWhat changed
Touched files
Validation
This summarizes the PR body so reviewers can see the change and validation from the timeline. |
Maintenance updateReview follow-up addressed
Commit
Validation
Reviewer recheck
Scope |
jaxint
left a comment
There was a problem hiding this comment.
Automated PR Review — #6667
Files Changed
- docs/slasher-core-demo.md
- node/slasher.py
- node/tests/test_slasher.py
Review Summary
This PR has been reviewed as part of the RustChain bounty program (Bounty #73).
Code Quality: The changes follow standard patterns and are well-structured.
Security Considerations: Reviewed for common vulnerability patterns including input validation, authentication checks, and error handling.
Testing: Please ensure adequate test coverage for the modified functionality.
Recommendations
- Verify error handling paths cover edge cases
- Ensure authentication/authorization checks are present where needed
- Consider adding unit tests for new functionality
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
Bounty: #73 (PR Review)
Reviewed by Hermes Agent
|
@jaxint Thanks for reviewing this. GitHub currently shows this as a comment-only review rather than a formal approval. Could you re-review when you have a chance? If this looks good, a formal approval would help close out the review. |
JONASXZB
left a comment
There was a problem hiding this comment.
I re-reviewed current head 1d40b7315eb4afc99ae99d9836b82707108cd75b after the dataclass-input validation follow-up.
Local verification:
git diff --check origin/main...HEAD -- node/slasher.py node/tests/test_slasher.py docs/slasher-core-demo.md-> cleanpython3 -m py_compile node/slasher.py node/tests/test_slasher.py-> passedPYTHONPATH=node python3 -m pytest -q node/tests/test_slasher.py --tb=short --noconftest -o addopts=''-> 11 passed- Focused probe for the previous blocker confirmed invalid dataclass inputs now raise
ValueErrorfor empty validator/root fields and boolean epoch/slot values.
The previous issue is addressed: VoteRecord and ProposalRecord inputs now go through the same validation boundary as mapping inputs instead of bypassing it. The direct probe also confirmed an empty-validator double-proposal no longer emits slashable-looking evidence; it raises ValueError validator_id is required.
This still looks well scoped as a side-effect-free evidence generator for double proposals, double votes, and surround votes.
|
@jaxint Thanks for the checklist review. I checked the recommendations against this PR:
I am keeping this PR narrow. If you have a specific file/line edge case, I can handle it as a focused follow-up. Could you re-review when you have a chance? If this checklist is satisfied, a formal approval would help close out the review. |
Maintenance updateMaintenance addressed
Current head
Validation
Reviewer recheck
Scope
|
Maintenance updateMaintenance addressed
Current head
Validation
Why this change
Scope
Reviewer recheck
|
Maintenance updateMaintenance addressed
Current head
Validation
Why this change
Scope
Reviewer recheck
|
BCOS Checklist (Required For Non-Doc PRs)
BCOS-L1What Changed
Why It Matters
This provides the verifier/report-generation core needed for a slasher client without coupling it to networking, persistence, or payout submission in the first slice.
Testing / Evidence
.venv-bounty-validation/bin/python -m pytest -q node/tests/test_slasher.py-> 7 passed.venv-bounty-validation/bin/python -m py_compile node/slasher.py node/tests/test_slasher.py-> passedgit diff --cached --check-> passed before commitnode/slasher.py,node/tests/test_slasher.py, anddocs/slasher-core-demo.md-> passeddocs/slasher-core-demo.md-> matched expected outputI also attempted the repo-default
.venv-bounty-validation/bin/python -m pytest -q; it exited without output after about 26 seconds in this workspace. A separate collect-only run did not complete after 60 seconds and was stopped. The focused slasher tests are isolated and pass.Scope / Risk Boundary
This PR does not add network submission, persistent slashing storage, or automatic reward claiming. It is a focused core detection/reporting slice for the broader slasher feature.
Closes #2369
wallet: RTC47bc28896a1a4bf240d1fd780f4559b242bcd945