Capella: use fork version in address change gossip

[michaelsproul]

Presently BlsToExecutionChange messages are verified using the fork version from the head state, which means ones gossiped before the fork are required to be signed against the Bellatrix fork, making them invalid for inclusion in blocks.

@ajsutton suggested that we could remedy this by verifying them with respect to the fork version of the gossip topic. That way as soon as nodes subscribe to the bls_to_execution_topic with the Capella fork digest they can start collecting messages for inclusion in blocks post-fork. Nodes may subscribe as soon as they know about the upcoming fork, or a few epochs in advance.

[ralexstokes]

any problem w/ just letting BlsToExecutionChanges from before the capella fork fail like they do now? then we don't need to add any special logic to handle this case

[michaelsproul]

Yeah I think that's preferable actually. I was hoping this would allow us to queue address changes before the fork, but Lighthouse only subscribes to the new topic 2 slots before the fork anyway.

Presently we have a bug where we actually accept the address changes before the fork if they're signed with the Bellatrix fork version, but we can add an explicit check to ensure Capella is enabled to prevent that issue.

[ajsutton]

I think what's weird to me is that we have gossip topics which specify the specific fork used in their name but then we verify the contents based on a different fork. It's not specific to BLS changes but creates some weirdness at every upgrade because valid gossip can wind up being marked invalid just because the receiving node's head state hasn't yet updated to be in the latest milestone. The same message would then be valid seconds later when it is received.

So in this particular case, you could be sending a perfectly valid BlsToExecutionChange signed with the capella fork and it will be rejected if the receiver's head state is still in Bellatrix.

It's not a huge problem because it resolves itself once the first Capella block is imported, but the fact the p2p spec doesn't actually specify which fork to use for the validation feels like a spec bug to be honest. Even if using the head state is what we want, we probably should specify that.

[dapplion]

Would prefer to not merge this PR. The same effect can be done with:

• Signing tooling signs messages with future fork
• Signing tooling holds message until fork, can subscribe to beacon node to know when
• OR beacon node local operation pool holds message until fork, only then broadcasting to p2p

[ajsutton]

Would prefer to not merge this PR. The same effect can be done with:

• Signing tooling signs messages with future fork
• Signing tooling holds message until fork, can subscribe to beacon node to know when
• OR beacon node local operation pool holds message until fork, only then broadcasting to p2p

None of these solve the problem at the next hard fork though when either side of the fork would be valid to use to sign. The fundamental problem with the spec is that despite having a fork digest literally specified in the topic name, it's completely unspecified which fork digest should be used for gossip on that topic.

That said, this PR only solves that for a single message type (admittedly the one that's most ambiguous) but it is probably better handled consistently for all gossip messages one way or another.

[dapplion]

That said, this PR only solves that for a single message type (admittedly the one that's most ambiguous) but it is probably better handled consistently for all gossip messages one way or another.

Good point, what about adding a notion of singing_epoch to the message? It has worked well in this objects which suffer from a similar issue https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#lightclientupdate signature_slot

[ajsutton]

I actually don't think it's an issue that bls changes are a bit hard to submit around a fork - they're a one off operation so people can just try again if it gets dropped. It's more an issue that peers will downscore nodes because they had a different view of the current chain head and so wound up using a different fork digest to validate the message. The low volume of BLS change gossip means it likely won't be an issue but attestations are more problematic and could potentially cause a peer to be downscored enough to be disconnected during a fork even though it only sent valid messages.

[djrtwo]

I'm okay with doing this if it has engineering support, but as @ajsutton points out, we should probably make this more of a comprehensive pattern than just a one-off patch here.

Agreed that this either is a bug or at least just an ambiguity given the state that gets passed into underlying function referenced. intention is certainly to utilize the relevant (correct fork version, head state transitioned forwrad to the slot/epoch in question, etc) but that intention does not come through here -- and could case undue cost in some scenarios

[djrtwo]

closing in favor of #3206