Skip to content

Comments

Rebase eip7805 fork onto Electra#15088

Open
jihoonsong wants to merge 8 commits intoOffchainLabs:focilfrom
jihoonsong:focil
Open

Rebase eip7805 fork onto Electra#15088
jihoonsong wants to merge 8 commits intoOffchainLabs:focilfrom
jihoonsong:focil

Conversation

@jihoonsong
Copy link
Contributor

@jihoonsong jihoonsong commented Mar 23, 2025

This PR rebases eip7805 fork 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:

  electra_fork_epoch: 0
  eip7805_fork_epoch: 1

Current implementation of ethereum-package sets Osaka time only when Fulu fork is set. Our original Geth's ForkchoiceUpdatedV3 checks 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 within ForkchoiceUpdatedV3. It's available at jihoonsg/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:

  electra_fork_epoch: 0
  fulu_fork_epoch: 1
  eip7805_fork_epoch: 2

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 UpgradeToEip7805 in beacon-chain/core/eip7805/upgrade.go, we could use Fork.PreviousVersion: params.BeaconConfig().ElectraForkVersion instead of Fork.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 been Fork.PreviousVersion == ElectraForkVersion. (You can refer to UpgradeState in beacon-chain/core/transition.go.)

With this change, this following config works:

  electra_fork_epoch: 0
  fulu_fork_epoch: 1
  eip7805_fork_epoch: 1

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

@jihoonsong jihoonsong requested a review from a team as a code owner March 23, 2025 14:17
@jihoonsong jihoonsong requested review from Inspector-Butters, james-prysm and kasey and removed request for a team March 23, 2025 14:17
@jihoonsong
Copy link
Contributor Author

A PR adding fork.md for eip7805: ethereum/consensus-specs#4191

@jihoonsong
Copy link
Contributor Author

I found a bug that this change kills Dora after eip7805 fork activated epoch is passed. I'll add a fixing commit.

Copy link
Collaborator

@terencechain terencechain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

NextWithdrawalValidatorIndex: vi,
HistoricalSummaries: summaries,

DepositRequestsStartIndex: params.BeaconConfig().UnsetDepositRequestsStartIndex,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DepositRequestsStartIndex just needs to be beaconState.DepositRequestsStartIndex(). Setting to UnsetDepositRequestsStartIndex is a one time operation when upgrading to Electra

@jihoonsong jihoonsong force-pushed the focil branch 2 times, most recently from 18801df to 5b1c9c3 Compare March 31, 2025 07:59
@jihoonsong
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants