New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rm forkchoice attestation verification #8301
Conversation
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if indexedAtt.AttestingIndices == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was moved to IsValidAttestationIndices
@@ -39,7 +39,6 @@ var ErrTargetRootNotInDB = errors.New("target root does not exist in db") | |||
// | |||
// # Update latest messages for attesting indices | |||
// update_latest_messages(store, indexed_attestation.attesting_indices, attestation) | |||
// TODO(#6072): This code path is highly untested. Requires comprehensive tests and simpler refactoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue has been closed a while ago
Codecov Report
@@ Coverage Diff @@
## develop #8301 +/- ##
==========================================
Coverage ? 57.09%
==========================================
Files ? 449
Lines ? 31522
Branches ? 0
==========================================
Hits ? 17996
Misses ? 10726
Partials ? 2800 |
return nil, errors.New("nil attesting indices") | ||
} | ||
// Note that signature verification is ignored here because it was performed in sync's validation pipeline: | ||
// validate_aggregate_proof.go and validate_beacon_attestation.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this is validated there indeed
What type of PR is this?
What does this PR do? Why is it needed?
Remove forkchoice attestation signature verification as we should assume trusted input for the pipelines in fork choice. I opt in the cheaper verification for attesting indices. See more commentary in diff
More rationale: https://hackmd.io/LZONsUmKSReJTSUYQR88Rw?view
Which issues(s) does this PR fix?
N/A
Other notes for review
N/A