Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fall back to non-fcu fork choice on epoch boundaries #5195

Merged
merged 5 commits into from
Jul 17, 2023
Merged

fall back to non-fcu fork choice on epoch boundaries #5195

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d12a03
fall back to non-fcu fork choice on epoch boundaries
tersec Jul 17, 2023
bf512a7
Future[bool]
tersec Jul 17, 2023
e72cbf1
fix
tersec Jul 17, 2023
ef599a0
Update beacon_chain/consensus_object_pools/consensus_manager.nim
tersec Jul 17, 2023
88ea3e2
make things consistent with Opt[void] return
tersec Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions beacon_chain/consensus_object_pools/consensus_manager.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,11 @@ proc getGasLimit*(
from ../spec/datatypes/bellatrix import PayloadID

proc runProposalForkchoiceUpdated*(
self: ref ConsensusManager, wallSlot: Slot) {.async.} =
self: ref ConsensusManager, wallSlot: Slot): Future[bool] {.async.} =
tersec marked this conversation as resolved.
Show resolved Hide resolved
let
nextWallSlot = wallSlot + 1
(validatorIndex, nextProposer) = self.checkNextProposer(wallSlot).valueOr:
return
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? self.checkNextProposer(wallSlot), if going with Opt[void]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will update these to be consistent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, Future[Opt[void]], accidentally forgot the Future wrapper on first attempt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

88ea3e2

I did try ? self.checkNextProposer and it didn't seem to work, but can investigate later. The valueOr approach still works.

debug "runProposalForkchoiceUpdated: expected to be proposing next slot",
nextWallSlot, validatorIndex, nextProposer

Expand All @@ -353,7 +353,7 @@ proc runProposalForkchoiceUpdated*(
if nextWallSlot.is_epoch:
debug "runProposalForkchoiceUpdated: not running early fcU for epoch-aligned proposal slot",
nextWallSlot, validatorIndex, nextProposer
return
return false

# Approximately lines up with validator_duties version. Used optimistically/
# opportunistically, so mismatches are fine if not too frequent.
Expand Down Expand Up @@ -382,7 +382,7 @@ proc runProposalForkchoiceUpdated*(
headBlockHash = self.dag.loadExecutionBlockHash(beaconHead.blck)

if headBlockHash.isZero:
return
return false

try:
let safeBlockHash = beaconHead.safeExecutionPayloadHash
Expand Down Expand Up @@ -410,6 +410,8 @@ proc runProposalForkchoiceUpdated*(
except CatchableError as err:
error "Engine API fork-choice update failed", err = err.msg

true

proc updateHeadWithExecution*(
self: ref ConsensusManager, initialNewHead: BeaconHead,
getBeaconTimeFn: GetBeaconTimeFn) {.async.} =
Expand Down
33 changes: 18 additions & 15 deletions beacon_chain/gossip_processing/block_processor.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,30 +638,33 @@ proc storeBlock*(
# `forkchoiceUpdated` necessary for EL client only.
self.consensusManager[].updateHead(newHead.get.blck)

if self.consensusManager.checkNextProposer(wallSlot).isNone:
# No attached validator is next proposer, so use non-proposal fcU

template callForkchoiceUpdated(payloadAttributeType: untyped): auto =
await elManager.expectValidForkchoiceUpdated(
headBlockPayloadAttributesType = payloadAttributeType,
headBlockHash = headExecutionPayloadHash,
safeBlockHash = newHead.get.safeExecutionPayloadHash,
finalizedBlockHash = newHead.get.finalizedExecutionPayloadHash,
receivedBlock = signedBlock)

template callExpectValidFCU(payloadAttributeType: untyped): auto =
await elManager.expectValidForkchoiceUpdated(
headBlockPayloadAttributesType = payloadAttributeType,
headBlockHash = headExecutionPayloadHash,
safeBlockHash = newHead.get.safeExecutionPayloadHash,
finalizedBlockHash = newHead.get.finalizedExecutionPayloadHash,
receivedBlock = signedBlock)

template callForkChoiceUpdated: auto =
case self.consensusManager.dag.cfg.consensusForkAtEpoch(
newHead.get.blck.bid.slot.epoch)
of ConsensusFork.Capella, ConsensusFork.Deneb:
callForkchoiceUpdated(payloadAttributeType = PayloadAttributesV2)
callExpectValidFCU(payloadAttributeType = PayloadAttributesV2)
of ConsensusFork.Phase0, ConsensusFork.Altair,
ConsensusFork.Bellatrix:
callForkchoiceUpdated(payloadAttributeType = PayloadAttributesV1)
callExpectValidFCU(payloadAttributeType = PayloadAttributesV1)

if self.consensusManager.checkNextProposer(wallSlot).isNone:
# No attached validator is next proposer, so use non-proposal fcU
callForkChoiceUpdated()
else:
# Some attached validator is next proposer, so prepare payload. As
# updateHead() updated the DAG head, runProposalForkchoiceUpdated,
# which needs the state corresponding to that head block, can run.
await self.consensusManager.runProposalForkchoiceUpdated(
wallSlot)
if not await self.consensusManager.runProposalForkchoiceUpdated(
wallSlot):
callForkChoiceUpdated()
else:
await self.consensusManager.updateHeadWithExecution(
newHead.get, self.getBeaconTime)
Expand Down