add el_offline to /eth/v1/node/syncing
#4860
Conversation
Add compatibility with ethereum/beacon-APIs#290 to the beacon node. Behaviour when configured with multiple ELs is not specified; intention suggests to indicate whether all ELs are offline.
| if node.currentSlot().epoch() >= node.dag.cfg.CAPELLA_FORK_EPOCH: | ||
| some(not node.elManager.hasAnyWorkingConnection) |
There was a problem hiding this comment.
I think the concern here is that an old client may be strict about inclusion of this extra key.
Mostly theoretical, only gnosis / research, as capella shipped already
There was a problem hiding this comment.
The API spec requires from the client to ignore unfamiliar fields and this field has some value even before the capella fork epoch, so I think it would be better to introduce the small risk of incompatibility here.
There was a problem hiding this comment.
Well, out own VC has a history of not ignoring them. But I guess, those versions are incompatible with Capella based BN anyway.
Anyway, I'm fine with either, but for execution_optimistic there is a similar guard for bellatrix fork epoch, which is enforced across 3 different modules. I'd personally rather err on the consistent side of having the guard (and then remove them altogether if we decide that's the way to go), over having it sometimes but not other times.
If you strongly feel that the guard should be removed, I can remove it; I don't think that there are practical consequences from having it or not having it.
(followup PR, depending on your preference)
Add compatibility with ethereum/beacon-APIs#290 to the beacon node. Behaviour when configured with multiple ELs is not specified; intention suggests to indicate whether all ELs are offline.