Payload ID caching #10481
Payload ID caching #10481
Conversation
terencechain
added
Ready For Review
Merge
| // Get fee recipient. | ||
| feeRecipient := params.BeaconConfig().DefaultFeeRecipient | ||
| recipient, err := s.cfg.BeaconDB.FeeRecipientByValidatorID(ctx, proposerID) | ||
| switch err == nil { |
There was a problem hiding this comment.
cleaner to just do a switch {} and in each case be more expliti, such as err != nil
| // the pending queue as it has been marked as a 'bad' block. | ||
| span.End() | ||
| continue | ||
| if !errors.Is(ErrOptimisticParent, err) { |
| f.Lock() | ||
| defer f.Unlock() | ||
| var vIdBytes [vIdLength]byte | ||
| copy(vIdBytes[:], bytesutil.Uint64ToBytesBigEndian(uint64(vId))) |
There was a problem hiding this comment.
Why big endian for these? We should probably stick to little endian across the repo except for engine API calls
There was a problem hiding this comment.
payloadID in the specs is [8]bytes by definition https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md. I would think that we are better of without any conversion to an integer type.
There was a problem hiding this comment.
big-endian works because it's not consensus, we use Uint64ToBytesBigEndian throughout prysm, we don't have BytesToUint64LittleEndian hah
| f.Lock() | ||
| defer f.Unlock() | ||
| var vIdBytes [vIdLength]byte | ||
| copy(vIdBytes[:], bytesutil.Uint64ToBytesBigEndian(uint64(vId))) |
There was a problem hiding this comment.
payloadID in the specs is [8]bytes by definition https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md. I would think that we are better of without any conversion to an integer type.
| // getPayloadAttributes returns the payload attributes for the given state and slot. | ||
| // The attribute is required to initiate a payload build process in the context of an `engine_forkchoiceUpdated` call. | ||
| func (s *Service) getPayloadAttribute(ctx context.Context, st state.BeaconState, slot types.Slot) (bool, *enginev1.PayloadAttributes, types.ValidatorIndex, error) { | ||
| proposerID, _, ok := s.cfg.ProposerSlotIndexCache.GetProposerPayloadIDs(slot) |
There was a problem hiding this comment.
In the event of a cache miss, shouldn't we be calling the engine API instead of just failing?
There was a problem hiding this comment.
That's right, in the event of a cache miss, we return false and call engine API. It doesn't fail
| // This returns the execution payload of a given slot. The function has full awareness of pre and post merge. | ||
| // The payload is computed given the respected time of merge. | ||
| func (vs *Server) getExecutionPayload(ctx context.Context, slot types.Slot, vIdx types.ValidatorIndex) (*enginev1.ExecutionPayload, error) { | ||
| proposerID, payloadId, ok := vs.ProposerSlotIndexCache.GetProposerPayloadIDs(slot) | ||
| if ok && proposerID == vIdx && payloadId != [8]byte{} { // Payload ID is cache hit. Return the cached payload ID. |
There was a problem hiding this comment.
This conditional is repeated in the cache itself. Maybe you could expose a function called IsCacheHit
There was a problem hiding this comment.
The condition is different than the one in the cache. It'll be hard to combine them.
In here. To use the payload ID. These three all have to be satisfied:
- there exists a slot entry
- pre-cached proposer ID matched the proposer ID requesting this block
- the payload ID is non-empty
In the cache. To override the payload. One of these two has to be satisfied:
- there doesn't exist a slot entry
- there exists a slot entry and the payload is nonempty (reorg case)
There was a problem hiding this comment.
I would like to see one more approval on this before we merge, perhaps from @potuz
This PR implements the payload ID cache which ensures BN gives EE enough time to prepare the payload. The accompanying doc summarizes the design and tradeoff: https://www.notion.so/Payload-ID-cache-582725c0205c45fbb853d3218e062658