fix: column announcement before fork transition#7943
Conversation
| // backfilled the new groups. | ||
| // this.custodyConfig.updateAdvertisedCustodyGroupCount(targetCustodyGroupCount); | ||
| // this.emitter.emit(ChainEvent.updateAdvertisedGroupCount, this.custodyConfig.advertisedCustodyGroupCount); | ||
| if (this.config.getForkSeq(slot) < ForkSeq.fulu) { |
There was a problem hiding this comment.
need to confirm FULU_FORK_EPOCH is defined first?
There was a problem hiding this comment.
If we have ForkSeq.fulu that implies FULU_FORK_EPOCH is defined. It can be Infinity though. In that case I think the logic still valid as the current fork will be behind fulu.
There was a problem hiding this comment.
@nazarhussain not correct, see the spec PR
This new field MUST be added once `FULU_FORK_EPOCH` is assigned any value other than `FAR_FUTURE_EPOCH`.
There was a problem hiding this comment.
ah @twoeths is correct, we shouldn't advertise this if fulu epoch is not scheduled in the config
There was a problem hiding this comment.
We discussed this at interop. I will add a change to the spec. It needs to be advertised beforehand for peer management at the transition
There was a problem hiding this comment.
And actually there is another component to this. This change just makes sure the cgc is correct once it gets announced. The announcement is unrelated to this change
There was a problem hiding this comment.
It needs to be advertised beforehand for peer management at the transition
but this is still the case as soon as we set the fork epoch value, in practice this will likely 2-3 weeks before the fork happens
|
|
**Motivation** Follow-up PR to #7943. As per spec > This new field MUST be added once `FULU_FORK_EPOCH` is assigned any value other than `FAR_FUTURE_EPOCH`. we should only announce these if fulu is scheduled in config **Description** Only announce columns if fulu is scheduled in config
|
🎉 This PR is included in v1.34.0 🎉 |
Motivation
Column announcement needs to be updated for validator custody prior to fulu fork transition
Relevant spec: ethereum/consensus-specs#4373