defer payload attribute computation #14644

kasey · 2024-11-15T21:11:34Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

Currently payload attribute event processing for the beacon api event feed occurs in the goroutine that reads event messages from the internal event feeds. If payload computation is slow, the goroutine reading from the event feed will be blocked. If it blocks long enough for the internal event feed to fill the queue, this would defeat the purpose of the async event stream writer design, which relies on this goroutine to process events in roughly linear time so it can consume the event queue in a timely fashion and detect the scenario where the outgoing response writing queue has backed up.

This PR mitigates that issue by spawning an extra goroutine to handle payload attribute event computation in a non-blocking fashion.

The other important change is that this event is now triggered by calls to notifyForkchoiceUpdate and in lateBlockTasks, rather than piggybacking onto missed slot and new head events. This means that payload attribute events are no longer a special case, leading to simpler code, and their emission for builders is in sync with where we would notify the engine api. This also allows us to reuse the attributes that are computed for the engine api, which makes the event lightweight in the case where the proposer is tracked by the bn.

Other notes for review

Acknowledgements

I have read CONTRIBUTING.md.
I have made an appropriate entry to CHANGELOG.md.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

james-prysm · 2024-11-15T21:40:39Z

beacon-chain/blockchain/process_block.go

@@ -620,9 +618,6 @@ func (s *Service) lateBlockTasks(ctx context.Context) {
 	if !s.inRegularSync() {
 		return
 	}
-	s.cfg.StateNotifier.StateFeed().Send(&feed.Event{


don't we still need this? Missed slot feed. #12153 based on this it needs to send when it's late.

I know you mentioned

The other important change is that this event is now triggered by calls to FCU, rather than piggybacking onto missed slot and new head events. This means that payload attribute events are no longer a special case, leading to simpler code, and their emission for builders is in sync with where we would notify the engine API.

but wouldn't it still need to be there?

If the proposer is a tracked validator, lateBlockTasks will call notifyForkchoiceUpdate, which fires the PayloadAttributes event. Otherwise lateBlockTasks will early return before calling notifyForkchoiceUpdate, in which case it won't fire. I need to check if builder/relay really wants these events for untracked validators. If it does, then we should fire the event right before the early return. Will try to ping metachris in the other issue for more insight.

In the meantime I'll add it back in.

I believe it's still needed for untracked ( this was opened a while back, I updated to reflect the same as lighthouse #14204) but it's related to the untracked and submitting it, the builder should only use the registered info though.

but it's related to the untracked and submitting it, the builder should only use the registered info though.

I can't quite make sense of your comment. Right now the code fires events for head updates and missed slots, whether or not the upcoming proposer is tracked. If it is tracked, the fee recipient for the tracked validator will be used. Is that the correct behavior?

that's the correct behavior, just meant that regardless the builder should use the data from the validator registrations and not from the proposer attributes event

james-prysm · 2024-11-18T19:42:22Z

beacon-chain/rpc/eth/events/events.go

@@ -31,6 +33,7 @@ import (
 )

 const DefaultEventFeedDepth = 1000
+const payloadAttributeTimeout = 2 * time.Second


how was this timeout chosen?

It's a guesstimate based on order of magnitude: 100ms possibly too short, 10s too long (would go into the next slot when triggered by lateBlockTasks). So 1s seems about right, and then I added some wiggle room.

james-prysm · 2024-11-18T19:50:50Z

beacon-chain/rpc/eth/events/events.go

-	headPayload, err := headBlock.Block().Body().Execution()
-	if err != nil {
-		return nil, errors.Wrap(err, "could not get execution payload")
+	feeRecpt := params.BeaconConfig().DefaultFeeRecipient.Bytes()


i think this might be wrong if you set the suggested fee recipient flag for untracked validators, I think it needs to be the burn address

This preserves the existing behavior: https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/rpc/eth/events/events.go#L600-L604

If we want to change it I'd rather do that in a separate PR.

terencechain · 2024-11-18T20:57:48Z

beacon-chain/blockchain/execution_engine.go

@@ -167,6 +171,35 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *fcuConfig) (*
 	return payloadID, nil
 }

+func firePayloadAttributesEvent(ctx context.Context, f event.SubscriberSender, cfg *fcuConfig) {


Another option is to make it a method with the receiver (s *Service), allowing access to event.SubscriberSender directly instead of passing it as an extra parameter. A function is probably a better choice than a method in this case

terencechain · 2024-11-18T21:02:50Z

beacon-chain/blockchain/process_block.go

-	// return early if we are not proposing next slot
-	if attribute.IsEmpty() {
-		return
-	}

 	s.headLock.RLock()


This changes an invariant: previously, we didn’t need to call the head block if we weren’t proposing the next block, as calling the head block isn’t free. Now, we must call the head block even if we’re not proposing the next block when a late block is triggered. This might be fine, but if we want to be precise about it, we could move the head block call inside the goroutine

I've changed the event firing code and the handler to accommodate the head block being nil, looking up the block in the handler if it is nil, and making sure the root matches the head root in the event. If the head root in the event is different, we just drop the event. In this circumstance we know the mismatched event was fired by lateBlockTasks, then essentially preempted by block processing updated head. The head update will stream its own proposer attribute event which is what the consumer really wants, so it should be fine to drop the late block event in this race.

terencechain

Lgtm

kasey requested a review from a team as a code owner November 15, 2024 21:11

kasey requested review from nalepae, prestonvanloon and james-prysm November 15, 2024 21:11

kasey force-pushed the precompute-payload-attrs branch from 8f86592 to 8222cb2 Compare November 15, 2024 21:20

james-prysm reviewed Nov 15, 2024

View reviewed changes

kasey force-pushed the precompute-payload-attrs branch 2 times, most recently from 42ffe44 to 5c6a7ed Compare November 18, 2024 17:43

kasey added 6 commits November 18, 2024 12:08

defer payload attribute computation

d518182

fire payload event on skipped slots

d2dfd75

changelog

517c71a

fix test and missing version attr

9e02bcd

fix lint

262ed05

deepsource

97dcbd4

kasey force-pushed the precompute-payload-attrs branch from 1edfa82 to 97dcbd4 Compare November 18, 2024 18:27

kasey enabled auto-merge November 18, 2024 18:58

james-prysm reviewed Nov 18, 2024

View reviewed changes

terencechain reviewed Nov 18, 2024

View reviewed changes

mv head block lookup for missed slots to streamer

8c22fbf

terencechain approved these changes Nov 19, 2024

View reviewed changes

kasey added this pull request to the merge queue Nov 19, 2024

Merged via the queue into develop with commit 8d6577b Nov 19, 2024
18 checks passed

kasey deleted the precompute-payload-attrs branch November 19, 2024 17:12

kasey added the changelog/changed Changelog Section: Changed label Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defer payload attribute computation #14644

defer payload attribute computation #14644

kasey commented Nov 15, 2024 •

edited

Loading

james-prysm Nov 15, 2024

kasey Nov 16, 2024 •

edited

Loading

kasey Nov 16, 2024

kasey Nov 16, 2024

james-prysm Nov 18, 2024

kasey Nov 18, 2024

james-prysm Nov 18, 2024

james-prysm Nov 18, 2024

kasey Nov 18, 2024

james-prysm Nov 18, 2024

kasey Nov 18, 2024

terencechain Nov 18, 2024

terencechain Nov 18, 2024

kasey Nov 19, 2024

terencechain left a comment

defer payload attribute computation #14644

defer payload attribute computation #14644

Conversation

kasey commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

kasey Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

kasey commented Nov 15, 2024 •

edited

Loading

kasey Nov 16, 2024 •

edited

Loading