Rebase eip7805 fork onto Electra#15088
Rebase eip7805 fork onto Electra#15088jihoonsong wants to merge 8 commits intoOffchainLabs:focilfrom
eip7805 fork onto Electra#15088Conversation
|
A PR adding fork.md for eip7805: ethereum/consensus-specs#4191 |
|
I found a bug that this change kills Dora after eip7805 fork activated epoch is passed. I'll add a fixing commit. |
terencechain
left a comment
There was a problem hiding this comment.
Please note that the team is generally against boilerplate — for example, #14771 was later reverted in #14905
I don't think we should add any EIP-7805 related boilerplate and should only include the minimal code necessary to test functionality
Could we strip out the parts we don't need in this PR (ex: block and state do not change in EIP7805) and review it based on that? Otherwise this PR becomes too big to review
beacon-chain/core/eip7805/upgrade.go
Outdated
| NextWithdrawalValidatorIndex: vi, | ||
| HistoricalSummaries: summaries, | ||
|
|
||
| DepositRequestsStartIndex: params.BeaconConfig().UnsetDepositRequestsStartIndex, |
There was a problem hiding this comment.
DepositRequestsStartIndex just needs to be beaconState.DepositRequestsStartIndex(). Setting to UnsetDepositRequestsStartIndex is a one time operation when upgrading to Electra
18801df to
5b1c9c3
Compare
|
I've removed boilerplates and reduced changed files by ~66%. There might be more changes that are not necessary like configuring minimal or Sepolia config, etc. I've removed things a bit conservatively so please let me know if those are better to be gone. This changes also not kill Dora after the transition to epoch 2. |
485c873 to
c2fffcc
Compare
040911e to
929bbb8
Compare
This PR rebases
eip7805fork onto Electra. The rest two commits correct IL submission behavior according to the EIP (i.e., submission deadline is 8 second into the slot whereas propagation cut off is 9).After this PR, we can have Prysm and Geth interop with the following config:
Current implementation of
ethereum-packagesets Osaka time only when Fulu fork is set. Our original Geth'sForkchoiceUpdatedV3checks if current fork version is Osaka so it doesn't work without Fulu. To make Prysm and Geth work, I've uploaded Geth image that doesn't check fork version withinForkchoiceUpdatedV3. It's available atjihoonsg/geth-focil:no-osaka.To sum up, this Kurtosis config reflects these two changes.
If we want to set Osaka time, we can set fork epochs as:
While this works like a charm, it invalidates the whole effort to rebase onto Electra.
One hacky thing allows us to have both fulu and eip7805 fork at epoch 1. At
UpgradeToEip7805inbeacon-chain/core/eip7805/upgrade.go, we could useFork.PreviousVersion: params.BeaconConfig().ElectraForkVersioninstead ofFork.PreviousVersion: params.BeaconConfig().CurrentVersion.This is because when we upgrade states, fulu state changes occur right before eip7805 state changes within the same epoch. So we end up with
Fork.PreviousVersion == FuluForkVersion, which should have beenFork.PreviousVersion == ElectraForkVersion. (You can refer toUpgradeStateinbeacon-chain/core/transition.go.)With this change, this following config works:
However, this is very error-prone and high-contextual workaround, which I don't really like. Needless to say, having fulu fork activated is the opposite of why we're trying to rebase onto Electra, i.e., to not affected by PeerDAS and other changes. Hence, I'd like to suggest to use eip7805 fork in the EL as well to become independent from fulu completely. I want to discuss this in the upcoming FOCIL breakout.
I think we need fork.md something like that of fulu. I'll open a PR to consensus-spec adding fork.md soon.
This PR is heavily inspired by #14771. I'd like to appreiciate the PR author and reviewer @nalepae @rkapka