adding nil checks on attestation interface #14638
Conversation
proto/prysm/v1alpha1/attestation.go
Outdated
| return errors.New("attestation aggregation bits is empty") | ||
| } | ||
|
|
||
| if len(a.GetSignature()) == 0 { |
There was a problem hiding this comment.
I wonder if we need to check this? The main risk is that code that prepares an attestation for validator signing could run into an error if it uses IsNil somewhere for runtime safety reasons. I feel like these IsNil implementations blur the line between runtime safety and value validation when we start checking for non-zero signature and aggregation bits.
There was a problem hiding this comment.
it might not be needed, this check was part of the existing REST implementation check, perhaps it's not needed
There was a problem hiding this comment.
maybe i just need to check outside of this
proto/prysm/v1alpha1/attestation.go
Outdated
| // IsNil -- | ||
| func (a *AggregateAttestationAndProof) IsNil() error { | ||
| if a == nil { | ||
| return errors.New("aggregateAttestationAndProof is nil") |
There was a problem hiding this comment.
I don't think we should lowercase the name of exported types in error messages.
There was a problem hiding this comment.
you can always say nil AggregateAttestationAndProof value if you're trying to follow lowercase error style guide rules.
proto/prysm/v1alpha1/attestation.go
Outdated
| if a.GetData() == nil { | ||
| return errors.New("attestation data is nil") | ||
| } | ||
| if a.GetData().Source == nil || a.GetData().Target == nil { |
There was a problem hiding this comment.
will remove this from here as it breaks a lot of existing unit tests, will keep it in the validate functions
| @@ -185,12 +185,13 @@ func IsValidAttestationIndices(ctx context.Context, indexedAttestation ethpb.Ind | |||
| _, span := trace.StartSpan(ctx, "attestationutil.IsValidAttestationIndices") | |||
| defer span.End() | |||
|
|
|||
| if indexedAttestation == nil || | |||
| indexedAttestation.GetData() == nil || | |||
| indexedAttestation.GetData().Target == nil || | |||
There was a problem hiding this comment.
why does this only check target and not source?
| indexedAttestation.GetData().Target == nil || | ||
| indexedAttestation.GetAttestingIndices() == nil { | ||
| if indexedAttestation == nil || indexedAttestation.IsNil() || | ||
| indexedAttestation.GetData().Target == nil || indexedAttestation.GetData().Source == nil { |
There was a problem hiding this comment.
I added a Source nil check here, is that ok?
What type of PR is this?
Other
What does this PR do? Why is it needed?
nil checks are not working correctly for some checks as they are interfaces, tested in https://go.dev/play/p/T-wHFxweyxX
instead we should have IsNil functions to check the underlying type for nil like https://go.dev/play/p/YPJdA_LMuQz
interfaces in Go: An interface is nil only when its type and value are both nil.
Typed nil in Interfaces: An interface holding a typed nil is not nil.
IsNil() is needed because we cannot rely on a simple nil check on the interface and must check if the underlying value is nil.
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements