[Merged by Bors] - Add `finalized` to HTTP API responses #3753

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 <michael@sigmaprime.io>

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 <michael@sigmaprime.io>

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 <michael@sigmaprime.io>

- 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 <michael@sigmaprime.io>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add `finalized` to HTTP API responses #3753

[Merged by Bors] - Add `finalized` to HTTP API responses #3753

danielrachi1 commented Nov 26, 2022 •

edited

Loading

michaelsproul left a comment

danielrachi1 commented Dec 4, 2022 •

edited

Loading

michaelsproul commented Dec 16, 2022

michaelsproul left a comment

michaelsproul left a comment

danielrachi1 commented Mar 14, 2023

michaelsproul commented Mar 17, 2023

michaelsproul commented Mar 30, 2023

michaelsproul left a comment

michaelsproul commented Mar 30, 2023

michaelsproul commented Mar 30, 2023

bors bot commented Mar 30, 2023

bors bot commented Mar 30, 2023

[Merged by Bors] - Add finalized to HTTP API responses #3753

[Merged by Bors] - Add finalized to HTTP API responses #3753

Conversation

danielrachi1 commented Nov 26, 2022 • edited Loading

Issue Addressed

Proposed Changes

michaelsproul left a comment

Choose a reason for hiding this comment

danielrachi1 commented Dec 4, 2022 • edited Loading

michaelsproul commented Dec 16, 2022

michaelsproul left a comment

Choose a reason for hiding this comment

michaelsproul left a comment

Choose a reason for hiding this comment

danielrachi1 commented Mar 14, 2023

michaelsproul commented Mar 17, 2023

michaelsproul commented Mar 30, 2023

michaelsproul left a comment

Choose a reason for hiding this comment

michaelsproul commented Mar 30, 2023

michaelsproul commented Mar 30, 2023

bors bot commented Mar 30, 2023

bors bot commented Mar 30, 2023

[Merged by Bors] - Add `finalized` to HTTP API responses #3753

[Merged by Bors] - Add `finalized` to HTTP API responses #3753

danielrachi1 commented Nov 26, 2022 •

edited

Loading

danielrachi1 commented Dec 4, 2022 •

edited

Loading