VC: Fix getBlockRoot() response with execution_optimistic. #4622
Conversation
…in `execution_optimistic` field. Fix sync committee service to check `execution_optimistic` field of getBlockRoot() response.
| proc consensusForkAtEpoch*(vc: ValidatorClientRef, | ||
| epoch: Epoch): Result[ConsensusFork, cstring] = | ||
| let fork = vc.forkAtEpoch(epoch) | ||
| if fork.current_version == defaultRuntimeConfig.GENESIS_FORK_VERSION: |
There was a problem hiding this comment.
Does the validator client have any idea if these FORK_VERSIONs are changed from defaultRuntimeConfig? That is a kind of strange object which is sort-of-mainnet-like but not exactly. It is not guaranteed to line up with any particular network someone might try to connect to.
There was a problem hiding this comment.
indeed, these must come from the server - the ideal solution would actually be to construct a runtimeconfig based on the response that the rest server gives - then we could unify a lot of helpers between vc and bn
There was a problem hiding this comment.
The https://ethereum.github.io/beacon-APIs/#/Config/getForkSchedule call the VC uses to get the, well, fork schedule returns a sortable sequence of:
nimbus-eth2/beacon_chain/spec/datatypes/base.nim
Lines 341 to 347 in 4aae1f0
This indicates, yes, an order, of the forks based on fork epoch. It does not say which forks, in terms of Phase 0/Altair/Bellatrix/etc, they are.
In theory, one can imagine two or more fork transitions per epoch, say, from Altair to Capella at once, but ethereum/consensus-specs#2902 notes that create some issues, and it does not appear that anyone has done this.
Still, no one was willing to just outright say yes, it should be fixed, or no, it should be banned, so it remains in this kind of gray area. The Nimbus beacon node explicitly does not support these configurations (checkForkConsistency asserts on startup if someone tries), the Nimbus validator client thus far has not, I believe, made any explicit assumptions around this topic.
It is safe to say that if one has seen two fork transitions, it must be >= Bellatrix, and if one is willing to put the same requirements for at most one hard fork transition per epoch into the VC, one can count exactly the hardfork-offset-from-genesis, but that seems to be it.
Increasingly, testnets, for example, are starting at Bellatrix, so cannot make these inferences in the same way. The documentation of the REST API call states "Retrieve all forks, past present and future, of which this node is aware." If neither phase 0 nor altair ever existed on a network, is a node necessarily aware of them?
Indeed, I would question a bit whether consensusForkAtEpoch is a requisite abstraction here: the actual question this PR needs to answer is whether a head is optimistic. One simple approach, and probably no less reliable than the pile of kludges one would need via the consensusForkAtEpoch approach, is just, is execution_optimistic there? If yes, it is optimistic. If not (including whether it is pre-Bellatrix) it is fully synced and not optimistic.
Sure, this means the VC is not sanity-checking the protocol-correctness of the BN, but it does not have the tools to do so, so it might be better off not faking having such.
There was a problem hiding this comment.
indeed, these must come from the server - the ideal solution would actually be to construct a runtimeconfig based on the response that the rest server gives - then we could unify a lot of helpers between vc and bn
To achieve this, might try to use https://ethereum.github.io/beacon-APIs/#/Config/getSpec and construct the RuntimeConfig from that. But the approach this PR takes, of trying to use getForkSchedule, will not reliably accomplish this goal.
A middle ground is to use just the BELLATRIX_FORK_EPOCH from getSpec, and just >=. That is all this code needs, even in the more scrupulously correct form.
Still, I would suggest first evaluating the requirement in light of this PR specifically, not necessarily in light of broader goals of BN/VC unification. Not at all convinced this PR should care about the fork to begin with.
There was a problem hiding this comment.
well, optimistic is a bellatrix novelty so .. a fork check seems kind of appropriate to make it .. correct.
There was a problem hiding this comment.
Ok, do you have any other ideas how i can get this information? Because for some reason there is only one way to get this constants - using defaultRuntimeConfig object.
There was a problem hiding this comment.
in the BN, we load RuntimeConfig from a yaml file:
https://github.com/status-im/nimbus-eth2/blob/stable/beacon_chain/spec/presets.nim#L488
The right way in the VC would be to do the same, but load it from the getSpec call linked above.
No description provided.