feat: support non-EigenPod withdrawal credentials - #485
Open
pankajjagtapp wants to merge 5 commits into
Open
Conversation
Validators can now be spun up against withdrawal credentials that point at their EtherFiNode instead of at an EigenPod. instantiateEtherFiNode(false) already produced a pod-less node; the three StakingManager creation paths hardcoded the pod as the credential target. Add EtherFiNodesManager.withdrawalCredentialTarget(node), which returns the pod when the node has one and the node itself otherwise, and route createBeaconValidators, registerBeaconValidators and confirmAndFundBeaconValidators through it. One resolver means all three agree bit-for-bit; a disagreement would revert IncorrectBeaconRoot and strand the 1 ETH deposit. The target is derived rather than stored. A node's pod is fixed at instantiation: createEigenPod is callable only by StakingManager, whose only call site is inside instantiateEtherFiNode, and createPod reverts if a pod already exists. Storage layouts are byte-identical to master for all three contracts. EtherFiNode calls the EIP-7002 and EIP-7251 predeploys directly when there is no pod, since the node is then the validators' withdrawal address, and reads request fees from the predeploys instead of the pod. Both request paths now reject batches mixing validators from different nodes: with a pod EigenLayer enforced this, but the predeploys accept any pubkey and the consensus layer silently drops requests whose source withdrawal address is not the caller. Also adds disablePod and withdrawDisabledPodETH for EigenLayer v1.14.0 pod retirement. That version is not on mainnet yet, so the call reverts rather than silently succeeding. Existing pod-backed nodes are unaffected and need no migration.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0bcaaaa. Configure here.
📊 Forge Coverage ReportGenerated by workflow run #782 |
sweepFunds(address node) sweeps a node straight to the liquidity pool without resolving a validator id, matching the address/id overload pattern the rest of the contract already uses. Validators in the new credential regime pay out to the node itself, so the node address is the natural handle. The id overload is untouched: the address path adds _validateNode so the overload cannot be used to call sweepFunds() on an arbitrary contract, while leaving the id path's behaviour exactly as it was for legacy nodes that may not be backfilled. Add an end-to-end lifecycle suite covering spawner registration, operator whitelisting, bidding, pod-less node creation, credential derivation, register, 1 ETH create, 31 ETH top-up and the sweep, plus the gate at each step. Review fixes: - cast to address before comparing contract-type variables (solc 9170) - declare disablePod / withdrawDisabledPodETH on IEtherFiNodesManager, which already carried the PodDisabled event and MixedNodeRequest error
…g tests Four suites that pass on master were broken by this branch. Root causes: 1. Fee reads were routed through the EtherFiNode, a function that only exists on the new implementation. That made the EtherFiNodesManager upgrade depend on the EtherFiNode beacon being upgraded first, and EtherFiNode's `fallback() payable` swallows unknown selectors, so it surfaced as an ABI decode failure rather than a clear revert. Pod-backed nodes now read the fee off the pod exactly as before; only the pod-less path uses the node. The two upgrades are independently deployable again. 2. The MixedNodeRequest batch check applied to pod-backed nodes, where EigenLayer already enforces pod membership against the pod's own validator set. Ours checked the pubkey mapping, which does not have every legacy validator linked. Scoped to pod-less nodes, which is the only place the check is needed. 3. The credential resolver imposed _validateNode on the request paths, which never required deployedEtherFiNodes and would have broken legacy nodes that were never backfilled. The validated resolver still gates the creation paths, where the target is baked into a deposit; the request paths use an unvalidated derivation for the emitted event only. validator-key-gen.t.sol now upgrades EtherFiNodesManager alongside StakingManager, which production does anyway. Forwarding: forwardEigenPodCall and forwardExternalCall accept either eigenpod or housekeeping operations, so the withdrawal-completion cron can batch across nodes. The per-caller selector whitelist still applies. Tests: EIP-7002 partial and batch withdrawals, EIP-7251 switch-to-compounding, in-node consolidation and consolidation to an external target, fee and batch rejection cases, and the full disabled-pod sweep to the LiquidityPool against a v1.14.0 stub.
…Manager Removes five overloads that were one-line delegations to their address twins: queueETHWithdrawal, queueWithdrawals, completeQueuedETHWithdrawals, completeQueuedWithdrawals and sweepFunds. Nothing in src/ called them, and the operations tooling already encodes the address form: queueETHWithdrawal(address,uint256) in run_consolidation_python.py and unrestake_validators.py, completeQueuedETHWithdrawals(node, bool) in CompleteEigenLayerWithdrawals.s.sol. sweepFunds(address) also drops its _validateNode check. Reclaims 845 bytes of EtherFiNodesManager, which was at 541 bytes of margin under the 24576 limit. Now 23190, margin 1386. Callers in prelude.t.sol and EtherFiNodesManager.t.sol resolve the node address first. Those resolutions are hoisted above any preceding cheatcode: vm.prank and vm.expectRevert are each consumed by the next call, and the resolver is itself a call, so evaluating it inside a pranked argument list silently moves the assertion onto the wrong call. The four *_byId_blockedByPauseContractUntil tests are deleted rather than converted; each has a _byAddress_ twin covering the same pause behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spin up validators whose withdrawal credentials point at their
EtherFiNodeinstead of an EigenPod.instantiateEtherFiNode(false)already produced a pod-less node. The blocker was that all threeStakingManagercreation paths hardcoded the pod as the credential target.Change
EtherFiNodesManagerdisablePod/withdrawDisabledPodETH;sweepFunds(address); forwarding open to housekeepingStakingManagerEtherFiNodeLiquidityPoolOne resolver means the three creation paths agree bit-for-bit. Disagreement reverts
IncorrectBeaconRootand strands the 1 ETH.Why the node is the target: already a
BeaconProxybehindetherFiNodeBeacon, so oneupgradeTocovers every instance. Already hasfallback() payableand_sweepToLiquidityPool(). Decisively, the oracle and DOSE already mappubkeyHash -> EtherFiNode— no registry, no factory-event scan, nothing new for sweep and monitoring jobs to walk. At 500k ETH / 32 ETH per validator that set would hold ~15,600 entries.The pod path is byte-identical
Everything an existing pod-backed node does is unchanged, deliberately:
EtherFiNodebeacon being upgraded first — and sinceEtherFiNodehasfallback() external payable {}, an un-upgraded node swallows the unknown selector and returns empty, surfacing as an ABI decode failure rather than a clear revert. The two upgrades are independently deployable again._validateNodegates the creation paths, where the target is baked into a deposit. The request paths use an unvalidated derivation for the emitted event only — they never requireddeployedEtherFiNodes, and legacy nodes are not all backfilled into it.No stored credential regime
The target is derived, not stored, because a node's pod is already immutable:
createEigenPodrevertsInvalidCallerunlessmsg.sender == stakingManager—EtherFiNodesManager.sol:95StakingManager's only call site is insideinstantiateEtherFiNode—StakingManager.sol:111createPod()revertsEigenPodAlreadyExists;disablePodnever clearsownerToPodStorage layouts are byte-identical to master for all three contracts (
forge inspect <c> storageLayout). These are UUPS proxies, so that removes a class of upgrade risk a stored flag would have carried.createEigenPodmust stay reachable only frominstantiateEtherFiNode. A second call site would let a target change after validators are funded. A comment on the resolver records this.Security fix
Pod-less request batches reject sources from different nodes (
MixedNodeRequest). The predeploys accept any pubkey from any caller, and the consensus layer silently drops requests whose source withdrawal address isn't the caller. Unchecked, a batch would burn the fee, consume the rate limiter, and emit exit events for exits that never happened.requestConsolidationdeliberately leaves the target unconstrained — it may live outside the node.One candidate was investigated and rejected as pre-existing: a maliciously backfilled node could always redirect credentials by returning any address from
getEigenPod(), both before and after this change.EigenLayer v1.14.0 readiness
Every post-
disablePodflow is reachable, and pod ETH reaches the pool:disablePod()podOwnerwithdrawDisabledPodETH_sweepToLiquidityPoolrequestConsolidation(owner-only once disabled)requestWithdrawalcompleteQueuedETHWithdrawalsNoCompleteableWithdrawals, loudlyThe sweep is tested repeatable, since a retired pod keeps receiving skimmed rewards and full exits.
disablePoditself is timelock-gated because it is irreversible.Tests
Both suites inherit
PreludeTest, so the 42 existing pod-backed tests run alongside as the old-regime regression signal.non-eigenpod-credentials.t.solnon-eigenpod-validator-lifecycle.t.solprelude/validator-key-genEL-withdrawals/Request-consolidation/Consolidation-through-EOACovers EIP-7002 full, partial and batch withdrawals; EIP-7251 switch-to-compounding, in-node consolidation, and consolidation to an external target; fee and batch rejection cases; both roles on both forwarding entrypoints; and the full lifecycle from spawner registration to a funded 32 ETH validator with the gate at each step.
Verified by mutation, not assumed: dropping the pod-less branch from the resolver fails 4 tests, including the full-flow test with
IncorrectBeaconRoot. That test builds expected credentials literally rather than reading them back from the resolver — deriving them would make it self-consistent and unable to catch a resolver bug.Three gotchas worth knowing for future work in this repo:
forge test --force. Test artifacts embed implementation creation-bytecode vianew EtherFiNodesManager(...), and incremental compilation doesn't invalidate them, so a mutated implementation is compiled but never deployed into the fork and everything still passes.vm.expectRevertis consumed by a helper call in the argument list. Build deposit data into a local first, or the assertion tests nothing.EtherFiNode.fallback()accepts any unknown selector silently. A missing function on the node reads as an ABI decode failure several frames away, not as a clear revert.Notes for review
EtherFiNodesManageris at 24,035 / 24,576 — 541 bytes of margin, and there is no CI size gate.EtherFiNodeandStakingManagerare comfortable (13.6k and 9.8k spare). Reduction options measured and listed in the thread; worth deciding before the next feature lands here.disablePodcan't be fork-tested against real code — v1.14.0 isn't on mainnet, so the liveEigenPodManagerhas no such selector and no fallback, meaning the call reverts rather than silently succeeding. A no-op would be dangerous: it would let us believe a pod was retired and consolidate out of a live pod, cutting the beacon slashing factor. Signatures, predeploy addresses and calldata encoding were read from EigenLayer PR #1758's head, not from memory.ValidatorWithdrawalRequestSent/ValidatorConsolidationRequestedkeep their signatures but now carry the credential target, which is the node for pod-less validators. Indexers assuming that field is an EigenPod need updating.sweepFunds(uint256 id)behaviour is unchanged. The address overload adds_validateNode; the id path does not, so legacy nodes never backfilled keep working.EigenPodrefunds to its caller, which is the node.b4a09680:Validator-Flows.t.solandWithdraw.t.solfail insetUpwithNotRegistered();LiquidityPool.t.solfails 16/119. All mainnet drift from forking at latest block.Design doc:
docs/superpowers/specs/2026-08-07-non-eigenpod-withdrawal-credentials-design.mdNote
High Risk
Changes staking deposit credential resolution, validator exit/consolidation fee routing, and UUPS upgrade surfaces; mistakes could misdirect beacon ETH or burn exit fees on pod-less batches.
Overview
Enables validators whose withdrawal credentials point at
EtherFiNodeinstead of an EigenPod, using a derivedwithdrawalCredentialTarget(pod when present, otherwise the node) with no new storage.StakingManagernow builds deposit credentials through that resolver on all three creation paths;registerBeaconValidatorsno longer requires an existing pod.EtherFiNodecalls EIP-7002/7251 predeploys directly when pod-less, exposes fee reads, and addsdisablePod/withdrawDisabledPodETH.EtherFiNodesManageradds the resolver, pod retirement and sweep helpers,sweepFunds(address)(replacing id-based overloads),MixedNodeRequestchecks on pod-less exit/consolidation batches, and allows housekeeping on call forwarding alongside eigenpod ops.EigenLayer v1.14.0 interfaces are extended locally; exit/consolidation events still use the same signatures but emit the credential target (node for pod-less validators). Large fork/behaviour test suites and a design doc cover the new flows and regressions for pod-backed nodes.
Reviewed by Cursor Bugbot for commit 2dac724. Bugbot is set up for automated code reviews on this repo. Configure here.