From 3a92bf3914d623c02f7ada89d6fbd31b6600cf48 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 15 May 2023 22:42:42 +0200 Subject: [PATCH] use dependent root as `execution_optimistic` basis for duties (#4955) The validator beacon APIs `getAttesterDuties`, `getProposerDuties`, and `getSyncCommitteeDuties`, have reported the `execution_optimistic` state for the current head block. This can lead to a race if duties are requested around the slot start, if a new head block is currently being processed by the EL, during which the BN head may be briefly optimistic. `execution_optimistic` is documented in beacon APIs as: > True if the response references an unverified execution payload. > Optimistic information may be invalidated at a later time. > If the field is not present, assume the False value. As the duty endpoints reference the shuffling dependent root instead of the currently selected head block, `execution_optimistic` is now fetched based on that shuffling dependent block root. As this dependent block is in the past it doesn't usually become optimistic when adding new blocks. Note that the endpoints requested 4/8 seconds into the slot that perform the actual duties instead of just querying for duty schedule, still report `execution_optimistic` based on the BN head block. --- beacon_chain/rpc/rest_utils.nim | 11 +++++ beacon_chain/rpc/rest_validator_api.nim | 55 ++++++++++++++++--------- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/beacon_chain/rpc/rest_utils.nim b/beacon_chain/rpc/rest_utils.nim index 6a78071be..5712288ec 100644 --- a/beacon_chain/rpc/rest_utils.nim +++ b/beacon_chain/rpc/rest_utils.nim @@ -272,6 +272,17 @@ func keysToIndices*(cacheTable: var Table[ValidatorPubKey, ValidatorIndex], indices[listIndex[]] = some(ValidatorIndex(validatorIndex)) indices +proc getShufflingOptimistic*(node: BeaconNode, + dependentSlot: Slot, + dependentRoot: Eth2Digest): Option[bool] = + if node.currentSlot().epoch() >= node.dag.cfg.BELLATRIX_FORK_EPOCH: + if dependentSlot <= node.dag.finalizedHead.slot: + some[bool](false) + else: + some[bool](node.dag.is_optimistic(dependentRoot)) + else: + none[bool]() + proc getStateOptimistic*(node: BeaconNode, state: ForkedHashedBeaconState): Option[bool] = if node.currentSlot().epoch() >= node.dag.cfg.BELLATRIX_FORK_EPOCH: diff --git a/beacon_chain/rpc/rest_validator_api.nim b/beacon_chain/rpc/rest_validator_api.nim index dbc2ffff6..de9cbf248 100644 --- a/beacon_chain/rpc/rest_validator_api.nim +++ b/beacon_chain/rpc/rest_validator_api.nim @@ -65,11 +65,12 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = return RestApiResponse.jsonError(Http400, InvalidEpochValueError, "Cannot request duties past next epoch") res - let (qhead, qoptimistic) = + let (qhead, _) = block: let res = node.getSyncedHead(qepoch) if res.isErr(): - return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError) + return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError, + $res.error()) res.get() let shufflingRef = node.dag.getShufflingRef(qhead, qepoch, true).valueOr: return RestApiResponse.jsonError(Http400, PrunedStateError) @@ -102,11 +103,9 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = ) res - let optimistic = - if node.currentSlot().epoch() >= node.dag.cfg.BELLATRIX_FORK_EPOCH: - some(qoptimistic) - else: - none[bool]() + let optimistic = node.getShufflingOptimistic( + qepoch.shufflingDependentSlot, + shufflingRef.attester_dependent_root) return RestApiResponse.jsonResponseWRoot( duties, shufflingRef.attester_dependent_root, optimistic) @@ -127,11 +126,12 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = return RestApiResponse.jsonError(Http400, InvalidEpochValueError, "Cannot request duties past next epoch") res - let (qhead, qoptimistic) = + let (qhead, _) = block: let res = node.getSyncedHead(qepoch) if res.isErr(): - return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError) + return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError, + $res.error()) res.get() let epochRef = node.dag.getEpochRef(qhead, qepoch, true).valueOr: return RestApiResponse.jsonError(Http400, PrunedStateError, $error) @@ -155,11 +155,9 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = ) res - let optimistic = - if node.currentSlot().epoch() >= node.dag.cfg.BELLATRIX_FORK_EPOCH: - some(qoptimistic) - else: - none[bool]() + let optimistic = node.getShufflingOptimistic( + (qepoch + 1).shufflingDependentSlot, + epochRef.proposer_dependent_root) return RestApiResponse.jsonResponseWRoot( duties, epochRef.proposer_dependent_root, optimistic) @@ -245,8 +243,22 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = headEpoch = node.dag.head.slot.epoch headSyncPeriod = sync_committee_period(headEpoch) + dependentSlot = max( + node.dag.cfg.ALTAIR_FORK_EPOCH.start_slot, + if qSyncPeriod >= 2.SyncCommitteePeriod: + (qSyncPeriod - 1).start_slot + else: + GENESIS_SLOT + 1) - 1 + dependentRoot = + if dependentSlot <= node.dag.finalizedHead.slot: + node.dag.finalizedHead.blck.root # No need to look up the actual root + else: + let bsi = node.dag.head.atSlot(dependentSlot) + doAssert bsi.blck != nil, "Non-finalized block has `BlockRef`" + bsi.blck.root + optimistic = node.getShufflingOptimistic(dependentSlot, dependentRoot) + if qSyncPeriod == headSyncPeriod: - let optimistic = node.getStateOptimistic(node.dag.headState) let res = withState(node.dag.headState): when consensusFork >= ConsensusFork.Altair: produceResponse(indexList, @@ -256,7 +268,6 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = emptyResponse() return RestApiResponse.jsonResponseWOpt(res, optimistic) elif qSyncPeriod == (headSyncPeriod + 1): - let optimistic = node.getStateOptimistic(node.dag.headState) let res = withState(node.dag.headState): when consensusFork >= ConsensusFork.Altair: produceResponse(indexList, @@ -288,7 +299,6 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = return RestApiResponse.jsonError(Http404, StateNotFoundError) node.withStateForBlockSlotId(bsi): - let optimistic = node.getStateOptimistic(state) let res = withState(state): when consensusFork >= ConsensusFork.Altair: produceResponse(indexList, @@ -817,9 +827,14 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = res.get() # Check if node is fully synced. - let sres = node.getSyncedHead(qslot) - if sres.isErr() or sres.get().optimistic: - return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError) + block: + let res = node.getSyncedHead(qslot) + if res.isErr(): + return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError, + $res.error()) + let tres = res.get() + if tres.optimistic: + return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError) var contribution = SyncCommitteeContribution() let res = node.syncCommitteeMsgPool[].produceContribution(