EIP-7549: p2p and sync #14085
EIP-7549: p2p and sync #14085
Conversation
| } | ||
|
|
||
| // Reset our attestation map. | ||
| AttestationMap = map[[4]byte]func() (ethpb.Att, error){ |
There was a problem hiding this comment.
doing this for all topics this way is pretty ugly, not strictly against your PR but if we are going to do this for multiple types, it might be better to have a cleaner way to set this up. Rather than manually transcribing it for each fork. Something to do as a follow up
| @@ -46,6 +46,12 @@ func TestInitializeDataMaps(t *testing.T) { | |||
| tt.action() | |||
| _, ok := BlockMap[bytesutil.ToBytes4(params.BeaconConfig().GenesisForkVersion)] | |||
| assert.Equal(t, tt.exists, ok) | |||
| _, ok = MetaDataMap[bytesutil.ToBytes4(params.BeaconConfig().GenesisForkVersion)] | |||
| assert.Equal(t, tt.exists, ok) | |||
| _, ok = AttestationMap[bytesutil.ToBytes4(params.BeaconConfig().GenesisForkVersion)] | |||
There was a problem hiding this comment.
Lets also test that they return the correct types
beacon-chain/sync/decode_pubsub.go
Outdated
| @@ -51,7 +57,19 @@ func (s *Service) decodePubsubMessage(msg *pubsub.Message) (ssz.Unmarshaler, err | |||
| } | |||
| // Handle different message types across forks. | |||
| if topic == p2p.BlockSubnetTopicFormat { | |||
beacon-chain/sync/decode_pubsub.go
Outdated
| } | ||
| } | ||
| if topic == p2p.AttestationSubnetTopicFormat { | ||
| m, err = extractDataType(types.AttestationMap, fDigest[:], s.cfg.clock) |
There was a problem hiding this comment.
instead of passing a map for each type, we could just pass the topic and the method does the rest.
There was a problem hiding this comment.
but then the function would no longer be generic and the same code would have to be duplicated for each map
There was a problem hiding this comment.
You infer the type from the topic
There was a problem hiding this comment.
ex: an attestation topic will always use the attestation map and never anything else. This switching is done in the extractDataType method itself
|
|
||
| func Test_attsAreEqual_committee(t *testing.T) { |
There was a problem hiding this comment.
Use a more proper test signature ?
| if !ok { | ||
| err := errors.New("attestation has wrong type") | ||
| tracing.AnnotateError(span, err) | ||
| return pubsub.ValidationReject, err |
There was a problem hiding this comment.
this should be an ignore, it would point there to being an error in our pipeline
| if !ok { | ||
| err := errors.New("attestation has wrong type") | ||
| tracing.AnnotateError(span, err) | ||
| return pubsub.ValidationReject, err |
| a, ok := att.(*eth.AttestationElectra) | ||
| // This will never fail in practice because we asserted the version | ||
| if !ok { | ||
| return pubsub.ValidationReject, fmt.Errorf("attestation has wrong type (expected %T, got %T)", ð.AttestationElectra{}, att) |
beacon-chain/sync/decode_pubsub.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dt, err := extractDataTypeFromTopic(topic, fDigest[:], s.cfg.clock) |
There was a problem hiding this comment.
should change its name to be more accurate as it does not apply for all types.
extractValidDataTypeFromTopic or something to that effect
| @@ -33,6 +35,8 @@ import ( | |||
| // - attestation.data.slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (attestation.data.slot + ATTESTATION_PROPAGATION_SLOT_RANGE >= current_slot >= attestation.data.slot). | |||
| // - The signature of attestation is valid. | |||
| func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, pid peer.ID, msg *pubsub.Message) (pubsub.ValidationResult, error) { | |||
| var validationRes pubsub.ValidationResult | |||
There was a problem hiding this comment.
move validationRes lower to where its used
| a, ok := att.(*eth.AttestationElectra) | ||
| // This will never fail in practice because we asserted the version | ||
| if !ok { | ||
| return pubsub.ValidationReject, fmt.Errorf("attestation has wrong type (expected %T, got %T)", ð.AttestationElectra{}, att) |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
extractDataTypefunction instead of a separate function for each object type (blocks, metadata, attestations etc)