core: implement cached PTC window in state#16573
Conversation
347747d to
1d67dca
Compare
48cf92f to
081c35a
Compare
d10f188 to
b73e843
Compare
b73e843 to
6dac4a4
Compare
| // if is_builder or ( | ||
| // is_builder_withdrawal_credential(deposit_request.withdrawal_credentials) | ||
| // and not is_validator | ||
| // and not is_pending_validator(state, deposit_request.pubkey) | ||
| // ): |
There was a problem hiding this comment.
Because update spec test, i have to change this, this is implemented in #16532
potuz
left a comment
There was a problem hiding this comment.
I'm worried about the committee cache usage of the same seed to get epoch+2 and epoch+1 committees.
|
|
||
| func ptcSlotFromValidatorIndices(indices []primitives.ValidatorIndex) *eth.PTCs { | ||
| result := ð.PTCs{ | ||
| ValidatorIndices: make([]uint64, len(indices)), |
There was a problem hiding this comment.
Shouldn't these be always PTCSize?
|
|
||
| newSlots := make([]*eth.PTCs, slotsPerEpoch) | ||
| for i := range slotsPerEpoch { | ||
| ptc, err := computePTC(ctx, st, startSlot+primitives.Slot(i)) |
There was a problem hiding this comment.
Something that worries me here is that we are now calling to get a committee 2 epochs ago from now, with the current state's seed (we add 2 in line 385 and then subtract 2 in Seed). This is guaranteed not to be in the cache, so BeaconCommitteeFromState will fail in the committeeCache. This will then call ActiveValidatorIndices that will also fail the cache and then recompute the cache with the current state's seed but for epoch e+2. The problem is if we use the seed for e+1 in a few places like Attester Duties but the cache in only keyed by seed. This means that either we will be getting a false seed on some cache hits, or we will be overriding them with some other committees depending on whether we call with e+1 or e+2.
| roots := make([][32]byte, len(slice)) | ||
|
|
||
| for i, slot := range slice { | ||
| r, err := slot.HashTreeRoot() |
There was a problem hiding this comment.
Unlikely, but it is possible for a slot to be nil. Having a nil check may prevent a panic.
| r, err := slot.HashTreeRoot() | |
| if slot == nil { | |
| return [32]byte{}, fmt.Errorf("invalid PTC at position %d", i) | |
| } | |
| r, err := slot.HashTreeRoot() |
This suggestion probably needs gofmt
proto/prysm/v1alpha1/gloas.proto
Outdated
| // Vector[ValidatorIndex, PTC_SIZE] | ||
| message PTCs { | ||
| repeated uint64 validator_indices = 1 | ||
| [ (ethereum.eth.ext.ssz_size) = "ptc_committee_indices.size" ]; |
There was a problem hiding this comment.
Could you cast this to a ValidatorIndex? Then you could avoid casting/copying with methods like validatorIndicesFromUint64
| func validatorIndicesFromUint64(indices []uint64) []primitives.ValidatorIndex { | ||
| result := make([]primitives.ValidatorIndex, len(indices)) | ||
| for i, index := range indices { | ||
| result[i] = primitives.ValidatorIndex(index) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Commented on this in the proto file, but shouldn't you just make the PTCs message have ValidatorIndices be type []primitive.ValidatorIndex?
This reverts commit a46c44b.
potuz
left a comment
There was a problem hiding this comment.
On every single call to process a payload attestation message, we inconditionally call PayloadCommittee that will copy 2KB and allocate them on the heap. This happens in validatorIndicesFromUint64. I think we want to return the original slice as a read only object.
This is funny in processing the block since we call in a loop indexedPayloadAttestation that will get a committee that should be guaranteed to be the same on each round, although in this case the number of copies will be smaller (just 4 instead of 512).
This adds the PTC cache to the Gloas proto and native beacon state, updates SSZ/hash-tree-root handling, initializes the cache on Gloas upgrade, rotates it during epoch processing, and switches payload committee lookups to read from the cached window instead of recomputing PTC assignments on demand
Reference: ethereum/consensus-specs#4979