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

Don't use the wrong SECONDS_PER_ETH1_BLOCK constant #4709

Closed
wants to merge 1 commit into from

Conversation

zah
Copy link
Member

@zah zah commented Mar 9, 2023

No description provided.

Comment on lines -396 to +397
(blk.timestamp + cfg.SECONDS_PER_ETH1_BLOCK * cfg.ETH1_FOLLOW_DISTANCE <= period_start) and
(blk.timestamp + cfg.SECONDS_PER_ETH1_BLOCK * cfg.ETH1_FOLLOW_DISTANCE * 2 >= period_start)
(blk.timestamp + SECONDS_PER_SLOT * cfg.ETH1_FOLLOW_DISTANCE <= period_start) and
(blk.timestamp + SECONDS_PER_SLOT * cfg.ETH1_FOLLOW_DISTANCE * 2 >= period_start)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is that the spec accidentally got wrong as a result of an oversight. The change here preserves the spirit of the original spec. Arguably, since this is the "honest validator" spec, some deviations are reasonable. It's unlikely that the proposed fix will cause any issues in practice because it effectively increases the size of the candidate set which should only prevent potential situations where Nimbus decides to disagree with the establishing majority in the Eth1Data voting.

@github-actions
Copy link

Unit Test Results

         9 files  ±0    1 065 suites  ±0   33m 49s ⏱️ + 3m 3s
  3 555 tests ±0    3 318 ✔️ ±0  237 💤 ±0  0 ±0 
15 422 runs  ±0  15 157 ✔️ ±0  265 💤 ±0  0 ±0 

Results for commit 280223d. ± Comparison against base commit f5a3ea6.

@zah
Copy link
Member Author

zah commented Apr 4, 2023

Since, we've merged some alternative mitigations, I think we can wait for https://eips.ethereum.org/EIPS/eip-6110 as the eventual resolution for this.

@zah zah closed this Apr 4, 2023
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

2 participants