search

paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Revert splitting `Peerset` into `PeerStore` & `ProtocolController` #14498

Closed dmitry-markin closed 1 year ago

dmitry-markin commented 1 year ago

This PR is intended to remove the possible cause of the peer count instabilities before the release. There are "more proper" fixes on the way, but they haven't been thoroughly tested, so just reverting seems to be the best option.

altonen commented 1 year ago

I'm not sure if we want merge this after all. The peer count during my GCP tests is flaky and it looks like this gives marginal improvement. Based on these findings I bisected the history and at least for me, for some bizarre reason, the parity-db update causes instability in the peer count. Here are peer counts before and after the update.

before after

I don't understand why peer stability would have anything to do with parity-db

dmitry-markin commented 1 year ago

I have converted the PR into draft so somebody accidentally do not merge it.

altonen commented 1 year ago

The parity-db change has definitely changed something but it's not used anywhere in sc-network. The number of OpenDesiredByRemote(SetId(0)) has halved between the parity-db versions. The only thing I can come up with is that the substream spam is caused by some local async issue and that is alleviated by the parity-db change, this PR and disabling inbound connections for light clients. Considering that we want this spam to decrease and that the logs don't seem to indicate any other concerns, let's not merge this PR and instead focus on shipping QUIC + hole punching sooner.