Add Gloas beacon state package#15611
Conversation
4864197 to
442588a
Compare
a02c7b6 to
0d06b2f
Compare
77937de to
1089cb3
Compare
197d571 to
89fef62
Compare
dbb6439 to
31e7558
Compare
4821bb4 to
d0d04a7
Compare
7ad2960 to
ffa57b7
Compare
31e7558 to
4a30dde
Compare
ffa57b7 to
611e9b9
Compare
c5130b9 to
2ff4793
Compare
20513d4 to
c7f23a4
Compare
| BuilderPendingWithdrawals: b.builderPendingWithdrawals, | ||
| LatestBlockHash: b.latestBlockHash, | ||
| LatestWithdrawalsRoot: b.latestWithdrawalsRoot, | ||
| LatestExecutionPayloadBid: b.executionPayloadbid, |
There was a problem hiding this comment.
Maybe change executionPayloadBid to latestExecutionPayloadBid to unify naming in both types?
|
|
||
| payments := make([]*ethpb.BuilderPendingPayment, len(b.builderPendingPayments)) | ||
| for i, payment := range b.builderPendingPayments { | ||
| if payment != nil { |
There was a problem hiding this comment.
You already have a nil check inside Copy() so this check is technically not needed
|
|
||
| withdrawals := make([]*ethpb.BuilderPendingWithdrawal, len(b.builderPendingWithdrawals)) | ||
| for i, withdrawal := range b.builderPendingWithdrawals { | ||
| if withdrawal != nil { |
There was a problem hiding this comment.
same here, no need to check
| @@ -0,0 +1,78 @@ | |||
| package state_native | |||
There was a problem hiding this comment.
Having all Gloas logic in a gloas.go file is new, so far we have been splitting everything by functionality. For example latestWithdrawalsRootVal would go to getters_withdrawal.go.
There was a problem hiding this comment.
I like it better in the new fork specific file.
c6279ec to
839c895
Compare
839c895 to
d221794
Compare
| case LatestExecutionPayloadHeaderDeneb: | ||
| return "latestExecutionPayloadHeaderDeneb" | ||
| case ExecutionPayloadBid: | ||
| return "executionPayloadBid" |
There was a problem hiding this comment.
This is a little confusing in that the other fields are called by their names, should this be "latestExecutionPayloadBid"?
|
|
||
| // Gloas fields | ||
| latestExecutionPayloadBid *ethpb.ExecutionPayloadBid | ||
| executionPayloadAvailability []byte |
There was a problem hiding this comment.
JustificationBits is a bitvector, why is executionPayloadAvailability of type []byte?
| PendingConsolidations []*ethpb.PendingConsolidation `json:"pending_consolidations" yaml:"pending_consolidations"` | ||
| ProposerLookahead []primitives.ValidatorIndex `json:"proposer_look_ahead" yaml:"proposer_look_ahead"` | ||
| LatestExecutionPayloadBid *ethpb.ExecutionPayloadBid `json:"latest_execution_payload_bid" yaml:"latest_execution_payload_bid"` | ||
| ExecutionPayloadAvailability []byte `json:"execution_payload_availability" yaml:"execution_payload_availability"` |
There was a problem hiding this comment.
Same here, justification bits is a Bitvector above.
| BuilderPendingWithdrawals: b.builderPendingWithdrawals, | ||
| LatestBlockHash: b.latestBlockHash, | ||
| LatestWithdrawalsRoot: b.latestWithdrawalsRoot, | ||
| LatestExecutionPayloadBid: b.latestExecutionPayloadBid, |
There was a problem hiding this comment.
should this be ordered as in the state?
| BuilderPendingWithdrawals: b.builderPendingWithdrawalsVal(), | ||
| LatestBlockHash: b.latestBlockHashVal(), | ||
| LatestWithdrawalsRoot: b.latestWithdrawalsRootVal(), | ||
| LatestExecutionPayloadBid: b.latestExecutionPayloadBid.Copy(), |
There was a problem hiding this comment.
Should this be ordered as in the state?
|
|
||
| fieldRoots[types.BuilderPendingWithdrawals.RealPosition()] = bpwRoot[:] | ||
|
|
||
| lbhRoot := bytesutil.ToBytes32(state.latestBlockHash) |
There was a problem hiding this comment.
Do we really need these copies? if there's any doubt I would rather keep these copies indeed.
| types.PendingPartialWithdrawals, | ||
| types.PendingConsolidations, | ||
| types.ProposerLookahead, | ||
| types.ExecutionPayloadBid, |
There was a problem hiding this comment.
Should this be ordered as in the state?, also perhaps it's better to add a list with the fields that are strictly added in deneb and use them both in the deneb field declaration and here.
| denebSharedFieldRefCount = 7 | ||
| electraSharedFieldRefCount = 10 | ||
| fuluSharedFieldRefCount = 11 | ||
| gloasSharedFieldRefCount = 12 |
There was a problem hiding this comment.
Probably add a comment with what is the field added here?
| b.sharedFieldReferences[types.Slashings] = stateutil.NewRef(1) | ||
| b.sharedFieldReferences[types.PreviousEpochParticipationBits] = stateutil.NewRef(1) | ||
| b.sharedFieldReferences[types.CurrentEpochParticipationBits] = stateutil.NewRef(1) | ||
| b.sharedFieldReferences[types.ExecutionPayloadBid] = stateutil.NewRef(1) // New in Gloas. |
There was a problem hiding this comment.
Why is this shared? In fact why is the execution payload header even shared in previous states? cc @rkapka
I'm pretty sure this was just copied/pasted from previous code, but this seems wrong to me.
| ) | ||
|
|
||
| // ExecutionPayloadAvailabilityRoot computes the merkle root of an execution payload availability bitvector. | ||
| func ExecutionPayloadAvailabilityRoot(bitvector []byte) ([32]byte, error) { |
There was a problem hiding this comment.
Note to myself, this function needs to be optimized, copying here all the bytes and chunking doesn't make sense. For smaller bitfields it does make sense, but this will have 256 chunks
| } | ||
|
|
||
| capellaFields = append( | ||
| append([]types.FieldIndex{}, altairFields...), |
There was a problem hiding this comment.
This is a really weird pattern.
| electraSharedFieldRefCount = 10 | ||
| fuluSharedFieldRefCount = 11 | ||
| gloasSharedFieldRefCount = 12 | ||
| gloasSharedFieldRefCount = 12 // Adds LatestExecutionPayloadBid to the shared-ref set. |
There was a problem hiding this comment.
If you remove the payload bid then this comment needs to change?
There was a problem hiding this comment.
Sorry, i meant to say Adds pending builder withdrawals to the shared-ref set, not LatestExecutionPayloadBid
| denebFields = slices.Concat( | ||
| altairFields, | ||
| append([]types.FieldIndex{types.LatestExecutionPayloadHeaderDeneb}, withdrawalAndHistoricalSummaryFields...), |
There was a problem hiding this comment.
concat takes several inputs, this can be written as
denebFields = slices.Concat(
altairFields,
[]types.FieldIndex{types.LatestExecutionPayloadHeaderDeneb},
withdrawalAndHistoricalSummaryFields,
)
| electraSharedFieldRefCount = 10 | ||
| fuluSharedFieldRefCount = 11 | ||
| gloasSharedFieldRefCount = 12 // Adds LatestExecutionPayloadBid to the shared-ref set. | ||
| gloasSharedFieldRefCount = 12 // Adds pending builder withdrawals to the shared-ref set. |
There was a problem hiding this comment.
gloasSharedFieldRefCount = 12 // Adds pending builder withdrawals to the shared-ref set and LatestExecutionPayloadHeader is removed
|
|
||
| capellaFields = slices.Concat( | ||
| altairFields, | ||
| append([]types.FieldIndex{types.LatestExecutionPayloadHeaderCapella}, withdrawalAndHistoricalSummaryFields...), |
There was a problem hiding this comment.
Same here, the second append is not needed.
This PR adds minimal support for the Gloas beacon state package. It ensures that the custom hash tree root function passes the SSZ vector spec test. Getters and setters are not included in this PR and will be added in follow-up PRs as needed. This implementation covers initialization and ensures the package can be used in future tests
Note to the reviewer: please double check
InitializeFromProtoUnsafeGloaswhich i have pending payments and withdrawals as shared reference. It's not clear to me if they should be shared or not, please correct me if i am wrong here: