Skip to content
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

Activate PeerDAS with the EIP7594 Fork Epoch #14184

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Jul 4, 2024

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

In preparation of eventually merging this into develop, we remove the use of the peerDAS flag and instead trigger it via the EIP7594 fork epoch. The reason this is done is mainly:

  • Brings us in line with how other clients are doing it and also allows us to set devnets with dynamic peerDAS forks. In our current branch we have hardcoded peerDAS to be triggered with deneb. However with this change, peerDAS would also be able to work on top of electra.
  • The expectation is that peerDAS will be triggered with electra or a few epochs after. These changes allow us to do that as we were simply using a flag to activate the paths.

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas added Ready For Review A pull request ready for code review Networking P2P related items peerDAS labels Jul 4, 2024
@nisdas nisdas requested a review from a team as a code owner July 4, 2024 10:18
@nisdas nisdas requested review from saolyn, terencechain and rkapka and removed request for a team July 4, 2024 10:18
@@ -53,6 +53,11 @@ func HigherEqualThanAltairVersionAndEpoch(s state.BeaconState, e primitives.Epoc
return s.Version() >= version.Altair && e >= params.BeaconConfig().AltairForkEpoch
}

// PeerDASIsActive checks whether peerDAS is active at the provided slot.
func PeerDASIsActive(slot primitives.Slot) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

PeerDASIsActive ==> isPeerDASActive?

(This convention seems to be used in other places in the code and indicates clearly the function returns a bool.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the current version more because if you write if PeerDasIsActive() then it reads very smoothly. But we don't have a rule for this

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following from terms like DenebEnabled as a guide rather than posing it as a question. I don't feel too strongly but if we do start using isPeerDASActive I would prefer it to be standardized for all similar functions.

ElectraForkVersion []byte `yaml:"ELECTRA_FORK_VERSION" spec:"true"` // ElectraForkVersion is used to represent the fork version for deneb.
ElectraForkEpoch primitives.Epoch `yaml:"ELECTRA_FORK_EPOCH" spec:"true"` // ElectraForkEpoch is used to represent the assigned fork epoch for deneb.
Eip7594ForkEpoch primitives.Epoch `yaml:"EIP7594_FORK_EPOCH" spec:"true"` // EIP7594ForkEpoch is used to represent the assigned fork epoch for peer das.
ForkVersionSchedule map[[fieldparams.VersionLength]byte]primitives.Epoch // Schedule of fork epochs by version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to keep the blank line between Eip7594ForkEpoch and ForkVersionSchedule so the whole block does not get rewritten by gofmt?

@nisdas nisdas merged commit d57db9b into peerDAS Jul 8, 2024
15 of 16 checks passed
@nisdas nisdas deleted the replaceFlagWithForkEpoch branch July 8, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking P2P related items peerDAS Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants