Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add a protocol that answers finality proofs #5718

Merged
merged 5 commits into from
Apr 22, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Apr 21, 2020

cc #5670

This adds something that we forgot in #3520: finality proofs.

The code that this PR adds is mostly copy-pasted from block_requests.rs (which is known to be working) and was trivial to adjust.

@tomaka tomaka added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Apr 21, 2020
@tomaka tomaka requested a review from twittner April 21, 2020 12:13
@@ -45,6 +45,8 @@ pub struct Behaviour<B: BlockT, H: ExHashT> {
discovery: DiscoveryBehaviour,
/// Block request handling.
block_requests: protocol::BlockRequests<B>,
/// Finality proof request handling.
finality_proof_requests: protocol::FinalityProofRequests<B>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use Toggle<protocol::FinalityProofRequests<B>> here instead of storing an Option<Arc<dyn FinalityProofProvider<B>>> in FinalityProofRequests and erroring on incoming requests if it is a None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for keeping the current behaviour because I didn't know when the Option can be None. But I think it is None for light clients (which can't serve finality proofs) and Some for full nodes.

I'll do the change.

@tomaka tomaka added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 22, 2020
@tomaka tomaka merged commit 999f483 into paritytech:master Apr 22, 2020
@tomaka tomaka deleted the answer-finality-proofs branch April 22, 2020 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants