[Merged by Bors] - Add `finalized` to HTTP API responses by danielrachi1 · Pull Request #3753 · sigp/lighthouse

danielrachi1 · 2022-11-26T19:32:59Z

Issue Addressed

#3708

Proposed Changes

Add is_finalized_block method to BeaconChain in beacon_node/beacon_chain/src/beacon_chain.rs.
Add is_finalized_state method to BeaconChain in beacon_node/beacon_chain/src/beacon_chain.rs.
Add fork_and_execution_optimistic_and_finalized in beacon_node/http_api/src/state_id.rs.
Add ExecutionOptimisticFinalizedForkVersionedResponse type in consensus/types/src/fork_versioned_response.rs.
Add execution_optimistic_finalized_fork_versioned_responsefunction in beacon_node/http_api/src/version.rs.
Add ExecutionOptimisticFinalizedResponse type in common/eth2/src/types.rs.
Add add_execution_optimistic_finalized method in common/eth2/src/types.rs.
Update API response methods to include finalized.
Remove execution_optimistic_fork_versioned_response

michaelsproul

This is great, thanks for digging into it!

You're definitely on the right track, I just left a few pointers about how to progress from here

beacon_node/http_api/src/lib.rs

beacon_node/http_api/src/block_id.rs

common/eth2/src/types.rs

…k_id}

danielrachi1 · 2022-12-04T23:27:37Z

I think that's all :)

Some things to settle before it's... finalized:

I don't know how I should do the error handling for the two functions I added (is_finalized_block and is_finalized_state). I don't see how they could fail / when I should return an error. So, any help here would be great. (Maybe just unwrap_or(false) ?)
execution_optimistic_fork_versioned_response is no longer used. So, I'll delete it in my next commit. (I wanted to mention this explicitly, just in case.)
I haven't added any tests. I think I should read the existing tests on beacon_node/http_api/tests/ and modify them so they check if finalized is being returned with the right value. Is this the right way to go?

michaelsproul · 2022-12-16T21:06:11Z

Sorry I didn't get back to you about this!

Your approach to error handling looks good at a quick glance and I agree we don't need execution_optimistic_fork_versioned_response if it's unused.

I'll try to give this a more thorough review on Monday & help with writing tests if you need help

michaelsproul

This is looking really nice on the whole, I've just made a few suggestions which I hope will decrease the impact on performance.

In general my suggestions try to avoid disk I/O and repetition. Disk I/O, particularly loading full states, is a major source of slowness.

It would also be really cool to do some real-world benchmarks of nodes before and after this change is applied. We could write a utility into lcli which uses the API wrappers from eth2 to make different calls against a live node, and record how long they take. I'd be happy to work on this as it would also be useful for testing other API changes and optimisations.

beacon_node/http_api/src/lib.rs

michaelsproul

Looks good

There are a few merge conflicts to fix up but once they're resolved I'll give it another look and we can merge.

Most of the conflicts will be from #4068, which changed all the HTTP handlers to return warp::reply::Response. Sprinkling a few into_response() calls in a couple of places might be all we need. The blocking_json_task already takes care of returning a Response, so any calls using that should be OK.

…zed_flag

danielrachi1 · 2023-03-14T16:43:43Z

Unless I missed something, it seems like the conflicts were quite simple. Just some use statements that needed to be updated.

michaelsproul · 2023-03-17T00:47:58Z

Thanks @danielrachi, seeing as this is quite a major change and isn't essential for Capella I'm nominating it for inclusion in v4.1.0, which should be released in a few weeks.

- Remove unnecessary tuple nesting like `((data, finalized), optimistic)` - Return `finalized` from `BlockId` functions to avoid repeated block loads from disk - Add `finalized` status to new rewards APIs added after this PR was opened

michaelsproul · 2023-03-30T01:44:13Z

@danielrachi I pushed some minor changes in 5e2be5f that I thought of while reviewing. I'll deploy this PR on some of our nodes, and then I think it's good to merge

michaelsproul

This has been:

Manually reviewed
Benchmarked
Deployed on Goerli

We're good to go

michaelsproul · 2023-03-30T06:06:05Z

bors r+

## Issue Addressed #3708 ## Proposed Changes - Add `is_finalized_block` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `is_finalized_state` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `fork_and_execution_optimistic_and_finalized` in `beacon_node/http_api/src/state_id.rs`. - Add `ExecutionOptimisticFinalizedForkVersionedResponse` type in `consensus/types/src/fork_versioned_response.rs`. - Add `execution_optimistic_finalized_fork_versioned_response`function in `beacon_node/http_api/src/version.rs`. - Add `ExecutionOptimisticFinalizedResponse` type in `common/eth2/src/types.rs`. - Add `add_execution_optimistic_finalized` method in `common/eth2/src/types.rs`. - Update API response methods to include finalized. - Remove `execution_optimistic_fork_versioned_response` Co-authored-by: Michael Sproul <[email protected]>

michaelsproul · 2023-03-30T06:08:16Z

Will batch this actually

bors r-
bors r+

bors · 2023-03-30T06:08:18Z

Canceled.

## Issue Addressed #3708 ## Proposed Changes - Add `is_finalized_block` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `is_finalized_state` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `fork_and_execution_optimistic_and_finalized` in `beacon_node/http_api/src/state_id.rs`. - Add `ExecutionOptimisticFinalizedForkVersionedResponse` type in `consensus/types/src/fork_versioned_response.rs`. - Add `execution_optimistic_finalized_fork_versioned_response`function in `beacon_node/http_api/src/version.rs`. - Add `ExecutionOptimisticFinalizedResponse` type in `common/eth2/src/types.rs`. - Add `add_execution_optimistic_finalized` method in `common/eth2/src/types.rs`. - Update API response methods to include finalized. - Remove `execution_optimistic_fork_versioned_response` Co-authored-by: Michael Sproul <[email protected]>

bors · 2023-03-30T08:37:03Z

Pull request successfully merged into unstable.

Build succeeded:

## Issue Addressed sigp#3708 ## Proposed Changes - Add `is_finalized_block` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `is_finalized_state` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `fork_and_execution_optimistic_and_finalized` in `beacon_node/http_api/src/state_id.rs`. - Add `ExecutionOptimisticFinalizedForkVersionedResponse` type in `consensus/types/src/fork_versioned_response.rs`. - Add `execution_optimistic_finalized_fork_versioned_response`function in `beacon_node/http_api/src/version.rs`. - Add `ExecutionOptimisticFinalizedResponse` type in `common/eth2/src/types.rs`. - Add `add_execution_optimistic_finalized` method in `common/eth2/src/types.rs`. - Update API response methods to include finalized. - Remove `execution_optimistic_fork_versioned_response` Co-authored-by: Michael Sproul <[email protected]>

- Add `is_finalized_block` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `is_finalized_state` method to `BeaconChain` in `beacon_node/beacon_chain/src/beacon_chain.rs`. - Add `fork_and_execution_optimistic_and_finalized` in `beacon_node/http_api/src/state_id.rs`. - Add `ExecutionOptimisticFinalizedForkVersionedResponse` type in `consensus/types/src/fork_versioned_response.rs`. - Add `execution_optimistic_finalized_fork_versioned_response`function in `beacon_node/http_api/src/version.rs`. - Add `ExecutionOptimisticFinalizedResponse` type in `common/eth2/src/types.rs`. - Add `add_execution_optimistic_finalized` method in `common/eth2/src/types.rs`. - Update API response methods to include finalized. - Remove `execution_optimistic_fork_versioned_response` Co-authored-by: Michael Sproul <[email protected]>

danielrachi1 force-pushed the add_finalized_flag branch from e7652a7 to f31a4e2 Compare November 29, 2022 03:04

Add finalized flag to /eth/v2/beacon/blocks/{block_id}

ee990af

danielrachi1 force-pushed the add_finalized_flag branch from f31a4e2 to ee990af Compare November 29, 2022 03:36

Add finalized flag to beacon/blocks/{block_id}/root

4f8a14e

michaelsproul reviewed Nov 29, 2022

View reviewed changes

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved

beacon_node/http_api/src/block_id.rs Outdated Show resolved Hide resolved

common/eth2/src/types.rs Outdated Show resolved Hide resolved

danielrachi1 added 3 commits November 29, 2022 21:43

fix is_finalized function

d4fb8e1

replace is_finalized (BlockId) for is_finalized_block (BeaconChain)

c827f29

delete is_finalized function

5679214

danielrachi1 force-pushed the add_finalized_flag branch from 7e8a079 to 5679214 Compare November 30, 2022 05:19

danielrachi1 added 6 commits November 30, 2022 00:30

Add finalized flag to beacon/blocks/{block_id}/attestations

953ea6d

Add finalized flag to beacon/blinded_blocks/{block_id}

9c60a65

implement is_finalized_state in BeaconChain

c020bef

Add finalized flag to beacon/states/{state_id}/*

948ce11

Add finalized flag to GET beacon/headers and GET beacon/headers/{bloc…

baf1d16

…k_id}

Add finalized flag to GET debug/beacon/states/{state_id}

79fd163

danielrachi1 added 2 commits December 16, 2022 11:26

Remove execution_optimistic_fork_versioned_response

5024517

Proper error handling for the finalized flag

6e8be18

michaelsproul added work-in-progress PR is a work-in-progress HTTP-API labels Dec 16, 2022

danielrachi1 added 2 commits December 18, 2022 13:24

Update BeaconNodeHttpClient to handle finalized responses

5802d05

Add tests for the finalized flag

d3eb156

danielrachi1 marked this pull request as ready for review December 18, 2022 23:14

fix tests

5e049e1

danielrachi1 requested a review from michaelsproul December 21, 2022 15:26

macladson added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jan 8, 2023

michaelsproul mentioned this pull request Jan 13, 2023

Implement sync_committee_rewards API (per-validator reward) #3790

Closed

michaelsproul requested changes Jan 25, 2023

View reviewed changes

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved

michaelsproul reviewed Mar 14, 2023

View reviewed changes

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Mar 14, 2023

Merge branch 'unstable' of github.com:sigp/lighthouse into add_finali…

566bb66

…zed_flag

danielrachi1 requested a review from michaelsproul March 14, 2023 16:46

michaelsproul added ready-for-review The code is ready for review v4.1.0 Post-Capella minor release and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Mar 17, 2023

Merge remote-tracking branch 'origin/unstable' into add_finalized_flag

1bc5b97

michaelsproul mentioned this pull request Mar 30, 2023

Delete ExecutionOptimisticForkVersionedResponse #4146

Closed

A few clean-ups and optimisations

5e2be5f

- Remove unnecessary tuple nesting like `((data, finalized), optimistic)` - Return `finalized` from `BlockId` functions to avoid repeated block loads from disk - Add `finalized` status to new rewards APIs added after this PR was opened

Fix tests

51465c5

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Mar 30, 2023

michaelsproul approved these changes Mar 30, 2023

View reviewed changes

bors bot changed the title ~~Add finalized to HTTP API responses~~ [Merged by Bors] - Add finalized to HTTP API responses Mar 30, 2023

bors bot closed this Mar 30, 2023

paulhauner added the backwards-incompat Backwards-incompatible API change label Apr 13, 2023

michaelsproul mentioned this pull request May 5, 2023

Add finalized to HTTP API responses #3708

Closed

Conversation

danielrachi1 commented Nov 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielrachi1 commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelsproul commented Dec 16, 2022

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

danielrachi1 commented Mar 14, 2023

Uh oh!

michaelsproul commented Mar 17, 2023

Uh oh!

michaelsproul commented Mar 30, 2023

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Mar 30, 2023

Uh oh!

michaelsproul commented Mar 30, 2023

Uh oh!

bors bot commented Mar 30, 2023

Uh oh!

bors bot commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielrachi1 commented Nov 26, 2022 •

edited

Loading

danielrachi1 commented Dec 4, 2022 •

edited

Loading