[Merged by Bors] - Subscribe to altair gossip topics 2 slots before fork

[pawanjay176]

Issue Addressed

N/A

Proposed Changes

Add a fork_digest to ForkContext only if it is set in the config.
Reject gossip messages on post fork topics before the fork happens.

Edit: Instead of rejecting gossip messages on post fork topics, we now subscribe to post fork topics 2 slots before the fork.

[pawanjay176]

@michaelsproul can we tweak the ChainSpec deserialisation such that if ALTAIR_FORK_EPOCH == u64::max() in config.yaml, we set chain_spec.altair_fork_epoch = None. Didn't see any issue with that after a brief look.

[michaelsproul]

@michaelsproul can we tweak the ChainSpec deserialisation such that if ALTAIR_FORK_EPOCH == u64::max() in config.yaml, we set chain_spec.altair_fork_epoch = None. Didn't see any issue with that after a brief look.

Yeah I think this would fine. No fork is actually going to activate at u64::max(), so it makes sense to map that to None. It might be nice to get this changed in the spec though. They could set it to null in the YAML.

[AgeManning]

There's a few things we probably need to consider here.

If we receive messages on a topic we are subscribed and then reject them, this is considered censoring and the gossipsub scoring algo will penalize us. The correct approach is to only subscribe to a topic when we want to receive messages.

If there is a period of time we dont want to receive messages, we should not subscribe to the topic. In this case it seems we dont want to subscribe to the topic until the fork.

If we do want to keep this current logic, it might also be nicer to filter the messages at the behaviour level. The behaviour has a fork_context so I think its possible to filter the messages as we get them, rather than sending them up to the network to do it for us.

[pawanjay176]

If there is a period of time we dont want to receive messages, we should not subscribe to the topic. In this case it seems we dont want to subscribe to the topic until the fork.

I subscribed to all topics before the fork mainly because the spec says

The topics that are not removed in a fork are updated with a new ForkDigestValue. In advance of the fork, a node SHOULD subscribe to the post-fork variants of the topics.

Subscriptions are expected to be well-received, all updated nodes should subscribe as well. Topic-meshes can be grafted quickly as the nodes are already connected and exchanging gossip control messages.

We could also interpret it as subscribe to the post-fork topics a few epochs in advance. But we would still have to deal with the same problem of rejecting messages only for a shorter duration.

If we receive messages on a topic we are subscribed and then reject them, this is considered censoring and the gossipsub scoring algo will penalize us. The correct approach is to only subscribe to a topic when we want to receive messages.

I think this is fine because a node sending us post fork messages before the fork slot is either malicious or has clock issues. So we probably wouldn't want to peer with such a node anyway.

If we do want to keep this current logic, it might also be nicer to filter the messages at the behaviour level.

I started out implementing this but it got ugly because the behaviour doesn't have the slot clock which we need to check if we are within MAXIMUM_GOSSIP_CLOCK_DISPARITY of the fork instant.

We could simplify things by not accounting for MAXIMUM_GOSSIP_CLOCK_DISPARITY and just checking if the topic on which the message was received is the one corresponding to the current fork. But this would again cause issues because we will end up downscoring peers sending messages on pre-fork topics after the fork. This is possible due to lagging IWANT messages as mentioned in the spec.

Peers who propagate messages on the pre-fork topics MUST NOT be scored negatively. Lagging IWANT may force them to.

With all this, I felt it was the cleanest to handle it in the network service. Please let me know if there's a better way of handling it 🙏

[michaelsproul]

The fork epoch stuff is looking good 👌

[AgeManning]

I think its still not right to subscribe to a topic and then try and downscore peers who send us information on a topic we've subscribed to. Although this may work at a lighthouse level, we're breaking a few things in gossipsub, which is designed to give you messages for which you are subscribed (i.e we will also get gossip (IHAVE messages) from random other peers subscribed to the topic if they accept them at the gossip level, and i'm worried we could be banning misc peers).

If we don't want messages from a topic, we shouldn't subscribe to it. Building the mesh should be relatively quick (so we don't need much time to subscribe in advance). From reading the spec, it just says we should subscribe in advance (which I agree with) but maybe like 1 or two slots in advance of the fork. This way we don't have to worry about which topic we get messages on, we let gossipsub filter things we don't want out, simply by our subscriptions (all the gossipsub scoring logic should remain fine this way also).

In terms of implementation, I've not looked closely at the gossipsub subscription timing logic. But I imagine the Network service keeps track of when we are forking, and potentially it can have an event that fires 1/2 slots before the fork to tell the network to subscribe to the new topics. Then that's all that needs to be done and gossipsub will filter everything for us before this time.

I could be missing something, so let me know if there's other things you think we should consider here.

[pawanjay176]

@AgeManning Agree with your points above. Reverted the gossipsub filtering and added a new timer that fires SUBSCRIBE_DELAY_SLOTS = 2 slots before the fork. We then subscribe to all currently subscribed topics with the new fork_digest.

We subscribe to both base and altair fork_digests if the beacon node is started between fork_slot - SUBSCRIBE_DELAY_SLOTS and fork_slot and just altair fork_digest after the fork.

Tested all scenarios on local testnets and the simulator and it is working as expected.

[paulhauner]

I ran this for the Prater Altair fork and it went well!

Just for clarity, my understanding is that we're blocked on approval from @AgeManning. No rush, just wanted to make it clear.

[AgeManning]

We discussed this internally and decided we should merge this for mainnet and make a side PR to shift some of the logic into the behaviour.

[AgeManning]

@pawanjay176 - Could you merge in the latest unstable and resolve the conflicts, then I think we're good to go.

[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

[gagadbug]

• kdkslL