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

Fix some issues in UpgradeToElectra #14598

Closed
wants to merge 5 commits into from

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Oct 31, 2024

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Primarily, this PR fixes some issues in UpgradeToElectra.

  • The logic for calculating earliestExitEpoch wasn't quite right.
    • It was finding the max exit epoch, not the minimum.
    • I believe my changes should work as intended, though it still deviates from the spec.
  • ExitBalanceToConsume and ConsolidationBalanceToConsume should be set to 0.
    • These were being set to the total active balance.
    • The copy-paste spec for this says they should be zero, so not sure.
      • Speaking of this, the godoc function is out of date, but this PR doesn't fix that.
      • I've chatted with Kasey some about a long-term fix for this type of issue.
  • Fix tests & rename some variables.
    • I don't think names like lwvi are very readable.

Also, some unrelated fixes:

  • Delete unused ProcessSyncAggregate alias.
  • In PendingDeposit, Amount was specified in the wrong order.

Acknowledgements

  • I have read CONTRIBUTING.md.
  • I have made an appropriate entry to CHANGELOG.md.
  • I have added a description to this PR with sufficient context for reviewers to understand this PR.

@jtraglia jtraglia requested a review from a team as a code owner October 31, 2024 21:10
@jtraglia jtraglia changed the title Fix some issuess in UpgradeToElectra Fix some issues in UpgradeToElectra Oct 31, 2024
Copy link
Member

@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.

A bunch of spec tests are failing after this change.

  • TestMainnet_Electra_Transition
  • TestMainnet_UpgradeToElectra

Are we sure it's the min? the spec looks max to me:

  exit_epochs = [v.exit_epoch for v in pre.validators if v.exit_epoch != FAR_FUTURE_EPOCH]
    if not exit_epochs:
        exit_epochs = [get_current_epoch(pre)]
    earliest_exit_epoch = max(exit_epochs) + 1

@jtraglia
Copy link
Contributor Author

Are we sure it's the min? the spec looks max to me

Ah yes, you're right. I misread that. My fault. Will revert that change now.

Comment on lines -271 to +264
ExitBalanceToConsume: helpers.ActivationExitChurnLimit(primitives.Gwei(tab)),
ExitBalanceToConsume: 0,
Copy link
Member

Choose a reason for hiding this comment

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

I dont think this is right either. We should assign them to churn limit based on total active balances

    post.exit_balance_to_consume = get_activation_exit_churn_limit(post)
    post.consolidation_balance_to_consume = get_consolidation_churn_limit(post)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh. Once again you're right. I'm going to close this.

@jtraglia jtraglia closed this Oct 31, 2024
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.

2 participants