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

test(spec): fix attestors slashing specs for electra fork #6758

Merged
merged 5 commits into from
May 10, 2024

Conversation

nazarhussain
Copy link
Contributor

Motivation

Improve the spec coverage for electra fork

Description

Fix attestor slashing for the elctra-fork.

Steps to test or reproduce

  • Run all tests

@nazarhussain nazarhussain requested a review from a team as a code owner May 10, 2024 12:24
@nazarhussain nazarhussain self-assigned this May 10, 2024
@@ -52,7 +52,7 @@ export function computeExitEpochAndUpdateChurn(state: CachedBeaconStateElectra,
// Exit doesn't fit in the current earliest epoch.
if (exitBalance > exitBalanceToConsume) {
const balanceToProcess = Number(exitBalance) - exitBalanceToConsume;
const additionalEpochs = Math.floor((balanceToProcess - 1) / (perEpochChurn + 1));
const additionalEpochs = Math.ceil((balanceToProcess - 1) / (perEpochChurn + 1));
Copy link
Member

Choose a reason for hiding this comment

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

Could you detail this? Also, is it worth having some test for this, as apparently this change didn't require any associated test change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally my preferences is to have unit tests for every function, but unfortunatley we don't have it yet.

I updated with a code comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I misread the spec here.
We should do

const additionalEpochs = Math.floor((balanceToProcess - 1) / perEpochChurn) + 1

Let me know if the spec test's still passing after making this change

@nazarhussain nazarhussain requested a review from jeluard May 10, 2024 12:40
@@ -52,7 +52,7 @@ export function computeExitEpochAndUpdateChurn(state: CachedBeaconStateElectra,
// Exit doesn't fit in the current earliest epoch.
if (exitBalance > exitBalanceToConsume) {
const balanceToProcess = Number(exitBalance) - exitBalanceToConsume;
const additionalEpochs = Math.floor((balanceToProcess - 1) / (perEpochChurn + 1));
const additionalEpochs = Math.ceil((balanceToProcess - 1) / (perEpochChurn + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I misread the spec here.
We should do

const additionalEpochs = Math.floor((balanceToProcess - 1) / perEpochChurn) + 1

Let me know if the spec test's still passing after making this change

@nazarhussain
Copy link
Contributor Author

Let me know if the spec test's still passing after making this change

Yes both expressions have same impact. Either to use Math.ceil or use Math.floor ... + 1. I updated the coe.

@nazarhussain nazarhussain requested a review from ensi321 May 10, 2024 18:02
Copy link
Contributor

@ensi321 ensi321 left a comment

Choose a reason for hiding this comment

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

lgtm thanks

@ensi321 ensi321 merged commit 9c1a4ca into electra-fork May 10, 2024
10 of 17 checks passed
@ensi321 ensi321 deleted the nh/attestors-slashing-specs branch May 10, 2024 22:01
g11tech pushed a commit that referenced this pull request May 24, 2024
* Fix attester slashing specs for electra

* Remove unused import

* Add code comment

* Update the expression

* Update the fork check
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.

None yet

3 participants