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

Conversation

@octol
Copy link
Contributor

@octol octol commented Mar 4, 2021

Follow up to #7640 , where we add support for also sending multiple Justifications across the network. We include in the request if we support receiving multiple at once, and are backwards compatible with old nodes.

Resolves #8172

@octol octol added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 4, 2021
@octol octol linked an issue Mar 4, 2021 that may be closed by this pull request
@octol octol marked this pull request as ready for review March 17, 2021 23:34
@github-actions github-actions bot added A0-please_review Pull request needs code review. A7-needspolkadotpr and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 17, 2021
@octol octol requested a review from andresilva March 24, 2021 07:06
@octol octol force-pushed the jon/send-multiple-justifications branch from ba2a87a to da3d708 Compare March 29, 2021 10:19
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Overall the changes LGTM but I'd like to make sure this is adequately tested to avoid any breakage.

@andresilva
Copy link
Contributor

What's the difference between #7556 and #8172? Isn't #8172 a subset of #7556?

@octol
Copy link
Contributor Author

octol commented Apr 16, 2021

What's the difference between #7556 and #8172? Isn't #8172 a subset of #7556?

#8172 was supposed to track the pieces specific to the networking and also the remaining work needed. It's the one that the code refers to in client/network when the adding the GRANDPA consensus engine ID tag

@octol
Copy link
Contributor Author

octol commented Apr 23, 2021

Overall the changes LGTM but I'd like to make sure this is adequately tested to avoid any breakage.

Making a push to properly test this so we can hopefully merge it soon

@octol octol requested review from andresilva and tomaka April 27, 2021 09:14
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Looks good

@andresilva
Copy link
Contributor

@octol Just to be sure before I click the merge button: when you tested this locally you tested both scenarios right? I.e. new nodes serving old nodes, and old nodes serving new nodes.

@octol
Copy link
Contributor Author

octol commented Apr 28, 2021

@octol Just to be sure before I click the merge button: when you tested this locally you tested both scenarios right? I.e. new nodes serving old nodes, and old nodes serving new nodes.

I tested having two nodes built using this PR finalizing blocks, and then a third and fourth node connecting with --no-grandpa, one based on master and one on this PR, confirming that they are both able to follow finality (at sync and when the set id changes).

And then I redid this with the same setup but the nodes finalizing blocks were built using master.

This should cover both scenarios I think

@andresilva
Copy link
Contributor

andresilva commented Apr 28, 2021

@octol Thanks!

@andresilva
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Apr 28, 2021

Trying merge.

@ghost ghost merged commit 6181e37 into master Apr 28, 2021
@ghost ghost deleted the jon/send-multiple-justifications branch April 28, 2021 10:59
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support sending multiple Justifications

4 participants