chore(block-builder): a few minor cleanups #2412
Conversation
beacon/validator/block_builder.go
Outdated
| startTime := time.Now() | ||
| defer s.metrics.measureRequestBlockForProposalTime(startTime) | ||
|
|
||
| // payload builder is disabled, so nothing to do | ||
| if s.localPayloadBuilder.Enabled() { |
There was a problem hiding this comment.
a minor optimization: if block building is disabled (as it should be, e.g. in full nodes) we could return early here
| // remotePayloadBuilders represents a list of remote block builders, these | ||
| // builders are connected to other execution clients via the EngineAPI. | ||
| remotePayloadBuilders []PayloadBuilder |
There was a problem hiding this comment.
not sure what is this for, but not really used. Dropped
| @@ -65,6 +65,8 @@ implementation = "{{.BeaconKit.KZG.Implementation}}" | |||
| [beacon-kit.payload-builder] | |||
| # Enabled determines if the local payload builder is enabled. | |||
| # It should be enabled for validators, but it can be disabled | |||
There was a problem hiding this comment.
hopefully this clarifies how this variable should be set
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2412 +/- ##
==========================================
+ Coverage 27.12% 27.30% +0.18%
==========================================
Files 351 350 -1
Lines 15523 15482 -41
Branches 20 20
==========================================
+ Hits 4210 4227 +17
+ Misses 11110 11052 -58
Partials 203 203
|
| @@ -95,9 +93,6 @@ func (s *Service) BuildBlockAndSidecars( | |||
| if err != nil { | |||
| return nil, nil, err | |||
| } | |||
| if envelope == nil { | |||
There was a problem hiding this comment.
this check is not necessary since all code path leading here do intercept the condition envelope == nil and return an error.
I added a unit test around GetPayload to show this
| return nil, err | ||
| } | ||
| return result, nil | ||
| return result, s.Call(ctx, result, GetPayloadMethodV3, payloadID) |
There was a problem hiding this comment.
nit for readability, don't feel strongly about this.
The only relevant point here is that ExecutionPayload is not nil
| @@ -173,18 +173,13 @@ func (pb *PayloadBuilder) RetrievePayload( | |||
| } | |||
|
|
|||
| payload := envelope.GetExecutionPayload() | |||
| if !payload.IsNil() { | |||
There was a problem hiding this comment.
this check does not make sense. Note that we use payload below, without checking if is nil
abi87
force-pushed
the
block-builder-minor-cleanup
branch
from
6f62f69 to
a2df05f
Compare
| @@ -47,26 +47,27 @@ func (pb *PayloadBuilder) RequestPayloadAsync( | |||
| } | |||
|
|
|||
| if payloadID, found := pb.pc.Get(slot, parentBlockRoot); found { | |||
| pb.logger.Warn( | |||
There was a problem hiding this comment.
not sure why this is warning, I don't think this is an error condition really?
There was a problem hiding this comment.
it should be for validators since if optimistic build is enabled, still we should probably revert to info
| startTime := time.Now() | ||
| defer s.metrics.measureRequestBlockForProposalTime(startTime) | ||
|
|
||
| if !s.localPayloadBuilder.Enabled() { |
There was a problem hiding this comment.
a minor optimization: if block building is disabled (as it should be, e.g. in full nodes) we could return early here.
This is currently only done in retrieveExecutionPayload so we partially build the block, to reject it all
| @@ -298,7 +291,7 @@ func (s *Service) buildBlockBody( | |||
| // Dequeue deposits from the state. | |||
| depositIndex, err := st.GetEth1DepositIndex() | |||
| if err != nil { | |||
| return ErrNilDepositIndexStart | |||
There was a problem hiding this comment.
we silence the error here, which I do not like. Improved the error handling
|
|
||
| // ErrNilPayload is an error for when there is no payload | ||
| // in a beacon block. | ||
| ErrNilPayload = errors.New("nil payload in beacon block") |
There was a problem hiding this comment.
not used anymore since we do not check in validators anymore that payload is not nil.
| // ErrNilBlkBody is an error for when the block body is nil. | ||
| ErrNilBlkBody = errors.New("nil block body") | ||
|
|
||
| // ErrNilBlobsBundle is an error for when the blobs bundle is nil. | ||
| ErrNilBlobsBundle = errors.New("nil blobs bundle") | ||
|
|
||
| // ErrNilDepositIndexStart is an error for when the deposit index start is | ||
| // nil. | ||
| ErrNilDepositIndexStart = errors.New("nil deposit index start") |
There was a problem hiding this comment.
dropped and used the real error returned by KvStore getter instead
|
|
||
| // TestGetPayloadV3NeverReturnsEmptyPayload shows that execution payload | ||
| // returned by ethClient is not nil. | ||
| func TestGetPayloadV3NeverReturnsEmptyPayload(t *testing.T) { |
There was a problem hiding this comment.
this UT should give confidence that the payload from GetPayload call is never nil in current implementation
| @@ -26,11 +26,11 @@ import ( | |||
|
|
|||
| // Client - Ethereum rpc client. | |||
| type Client struct { | |||
| *rpc.Client | |||
| rpc.Client | |||
There was a problem hiding this comment.
I replaced the rpc.Client implementation with an interface to ease up unit testing (allows easier mocks/stubs.
| // convenient way to interact with an Ethereum node. | ||
| type Client struct { | ||
| type client struct { |
There was a problem hiding this comment.
unexported as much as possible
| jwtSecret: secret, | ||
| jwtRefreshInterval: jwtRefreshInterval, |
There was a problem hiding this comment.
for a limited number of parameters I much prefer passing them explicitly in the New function rather than using options.
Options seems to be strictly related to the implementation of Client interface and I have found an easy way to mock them.
| ) | ||
|
|
||
| // WithJWTSecret sets the JWT secret for the RPC client. | ||
| func WithJWTSecret(secret *jwt.Secret) func(rpc *Client) { |
There was a problem hiding this comment.
We do use this approach with options in other parts of the codebase, but I personally don't really see much value to it.
We don't usually build structs in multiple ways, i.e. we don't generally have a struct specifying some options and not others.
If I am missing something, LMK
abi87
changed the title
| Client: ethclient.New( | ||
| ethclientrpc.NewClient( | ||
| cfg.RPCDialURL.String(), | ||
| ethclientrpc.WithJWTSecret(jwtSecret), | ||
| ethclientrpc.WithJWTRefreshInterval( | ||
| cfg.RPCJWTRefreshInterval, | ||
| ), |
There was a problem hiding this comment.
just moved the instantiation of ethClient up and replace the options with actual parameter in its New function
| eip4844.KZGCommitment, eip4844.KZGProof, eip4844.Blob, | ||
| eip4844.KZGCommitment, | ||
| eip4844.KZGProof, | ||
| eip4844.Blob, | ||
| ]]{ | ||
| ExecutionPayload: t.Empty(version.Deneb), |
There was a problem hiding this comment.
This is one of key point of this cleanup: ExecutionPayload is never nil, cause Empty is the default empty ExecutionPayloadEnvelope struct.
I added a UT to verify it
| @@ -166,29 +167,9 @@ func (pb *PayloadBuilder) RetrievePayload( | |||
| return nil, err | |||
| } | |||
|
|
|||
| overrideBuilder := envelope.ShouldOverrideBuilder() | |||
There was a problem hiding this comment.
below is just stuff for logging. I consolidated it slightly
This PR collects a few minor cleanup I collected while reviewing the block building code:
BuildBlockAndSidecarsin case block building is disabled (as it could be for full nodes)rpc.Clientto simplify the unit test that proves itremotePayloadBuilderswhich are not really used at this point in timeapp.tomlconfig