[Merged by Bors] - Fixed Gossip Topics on Fork Boundary

[ethDreamer]

Issue Addressed

The p2p-interface section of the altair spec says you should subscribe to the topics for a fork "In advance of the fork" and unsubscribe from old topics 2 Epochs after the new fork is activated. We've chosen to subscribe to new fork topics 2 slots before the fork is initiated.

This function is supposed to return the required fork digests at any given time but as it was currently written, it doesn't return the fork digest for a previous fork if you've switched to the current fork less than 2 epoch's ago. Also this function required modification for every new fork we add.

Proposed Changes

Make this function fork-agnostic and correctly handle the previous fork topic digests when you've only just switched to the new fork.

[AgeManning]

@pawanjay176 and I had a chat about this internally: see #2532 .

There are modifications we should do (as you've pointed out) to the logic as its currently written. However, the current concern is that the current code has been tested across a number of testnet forks. Mainnet is the next in line to fork and we are hesitant to change any fork-switching logic and go into a mainnet fork with untested code.

I think we want this PR (and some of the other things we've discussed internally) but we probably should wait until after the mainnet fork.

[pawanjay176]

To add to what Age said, I don't think we should complicate our logic to subscribe to previous fork topics if we start our node between fork_epoch to fork_epoch + 2.

In my understanding, the unsubscribe after 2 epochs is to ensure that we unsubscribe only after lagging gossip messages on pre-fork topics are received by all nodes. The restarted node could potentially loose out on useful pre-fork attestations and aggregates, but that would only be 1-2 slots worth which is okay imo. Also most nodes would not restart during the fork transition so I'm not totally convinced we should handle this case.

Re not making the function fork agnostic, I only realised that it's ugly after the prater fork, so that's totally on me 😅
But as Age pointed out, what we have worked for the prater fork and I would be hesitant to go into the mainnet fork with fork switching logic that's untested on public testnets.

[ethDreamer]

most nodes would not restart during the fork transition

Ah - It wasn't obvious to me from the context that this code would only be called during startup and that this deviation from the spec would only cause minor problems in that limited context

Originally I thought this wasn't complicated but yeah I suppose in the context of node startup it would mean we'd have to be careful about setting the next_unsubscribe delay because currently we only set it after crossing a fork boundary.

I've removed subscribing to previous fork topics.

@AgeManning @pawanjay176
Does it look good to you guys now? I just want to get this PR to a place where it's ready to be merged into unstable - we don't have to do it until after the mainnet altair fork but having it ready allows me to merge it into the merge-f2f branch and will make the diff easier to read when it is eventually merged into unstable

[pawanjay176]

LGTM. Just a minor nit.

[pawanjay176]

I think this can be unblocked now that the altair fork is done.

[AgeManning]

LGTM! Nice

[AgeManning]

bors r+

[bors]

Pull request successfully merged into unstable.

Build succeeded:

• arbitrary-check
• cargo-audit
• cargo-fmt
• cargo-udeps
• check-benchmarks
• check-consensus
• clippy
• debug-tests-ubuntu
• dockerfile-ubuntu
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• no-eth1-simulator-ubuntu
• release-tests-ubuntu
• release-tests-windows
• state-transition-vectors-ubuntu