lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Implement weak subjectivity safety checks

Open eserilev opened this issue 8 months ago • 9 comments

Issue Addressed

Closes #7273

Proposed Changes

https://github.com/ethereum/consensus-specs/pull/4179

eserilev avatar Apr 22 '25 19:04 eserilev

hey @dapplion when you have a chance, could you take a look at what I have so far?

The general flow is

  • user sets the weak subjectivity period checkpoint via the existing wss-checkpoint flag
  • on startup, we make the weak subjectivity calculation, using the user provided ws checkpoint, the head snapshot and the most recent finalized checkpoint

If the user doesn't set a weak subjectivity checkpoint, what are we supposed to do here?

eserilev avatar Apr 22 '25 19:04 eserilev

If the user doesn't set a weak subjectivity checkpoint, what are we supposed to do here?

We should assert that the initial state is within the weak subjectivity period on any type of start.

We should also offer a flag such that

  • If flag set and starting from outside weak subjectivity period: big warn log, continue start
  • If flag not set and starting from outside weak subjectivity period: error, abort start

dapplion avatar Apr 23 '25 14:04 dapplion

Thanks for all the help Lion. I've made changes based on your feedback and added additional test coverage

eserilev avatar Apr 24 '25 19:04 eserilev

This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏

mergify[bot] avatar Oct 14 '25 18:10 mergify[bot]

Some required checks have failed. Could you please take a look @eserilev? 🙏

mergify[bot] avatar Nov 26 '25 18:11 mergify[bot]

Some required checks have failed. Could you please take a look @eserilev? 🙏

mergify[bot] avatar Dec 02 '25 16:12 mergify[bot]

There are some tests failures. Maybe we could use a small non-zero constant pre-Electra to smooth over this issue, or we could update these tests to only run post-Electra.

michaelsproul avatar Dec 03 '25 01:12 michaelsproul

Yeah I'm going to fix theses tests to run post Electra

eserilev avatar Dec 03 '25 01:12 eserilev

InvalidPayloadRig wasnt using the fork_from_env stuff when constructing spec so we weren't actually running related tests against RECENT_FORKS in some cases.

revert_minority_fork_on_resume was an annoying test to try to get right when setting the WS calculation to 0 pre-electra. Setting the calculation to a min of 5 fixes the test.

basic sim and fallback sim were also failing and I havent had a chance to look at why. I've ramped up the pre electra ws calc to 256 to fix this (5 was too small of a number apparently).

eserilev avatar Dec 03 '25 14:12 eserilev