- 
                Notifications
    You must be signed in to change notification settings 
- Fork 931
feat!: Optional execution proofs #8316
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
Conversation
| if O::observe() { | ||
| observe_gossip_execution_proof(&execution_proof, chain)?; | ||
| } | ||
| return Err(GossipExecutionProofError::PriorKnownUnpublished); | 
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.
Gossip has a de-duplication mechanism based on the message content. Is there a single possible ExecutionProof per subnet and slot that is valid? If yes, the observe cache feels unnecessary.
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.
Yep -- there is only one subnet now but there are multiple proofs for the slot. So there is a single possible ExecutionProof for a (proof_id, slot_number, beacon_block_root) combination
| peer_id: PeerId, | ||
| execution_proof: Option<Arc<ExecutionProof>>, | ||
| seen_timestamp: Duration, | ||
| }, | 
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.
I would not touch sync for now, get the core logic in first
| closing, spoke with team :) | 
Issue Addressed
This is a parallel PR to #7755 that includes more of the logic needed to for example sync.
Proposed Changes
Please list or describe the changes introduced by this PR.
Additional Info
Please provide any additional information. For example, future considerations
or information useful for reviewers.