Remove `NetworkSpecialization` #4665

rphmeier · 2020-01-17T16:51:08Z

This PR removes the NetworkSpecialization from the network service.
NetworkSpecialization was an early attempt at customizing the node's network behavior for Polkadot, but there are now better APIs for doing so (register_notifications_protocol).

Historical notes on backwards compatibility

This PR does not introduce network backwards-incompatibility. Potential network backwards incompatibility has been addressed by this series of issues:

Polkadot: begin to ignore the chain_status parameter: Allow empty NetworkSpecialization handshake polkadot#779
Substrate: begin to ignore the chain_status in the handshake when decoding: deprecate chain_status field of network handshake #4675
A few releases down the road (once Polkadot no longer depends on it), remove the chain_status member from the handshake: Remove support of network protocols before 6 #4674

Breaking API Changes:
The service API is no longer the same. The construct_simple_protocol macro with_network_protocol function have been removed. Invocations of the macro and function should be deleted without loss of functionality (except when NetworkSpecialization was non-empty - then you need to port to the register_notifications_protocol).

tomaka

Changes look good to me!
Huge +1 for the removal of that creature

rphmeier · 2020-02-06T10:05:56Z

Once paritytech/polkadot#788 is in we can start to look at merging this. It still needs a follow-up to enable the new network protocol.

mxinden

For what my review is worth, this looks good to me. Just a small question related to tests:

mxinden · 2020-02-10T14:15:01Z

client/network/src/protocol/legacy_proto/tests.rs

 		for _ in 0..200 { // Note: don't make that number too high or the CPU usage will explode.
-			let msg = (0..10).map(|_| rand::random::<u8>()).collect::<Vec<_>>();
-			to_send.push(Message::<Block>::ChainSpecific(msg));
+			let req_id = loop {


Just to make sure I understand this correctly, you have witnessed a u64 collision repeatedly? Even with the birthday attack in mind, that is rather unlikely, no? Am I missing something?

https://en.wikipedia.org/wiki/Birthday_attack#Mathematics

According to the diagram, this test would fail with a probability of about 1/10^15. So yeah, probably overkill. TBH, I thought that RequestId was u32 (and I suppose the type may yet change), so it seems safer to protect against that. If you push for it I'll change it.

gnunicorn · 2020-02-18T14:17:33Z

paritytech/polkadot#788 has been merged into polkadot. Shall we try again to get this merged in substrate, too?

rphmeier · 2020-02-18T16:13:17Z

@gnunicorn We need a non-trivial follow-up or it will break Polkadot master. I'm hoping to do that this week. Regardless, we should merge before feature-freeze.

gnunicorn · 2020-02-18T16:23:08Z

Thanks for the update, @rphmeier .

One idea behind the freeze is to give us some time to upgrade any dependents accordingly without having many unrelated (API) changes... fixing Polkadot after the freeze/during alpha+beta is totally reasonable.

Could someone merge latest master in?

tomaka · 2020-02-18T17:48:08Z

I don't think merging master is hard, but let me know if I should do it.

rphmeier · 2020-02-18T23:03:58Z

fixing Polkadot after the freeze/during alpha+beta is totally reasonable.

Our rule is usually to have a PR for Polkadot waiting. My TODO list is fairly long before I can get to that. We will probably have to make an exception here, because I'd like to remove NetworkSpecialization for 2.0.

@tomaka

I don't think merging master is hard, but let me know if I should do it.

Feel free! Thanks

tomaka · 2020-02-19T10:53:03Z

Merged master.

gnunicorn · 2020-02-19T12:05:49Z

@rphmeier anything else to do here before we can merge into substrate?

Re Polkadot: I think we can make an exception for the freeze.

gnunicorn · 2020-02-19T15:54:28Z

needs another merge with master...

rphmeier · 2020-02-19T16:29:43Z

OK, shall we merge directly before the freeze? I'd like to minimize breakage in Polkadot as much as possible.

andresilva · 2020-02-19T20:25:35Z

@rphmeier We can also exclude this PR when backporting to polkadot-master until we have support for this in polkadot.

gnunicorn · 2020-02-20T12:27:08Z

icing until the freeze (pun intended)

gnunicorn · 2020-02-21T11:12:52Z

could someone with enough expertise update to master, resolved the conflicts and merge this into master?

//cc @rphmeier, @tomaka

tomaka · 2020-02-21T11:46:55Z

Master-merging round 3 done. Not totally sure that CI passes.

tomaka · 2020-02-21T12:51:28Z

The PR's ready to be merged. Since I haven't really followed the story regarding the Substrate freeze and Polkadot, I will let someone else do it.

rphmeier · 2020-02-21T18:14:55Z

I have a PR in the works which adjusts Polkadot to use this.

This reverts commit 3cc0973.

* remove networkspecialization * Fix most of the fallout * get all tests compiling Co-authored-by: Pierre Krieger <[email protected]>

rphmeier added 2 commits January 17, 2020 17:18

remove networkspecialization

3e35a7a

Fix most of the fallout

d40d7d1

rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. B8-breakseverything labels Jan 17, 2020

rphmeier requested a review from tomaka January 17, 2020 16:51

get all tests compiling

95c0bef

rphmeier mentioned this pull request Jan 19, 2020

deprecate chain_status field of network handshake #4675

Merged

tomaka mentioned this pull request Jan 20, 2020

Move DummySpecialization to sc-network #4680

Merged

rphmeier removed the B8-breakseverything label Feb 5, 2020

rphmeier marked this pull request as ready for review February 5, 2020 16:49

rphmeier requested review from Demi-Marie, andresilva and mxinden as code owners February 5, 2020 16:49

rphmeier added 2 commits February 5, 2020 17:50

Merge branch 'master' into rh-remove-specialization

61d326e

Merge branch 'master' into rh-remove-specialization

4f8d181

rphmeier added A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. labels Feb 5, 2020

tomaka approved these changes Feb 6, 2020

View reviewed changes

mxinden reviewed Feb 10, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into HEAD

95e610c

tomaka mentioned this pull request Feb 19, 2020

Cleanup the exports of sc-network #4983

Merged

rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 19, 2020

Merge remote-tracking branch 'upstream/master' into HEAD

7dfef12

andresilva approved these changes Feb 19, 2020

View reviewed changes

gnunicorn added the A1-onice label Feb 20, 2020

gnunicorn added this to the 2.0 milestone Feb 21, 2020

gnunicorn added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A1-onice labels Feb 21, 2020

Merge remote-tracking branch 'upstream/master' into HEAD

38fdd5b

gnunicorn added A4-awaitingci and removed A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 21, 2020

gnunicorn merged commit 3cc0973 into master Feb 21, 2020

gnunicorn deleted the rh-remove-specialization branch February 21, 2020 13:02

gnunicorn added B1-clientnoteworthy and removed A4-awaitingci labels Feb 21, 2020

bkchr added a commit that referenced this pull request Feb 27, 2020

Revert "Remove NetworkSpecialization (#4665)"

6a24229

This reverts commit 3cc0973.

General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 4, 2020

Remove NetworkSpecialization (paritytech#4665)

39ff1bc

* remove networkspecialization * Fix most of the fallout * get all tests compiling Co-authored-by: Pierre Krieger <[email protected]>

Remove NetworkSpecialization #4665

Remove NetworkSpecialization #4665

Uh oh!

Conversation

rphmeier commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Historical notes on backwards compatibility

Uh oh!

tomaka left a comment

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Feb 6, 2020

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

rphmeier Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Feb 18, 2020

Uh oh!

rphmeier commented Feb 18, 2020

Uh oh!

gnunicorn commented Feb 18, 2020

Uh oh!

tomaka commented Feb 18, 2020

Uh oh!

rphmeier commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaka commented Feb 19, 2020

Uh oh!

gnunicorn commented Feb 19, 2020

Uh oh!

gnunicorn commented Feb 19, 2020

Uh oh!

rphmeier commented Feb 19, 2020

Uh oh!

andresilva commented Feb 19, 2020

Uh oh!

gnunicorn commented Feb 20, 2020

Uh oh!

gnunicorn commented Feb 21, 2020

Uh oh!

tomaka commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaka commented Feb 21, 2020

Uh oh!

rphmeier commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Remove `NetworkSpecialization` #4665

Remove `NetworkSpecialization` #4665

rphmeier commented Jan 17, 2020 •

edited

Loading

rphmeier commented Feb 18, 2020 •

edited

Loading

tomaka commented Feb 21, 2020 •

edited

Loading