availability-recovery: use IfDisconnected::TryConnect for chunks

[ordian]

I was looking at how https://github.com/paritytech/cumulus/blob/master/client/pov-recovery works and it seems to me that it doesn't unless I'm overlooking something obvious.

Here we issue a recovery request without backing fast-path, so it will try to request chunks from all validators.

But you need to be connected over /validation/1 peerset, which has limited number of slots (up to 10) for non-validators. And these connection requests are only issued in gossip-support only for validators if IfDisconnected::ImmediateError is used. to at least 1/3 of validators for the recovery to work.

I've changed that to IfDisconnected::TryConnect (only for chunk requests).

Related paritytech/cumulus#1423.

cumulus companion: paritytech/cumulus#1711

• fix tests
• ensure companion works

[bkchr]

Hmm, given the docs and your explanation it sounds sensible. So, this only works in our tests because all nodes are already connected?

We have a zombienet test, could you please make one validator being only connected to one other validator using --reserved-nodes? Then this test should fail and start working with your test.

[ordian]

We have a zombienet test, could you please make one validator being only connected to one other validator using --reserved-nodes? Then this test should fail and start working with your test.

I assume you mean this test. Interesting. Last time I talked to @eskimor about req/resp protocols, our understanding was that it needs a connections specifically over /validation/1 peerset, not just --reserved-nodes. Looks like that assumption is not correct. But even if not, we'd still need that change in polkadot to make it work for collators.

[bkchr]

I mean the docs are talking about peer:

/// When sending a request, what to do on a disconnected recipient.
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum IfDisconnected {
	/// Try to connect to the peer.
	TryConnect,
	/// Just fail if the destination is not yet connected.
	ImmediateError,
}

What I meant with my changes is that currently on the zombienet test we are probably connected to all peers. So, when we do this request on the validation peerset, we actually already have a connection to the peer, but not yet on the validation peerset.

[bkchr]

But even if not, we'd still need that change in polkadot to make it work for collators.

Which is fine and I'm not against the fix ;) I just want to have a regression test that this behavior not is coming back by accident.

[ordian]

But even if not, we'd still need that change in polkadot to make it work for collators.

Which is fine and I'm not against the fix ;) I just want to have a regression test that this behavior not is coming back by accident.

Yes, I'll tweak the test to see if it work with 1 peer connection.

[eskimor]

No the peerset does not matter -as long as some connection exists, req/resp. works.

[ordian]

bot merge

[paritytech-processbot]

Error: "Check reviews" status is not passing for paritytech/cumulus#1711

[ordian]

bot merge