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

VC: Electra fixes. #6631

Merged
merged 22 commits into from
Oct 16, 2024
Merged

VC: Electra fixes. #6631

merged 22 commits into from
Oct 16, 2024

Conversation

cheatfate
Copy link
Contributor

This PR includes Electra fixes for VC.
And asyncraises annotations for VC.

Copy link

github-actions bot commented Oct 10, 2024

Unit Test Results

         9 files   -        3    1 355 suites   - 455   47m 4s ⏱️ - 8m 19s
  5 227 tests ±       0    4 879 ✔️ ±       0  348 💤 ±  0  0 ±0 
21 780 runs   - 7 277  21 376 ✔️  - 7 229  404 💤  - 48  0 ±0 

Results for commit aaec8ef. ± Comparison against base commit acad28d.

♻️ This comment has been updated with latest results.

beacon_chain/deposits.nim Outdated Show resolved Hide resolved
@tersec
Copy link
Contributor

tersec commented Oct 15, 2024

beacon_chain/validator_client/block_service.nim(602, 9) Hint: 'currentSlot' is declared but not used [XDeclaredButNotUsed]
beacon_chain/validator_client/block_service.nim(26, 3) Hint: 'PreparedBeaconBlock' is declared but not used [XDeclaredButNotUsed]
beacon_chain/validator_client/block_service.nim(32, 3) Hint: 'PreparedBlindedBeaconBlock' is declared but not used [XDeclaredButNotUsed]

@tersec
Copy link
Contributor

tersec commented Oct 15, 2024

beacon_chain/validator_client/block_service.nim(602, 9) Hint: 'currentSlot' is declared but not used [XDeclaredButNotUsed]
beacon_chain/validator_client/block_service.nim(26, 3) Hint: 'PreparedBeaconBlock' is declared but not used [XDeclaredButNotUsed]
beacon_chain/validator_client/block_service.nim(32, 3) Hint: 'PreparedBlindedBeaconBlock' is declared but not used [XDeclaredButNotUsed]

c881784

voluntary_exit_signature_fork(
fork, capellaForkVersion, currentEpoch, forkConfig.get.denebEpoch)
fork, capellaForkVersion, currentEpoch, capellaForkEpoch)
Copy link
Contributor

@etan-status etan-status Oct 15, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

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.

Copy link
Contributor

@etan-status etan-status Oct 15, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tersec tersec enabled auto-merge (squash) October 16, 2024 10:05
@tersec tersec merged commit fcf72a6 into unstable Oct 16, 2024
13 checks passed
@tersec tersec deleted the vc-electra branch October 16, 2024 17:20
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