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.

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

more efficient forkchoiceUpdated usage #4055

Merged
merged 6 commits into from
Sep 7, 2022
Merged

more efficient forkchoiceUpdated usage #4055

merged 6 commits into from
Sep 7, 2022

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Aug 31, 2022

No description provided.

@github-actions
Copy link

github-actions bot commented Aug 31, 2022

Unit Test Results

       12 files  ±0       860 suites  ±0   1h 13m 13s ⏱️ + 11m 18s
  1 982 tests ±0    1 835 ✔️ ±0  147 💤 ±0  0 ±0 
10 662 runs  ±0  10 472 ✔️ ±0  190 💤 ±0  0 ±0 

Results for commit 88e16cf. ± Comparison against base commit d9ceb61.

♻️ This comment has been updated with latest results.

@@ -47,6 +48,10 @@ type
# ----------------------------------------------------------------
eth1Monitor*: Eth1Monitor

# Allow determination of whether there's an upcoming proposal
# ----------------------------------------------------------------
attachedValidators*: ref ValidatorPool
Copy link
Member

Choose a reason for hiding this comment

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

we need to consider proposers that come in via rest api as well

Copy link
Contributor

Choose a reason for hiding this comment

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

ActionTracker could be an alternative that takes care of VCs, no?

func getNextProposalSlot*(tracker: ActionTracker, slot: Slot): Slot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2809f41 uses getNextProposalSlot

Copy link
Member

Choose a reason for hiding this comment

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

looking over the code, in addition to the action tracker we should also be checking https://github.com/status-im/nimbus-eth2/blob/7cac6f46db950ed7bdf758f82d3533526a1c1f4f/beacon_chain/spec/eth2_apis/dynamic_fee_recipients.nim which collects calls from https://ethereum.github.io/beacon-APIs/#/Validator/prepareBeaconProposer which is the offiical way of preparing - actiontracker derives "active" proposers from attestations which is a good enough proxy in many cases but not all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0782a71

Has risk of false positives, but if people prefer that, sure.

# 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.
asyncSpawn self.consensusManager.runProposalForkchoiceUpdated(
Copy link
Member

Choose a reason for hiding this comment

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

is the spawn here guaranteed to complete before the next fcu call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as Nimbus is concerned, yes

try:
let fcResult = awaitWithTimeout(
forkchoiceUpdated(
self.eth1Monitor,
headBlockRoot,
beaconHead.safeExecutionPayloadHash,
beaconHead.finalizedExecutionPayloadHash,
timestamp, randomData, feeRecipient),
FORKCHOICEUPDATED_TIMEOUT):
debug "runProposalForkchoiceUpdated: forkchoiceUpdated timed out"
ForkchoiceUpdatedResponse(
payloadStatus: PayloadStatusV1(status: PayloadExecutionStatus.syncing))
if fcResult.payloadStatus.status != PayloadExecutionStatus.valid or
fcResult.payloadId.isNone:
return

FORKCHOICEUPDATED_TIMEOUT is 8 seconds.

# TODO after things stabilize with this, check for upcoming proposal and
# don't bother sending first fcU, but initially, keep both in place
asyncSpawn self.runProposalForkchoiceUpdated()
if self.checkNextProposer(wallSlot).isSome:
Copy link
Member

Choose a reason for hiding this comment

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

this is called inside rpfu as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tersec tersec enabled auto-merge (squash) September 7, 2022 13:35
@arnetheduck arnetheduck enabled auto-merge (squash) September 7, 2022 18:17
@arnetheduck arnetheduck merged commit bf3a014 into unstable Sep 7, 2022
@arnetheduck arnetheduck deleted the OXg branch September 7, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants