feat: add slashing penalty core#6674
Conversation
zqleslie
left a comment
There was a problem hiding this comment.
Review of PR #6674 — feat: add slashing penalty core
Reviewed head 93b70b77ae1c170347e8e73d72e8e73d72e8e73d (3 files, 395 LOC + 103 LOC test).
✅ Strengths
Idempotency by evidence hash (line 184-196): The validator_slashes table uses evidence_hash as the PK, and the apply_slashing_evidence() function returns early with {"duplicate": True} if the hash already exists. This prevents replay attacks that could drain a validator balance — critical for a slashing module.
Schema-agnostic balance lookup (line 300-328): _get_balance_urtc() probes for both amount_i64 and balance_rtc columns, and even checks alternative key columns (miner_id / miner_pk / wallet). This is a pragmatic approach for a codebase with legacy schemas.
Epoch enrollment cleanup (line 356-372): _remove_future_enrollments() deletes rows where epoch > current_epoch AND epoch <= slashed_until_epoch, which correctly prevents a slashed validator from being selected in future epochs during the exclusion window.
⚠️ Issues
1. SQL injection risk in PRAGMA query (line 385)
def _table_columns(conn, table_name):
return tuple(row[1] for row in conn.execute(f"PRAGMA table_info({table_name})").fetchall())table_name is interpolated directly into the SQL string. While callers currently pass hardcoded values ("balances", "epoch_enroll"), this is a footgun — if any caller ever passed user-influenced input, it would be injectable. Recommend using parameterized query or at minimum validating against a whitelist of known table names:
VALID_TABLES = {"balances", "epoch_enroll", "validator_slashes", "slashed_validators"}
if table_name not in VALID_TABLES:
raise SlashingError("unknown_table")2. Balance underflow edge case in _debit_balance (line 330-354)
When using balance_rtc (REAL column), the penalty_rtc = penalty_urtc / URTC_PER_RTC division can introduce floating-point rounding. If the balance is very close to the penalty (e.g., 0.10000001 RTC with a 0.10 RTC penalty), floating-point comparison balance_rtc >= penalty_rtc could be unreliable. The CASE WHEN ... ELSE 0 provides a floor, but the subtraction could produce -1e-15 due to IEEE 754, which is then set to 0. This is safe in practice but worth documenting, or using integer math throughout.
3. No transaction boundary (line 183-234)
apply_slashing_evidence() performs multiple writes (INSERT into validator_slashes, INSERT/UPDATE slashed_validators, DELETE from epoch_enroll) without wrapping them in a single transaction. If the process crashes between the balance debit and the enrollment cleanup, the ledger state would be inconsistent (funds burned but validator still enrolled). Consider:
with conn: # auto-commits on success, rolls back on exception
...📝 Minor
- The test suite mocks
rustchain_cryptoat import time — this is fragile if the module gains new dependencies. Consider usingpytestfixtures orunittest.mock.patchfor more resilient test isolation. _table_columnsusesPRAGMA table_infowhich returns results in column-definition order, not sorted. The_first_presentlogic works correctly regardless, but this is worth a comment.
Verdict: Approve with suggestions. Solid first slice of slashing infrastructure.
I received RTC compensation for this review.
|
Reviewing under the open PR-review bounty (#2782). I ran the focused test suite locally:
Two things look worth tightening before this becomes reachable from a route or settlement pipeline:
These are not style issues; they affect whether the slashing record is an auditable proof ledger versus a caller-selected dedupe key plus a generic penalty switch. |
|
Correction on the bounty reference above: I meant the tracker issue in |
|
@autochamchikim-pixel 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. |
|
@zqleslie 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. |
|
@autochamchikim-pixel 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. |
Maintenance updateReview follow-up addressed
Commit
Validation
Reviewer recheck
Scope |
BCOS Checklist (Required For Non-Doc PRs)
BCOS-L1What Changed
epoch_enrollrows.Why It Matters
This gives #2327 a concrete penalty/exclusion mechanism once double-vote, double-proposal, surround-vote, or equivocation evidence has been verified. It keeps the first slice side-effect-bounded: no networking changes, no consensus rewiring, and no automatic evidence submission path.
Validation
.venv-bounty-validation/bin/python -m py_compile node/slashing_penalties.py node/tests/test_slashing_penalties.py-> passed.venv-bounty-validation/bin/python -m pytest -q node/tests/test_slashing_penalties.py --tb=short-> 7 passed.venv-bounty-validation/bin/python -m ruff check node/slashing_penalties.py node/tests/test_slashing_penalties.py-> passeddocs/slashing-penalty-demo.md-> matched expected outputgit diff --check upstream/main...HEAD-> passedupstream/main...HEADchanged files -> passedI also attempted the repo-default
.venv-bounty-validation/bin/python -m pytest -q. After installing the immediate missing test dependencies available for Python 3.9, collection still stopped on unrelated optional dependencies such asyamlandmatplotlib; the focused slashing penalty tests above are isolated and pass.Scope / Risk Boundary
Closes #2327
wallet: RTC47bc28896a1a4bf240d1fd780f4559b242bcd945