Implement EIP-7549 #5512
Implement EIP-7549 #5512
Conversation
eserilev
added
electra
beacon_node/http_api/tests/tests.rs
Outdated
| @@ -3271,6 +3271,7 @@ impl ApiTester { | |||
|
|
|||
| let mut attestation = Attestation { | |||
| aggregation_bits: BitList::with_capacity(duty.committee_length as usize).unwrap(), | |||
| index: <_>::default(), | |||
There was a problem hiding this comment.
Setting it to the value 0 feels more explicit than using default. use .into() or the type new()
| .map_err(|e| BeaconChainError::from(e).into()) | ||
| } | ||
| Attestation::Electra(att) => { | ||
| // TODO(eip7594) need to get access to the beacon state |
There was a problem hiding this comment.
Why access to the full state? You just need the entire shuffling
There was a problem hiding this comment.
I chatted about this with Eitan on Tuesday and recommended plumbing through the committee cache with the full set of committees. Just requires a bit of tweaking of map_committee_cache
consensus/state_processing/src/per_block_processing/process_operations.rs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| pub mod electra { |
There was a problem hiding this comment.
Not necessary to duplicate all this logic. The only difference is when converting an on-chain attestation to an indexed attestation which can be common in all forks. Just convert attestations from any fork into the electra indexed attestation. The relevant attestation data for reward processing is the same too.
consensus/types/src/beacon_block.rs
Outdated
| // TODO(eip7549) is setting to default ok here? | ||
| attestations: <_>::default(), |
There was a problem hiding this comment.
No, that is not representative of the maximum size. You need to create a list with max count of electra attestations of maximum size
|
|
||
| #[superstruct(only(Electra), partial_getter(rename = "attesting_indices_electra"))] | ||
| #[serde(with = "quoted_variable_list_u64")] | ||
| pub attesting_indices: VariableList<u64, E::MaxValidatorsPerCommitteePerSlot>, |
There was a problem hiding this comment.
oooohhh that's annoying.. Hmm in practice this change does not affect much as it strictly increases the maximum length of the list. This is only relevant when merkleizing attester slashings. In LH internals we can have a common type for both or just use the bigger electra variant everywhere, and downsize to the base when doing block inclusions
There was a problem hiding this comment.
im now using the electra variant everywhere, though i still need to add logic to downsize & enfore the smaller list size for the base variant
eserilev
force-pushed
the
eip-7549
branch
4 times, most recently
from
020fe50 to
1bc3b79
Compare
| proposer_index, | ||
| }), | ||
| Attestation::Electra(att) => { | ||
| PendingAttestation::Electra(PendingAttestationElectra { |
There was a problem hiding this comment.
this should just an error
| #[serde(untagged)] | ||
| #[tree_hash(enum_behaviour = "transparent")] | ||
| #[ssz(enum_behaviour = "transparent")] | ||
| #[serde(bound = "E: EthSpec", deny_unknown_fields)] | ||
| #[arbitrary(bound = "E: EthSpec")] | ||
| pub struct PendingAttestation<E: EthSpec> { |
There was a problem hiding this comment.
this doesn't need be superstructed (it is phase0 only)
Issue Addressed
#5130