[Merged by Bors] - Optimise head block root API #4799

michaelsproul · 2023-10-02T23:55:48Z

Issue Addressed

We've had a report of sync committee performance suffering with the beacon processor HTTP API prioritisations.

Proposed Changes

Increase the priority of /eth/v1/beacon/blocks/head/root requests, which are used by the validator client to form sync committee messages, here:

lighthouse/validator_client/src/sync_committee_service.rs

Lines 181 to 188 in 441fc16

    
           // Fetch `block_root` with non optimistic execution for `SyncCommitteeContribution`. 
        
           let response = self 
        
               .beacon_nodes 
        
               .first_success( 
        
                   RequireSynced::No, 
        
                   OfflineOnFailure::Yes, 
        
                   |beacon_node| async move { 
        
                       match beacon_node.get_beacon_blocks_root(BlockId::Head).await {

Additionally, avoid loading the blinded block in all but the block_id=block_root case. I'm not sure why we were doing this previously, I suspect it was just an oversight during the implementation of the finalized status on API requests.

Additional Info

I think this change should have minimal negative impact as:

The block root endpoint is quick to compute (a few ms max).
Only the priority of head requests is increased. Analytical processes that are making lots of block root requests for past slots are unable to DoS the beacon processor, as their requests will still be processed after attestations.

paulhauner

Good call. Seems to be some formatting issues but I'm down with the concept.

michaelsproul · 2023-10-03T01:13:32Z

Fixed the formatting and also pushed a change to optimise the actual implementation. Should be ready for review now.

Timings for head block roots are down from a few ms, to a few hundred microseconds:

With v4.5.0:

Oct 03 01:08:42.862 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 1.946989ms, module: http_api:193
Oct 03 01:08:43.870 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 1.853395ms, module: http_api:193
Oct 03 01:08:44.878 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 1.862565ms, module: http_api:193
Oct 03 01:08:45.886 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 3.156039ms, module: http_api:193
Oct 03 01:08:46.893 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 1.934108ms, module: http_api:193
Oct 03 01:08:47.901 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 3.226093ms, module: http_api:193

With this branch:

Oct 03 01:04:09.472 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 129.789µs, module: http_api:193
Oct 03 01:04:10.477 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 131.309µs, module: http_api:193
Oct 03 01:04:11.481 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 605.21µs, module: http_api:193
Oct 03 01:04:12.486 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 168.761µs, module: http_api:193
Oct 03 01:04:13.492 DEBG Processed HTTP API request, method: GET, path: /eth/v1/beacon/blocks/head/root, status: 200 OK, elapsed: 143.819µs, module: http_api:193

jimmygchen · 2023-10-03T04:06:21Z

beacon_node/http_api/src/lib.rs

+                    Priority::P1
+                };
+                task_spawner.blocking_json_task(priority, move || {
+                    let (block_root, execution_optimistic, finalized) = block_id.root(&chain)?;


Nice! I'm guessing this also improve other block id queries as we don't necessary need to load blinded blocks from the store for the block roots.

Yeah, by-slot queries in particular should be a lot faster, as they can just load the block root directly. It also removes the hashing of the block, which is sometimes substantial (at least a few ms)

paulhauner · 2023-10-03T23:59:12Z

bors r+

## Issue Addressed We've had a report of sync committee performance suffering with the beacon processor HTTP API prioritisations. ## Proposed Changes Increase the priority of `/eth/v1/beacon/blocks/head/root` requests, which are used by the validator client to form sync committee messages, here: https://github.com/sigp/lighthouse/blob/441fc1691b69f9edc4bbdc6665f3efab16265c9b/validator_client/src/sync_committee_service.rs#L181-L188 Additionally, avoid loading the blinded block in all but the `block_id=block_root` case. I'm not sure why we were doing this previously, I suspect it was just an oversight during the implementation of the `finalized` status on API requests. ## Additional Info I think this change should have minimal negative impact as: - The block root endpoint is quick to compute (a few ms max). - Only the priority of `head` requests is increased. Analytical processes that are making lots of block root requests for past slots are unable to DoS the beacon processor, as their requests will still be processed after attestations.

bors · 2023-10-04T00:41:03Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed We've had a report of sync committee performance suffering with the beacon processor HTTP API prioritisations. ## Proposed Changes Increase the priority of `/eth/v1/beacon/blocks/head/root` requests, which are used by the validator client to form sync committee messages, here: https://github.com/sigp/lighthouse/blob/441fc1691b69f9edc4bbdc6665f3efab16265c9b/validator_client/src/sync_committee_service.rs#L181-L188 Additionally, avoid loading the blinded block in all but the `block_id=block_root` case. I'm not sure why we were doing this previously, I suspect it was just an oversight during the implementation of the `finalized` status on API requests. ## Additional Info I think this change should have minimal negative impact as: - The block root endpoint is quick to compute (a few ms max). - Only the priority of `head` requests is increased. Analytical processes that are making lots of block root requests for past slots are unable to DoS the beacon processor, as their requests will still be processed after attestations.

michaelsproul added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! optimization Something to make Lighthouse run more efficiently. HTTP-API v4.6.0 ETA Q1 2024 labels Oct 2, 2023

michaelsproul changed the base branch from stable to unstable October 2, 2023 23:55

Increase head block root requests to priority 0

2f1dc24

michaelsproul force-pushed the sync-block-root-p0 branch from 0d82c26 to 2f1dc24 Compare October 2, 2023 23:56

paulhauner approved these changes Oct 3, 2023

View reviewed changes

michaelsproul added 2 commits October 3, 2023 11:32

Fix syntax snafus

b8e0902

Optimise actual implementation

371f021

michaelsproul changed the title ~~Increase head block root requests to priority 0~~ Optimise head block root API Oct 3, 2023

jimmygchen reviewed Oct 3, 2023

View reviewed changes

jimmygchen approved these changes Oct 3, 2023

View reviewed changes

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

bors bot changed the title ~~Optimise head block root API~~ [Merged by Bors] - Optimise head block root API Oct 4, 2023

bors bot closed this Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Optimise head block root API #4799

[Merged by Bors] - Optimise head block root API #4799

michaelsproul commented Oct 2, 2023 •

edited

Loading

paulhauner left a comment

michaelsproul commented Oct 3, 2023

jimmygchen Oct 3, 2023 •

edited

Loading

michaelsproul Oct 3, 2023

paulhauner commented Oct 3, 2023

bors bot commented Oct 4, 2023

	// Fetch `block_root` with non optimistic execution for `SyncCommitteeContribution`.
	let response = self
	.beacon_nodes
	.first_success(
	RequireSynced::No,
	OfflineOnFailure::Yes,
	\|beacon_node\| async move {
	match beacon_node.get_beacon_blocks_root(BlockId::Head).await {

[Merged by Bors] - Optimise head block root API #4799

[Merged by Bors] - Optimise head block root API #4799

Conversation

michaelsproul commented Oct 2, 2023 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

paulhauner left a comment

Choose a reason for hiding this comment

michaelsproul commented Oct 3, 2023

jimmygchen Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

michaelsproul Oct 3, 2023

Choose a reason for hiding this comment

paulhauner commented Oct 3, 2023

bors bot commented Oct 4, 2023

michaelsproul commented Oct 2, 2023 •

edited

Loading

jimmygchen Oct 3, 2023 •

edited

Loading