# Prysm nits
### Shouldn't the aggregate be 2 here?
```go
message AggregateAttestationAndProofElectra {
// The aggregator index that submitted this aggregated attestation and proof.
uint64 aggregator_index = 1 [(ethereum.eth.ext.cast_type) = "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives.ValidatorIndex"];
// The aggregated attestation that was submitted.
AttestationElectra aggregate = 3;
// 96 byte selection proof signed by the aggregator, which is the signature of the slot to aggregate.
bytes selection_proof = 2 [(ethereum.eth.ext.ssz_size) = "96"];
}
```
See: https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/validator.md#aggregateandproof
### Should be "an"
```go
// Representing a electra block.
SignedBeaconBlockElectra electra_block = 6;
```
### Incorrect attester slashing length checks
```go
err = slice.VerifyMaxLength(s.Attestation1.AttestingIndices, 2048)
if err != nil {
return nil, server.NewDecodeError(err, fmt.Sprintf("[%d].Attestation1.AttestingIndices", i))
}
```
See these two spots:
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L1187-L1190
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L1207-L1210
Yes, this used to be [2048](
https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#attestation
) but now it's [131072](https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/beacon-chain.md#attestation).
Also, max attester slashings per block is 1 now.
https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/beacon-chain.md#max-operations-per-block
So this will need to be updated too:
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L1166
### Should IndexedAttestation::ToConsensus() check lengths?
Should these ensure the number of attesting indices is valid?
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L710
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L735
### These should use named constants instead of number values.
* `fieldparams.BLSPubkeyLength`
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L246
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L333
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions.go#L246
### Missing length checks for execution request fields
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions_block.go#L2705-L2727
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/conversions_block.go#L3005-L3027
### Should delete ExecutionPayload*Electra aliases
I think we should delete these. To me they imply there are changes since Deneb.
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/api/server/structs/block.go#L570-L571
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/execution.go#L787
### Should say "electra fork"
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/proto/prysm/v1alpha1/beacon_state.proto#L440
### Missing a space between colon & string:
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/proto/ssz_proto_library.bzl#L29
Applies to minimal & mainnet.
### Condition should be in switch block
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/getters.go#L155-L189
Looks like we did this because there isn't a new execution payload, so we cannot tell between deneb/electra. We should just bite the bullet & fix this.
### The expected types in these error messages should be Electra types
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/factory.go#L491-L494
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/factory.go#L504
Similarily, this error message is a little confusing. I would recommend doing what we normally do: "blah has wrong type (expected %T, got %T)"
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/factory.go#L477
### `BeaconBlockContainerToSignedBeaconBlock` is missing Electra blocks
* https://github.com/prysmaticlabs/prysm/blob/705a9e8dcd8939185a2ef58b5ab58ec2ac08fb80/consensus-types/blocks/factory.go#L548-L553
### DataColumnSidecarSubnetCount/MaxRequestDataColumnSidecars is not part of Electra
Also, typo in comment: "// Values introduce in Electra upgrade". Should be "introduced".
Move this to the PeerDAS/Fulu section.
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/config/params/config.go#L245
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/config/params/config.go#L248
### Redundant parans
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/beacon-chain/core/helpers/validator_churn.go#L25
### This should be plural
`PendingDepositLimit` should be `PendingDepositsLimit`. And this doesn't appear to be used anywhere. There's a `PendingDepositsLimit` defined elsewhere :thinking_face:
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/config/params/config.go#L252
### Technically, this should be "==" not ">="
It's effectively the same, but to follow the spec use "==". These are processed sequentially.
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/beacon-chain/core/electra/consolidations.go#L209
### Request should be plural here
To match the style of deposit requests...
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/proto/engine/v1/electra.go#L76
* https://github.com/prysmaticlabs/prysm/blob/c48d40907c2652c99705a8148542d0904e135ac8/proto/engine/v1/electra.go#L86
### The docstring for `SetBitAt` is misleading
It says it returns a boolean when it doesn't.
### `CommitteeIndices` is correct but confusing
At first thought this was return all indices (not just the set ones). A comment that `BitIndices` only returns indices for bits that are 1 would be appreciated. Even better, rename the function to `BitIndicesForSetBits` or something.
### `get_validator_max_effective_balance` is now `get_max_effective_balance`
This function was renamed a while back.
### "are" should be "is" in comment
### When an error that shouldn't never happen occurs, it should return, not continue
These should never occur either, right?
* https://github.com/prysmaticlabs/prysm/blob/39cf2c8f0664c7f4c14aeb87816f5388eb500627/beacon-chain/core/electra/consolidations.go#L264-L286
On line 267 and 275 and 285.
Sidenote: adding those epochs could be prettier, see:
This would match behavior elsewhere in the function, eg:
### `PendingDeposit.PublicKey` should be `Pubkey` now
The the ethpb structure is out-of-date, the non-ethpb structure is correct.
### In PendingPartialWithdrawal, we renamed "index" to "validator_index"
### AppendInactivityScore should be last
Doesn't really matter, but to match the spec...
### In `ComputeProposerIndex` electra code `randomByte` should be plural
This should be `randomBytes` now.
Also... you can/should cache the hash. The spec doesn't do this, but clients should. This will prevent many unnecessary hashes.
### This code is from phase0, not deneb
* https://github.com/prysmaticlabs/prysm/blob/e577bb0dcf8b51c7ba6a48551eff61d0fb31a22a/beacon-chain/core/validators/validator.go#L78-L101
See: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#initiate_validator_exit