feat(stake): add liveness fallback to prevent empty validator set - #356
feat(stake): add liveness fallback to prevent empty validator set#356musitdev wants to merge 2 commits into
Conversation
- emit EmptyElectionFallback event in on_new_epoch() when election yields no qualifying validators, retaining the previous active set with refreshed voting power - extract estimate_validator_power() helper; mirror the fallback in next_validator_consensus_infos() so the consensus layer gets a consistent next-epoch preview - add a second validator in delegation pool and staking contract tests so the normal exit path (INACTIVE) is exercised without triggering the fallback
areshand
left a comment
areshand
left a comment
There was a problem hiding this comment.
One parity concern in the consensus preview path.
| @@ -1410,54 +1493,67 @@ module aptos_framework::stake { | |||
| } else { | |||
| vector::borrow(&cur_validator_set.pending_active, candidate_idx - num_cur_actives) | |||
There was a problem hiding this comment.
next_validator_consensus_infos() still walks pending_active in forward order here, but on_new_epoch() first merges pending_active through append(), and append() uses pop_back(), which reverses that segment before the real election runs. Upstream a818a43233 reversed the preview traversal to match the installed next-epoch set. As written, the preview can still disagree with on_new_epoch() on validator order and validator_index assignment whenever pending_active is non-empty, so preview/order parity is not fully preserved.
There was a problem hiding this comment.
Good catch, I've corrected the order and added a test to detect bad ordering.
|
On hold until further optimization has been done on the staking module. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| vector::push_back(&mut retained, info); | ||
| }); | ||
| validator_set.active_validators = retained; | ||
| validator_set.total_voting_power = retained_power; |
There was a problem hiding this comment.
[major] Fallback can retain zero-stake validators producing total_voting_power = 0
When the election is empty, the fallback at lines 1321-1344 retains every entry in validator_set.active_validators with voting_power computed by generate_validator_info / get_next_epoch_voting_power. That helper returns active + pending_active + pending_inactive, with no minimum-stake floor and no liveness check. If every retained validator already had zero stake (as exercised by your own test_liveness_fallback_retains_validator_when_election_is_empty which asserts total_emergency_voting_power == 0, total_emergency_voting_power == 0), the chain transitions into a new epoch with a non-empty active set but total_voting_power = 0.
What that breaks downstream:
- Consensus quorum math (>2/3 of total_voting_power) becomes degenerate when total is 0. The Rust consensus stack does not guarantee well-defined behavior in this regime, every signed vote contributes 0 weight, no proposal ever crosses the (trivial or infinite, depending on integer division) quorum threshold.
- The protocol enters a self-reinforcing state, the next epoch transition will once again find no qualifying candidates (their stake is still 0), call the fallback again, re-emit EmptyElectionFallback, and stay stuck. There is no automatic exit path: the validators have no active stake to generate rewards, and rewards are computed in calculate_rewards_amount(cur_active, ...) which short-circuits to 0 when cur_active == 0 (see estimate_validator_power line 1430).
In practical terms, the PR converts a clean halt (zero-stake elects empty set, error visible) into a silent permanent stall (zero-stake elects itself, looks alive in the validator set but cant make progress). Operators may discover this much later because get_validator_state returns ACTIVE.
Attack Path:
- Through any mechanism that drives all validators below minimum_stake (governance error, mass slashing, mass unlock + lockup expiry), the active set ends up with sum(active) == 0.
- on_new_epoch runs, election produces empty next_epoch_validators, fallback retains all with voting_power = 0.
- validator_set.total_voting_power = 0 is installed. EmptyElectionFallback emitted with total_emergency_voting_power = 0.
- Consensus cannot produce a certified block, the chain is non-progressing but no halt event is raised, and the next epochs election will again find 0-stake candidates and re-trigger the fallback indefinitely.
Suggested fix: at minimum, refuse to install a fallback set whose total_voting_power is 0. Prefer the upstream halt-on-empty behavior in that case so operators are alerted unambiguously, or fail an assertion. Something like:
if (vector::is_empty(&next_epoch_validators)) {
let retained = vector::empty<ValidatorInfo>();
let retained_power = 0u128;
vector::for_each_ref(&validator_set.active_validators, |vi| { ... });
assert!(retained_power > 0, ENO_LIVENESS_FALLBACK_POSSIBLE);
validator_set.active_validators = retained;
validator_set.total_voting_power = retained_power;
...
}Or, separately, expose a recovery primitive (governance-only) to drop the fallback retention and reset the set, so an operator has a defined escape hatch.
Confidence: 0.85 the logical chain is direct; the consensus-layer behavior at total_voting_power = 0 is presumed-bad but unconfirmed in this audit (downstream Rust code path).
Category: liveness / silent-failure
There was a problem hiding this comment.
The goal of the PR is to always have a non-empty validator set, so if one of the validator increase its staking the network can restart.
If all validator has zero stake the network is degenerated, if you send an error during epoch change you block the network, and no block will be processed. Allowing to keep a non-empty validator set allow processing the epoch and update validator staking later.
The argument The protocol enters a self-reinforcing state is wrong for me. If you update the validator stake the validator-set staking is updated, and you leave the empty case. Only if you have an empty set during epoch change update you enter the loop. It's a protection again empty validator set. It doesn't force anything and the exit is restaking of the network.
I think it's a false positive:
- Epoch change should never send an error. It halts the network.
- The update is only a protection again empty validator-set to allow processing new block and correct the staking issue. It doesn't create a self-reinforcing state.
There was a problem hiding this comment.
@rubujubi as indicated here and in the next comments is that the comment of the agent are incorrect on several aspects.. I wonder if its worth to have one or several new review agents at the end (new to drop the previous context window to loose bias) and that should re-evaluate the confidence level at the end ?
There was a problem hiding this comment.
Thanks for the feedback, let me see how it can be optimized, I will try to add another layer to review the result first. I think for the minor/informational ones they are fine? Currently it is prompted in a way to try to find vulnerability that can be attacked so maybe that led to false positives.
There was a problem hiding this comment.
I will request feedback from copilot for comparison @musitdev you can ignore those
|
|
||
| validator_set.active_validators = next_epoch_validators; | ||
| validator_set.total_voting_power = total_voting_power; | ||
| if (vector::is_empty(&next_epoch_validators)) { |
There was a problem hiding this comment.
[major] Fallback has no exit path; once entered, every subsequent epoch re-triggers it
After the fallback installs the previous active set, the post-fallback loop (line 1352 onward) renews lockups and resets perf counters. But the retained validators are exactly the ones that just failed the election, their StakePool.active is unchanged by the fallback path (its only the active_validators vector and the voting_power field that get overwritten, not the underlying balances). The next on_new_epoch will again find every validator below minimum_stake and trigger the fallback. No invariant or governance action breaks the cycle automatically.
This is partly by design (the fallback is intended as a last-ditch liveness preservation pending operator action), but a few things make it fragile:
- There is no on-chain signal beyond the event that the chain is stuck in fallback mode. Tooling that polls ValidatorSet sees a non-empty active set and reports healthy.
- The fallback re-runs every epoch but only the first emission is unambiguous, subsequent re-emissions look identical and operator tooling has to dedupe by epoch (and EmptyElectionFallback carries no epoch field; see footgun in summary).
- If new validators arrive with sufficient stake, they enter pending_active. on_new_epoch line 1284 appends them into active_validators before the election loop. They will be elected normally (the fallback only runs if the resulting next_epoch_validators is empty), which is correct, but the previously-retained zero-stake validators get dropped silently. Worth verifying the test coverage for this transition out of fallback.
Attack Path:
- Chain enters fallback (see prior major finding).
- EmptyElectionFallback emitted once.
- Every subsequent epoch, the fallback fires again but emits the same event. Operator dashboards that count occurrences see N epochs of fallback but no escalation in severity.
Suggested fix: emit a distinct sticky-state resource (e.g., InEmergencyMode with the epoch number and the retained set) so the persistent fallback state is queryable and tooling can alert on it. Add an explicit governance-callable function to exit emergency mode by replacing the active set with a fresh nomination, instead of relying on the next election to organically succeed.
Confidence: 0.8 design judgement; the underlying recurrence is mechanical and unambiguous.
Category: operational / silent-state
There was a problem hiding this comment.
You can't correct the minimum staking during the epoch change. The issue has been done before, and the idea is to avoid having a validator-set empty. Keeping a validator-set non-empty allow to increase the staking before the next epoch change.
An event is emitted, so this issue can be monitored.
If the epoch change is blocked (error emitted), you can't change anything later, so the solution proposed using a governance won't work.
What we can add from the proposition is a governance call that allow to define the new staking values to exit the empty validator-set loop. But even this proposition already exist by adding some stake, the normal way.
I think the IA focus on the code update but don't see that it's a consequence of other actions not related to epoch change. The easier way to avoid the issue is to block any staking removal if it put the network into a too low staking state for example.
So for me, it's a false positive. It doesn't correct anything to the algo and the proposition (emit an error) can be worth.
| assume cur_active + cur_pending_active + cur_reward <= MAX_U64; | ||
| assume cur_active + cur_pending_active + cur_pending_inactive + cur_reward <= MAX_U64; | ||
| }; | ||
| let power = cur_active |
There was a problem hiding this comment.
[minor] estimate_validator_power and generate_validator_info compute voting_power differently in the same epoch
In on_new_epoch the fallback calls generate_validator_info (line 1330) which delegates to get_next_epoch_voting_power (line 1829):
value_pending_active + value_active + value_pending_inactiveNote: no lockup-expiry adjustment, no reward computation.
In next_validator_consensus_infos the fallback calls estimate_validator_power (line 1417) which returns:
cur_active + cur_pending_active + (lockup_expired ? 0 : cur_pending_inactive) + cur_rewardThe numerical match between the two only holds because on_new_epoch runs update_stake_pool first (line 1273), which (a) merges rewards into active, (b) drains pending_active into active, (c) moves expired pending_inactive into inactive. After that, get_next_epoch_voting_power on the post-merge StakePool happens to equal what estimate_validator_power computed on the pre-merge StakePool.
This invariant is load-bearing for the PRs central claim (the consensus layers preview matches what on_new_epoch installs) but it is enforced by accident: change update_stake_pool ordering, change the reward formula, or change what get_next_epoch_voting_power includes, and the two helpers silently diverge with no test catching it.
Suggested fix:
- Either factor a single shared helper that both code paths call (with a flag indicating whether StakePool has already been updated), or
- Add a tested invariant or assertion that, for the fallback path, every retained validators voting_power equals the value next_validator_consensus_infos predicted for the same validator within the same reconfig.
Confidence: 0.7 the numerical equivalence holds in current code, this is a maintainability/correctness drift risk for future changes, not a present bug.
Category: maintainability / hidden-coupling
There was a problem hiding this comment.
Yes the staking module is complex and hard to maintain.
| timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS); | ||
| end_epoch(); | ||
| assert!(get_validator_state(validator_address) == VALIDATOR_STATUS_INACTIVE, 2); | ||
| // Validator remains ACTIVE — liveness fallback triggered (empty election would halt chain). |
There was a problem hiding this comment.
[minor] Test name no longer matches what it asserts
This test is named test_active_validator_having_insufficient_remaining_stake_after_withdrawal_gets_kicked, but the PR inverted the final assertion from VALIDATOR_STATUS_INACTIVE to VALIDATOR_STATUS_ACTIVE (the validator no longer gets kicked when its the only one; the liveness fallback retains it). The test is now exercising the OPPOSITE behavior from its name.
The test still has value (it documents that the fallback fires in the single-validator below-min case), but the name has become misleading. A future reader scanning for the kick-on-insufficient-stake regression test wont find it.
Suggested fix: either
- Rename to test_active_validator_having_insufficient_remaining_stake_retained_by_liveness_fallback, OR
- Restore the original test (using a 2-validator setup so the kick path is exercised) and add the fallback assertion as a separate test (you already have test_liveness_fallback_retains_validator_when_election_is_empty doing this).
The 2-validator counterexample is already covered by test_validator_kicked_normally_when_another_qualifies, so the cleanest move is just to rename the inverted test.
Confidence: 1.0 verifiable by reading the test body.
Category: test hygiene
There was a problem hiding this comment.
Yes the name is wrong and the proposition is better.
|
|
||
| // Emitted when the epoch-boundary election produces an empty validator set. | ||
| // The protocol retains the previous active set (with refreshed voting power) | ||
| // to preserve liveness. Signals a governance/economic failure requiring operator attention. |
There was a problem hiding this comment.
[nit] EmptyElectionFallback event lacks an epoch field
The fallback emits one EmptyElectionFallback per epoch where it fires. The struct carries minimum_stake, emergency_validator_count, total_emergency_voting_power, but no epoch number. If the chain stays in fallback for many epochs, every emission looks identical. Operator tooling has to correlate the events with the surrounding NewEpochEvent (which has the epoch number) to know which epoch each fallback corresponds to.
This is a footgun more than a bug. Querying historical fallback occurrences becomes harder than it needs to be, and dashboards have to dedupe by epoch externally.
Suggested fix: add a starting_epoch: u64 field to EmptyElectionFallback and populate it from validator_set.active_validators... actually, from staking_config or reconfiguration::epoch. Since this is also emitted before the post-loop work that finalizes the new epoch, the value should be the EPOCH BEING ENTERED (not the one ending), and the helper to read it is reconfiguration::current_epoch() called right after epoch advancement, or you can pre-stamp it from on_new_epoch.
Confidence: 1.0.
Category: observability
There was a problem hiding this comment.
Yes it's better to add the epoch to the event.
Claude Audit: PR #356 — feat(stake) add liveness fallback to prevent empty validator setRisk: critical (0.88 / 1.0) Risk Score Breakdown
Findings by Severity
Top 3 to Address
Other Findings (also posted inline)
VerdictRecommendation: hold for investigation Even if the stake-module liveness fallback design is accepted, this PR cannot be merged in its current state because the stale branch would silently revert four landed fix PRs (341, 344, 348 plus the save_ledger_pruner_progress changes that came in with 341). That alone is a blocker; the second major finding (total_voting_power = 0 silent stall) needs design discussion before the fallback ships at all. Follow-up items:
Appendix: Findings that could not be anchored inlineThe following finding refers to changes that appear in the diff between BASE_SHA and HEAD_SHA but are NOT in the PRs own commits (they are deletions caused by the branch being stale). GitHubs PR review API rejects inline anchors at these lines because the merge-base diff that GitHub uses does not include them. [critical] PR is stale; merging would revert four landed safety fixes This PRs merge base with m1 is 437d37f, but m1 HEAD is 829952c. The intermediate commits include four landed safety fixes that are NOT present in this branch. When GitHub computes the diff against current m1, this PR appears to revert all of them:
These are not changes the PR author touched. They show as deletions only because the branch has not been rebased onto current m1. The authors actual commits only touch stake.move, staking_contract.move, delegation_pool_integration_tests.move, and the regenerated docs. Attack Path: the panic re-introductions affect liveness on any node that hits the unreset BlockExecutor invariant or the fork-race state-update path. PR 348 specifically traded a panic for an Err for a documented fork-race that has been observed in practice. Re-introducing the panic puts validators back at risk of process exit on the losing fork. Suggested fix: rebase onto current m1, then push with force-with-lease. After rebasing, git diff origin/m1...HEAD --name-only should show ONLY the 5 stake-related files. Confidence: 1.0 verified by git merge-base and inspecting the listed merge commits on m1. Category: regression / stale-branch Generated by the audit-pr subagent. Individual findings are posted as inline review comments on the relevant lines. |
There was a problem hiding this comment.
Pull request overview
Adds a liveness safeguard to Aptos Framework staking epoch transitions to prevent the validator election from producing an empty active validator set (which would halt the chain), and aligns consensus’ next-epoch preview with the new on-chain behavior.
Changes:
- Introduces an
EmptyElectionFallbackevent and updateson_new_epoch()to retain the previous active validator set (with refreshed voting power) when an election would otherwise be empty. - Refactors
next_validator_consensus_infos()by extractingestimate_validator_power()and mirroring both the pending-active ordering and the new liveness fallback. - Updates staking/delegation-pool/staking-contract tests to either exercise the fallback explicitly or avoid triggering it by adding a second validator.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| aptos-move/framework/aptos-framework/sources/stake.move | Implements empty-election fallback + event emission; updates next-epoch consensus preview logic and adds tests for fallback/ordering. |
| aptos-move/framework/aptos-framework/tests/delegation_pool_integration_tests.move | Adjusts integration tests to avoid triggering the new fallback by ensuring elections remain non-empty. |
| aptos-move/framework/aptos-framework/sources/staking_contract.move | Updates end-to-end staking contract test to include a second validator so the normal kick path is exercised. |
| aptos-move/framework/aptos-framework/doc/stake.md | Documents the new event/function and updated epoch-transition/preview logic (but needs regeneration fixes noted in comments). |
| aptos-move/framework/aptos-framework/doc/fungible_asset.md | Doc regen picks up is_asset_type_dispatchable entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A second validator ensures the election is never empty, so the primary validator is | ||
| // kicked normally when its remaining stake drops below minimum — no liveness fallback. | ||
| // validator_2 joins first (index 0); validator joins second (index 1) and triggers end_epoch. | ||
| let (_sk_2, pk_2, pop_2) = generate_identity(); |
| total_emergency_voting_power: validator_set.total_voting_power, | ||
| }); | ||
| } else { | ||
| // Normal case, some validator participate to the next epoch. |
| } <b>else</b> { | ||
| <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_borrow">vector::borrow</a>(&cur_validator_set.pending_active, candidate_idx - num_cur_actives) | ||
| }; |
| } <b>else</b> { | ||
| <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_borrow">vector::borrow</a>(&cur_validator_set.pending_active, fallback_idx - num_cur_actives) | ||
| }; |
Description
Fix the issue corrected in this commit.
This commit contains a lot of formatting changes so it's not easy to identify the real update.
The issue can lead to a halt of the network, but it can not arrive on our networks because we always have at least one validator that have enough stakes. Not sure this correction need to be merged because it adds a significant change in the staking module that is already complex with an effective occurrence nearly impossible
Implements a clean-room liveness fallback for the epoch transition logic. When on_new_epoch() runs the validator election and every candidate drops below the minimum stake threshold, transitioning to an empty active set would halt
the chain. This PR detects that case and retains the previous active validator set (with voting power recomputed from post-reward, post-fee balances), emitting an
EmptyElectionFallbackevent to signal that operator/governanceattention is needed.
EmptyElectionFallback{ minimum_stake,emergency_validator_count, total_emergency_voting_power }
(staking contract) so the normal validator exit path (INACTIVE) is exercised without triggering the fallback
How Has This Been Tested?
cargo test -p aptos-framework move_framework_unit_tests --releaseKey Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist