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

default vc to block v3 endpoint and deprecate block-v3 flag #5292

Merged
merged 15 commits into from
Jul 26, 2024

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Feb 25, 2024

Issue Addressed

Closes #5040

Proposed Changes

Deprecate the produce-block-v3 flag, and use the v3 endpoint by default within the VC

This should only be merged after deneb is safely activated.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM

@dapplion dapplion added the ready-for-review The code is ready for review label Feb 27, 2024
@michaelsproul michaelsproul added the backwards-incompat Backwards-incompatible API change label Mar 6, 2024
@michaelsproul
Copy link
Member

michaelsproul commented Mar 6, 2024

I think we should wait until after v5.1.0 for this one

@eserilev eserilev added v5.2.0 Q2 2024 and removed blocked labels Mar 31, 2024
@michaelsproul
Copy link
Member

michaelsproul commented Apr 29, 2024

Looks like we'll be OK to merge this for v5.2.0. There was a bug in Nimbus's v3 implementation prior to v24.4.0, which is due imminently:

Bug:

We should call this out in the release notes so that users running Nimbus BNs know they need to upgrade.

I'm doing some more testing with other BNs and blockdreamer.

@michaelsproul
Copy link
Member

Idk about this. My Teku testing got blocked by a bug on their side involving eleel which I haven't had time to fix.

Let's just drop it from v5.2.0 for now so we can get moving.

@michaelsproul michaelsproul added val-client Relates to the validator client binary under-review A reviewer has only partially completed a review. and removed v5.2.0 Q2 2024 ready-for-review The code is ready for review labels Apr 30, 2024
@michaelsproul michaelsproul added the v5.3.0 Q3 2024 release with database changes! label Jul 15, 2024
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed under-review A reviewer has only partially completed a review. labels Jul 18, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM. Nice simplification.

@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 18, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Jul 18, 2024
@michaelsproul
Copy link
Member

I'm not sure why the fallback simulator is failing. Haven't had a chance to investigate

@michaelsproul michaelsproul removed the ready-for-merge This PR is ready to merge. label Jul 18, 2024
@jimmygchen
Copy link
Member

It looks like the BNs had some issues with getting payload from the EL. I've re-run the job to check if it's just a flaky error:

2024-07-22T19:21:17.0428622Z Jul 22 19:21:17.017 ERRO Error whilst producing block            info: block v3 proposal failed, this error may or may not result in a missed block, block_slot: Slot(110), error: "Error from beacon node when producing block: Recoverable(\"Error from beacon node when producing block: ServerMessage(ErrorMessage { code: 400, message: \\\"BAD_REQUEST: failed to fetch a block: GetPayloadFailed(EngineError(Api { error: PayloadIdUnavailable }))\\\", stacktraces: [] })\")", service: block, service: validator_2

@@ -584,7 +511,7 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
&metrics::BLOCK_SERVICE_TIMES,
&[metrics::BEACON_BLOCK_HTTP_GET],
);
let block_response = Self::get_validator_block_v3(
let block_response = Self::get_validator_block(
Copy link
Member

Choose a reason for hiding this comment

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

@jimmygchen
Copy link
Member

Oh I think the fallback is not working because we're always returning an Ok here instead of returning the result:

Ok::<_, BlockError>(block_response)

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed blocked labels Jul 25, 2024
@eserilev
Copy link
Collaborator Author

thanks @jimmygchen! That seems to have fixed it (locally at least). The job failed in CI but maybe its just flaky? ill retry later when I'm at my computer

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Nice, green CI now 🎉

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 26, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 26, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f60503c

mergify bot added a commit that referenced this pull request Jul 26, 2024
@mergify mergify bot merged commit f60503c into sigp:unstable Jul 26, 2024
29 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v5.3.0 Q3 2024 release with database changes! val-client Relates to the validator client binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants