feat(bond): notify slashed party on dispute slash (#768)#779
Conversation
- Extract slash_reason_recorded(pool, bond_id, expected) from timeout_slash_confirmed - timeout_slash_confirmed now delegates to it keyed on BondSlashReason::Timeout - Pure refactor, no behavior change
- apply_bond_resolution returns the confirmed-slashed bond rows
- non-range/taker rows confirmed via slash_reason_recorded(LostDispute)
- range maker rows returned only when the slice child row is newly inserted
- admin_settle/admin_cancel send a best-effort BondSlashed notice per row
- transient settle failures and idempotent retries yield no row, so a notice
is never untruthful and a winner is never re-notified
- apply_bond_resolution returns one confirmed row per slashed side - both-sides slash returns both rows; null payload returns none - transient settle failure returns no row (notice never untruthful) - non-range and range retries return no row (winner never re-notified) - range slash returns the child slice row with the slice amount
Walkthrough
ChangesPhase 2.5: Dispute-slash BondSlashed forfeiture notice
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/bond/slash.rs (1)
350-354: 💤 Low valueThe cloned
targetmay carry stale state fields.At line 353,
target.clone()copies the bond as it was beforeslash_oneran. The CAS insideslash_oneupdatesstate,slashed_reason,slashed_at, andnode_share_satsin the database, but those changes are not reflected in the returnedBond. If the caller (admin handlers) or downstream code relies on the returned row'sstateorslashed_atfor logging or theSmallOrderpayload, it will see the pre-slashLockedstate instead ofPendingPayout.Currently
notify_bond_slashedonly usespubkey,amount_sats, andorder_idfrom the bond, so this is benign today. However, consider either:
- Re-fetching the bond after confirmation (like the range path does via
find_slice_slash_child)- Documenting that the returned row's metadata fields may be stale
This is low-risk since the notice only needs
amount_satsandpubkey, but it's a potential footgun for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/bond/slash.rs` around lines 350 - 354, The `target` variable cloned into `notify_rows` at line 353 contains stale state because `slash_one` updates the database fields (state, slashed_reason, slashed_at, node_share_sats) but the in-memory `target` is not refreshed. To fix this, after the `slash_reason_recorded` check passes and before pushing to `notify_rows`, re-fetch the bond from the database (similar to how `find_slice_slash_child` works in the range path) to ensure the cloned bond reflects the updated state. Alternatively, if re-fetching is not feasible, add a comment documenting that the bond's metadata fields are stale and will not reflect the post-slash state, cautioning future maintainers that relying on state, slashed_at, or node_share_sats fields could be a footgun.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/bond/slash.rs`:
- Around line 350-354: The `target` variable cloned into `notify_rows` at line
353 contains stale state because `slash_one` updates the database fields (state,
slashed_reason, slashed_at, node_share_sats) but the in-memory `target` is not
refreshed. To fix this, after the `slash_reason_recorded` check passes and
before pushing to `notify_rows`, re-fetch the bond from the database (similar to
how `find_slice_slash_child` works in the range path) to ensure the cloned bond
reflects the updated state. Alternatively, if re-fetching is not feasible, add a
comment documenting that the bond's metadata fields are stale and will not
reflect the post-slash state, cautioning future maintainers that relying on
state, slashed_at, or node_share_sats fields could be a footgun.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 733d547b-4aa3-4633-a3e2-01ba47f1b382
📒 Files selected for processing (4)
docs/ANTI_ABUSE_BOND.mdsrc/app/admin_cancel.rssrc/app/admin_settle.rssrc/app/bond/slash.rs
There was a problem hiding this comment.
I reviewed the current head and I do not see a blocking issue.
What I checked closely:
- The notification is sent only for slashes that are actually confirmed, not merely intended. For non-range rows that confirmation is keyed off the durable
slashed_reasonwitness; for range maker rows it is keyed off a newly inserted slice child row. - That avoids the two failure modes I would care most about here: sending an untruthful
BondSlashednotice after a transient settle failure, and re-notifying on an idempotent admin retry. - The range path also looks correct: the returned row is the slice child with the proportional amount, not the full parent bond, so the notice payload matches what was actually forfeited on that slice.
- The admin handlers keep the notice best-effort and post-confirmation, which is the right failure boundary for this kind of user signal.
The added tests cover the important edge cases well: one-sided slash, both-sided slash, null payload, transient settle failure, non-range retry, range slash, and range retry.
Approved on the current revision.
fix #768
Dispute slashes now send
Action::BondSlashedto each slashed party, the same notice the timeout path already sends. Previously a settle/cancel resolution produced the same order message whether or not a bond was slashed, so the loser had no signal they forfeited a bond.How
apply_bond_resolutionreturns the confirmed-slashed bond rows; the admin settle/cancel handlers send one best-effort notice per row. Confirmation uses the durableslashed_reasonwitness (or a freshly-inserted range slicechild), so a transient settle failure and an idempotent admin retry both yield no notice — never untruthful, winner never re-notified. Amount is the full bond, or the slice's proportional share for range maker bonds.
No protocol/schema change.
How to test manually
[anti_abuse_bond] enabled = true,apply_to = "both".admin-settle/admin-cancelcarrying aBondResolutionthat slashes one side.bond-slashedwith the forfeited amount, right after theadmin-settled/admin-canceledmessage — while the non-slashed party gets no such notice.grep bond-slashed logs.txtshould show one message to the loser only.Commits
Summary by CodeRabbit
Release Notes
New Features
Documentation