-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Revert "network: Detect early that NotificationOutSubstream
was closed by the remote"
#13409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next time we should ad some zombienet test for syncing to ensure we don't break it again :D
I wonder why present tests didn't detect it though 🤔 |
I don't think this change really broke syncing though. The problem is that the peer count is just reported incorrectly to be 40 because the local node previously didn't know the substream was closed BUT because block request handler in either end is not informed of connected/disconnected sync peers and keeps on serving block requests as long as any connection is open, syncing works. So in other words the syncing works because there's a bug in the block request handler. |
I think the same thing. See also my explanation here: #12434. The "bug" that I mention is the one that #13396 fixes. One node with this bug fixed will indeed have trouble syncing, just like smoldot has trouble syncing today. But if every node on the network had this bug fixed, then all nodes would (normally) sync fine. |
…sed by the remote (paritytech#13396)" (paritytech#13409) This reverts commit dfa654d.
…sed by the remote (paritytech#13396)" (paritytech#13409) This reverts commit dfa654d.
…sed by the remote (paritytech#13396)" (paritytech#13409) This reverts commit dfa654d.
…sed by the remote (paritytech#13396)" (paritytech#13409) This reverts commit dfa654d.
Reverts #13396
After the PR the reported peer count dropped to ~0 peers, and sync doesn't really work (best block reported stays the same). I was not able to find a quick fix for this, so let's revert the PR for now.