fix(network): clear ENR nfd field when no next fork is scheduled during runtime transitions#9131
fix(network): clear ENR nfd field when no next fork is scheduled during runtime transitions#9131Alleysira wants to merge 4 commits intosigp:unstablefrom
nfd field when no next fork is scheduled during runtime transitions#9131Conversation
|
Thanks for the PR! Were you able to verify in kurtosis that this fixes persisted ENRs? I had a quick skim but I don't think we update nfd on startup, so it probably just uses what was persisted on disk. |
|
Thanks for you insight! You're right, the former fix only covers the init and runtime transition paths. Startup can still reuse a persisted
I've built a custom image and rerun Kurtosis to confirm the fix. The test verified that the
Test ProcedurePhase 1: Reproduce the bug with stock image
Phase 2: Swap image and restart
ResultsPhase 1 — Stock
|
| Client | fork_digest | nfd | seq | Correct? |
|---|---|---|---|---|
| Lighthouse | d0bc159b |
d0bc159b |
9 | NO |
| Teku | d0bc159b |
00000000 |
13 | YES |
| Nimbus | d0bc159b |
00000000 |
38 | YES |
Lighthouse nfd equals its own fork_digest — the runtime transition path sets nfd to the current fork digest instead of clearing it when no next fork is scheduled.
Phase 2 — Swapped to alleysira/lighthouse-nfd-fix:latest (restarted, same datadir):
| Client | fork_digest | nfd | seq | Correct? |
|---|---|---|---|---|
| Lighthouse | d0bc159b |
00000000 |
3 | YES |
| Teku | d0bc159b |
00000000 |
13 | YES |
| Nimbus | d0bc159b |
00000000 |
38 | YES |
After swapping to the fixed image and restarting with the same persisted datadir, Lighthouse nfd is now 00000000.
|
@Alleysira please dont respond with an LLM. it's not helpful to read your comment when its an obviously AI generated wall of text. Can you please summarize what you wrote above in your own words |
Hi @eserilev, sorry about that, I used an LLM to format the test results, thought would make the result easier to read 😭
Basically, as @jimmygchen said, Lighthouse would reuse persisted To verify the fix works, I ran a kurtosis testnet first with the old image, waited for next fork, and confirmed that |
Issue Addressed
No. But related to #9009 and #8996
Motivation
Hi there,
In PR #9009, we fixed the ENR
nfdinitialization path to use zero-valued bytes when no next fork is scheduled. However, the same bug pattern remains in the runtime fork transition path (update_next_fork_digest). After the final scheduled fork activates, thenfdENR field is set to the current fork digest instead of being cleared to0x00000000.This is the testing results from a kurtosis testnet. With
Fuluat epoch 0 andGloasas the final fork at epoch 1, after Gloas activates, only lighthouse has wrongnfd:Proposed Changes
ForkContext::next_fork_digest()to return[u8; 4](returning[0u8; 4]for "no next fork").Added tests:
test_next_fork_digest— existing test passes with non-Option return typetest_next_fork_digest_returns_zero_when_no_next_fork— init at last BPO fork returns[0u8; 4]test_next_fork_digest_zero_after_runtime_transition_to_last_fork— simulatesupdate_current_forkto last fork, then verifies zeroAdditional Info
The following tests/checks and passed:
Found this bug while differential testing, details in Prysm issue.
Thanks for your time!