This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Make transactions and block announces use notifications substreams#5360
Merged
gnunicorn merged 5 commits intoparitytech:masterfrom Mar 30, 2020
Merged
Make transactions and block announces use notifications substreams#5360gnunicorn merged 5 commits intoparitytech:masterfrom
gnunicorn merged 5 commits intoparitytech:masterfrom
Conversation
tomaka
added
A0-please_review
tomaka
commented
Mar 23, 2020
| for handler in &mut self.out_handlers { | ||
| if handler.protocol_name() != &protocol_name[..] { | ||
| break; | ||
| continue; |
Contributor
Author
There was a problem hiding this comment.
This is a bugfix. This loop tries all the protocols that have been registered. If it can't find any, it uses the legacy substream itself. Because of this break, we were using the legacy substream for all notification protocols expect the first one.
twittner
approved these changes
Mar 25, 2020
Member
|
check-polkadot needs to pass first :-P |
gavofyork
added
A7-needspolkadotpr
and removed
A0-please_review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the "notifications protocol" system introduced in #4284 and #4909, which are already used for GrandPa and Polkadot-specific protocols, to block announces and transactions.
This should normally be the last step to the transition to the new parachain-friendly networking protocol and the deeper integration with the libp2p protocol.
The next changes to
sc_networkshould normally only be clean-ups.Changes:
protocol.rsnow registers two notifications protocols:/<chainid>/block-announces/1and/<chainid>/transactions/1.ConsensusEngineIdto the handler to use as a fallback, we pass a full fallback message.protocol.rswe register, for each notifications protocol, how to interpret it. This is done with theFallbackenum. When we decode a block announce or list of transactions, we do the same thing as for the legacy protocol.Dangerosity of the change
This change is fully backwards-compatible, and is using the same mechanism for notifications as is used by GrandPa right now on the live network. #5201 suggests that there is a hidden bug somewhere, but it happens very rarely and I will spend the next days trying to find out what is happening.
I also tried on a local network (with
--dev), and everything works fine.Additionally, all the tests have passed on first try, which gives even more confidence.
Because of all this, I think the change should be quite smooth.