collator-protocol: Readvertise collations after peer disconnects #10464
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
eskimor
left a comment
eskimor
left a comment
There was a problem hiding this comment.
Looks correct. Can we have some test demonstrating the fixed race condition?
| // | ||
| // The `advertise_collation` ensures we are not readvertising the same collation | ||
| // multiple times. | ||
| if let Some(para_id) = state.collating_on { |
There was a problem hiding this comment.
This should be handled by
?Aka when the peer view is announced, it should be informed about the collations.
There was a problem hiding this comment.
I don't think this will work, we already set the collation as advertised in here. So it will not advertise again when peer reconnects.
Maybe we'd want to reset this bit to 0 when the validator disconnects if the status is not Requested. But, because it is a race, it might be that the validator has already seen the advertisement. We just don't know from the collator side. In that case, we'd have to check if the collator is punished in any way (for advertising twice).
There was a problem hiding this comment.
Also, I think this should be an exceptional case, likely a side-effect of some other underlying networking issue. Why was the validator disconnecting in the first place ?
There was a problem hiding this comment.
Indeed this is a race-case I've only encountered once.
It might happen due to network congestion or the following:
This debugging rabbit hole might improve the stability of litep2p even more 🙏 If the previous issue turns out to be correct, we are terminating the connections on fragmented socket reads due to a tiny offset mismatch in the poll_next implementation
There's a possible race case between peer connectivity and collation advertisement:
As a result of that, when the peer reconnects, the previous collation (C0) is not sent.
This happens when the collator has produced another collation (C1).
However, from the logs it looks like the collation C1 is advertising, but C0 is skipped.
This PR aims to resubmit collations on
PeerConectevents to mitigate these casesCloses #10463