-
Notifications
You must be signed in to change notification settings - Fork 364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: pectra compatibility #1053
base: feat/prooftra
Are you sure you want to change the base?
Conversation
7a6f8a6
to
fbe5ee2
Compare
8b3076f
to
8679ebb
Compare
8679ebb
to
392022e
Compare
d71e908
to
2e9ab01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK the fork timestamp has never been altered once broadcast, at least not in the last few years, so I think hardcoding it is fine
8beb418
to
d63fded
Compare
d849720
to
f5dbfb8
Compare
@@ -254,6 +255,7 @@ contract EigenPod is Initializable, ReentrancyGuardUpgradeable, EigenPodPausingC | |||
for (uint256 i = 0; i < validatorIndices.length; i++) { | |||
// forgefmt: disable-next-item | |||
totalAmountToBeRestakedWei += _verifyWithdrawalCredentials( | |||
beaconTimestamp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder about the usage of getParentBlockRoot
above.
Can beaconTimestamp
belong to the block after the fork, but the proof itself is over the parent block (aka pre-fork)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record I think I'm correct here. 2 scenarios:
- I try to verify withdrawal credentials the block after the hard fork.
getParentBlockRoot
grabs a block from before the hard fork; that's what my proof needs to be against. - I start a checkpoint just after the hard fork. The checkpoint block root used for proofs is from a block just before the hard fork.
Naive solution: have the fork selector logic in the proofs library check proofTimestamp - 12
. However, this runs into potential issues with skipped slots -- I might have a proof timestamp 2 blocks after the hard fork, but there was a skipped slot right at the fork, so the proof block is from before the fork.
I think, unfortunately, we need to pause proofs just before the fork, and unpause just after the fork (once we see the first valid block). Although I'm not sure this entirely fixes the issue... will think more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to pause + upgrade just after the fork + unpause? ugh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synced offline, here is what we need to do:
- Prevent deneb proofs from being submitted to pectra blocks
- Ensure that the
PECTRA_FORK_TIMESTAMP
is the first timestamp at or after the pectra hard fork for which there is a non-missed slot. Checkpoint proofs store the proof timestamp. If there are missed slots at the hard fork timestamp, it's possible, like Alex mentions above, that thebeaconTimestamp
is post pectra but the block header is deneb.
To do this, here is the process:
- Pause checkpoint starting & credential proofs
- Upgrade after fork is hit
- Set pectra fork timestamp to the first timestamp at which there is a pectra block header
- Unpause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the image above, we want to determine what proof type to use, given the proof timestamp tp. Because tp is used to look up a parent block in the EIP-4788 oracle, its proof type corresponds to the last non-skipped block. So, if:
- tp > t1, use pectra logic
- tp <= t1, use dencun logic
Given our beacon state tree height getter: https://github.com/layr-labs/eigenlayer-contracts/blob/b4852c74cdbe43fea2f7330ea0dc752fcf10b6e9/src/contracts/libraries/BeaconChainProofs.sol#L327-L334
... pectraForkTimestamp
should be set to the first valid, non-skipped block after the Pectra hard fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR targeting testnet?
We're going to need a different PR for mainnet, since that's landing before slashing, right? (This EigenPod file is compatible with testnet slashing, but not mainnet)
Yup, targeting testnet. There will be a separate mainnet PR. cc @wadealexc |
|
||
/// @notice Returns the timestamp of the Pectra fork, read from the `EigenPodManager` contract | ||
function getPectraForkTimestamp() external view returns (uint64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this added to the EigenPod interface?
It already exists on the EPM - why duplicate?
/// @dev We check if the proofTimestamp is <= pectraForkTimestamp because a `proofTimestamp` at the `pectraForkTimestamp` | ||
/// is considered to be Pre-Pectra given the EIP-4788 oracle returns the parent block. | ||
function getBeaconStateTreeHeight( | ||
uint64 proofTimestamp, | ||
uint64 pectraForkTimestamp | ||
) internal pure returns (uint256) { | ||
return proofTimestamp <= pectraForkTimestamp ? DENEB_BEACON_STATE_TREE_HEIGHT : PECTRA_BEACON_STATE_TREE_HEIGHT; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making some notes:
Before we set pectraForkTimestamp
in the EPM, it returns 0. This will cause all proofs to be interpreted as pectra proofs. However, we expect contracts to be paused if this value is unset, so that's fine 👍
Additionally, if the contracts are unpaused but the value is still unset, this logic means all proofs are interpreted as pectra proofs, even if we're working with a dencun block. This is good fallback behavior, as the worst case means some proofs will revert until we set the timestamp. 👍
It's the reverse behavior we wanted to avoid - where dencun proof sizes are allowed for pectra blocks. That shouldn't be possible here 👍
Updates checkpoint proof system to be Pectra compatible. The breaking change to EigenPods is the
BeaconState
container increasing to have 37 fields, which results in the tree height to be > 5.Overview
We need to solve for the following cases:
To do this, here is the upgrade process:
EigenPod
pectraForkTimestamp
Deprecation Plan
TODOs