Feature(pos): add validator stake delegation with delegator rewards and undelegation cooldown - #54
Conversation
… compose
The integration tests have been running against a STALE binary cached
on the self-hosted runner, not the binary built from this PR. Forensic
chain established from artifact `~/Downloads/logs_68905250139.zip`:
- Source on this branch (and merge ref `de70010`): `node v0.4.13`,
`multi_parent_casper_impl.rs` is an 8-line shim, the slashing surface
lives under `casper_engine/{snapshot,dispatch,finalization_runner,
block_admission,…}.rs`.
- Build job DID compile our code (build log: `Compiling casper v0.1.0
(/build/casper)`, finishes in 3m28s, `Compiling node v0.4.13`).
- Runtime binary reports `F1r3node Rust 0.4.2` and emits `tracing`
events with `filename:"casper/src/rust/multi_parent_casper_impl.rs",
line_number:1767` and log strings (`"Received DeployData..."`) that
exist NOWHERE in our git history. No `casper_engine/*.rs` filename
references at all.
Root cause:
`system-integration/integration-tests/docker-compose.rust.yml:21`:
image: ${F1R3FLY_RUST_IMAGE:-f1r3flyindustries/f1r3fly-rust-node:latest}
Our CI tags the freshly-built image as
`f1r3flyindustries/f1r3fly-rust:{amd64,latest}` (note: no `-node`
suffix). It sets `DEFAULT_IMAGE` for `pytest`, but compose reads
`F1R3FLY_RUST_IMAGE` — not `DEFAULT_IMAGE`. With `F1R3FLY_RUST_IMAGE`
unset, compose falls back to its default
`f1r3flyindustries/f1r3fly-rust-node:latest` — a DIFFERENT image
repository name. That tag had been pinned on the self-hosted runner
by an earlier process; compose silently used it for every PR build.
Fix:
- `Clean self-hosted runner state`: delete any stale
`f1r3fly-rust-node:latest` so the compose default cannot resolve to
cached state.
- `Import Docker Image`: after loading and tagging
`f1r3flyindustries/f1r3fly-rust:{arch,latest}`, also tag the same
image as `f1r3flyindustries/f1r3fly-rust-node:latest` so the compose
default points at THIS build.
This unblocks the slashing PR by making the integration tests actually
exercise the code they're meant to test. All the "regressions" we've
been debugging (heartbeat-shard, cross-validator stalls, LFB freezing
at #54) were against a pre-`f0b2934` binary and may not reflect any
real defect in this PR's changes — we'll know once the next run
produces a clean baseline.
Verification after push:
- `gh run download <new_id> --name integration-logs-amd64`
- `grep 'F1r3node Rust' integration-tests.log` should show `0.4.13`.
- `grep -oE '"filename":"casper/src/rust/[^"]+\.rs"' integration-tests.log
| sort -u` should include `casper_engine/*` paths.
|
Delegating stake permanently forks the network — Tested this branch (built as Timeline (namespace
It's proposing into a void — no peer will ever accept a descendant of the block it already flagged invalid. No panic, no crash in any node's error log — this is a silent, permanent consensus fork. Root cause (best guess, not 100% confirmed by reading
I checked: this diff does not touch Keeping To reproduce: 3 genesis validators (1000 REV self-bond each), one funded non-validator account, submit a Full logs (all 3 validators + bootstrap + observer, plus the harness's own read of |
|
@machieke Please take a look |
7a7f666 to
ee63bcd
Compare
GitHub Actions
Error
|
Rebased I also addressed and validated 9Neechan’s specific concern about delegation causing peers to reject blocks with The proposer and validation paths now agree on effective stake semantics for the consensus bond cache: runtime bond-cache derivation uses Validation performed against the exact reported live scenario:
|
ee63bcd to
d5183a3
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Waiting
jeffrey-l-turner
left a comment
jeffrey-l-turner
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review
Verdict: 🔍 Needs Review (100.0000% agreement)
Reviewed by:
| Provider | Model | Verdict | Confidence |
|---|---|---|---|
| anthropic | claude-fable-5 |
Abstain | 0 |
| bedrock | us.amazon.nova-pro-v1:0 |
Needs Review | 0.9 |
| openai | gpt-5.6-sol |
Needs Review | 0.94 |
| openrouter | moonshotai/kimi-k3 |
Needs Review | 0.68 |
| xai | grok-4.5 |
Abstain | 0 |
Summary
The delegation lifecycle is broadly implemented and tested, but the current design introduces significant epoch-accounting and consensus-snapshot concerns. Delegations affect both reward allocation and runtime stake reads immediately, enabling reward sniping and potentially changing consensus weights mid-epoch. The effective-bond cap also needs enforcement across every self-bond mutation path, and the unbounded per-delegator reward scan creates a recurring liveness risk. These issues should be resolved or explicitly validated against the protocol's epoch and consensus invariants before merge.
This PR implements a substantial and mostly coherent delegation lifecycle, including effective stake integration, cooldown withdrawals, reward claiming, and slashing updates. However, the economic/security surface is large: pending undelegations are slashable during cooldown, cooldown is hardwired to epoch length, and reward distribution uses truncation-heavy integer math plus expensive full-map folds each epoch. I would not merge without protocol-level review of slashing semantics for pending withdrawals, explicit undelegation cooldown configuration, and performance hardening for reward/delegation accounting.
The code introduces significant changes to support delegation in the PoS contract. While the overall approach seems sound, there are some potential security and logic issues that need to be addressed, along with minor style improvements and documentation suggestions. A thorough review and testing are recommended before merging.
Issues Found
🔴 Critical: Pending undelegation makes stake instantly slashable and may be griefable (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 652)
The contract removes delegated stake from effective bonds immediately at request time, while leaving principal locked in PoS vault as pending undelegations. If a slash happens during cooldown, both active delegation and pending undelegation are confiscated (slash transfers valBond + valDelegated + slashedPending). This is economically consistent but creates a hard griefing path: any invalidBlocks entry can burn pending withdrawals right before users claim them. In this diff, tests even assert that completeUndelegate cannot claim slashed pending stake, which confirms funds can be permanently seized during cooldown. Given slash authority may be broad in some environments, this materially increases loss surface for delegators.
Suggestion:
Add explicit governance/authorization controls around slash-triggered confiscation of pending undelegations (or delay when pending becomes slashable), and document threat model. Consider separate escrow accounting with delayed confiscation eligibility or dispute window.
🟠 Major: rewardsInfo now performs multiple full-map folds on every call (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 308)
rewardsInfo computes effectiveBonds fold, withdrawers fold, committedRewards fold, delegatorRewards fold, and pendingUndelegations nested fold. These are O(total state) and called by getCurrentEpochRewards, which itself folds all bonds. On large validator/delegator sets, closeBlock and read paths become expensive.
Suggestion:
Maintain running aggregates in state (totalBonds, totalCommittedRewards, totalDelegatorRewards, totalPendingUndelegations) updated incrementally on each mutating operation.
🟠 Major: Unbounded delegation maps create recurring closeBlock work (Reported by: openai)
File: casper/src/main/resources/PoS.rhox (line 315)
Delegation accepts any positive amount and imposes no minimum delegation or limit on delegators per validator. Epoch processing then scans the complete nested delegations map to calculate rewards, and rewardsInfo also scans effective bonds and every pending undelegation. An attacker can create many one-unit delegations, paying the creation cost once while imposing repeated system-contract work on every subsequent closeBlock. This creates state-growth and consensus-liveness risk.
Suggestion:
Set a meaningful minimum delegation and bounded delegator count, or use an accumulated reward-per-share index so epoch processing is O(number of validators) and each delegator settles rewards only when delegating, undelegating, or claiming.
🟠 Major: Potential integer overflow in delegation amount check (Reported by: bedrock)
File: casper/src/main/resources/PoS.rhox (line 355)
The code checks if the delegation amount would exceed the validator's maximum effective bond by comparing sums of integers. In languages like Rholang, integer overflow can occur if the sum exceeds the maximum representable integer value. This could lead to unexpected behavior or security vulnerabilities. Consider using arbitrary precision arithmetic or adding explicit overflow checks.
Suggestion:
Use arbitrary precision arithmetic or add explicit overflow checks to prevent integer overflow.
🟠 Major: computeDelegatorRewardDeltas scales O(delegations x validators) (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 360)
Reward distribution iterates every delegator and every validator mapping to compute shares each epoch. With many delegators this becomes quadratic-ish and can blow up block processing cost.
Suggestion:
Store per-validator delegator lists or aggregate share indexes to compute rewards per validator with direct lookups. Consider checkpoint/lazy-claim accounting patterns.
🟠 Major: Reward accounting is vulnerable to truncation drift and silent loss (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 459)
Validator/delegator split uses integer division in multiple places (reward * selfBond / effectiveBond and validatorReward * delegatedAmount / effectiveBond). Over many delegators and epochs, truncation can leave dust unallocated in vault while total liabilities grow. Because getCurrentEpochRewards subtracts totalDelegatorRewards and totalPendingUndelegations but not rounding dust explicitly, accounting may drift and either underpay future rewards or leave unclaimable residue.
Suggestion:
Add explicit dust bucket in state (e.g. rewardRemainder) or deterministic largest-remainder allocation. Add property tests comparing vault balance vs sum of all claimable liabilities.
🟠 Major: Incomplete handling of slashed pending undelegations (Reported by: bedrock)
File: casper/src/main/resources/PoS.rhox (line 465)
When a validator is slashed, the code removes the validator from the active set and adjusts the bonds, but it does not explicitly handle pending undelegations. This could lead to orphaned pending undelegation entries that are no longer associated with an active validator.
Suggestion:
Ensure that pending undelegations are properly cleaned up when a validator is slashed.
🟠 Major: Delegator identity derived from deployerId may collide across auth contexts (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 574)
delegate/undelegate/claim use getUser!(deployerId) and vaultAddressOps!(fromDeployerId) for payout. If deployerId mapping to public key/vault is not strictly one-to-one across all auth modes, reward/principal claims could be misrouted or blocked. This is especially important for contract-controlled deployers or wrapper wallets.
Suggestion:
Document identity assumptions and add negative tests for alternate auth paths. If supported, separate delegation owner key from payout destination.
🟠 Major: Delegation can capture a full epoch of rewards immediately (Reported by: openai)
File: casper/src/main/resources/PoS.rhox (line 576)
The delegate operation updates delegations and delegatedTotals immediately, while closeBlock calculates the entire epoch's rewards from the state present at close time. A delegator can therefore delegate immediately before closeBlock and receive a proportional share of fees accumulated before their stake was present. Similarly, undelegating immediately before closeBlock forfeits rewards for stake that was present for most of the epoch. This enables epoch-boundary reward sniping and unfairly dilutes long-lived delegators and validators.
Suggestion:
Activate delegation and undelegation changes only at epoch boundaries, or maintain epoch stake snapshots/reward-per-share indices with per-delegation reward debt so rewards are based on the duration for which stake was active.
🟠 Major: Maximum effective bond cap is enforced only when delegating (Reported by: openai)
File: casper/src/main/resources/PoS.rhox (line 590)
delegate checks selfBond + delegatedTotal + amount against maximumBond, but the invariant also has to be enforced by every path that can increase a validator's self-bond. The existing bond/update processing shown elsewhere is not updated here to account for delegatedTotals. If a validator can increase or replace its self-bond after receiving delegation, it can push its effective bond above maximumBond despite the new check.
Suggestion:
Centralize the effective-bond invariant and call it from delegate, bond, rebond, and bond-update processing. Add a test that delegates up to the cap and then attempts to increase the validator's self-bond.
🟠 Major: Maximum-bond check uses stale pre-update components and can reject/accept incorrectly (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 601)
delegate computes checks from selfBond and delegatedTotal before deposit and state mutation, but all checks are against current values only. In concurrent execution, other delegations could land between read and commit and push validator over maximumBond. runMVar serializes state mutation, but the checks occur inside same serialized block with stale snapshots from that block only; that part is okay. The subtle issue is that cap logic uses selfBond > maximumBond and delegatedTotal > maximumBond - selfBond style checks, which allows weird behavior when selfBond already equals maximumBond but delegatedTotal nonzero is impossible by construction. More importantly, repeated delegations from same delegator are allowed but pending undelegations are restricted to one per pair, creating asymmetry that can trap users.
Suggestion:
Either allow multiple pending undelegation entries per pair (list/queue) or enforce one active delegation lifecycle per pair in delegate too. Add invariants tests around concurrent multi-delegation and repeated delegate/undelegate cycles.
🟠 Major: Cooldown tied directly to epochLength is inflexible and can be very short/long (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 700)
undelegate sets unlockBlock = blockNumber +
Suggestion:
Introduce explicit undelegationCooldownBlocks parameter (or reuse quarantineLength deliberately) and wire it through config + tests + docs.
🟠 Major: Consensus-visible stake changes immediately during an epoch (Reported by: openai)
File: casper/src/rust/rholang/runtime.rs (line 1375)
Runtime bond reads now return getEffectiveBonds, and delegate/undelegate mutate delegatedTotals immediately rather than through the epoch transition. If this runtime query supplies consensus or fork-choice weights, a transaction can alter voting weight in the middle of an epoch even though active validator selection is only refreshed by closeBlock. Mid-epoch stake changes can invalidate assumptions that validator weights are fixed for a consensus epoch and may cause different protocol components to use inconsistent stake snapshots.
Suggestion:
Confirm that all consensus consumers use a single epoch snapshot. Prefer maintaining current-epoch and next-epoch effective bond maps, applying delegation changes at closeBlock, and having runtime consensus reads return only the immutable current-epoch map.
🟡 Minor: Inconsistent indentation (Reported by: bedrock)
File: casper/src/main/resources/PoS.rhox (line 170)
The code has inconsistent indentation, which can make it harder to read and maintain. Ensure consistent indentation throughout the file.
Suggestion:
Apply consistent indentation (e.g., 4 spaces) throughout the file.
🟡 Minor: Large nested helper contracts reduce readability and auditability (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 300)
New helpers (removeDelegationsForValidator, stripPendingUndelegationsForValidator, computeDelegatorRewardDeltas) are deeply nested with repeated map updates and inline branching. This is hard to audit and easy to regress.
Suggestion:
Refactor into smaller pure map utilities with unit-style tests per helper and invariant assertions.
🟡 Minor: Reward weighting truncates delegated stake in minimumBond-sized steps (Reported by: openai)
File: casper/src/main/resources/PoS.rhox (line 397)
getCurrentEpochRewards computes validator weight with bonds / minimumBond and activeBonds / minimumBond. Delegation amounts are unrestricted, so delegated stake below the next minimumBond boundary can increase consensus weight without increasing reward weight at all, while crossing the boundary causes a discontinuous jump. The later validator/delegator split uses exact effectiveBond values, producing inconsistent stake weighting between reward allocation and reward ownership.
Suggestion:
Use an overflow-safe multiply-then-divide implementation with exact effective bonds, such as pool * bond / activeBonds using arbitrary precision or checked arithmetic, rather than dividing each bond by minimumBond first.
🟡 Minor: completeUndelegate transfers before state cleanup (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 728)
The method transfers funds first and only then deletes pending undelegation. If transfer succeeds but subsequent state update fails unexpectedly, user could potentially double-claim. Rholang execution model may make this atomic in practice, but the code ordering increases risk and complicates reasoning.
Suggestion:
If platform semantics allow, mark claim as consumed before external transfer or add reentrancy guard / claim lock pattern.
🟡 Minor: Slash now depends on stale invalidBlocks mapping hash-to-validator without binding block context (Reported by: openrouter)
File: casper/src/main/resources/PoS.rhox (line 926)
Slash uses invalidBlocks.get(blockHash) and then slashes validator globally, including delegations and pending undelegations. If hash collisions or stale mappings exist, this can punish the wrong validator set. The diff doesn’t add additional validation of epoch/context for the invalid block.
Suggestion:
Bind invalidBlocks entries to (validator, blockHeight/epoch) and enforce slashing only within a bounded evidence window.
🟡 Minor: Runtime now reads effective bonds but method name still generic (Reported by: openrouter)
File: casper/src/rust/rholang/runtime.rs (line 1375)
Runtime stake query switched from getBonds to getEffectiveBonds, but any downstream consumers expecting raw self-bonds may silently change behavior. This is likely intended but should be flagged in API/changelog.
Suggestion:
Add explicit runtime API naming or versioned method to distinguish self-bond vs effective-bond consumers.
🟡 Minor: Critical economic parameters need explicit operator guidance (Reported by: openrouter)
File: docs/casper/POS_STAKE_DELEGATION.md (line 1)
Docs mention delegation lifecycle but should explicitly call out: cooldown source (epochLength), one pending undelegation per pair, slash exposure during cooldown, reward rounding behavior, and max effective bond checks.
Suggestion:
Add an operator-facing risk table and migration notes for validators/delegators.
⚪ Suggestion: Add comments for complex logic (Reported by: bedrock)
File: casper/src/main/resources/PoS.rhox (line 170)
The code contains complex logic for handling delegations, undelegations, and rewards. Adding comments to explain the purpose and behavior of each function and contract would improve readability and maintainability.
Suggestion:
Add descriptive comments to explain the purpose and behavior of each function and contract.
View individual reviewer assessments
anthropic (claude-fable-5)
ABSTAIN: invalid_request_error: Your credit balance is too low to access the Anthropic API. Please go to Plans & Billing to upgrade or purchase credits.
bedrock (us.amazon.nova-pro-v1:0)
The code introduces significant changes to support delegation in the PoS contract. While the overall approach seems sound, there are some potential security and logic issues that need to be addressed, along with minor style improvements and documentation suggestions. A thorough review and testing are recommended before merging.
openai (gpt-5.6-sol)
The delegation lifecycle is broadly implemented and tested, but the current design introduces significant epoch-accounting and consensus-snapshot concerns. Delegations affect both reward allocation and runtime stake reads immediately, enabling reward sniping and potentially changing consensus weights mid-epoch. The effective-bond cap also needs enforcement across every self-bond mutation path, and the unbounded per-delegator reward scan creates a recurring liveness risk. These issues should be resolved or explicitly validated against the protocol's epoch and consensus invariants before merge.
openrouter (moonshotai/kimi-k3)
This PR implements a substantial and mostly coherent delegation lifecycle, including effective stake integration, cooldown withdrawals, reward claiming, and slashing updates. However, the economic/security surface is large: pending undelegations are slashable during cooldown, cooldown is hardwired to epoch length, and reward distribution uses truncation-heavy integer math plus expensive full-map folds each epoch. I would not merge without protocol-level review of slashing semantics for pending withdrawals, explicit undelegation cooldown configuration, and performance hardening for reward/delegation accounting.
xai (grok-4.5)
ABSTAIN: request timed out after 115s (model: grok-4.5, tools: false)
Generated by Multi-Agent Review System
3 participants
Summary
This PR extends PoS to support delegation from external accounts to existing validators, and hardens the full delegation lifecycle:
What changed
PoS contract (PoS.rhox)
- claimDelegatorRewards
- getDelegatorRewards
- undelegate now creates pending withdrawal (no immediate principal transfer)
- completeUndelegate returns principal after unlock
- getPendingUndelegations
Runtime integration (casper/src/rust/rholang/runtime.rs)
Tests (PoSTest.rho)
- delegation success
- undelegation cooldown + completion
- delegator rewards claim
- withdraw restrictions with active delegations
Docs
- docs/casper/POS_STAKE_DELEGATION.md
Behavioral impact
Validation
- cargo test -p casper --test mod pos_spec -- --nocapture
Notes / follow-ups