-
Notifications
You must be signed in to change notification settings - Fork 252
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
VC: Electra fixes. #6631
VC: Electra fixes. #6631
Conversation
|
|
beacon_chain/deposits.nim
Outdated
voluntary_exit_signature_fork( | ||
fork, capellaForkVersion, currentEpoch, forkConfig.get.denebEpoch) | ||
fork, capellaForkVersion, currentEpoch, capellaForkEpoch) |
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.
the behaviour actually changes with deneb (https://eips.ethereum.org/EIPS/eip-7044).
As in, if it's deneb, then use the capella fork version for the exit signature, otherwise use the regular one.
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.
so, it is intended that this is a mix of capella fork version and deneb fork epoch.
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.
Well, if it's Capella it can use Capella too.
nimbus-eth2/beacon_chain/spec/signatures.nim
Lines 220 to 253 in 0ecd279
func voluntary_exit_signature_fork( | |
is_post_deneb: static bool, | |
state_fork: Fork, | |
capella_fork_version: Version): Fork = | |
when is_post_deneb: | |
# Always use Capella fork version, disregarding `VoluntaryExit` epoch | |
# [Modified in Deneb:EIP7044] | |
Fork( | |
previous_version: capella_fork_version, | |
current_version: capella_fork_version, | |
epoch: GENESIS_EPOCH) # irrelevant when current/previous identical | |
else: | |
state_fork | |
func voluntary_exit_signature_fork*( | |
consensusFork: static ConsensusFork, | |
state_fork: Fork, | |
capella_fork_version: Version): Fork = | |
const is_post_deneb = (consensusFork >= ConsensusFork.Deneb) | |
voluntary_exit_signature_fork(is_post_deneb, state_fork, capella_fork_version) | |
func voluntary_exit_signature_fork*( | |
state_fork: Fork, | |
capella_fork_version: Version, | |
current_epoch: Epoch, | |
deneb_fork_epoch: Epoch): Fork = | |
if current_epoch >= deneb_fork_epoch: | |
const is_post_deneb = true | |
voluntary_exit_signature_fork( | |
is_post_deneb, state_fork, capella_fork_version) | |
else: | |
const is_post_deneb = false | |
voluntary_exit_signature_fork( | |
is_post_deneb, state_fork, capella_fork_version) |
Nimbus is inconsistent about how it phrases this logic, but it's true, the Deneb version works too.
Either way,
if fork >= deneb:
use capella
else:
use real fork
and
if fork >= capella:
use capella
else:
use real fork
are equivalent.
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.
the difference being that within capella, capella signatures are only valid if the object's epoch is >= CAPELLA_FORK_EPOCH. while starting from deneb, the object epoch does not matter at all and can even be smth like 0.
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.
within capella, previous_version
is bellatrix and current_version
is capella.
from deneb onward, previous_version
and `current_version are both capella.
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.
the practical implication is when trying to include a VoluntaryExit originally signed during bellatrix, but only include it lateron during Capella. (during deneb it will become invalid)
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.
Ok, agree, it should be reverted.
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.
This PR includes Electra fixes for VC.
And
asyncraises
annotations for VC.