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.

Sign up for GitHub

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

defer payload attribute computation #14644

Merged
merged 7 commits into from
Nov 19, 2024
Merged

defer payload attribute computation #14644

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d518182
defer payload attribute computation
kasey Oct 25, 2024
d2dfd75
fire payload event on skipped slots
kasey Nov 16, 2024
517c71a
changelog
kasey Nov 16, 2024
9e02bcd
fix test and missing version attr
kasey Nov 16, 2024
262ed05
fix lint
kasey Nov 18, 2024
97dcbd4
deepsource
kasey Nov 18, 2024
8c22fbf
mv head block lookup for missed slots to streamer
kasey Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
- Return early for blob reconstructor during capella fork
- Updated block endpoint from V1 to V2
- Rename instances of "deposit receipts" to "deposit requests".
- non-blocking payload attribute event handling in beacon api [pr](https://github.com/prysmaticlabs/prysm/pull/14644)

### Deprecated

Expand Down
33 changes: 33 additions & 0 deletions beacon-chain/blockchain/execution_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import (

"github.com/ethereum/go-ethereum/common"
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/v5/async/event"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/cache"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/blocks"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed"
statefeed "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed/state"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/time"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/transition"
Expand Down Expand Up @@ -69,6 +72,7 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *fcuConfig) (*
if arg.attributes == nil {
arg.attributes = payloadattribute.EmptyWithVersion(headBlk.Version())
}
go firePayloadAttributesEvent(ctx, s.cfg.StateNotifier.StateFeed(), arg)
payloadID, lastValidHash, err := s.cfg.ExecutionEngineCaller.ForkchoiceUpdated(ctx, fcs, arg.attributes)
if err != nil {
switch {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

pidx, err := helpers.BeaconProposerIndex(ctx, cfg.headState)
if err != nil {
log.WithError(err).
WithField("head_root", cfg.headRoot[:]).
Error("Could not get proposer index for PayloadAttributes event")
return
}
headPayload, err := cfg.headBlock.Block().Body().Execution()
if err != nil {
log.WithError(err).Error("Could not get execution payload for head block")
return
}
f.Send(&feed.Event{
Type: statefeed.PayloadAttributes,
Data: payloadattribute.EventData{
ProposerIndex: pidx,
ProposalSlot: cfg.headState.Slot(),
ParentBlockNumber: headPayload.BlockNumber(),
ParentBlockRoot: cfg.headRoot[:],
ParentBlockHash: headPayload.BlockHash(),
Attributer: cfg.attributes,
HeadRoot: cfg.headRoot,
HeadState: cfg.headState,
HeadBlock: cfg.headBlock,
},
})
}

// getPayloadHash returns the payload hash given the block root.
// if the block is before bellatrix fork epoch, it returns the zero hash.
func (s *Service) getPayloadHash(ctx context.Context, root []byte) ([32]byte, error) {
Expand Down
14 changes: 5 additions & 9 deletions beacon-chain/blockchain/process_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (

"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/blocks"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed"
statefeed "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed/state"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers"
coreTime "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/time"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/transition"
Expand Down Expand Up @@ -620,9 +618,6 @@ func (s *Service) lateBlockTasks(ctx context.Context) {
if !s.inRegularSync() {
return
}
s.cfg.StateNotifier.StateFeed().Send(&feed.Event{
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@kasey kasey Nov 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the meantime I'll add it back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4edea3c

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Type: statefeed.MissedSlot,
})
s.headLock.RLock()
headRoot := s.headRoot()
headState := s.headState(ctx)
Expand All @@ -648,10 +643,6 @@ func (s *Service) lateBlockTasks(ctx context.Context) {
}

attribute := s.getPayloadAttribute(ctx, headState, s.CurrentSlot()+1, headRoot[:])
// return early if we are not proposing next slot
if attribute.IsEmpty() {
return
}

s.headLock.RLock()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

headBlock, err := s.headBlock()
Expand All @@ -668,6 +659,11 @@ func (s *Service) lateBlockTasks(ctx context.Context) {
headBlock: headBlock,
attributes: attribute,
}
// return early if we are not proposing next slot
if attribute.IsEmpty() {
go firePayloadAttributesEvent(ctx, s.cfg.StateNotifier.StateFeed(), fcuArgs)
return
}
_, err = s.notifyForkchoiceUpdate(ctx, fcuArgs)
if err != nil {
log.WithError(err).Debug("could not perform late block tasks: failed to update forkchoice with engine")
Expand Down
2 changes: 2 additions & 0 deletions beacon-chain/core/feed/state/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ const (
LightClientFinalityUpdate
// LightClientOptimisticUpdate event
LightClientOptimisticUpdate
// PayloadAttributes events are fired upon a missed slot or new head.
PayloadAttributes
)

// BlockProcessedData is the data sent with BlockProcessed events.
Expand Down
4 changes: 3 additions & 1 deletion beacon-chain/rpc/eth/events/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ go_library(
"//beacon-chain/core/feed/state:go_default_library",
"//beacon-chain/core/helpers:go_default_library",
"//beacon-chain/core/time:go_default_library",
"//beacon-chain/core/transition:go_default_library",
"//config/params:go_default_library",
"//consensus-types/payload-attribute:go_default_library",
"//consensus-types/primitives:go_default_library",
"//monitoring/tracing/trace:go_default_library",
"//network/httputil:go_default_library",
"//proto/engine/v1:go_default_library",
"//proto/eth/v1:go_default_library",
"//proto/eth/v2:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down Expand Up @@ -52,6 +53,7 @@ go_test(
"//config/fieldparams:go_default_library",
"//consensus-types/blocks:go_default_library",
"//consensus-types/interfaces:go_default_library",
"//consensus-types/payload-attribute:go_default_library",
"//consensus-types/primitives:go_default_library",
"//proto/eth/v1:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down
Loading
Loading