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

forkchoiceUpdate support #3199

Merged
merged 1 commit into from
Dec 17, 2021
Merged

forkchoiceUpdate support #3199

merged 1 commit into from
Dec 17, 2021

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Dec 15, 2021

@github-actions
Copy link

github-actions bot commented Dec 15, 2021

Unit Test Results

     12 files  ±0     759 suites  ±0   33m 45s ⏱️ - 3m 18s
1 535 tests ±0  1 489 ✔️ ±0  46 💤 ±0  0 ±0 
9 126 runs  ±0  9 030 ✔️ ±0  96 💤 ±0  0 ±0 

Results for commit 259dbef. ± Comparison against base commit 0f44d2e.

♻️ This comment has been updated with latest results.

timestamp = compute_timestamp_at_slot(state, state.slot)
random = get_randao_mix(state, get_current_epoch(state))
payloadId =
(await execution_engine.forkchoiceUpdated(
Copy link
Member

Choose a reason for hiding this comment

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

where do we handle exceptions for these calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In their callers, e.g.

executionPayloadStatus =
if hasExecutionPayload and
# Allow local testnets to run without requiring an execution layer
blck.blck.mergeData.message.body.execution_payload !=
default(merge.ExecutionPayload):
try:
awaitWithTimeout(
newExecutionPayload(
self.consensusManager.web3Provider,
blck.blck.mergeData.message.body.execution_payload),
web3Timeout):
info "runQueueProcessingLoop: newExecutionPayload timed out"
"SYNCING"
except CatchableError as err:
info "runQueueProcessingLoop: newExecutionPayload failed",
err = err.msg
"SYNCING"
else:
# Vacuously
"VALID"

I've found it useful to be able to decide how to interpret error conditions at a relatively outer layer of processing, rather than hide them in the raw RPC calling functions, because then one can treat a timeout, or certain errors, say, as equivalent to SYNCING, while others might be INVALID. This kind of policy isn't always consistent across callers (e.g., fcU has two callers: the block processor one can fail or time out, and it's mostly fine, but the validator duties one cannot) to bake into the lower-level functions.

Copy link
Member

Choose a reason for hiding this comment

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

hm, we should probably condense those errors to an enum that represents the meaningful / actionable options in the "outer" layers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a distinction here to be made, but (potentially distinct sets of) enums might be appropriate for both:

  1. the "VALID" / "SYNCING" / "INVALID" possibilities, specified by the engine API specification. This is defined in nim-web3. There was some issue with these, so in the interest of expediency, I reverted that field to a string. But one blocker for me in merging more of the kintsugi branch is figuring out what happened there, and properly using an enum again. This, however, shouldn't be adorned beyond what the specification prescribes, but rather mirror it.

  2. Separately, there's a set of states that amounts to a superset of (1), plus some possible error conditions either contacting the EL or that the EL might return in addition to or instead of "VALID" / "SYNCING" / "INVALID". If one wants to move timeout, exception, and other error handling into the inner functions, these would need to be characterized.

@tersec tersec merged commit 57974ce into unstable Dec 17, 2021
@tersec tersec deleted the xA5 branch December 17, 2021 12:23
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.

2 participants