Detect attestation with source or target lower then min as slahsable #7902
Conversation
…into protection-refactor
…into minimal_block_signing # Conflicts: # validator/db/iface/interface.go # validator/db/kv/manage_test.go # validator/db/kv/proposal_history_v2.go # validator/db/kv/proposal_history_v2_test.go # validator/slashing-protection/local/standard-protection-format/import_test.go
shayzluf
requested review from
rauljordan,
prestonvanloon and
terencechain
and removed request for
a team
shayzluf
changed the base branch from
protection-refactor
to
minimal_block_signing
| @@ -35,6 +37,21 @@ func (store *Store) AttestationHistoryForPubKeys( | |||
| } else { | |||
| attestationHistory = enc | |||
| } | |||
| var minAtt attHist.MinAttestation | |||
| var setMin bool | |||
| enc = bucket.Get(attHist.GetMinSourceKey(key)) | |||
There was a problem hiding this comment.
Naming here is confusing, on one hand we have bucket keys and also public keys, let's refactor this better
| minAtt.Source = bytesutil.BytesToUint64BigEndian(enc) | ||
| setMin = true | ||
| } | ||
| enc = bucket.Get(attHist.GetMinTargetKey(key)) |
There was a problem hiding this comment.
Does this mean that our attestinghistory package knows about boltdb keys? Is there a way we can better separate those concerns so they are independent?
| @@ -44,16 +61,18 @@ func (store *Store) AttestationHistoryForPubKeys( | |||
| copy(ehd, ah) | |||
| attestationHistoryForVals[pk] = ehd | |||
| } | |||
| return attestationHistoryForVals, err | |||
|
|
|||
| return attestationHistoryForVals, minAttForVal, err | |||
There was a problem hiding this comment.
Agree with these changes, we just need to make sure to amend the godoc for the db function to clarify what it returns
| ctx, span := trace.StartSpan(ctx, "Validator.AttestationHistoryForPubKey") | ||
| defer span.End() | ||
|
|
||
| var err error | ||
| var attestingHistory attHist.History | ||
| minAtt := attHist.MinAttestation{} |
There was a problem hiding this comment.
What if they are empty? Should we consider it as 0 or as far future epoch?
| }) | ||
| return err | ||
| } | ||
|
|
||
| // SaveMinAttestation saves min attestation values if they are lower from the ones that are currently set in db. | ||
| func (store *Store) SaveMinAttestation(ctx context.Context, pubKey [48]byte, minAtt attHist.MinAttestation) error { |
There was a problem hiding this comment.
The name MinAttestation isn't very informative. We need to rename this to something more explicit that is clear regardless of someone's background in slashing protection. I propose something like EarliestSignedAttestationForPubKey and make public functions follow that convention. EarliestSignedAttestation is more readable
|
|
||
| } | ||
|
|
||
| func (store *Store) MinAttestation(ctx context.Context, pubKey [48]byte) (*attHist.MinAttestation, error) { |
| @@ -90,6 +91,21 @@ func differenceOutsideWeakSubjectivityBounds(latestEpochWritten, targetEpoch uin | |||
| return latestEpochWritten >= wsPeriod && targetEpoch <= latestEpochWritten-wsPeriod | |||
| } | |||
|
|
|||
| func isLowerThenMin( | |||
There was a problem hiding this comment.
| func isLowerThenMin( | |
| func isLowerThanMin( |
| targetEpoch uint64, | ||
| ) bool { | ||
| if minAtt == nil { | ||
| return false |
There was a problem hiding this comment.
Perhaps add a comment where why false. To me, I think it's false because if you don't have a min attestation, that probably means the validator has never attested. Is that right?
| return false | ||
| } | ||
| // Check if source of target epoch of the attestation is lower then the minimum. | ||
| if sourceEpoch < minAtt.Source || targetEpoch < minAtt.Target { |
There was a problem hiding this comment.
What if it's equal to min source and target?
| @@ -19,8 +19,18 @@ const ( | |||
| signingRootSize = 32 | |||
| historySize = targetSize + sourceSize + signingRootSize | |||
| minimalSize = latestEpochWrittenSize | |||
| // Key prefix to minimal attestation source epoch in attestation bucket. | |||
| minimalAttestationSourceEpochKeyPrefix = "minimal-attestation-source-epoch" | |||
There was a problem hiding this comment.
We are mixing up bolt logic with this package that knows nothing about bolt. Perhaps move to bolt itself or refactor into something more general
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
rauljordan
changed the base branch from
develop
to
feature/slashing-interchange
|
Closing in favor of #7966 |
Builds off #7868
Feature
What does this PR do? Why is it needed?
To conform with minimal format of the data interchange format of slashing protection we have to apply the minimal source and target rules to the isslashable attestation method and to store those values during json import and by using db functions
Which issues(s) does this PR fix?
Part of #7813
Other notes for review