validator_discovery: pass PeerSet to the request #2372

ordian · 2021-02-03T13:57:42Z

ordian · 2021-02-03T14:14:56Z

We also handle on_peer_connected, on_peer_disconnected in validator discovery service. Not sure if we should take peer_set into account there as well 🤔

tomaka · 2021-02-03T14:24:56Z

Definitely!
find_connected_validators should also get a PeerSet parameter, and connected_peers should be a HashMap<(PeerId, PeerSet), ...>.

From the point of view of Polkadot, there' should never be a peer that is "connected" or "disconnected". It's always "has or has not a validation sustream" and "has or has not a collation substream".

eskimor

LGTM

rphmeier · 2021-02-03T15:38:11Z

guide changes?

rphmeier

Needs a guide change for ConnectToValidators

ordian · 2021-02-03T15:51:17Z

What changes exactly?

I'm currently investigating the adder-collator test failure.

rphmeier · 2021-02-03T15:58:37Z

Oh, I missed the change to the message definition earlier. Nvm

ordian · 2021-02-03T18:07:02Z

I'm currently investigating the adder-collator test failure.

Still investigating, b94a870 doesn't fix the issue unfortunately.

* master: Diagnostics quality of life improvements (#2375) Generic request/response infrastructure for Polkadot (#2352) Fix bug in statement table (#2369)

* master: Unused variable can be ignored (#2381) End multiplexer stream once one of its inputs end. (#2380) Cargo build step needs pre requisites not mentioned (#2379)

node/network/bridge/src/validator_discovery.rs

* master: node: pass local authorship info to the transaction pool (#2385) Explicit Para Lifecycle w/ Upgrades and Downgrades (#2354) Cancel Proxy Type (#2334) Cargo.lock: Update to libp2p-swarm v0.27.2 (#2384)

…ub.com:paritytech/polkadot into ao-pass-peerset-to-connecttovalidators-request

Why are you even reading this???

* master: bump spec versions in kusama, polkadot and westend (#2391)

ordian · 2021-02-05T17:46:05Z

This should be OK to merge now. Please re-review.

eskimor · 2021-02-05T18:44:41Z

node/network/pov-distribution/src/lib.rs

-					e.insert(vec![response_sender]);
-				}
+				connect_to_relevant_validators(&mut state.connection_requests, ctx, relay_parent, &descriptor).await;
+				e.insert(vec![response_sender]);


I think we should not insert the sender unconditionally. If there are no validators to connect to, the receiver will be pending forever.

I'm not sure about this. AFAIK a runtime request can fail spuriously while we can be already connected to the relevant peers.
And we will clean it up when the relay parent becomes irrelevant.

The previous check did not check whether we were able to connect to validators, but whether there are any relevant validators.

I think there should always be relevant validators in theory.
However, the caller should anyway handle the case that we issued the connection request but haven't received the PoV for a while. And it does this by issuing a separate job for it in the background that should not block progress on other jobs:

polkadot/node/core/backing/src/lib.rs

Line 568 in 57eb9d1

// spawn background task.

.

What I'm saying is yes, I've changed the behavior here, but it is equally "correct" as the previous one.

Just wanted to double check, the changed code sure looks nicer ;-)

node/subsystem-util/src/validator_discovery.rs

pepyakin · 2021-02-05T18:19:00Z

node/network/bridge/src/lib.rs

-					target: LOG_TARGET,
-					peer = ?peer,
-					"Peer sent us an invalid request",
-				);


Why do we want to remove this exactly?

This says "invalid request", whereas ReportPeer can be also used for increasing reputation, so the message is confusing.
In addition to that, we log it in on a higher level in subsystems.

node/network/bridge/src/lib.rs

node/network/bridge/src/validator_discovery.rs

node/network/pov-distribution/src/lib.rs

pepyakin · 2021-02-05T19:29:26Z

node/network/pov-distribution/src/lib.rs

+		// We only need one connection request per (relay_parent, para_id)
+		// so here we take this shortcut to avoid calling `connect_to_validators`
+		// more than once.
+		if !connection_requests.contains_request(&relay_parent) {


I am having trouble understanding, what would happen if this function were called twice with the same relay_parent and two different descriptors?

Good point. Although this code wasn't introduced in this PR, the comment says it should be unique per (relay_parent, para_id), but the only handles relay_parent.

cc @montekki #1990

If you check the code where this is called, you see that the the connection_requests are per pov_hash.

Wait, is it? state.connection_requests doesn't know about pov_hash and if we call it for two different pov_hashes with the same relay_parent, it will not issue the second request, or?

Ahh my comment was dumb 🤦

I had overseen that connection requests is global to the state.

The assumption was probably that we are only assigned to one parachain per relay parent?

Do I understand it right that this is a legit problem and requires an issue?

Do I understand it right that this is a legit problem and requires an issue?

Don't know, the following assumption seems legit to me:

The assumption was probably that we are only assigned to one parachain per relay parent?

Although, the less assumptions we make, the better. So if that assumption is easy to get rid of, I think that would be a good idea.

Depends on whether this assumption is wrong

The assumption was probably that we are only assigned to one parachain per relay parent?

If it is wrong, we'd probably need to change

polkadot/node/subsystem-util/src/validator_discovery.rs

Line 176 in 57eb9d1

id_map: HashMap<Hash, usize>,

to map from (Hash, ParaId) instead of Hash.

rphmeier

I don't quite follow why Distribute, at least with the current code, needs to connect to anybody.

Distribute places the PoV into a mapping for the relay-parent and then we advertise the PoV hashes to anyone who connects, after we get their view-update and see that they're on the same chains as us. Then they'll request the PoV.

I think this issue might be solved also by having nodes send their current view to fresh connections in the network-bridge.

That said, I think that the change is not a problem and is good to have. But it might be solving things for the wrong reason 🤔

* master: Send view to new peers (#2392)

ordian · 2021-02-06T13:36:01Z

I think this issue might be solved also by having nodes send their current view to fresh connections in the network-bridge.

I just tried it and the integration test still fails, there is no mention of /polkadot/validation/1 protocol in the sub-libp2p logs.

Distribute places the PoV into a mapping for the relay-parent and then we advertise the PoV hashes to anyone who connects, after we get their view-update and see that they're on the same chains as us. Then they'll request the PoV.

That's not how the pov-distribution currently works. It sends PoVs, not hashes on distribute:

polkadot/node/network/pov-distribution/src/lib.rs

Line 485 in 2d177ad

distribute_to_awaiting(

Are you're saying we should change this? Anyway, this is unrelated to the problem here.

But it might be solving things for the wrong reason thinking

I think the reason why we don't send PoVs w/o this fix, is that we need to explicitly open the /protocol/validation/1 substream, which is done in connect_to_validators request when we do here. @tomaka please correct me if I'm wrong.

tomaka · 2021-02-06T16:20:18Z

I just tried it and the integration test still fails, there is no mention of /polkadot/validation/1 protocol in the sub-libp2p logs.

In case that helps, validation is also referred to as SetId(4) in connections/disconnections messages. Collation is SetId(3).

I think the reason why we don't send PoVs w/o this fix, is that we need to explicitly open the /protocol/validation/1 substream, which is done in connect_to_validators request when we do here. @tomaka please correct me if I'm wrong.

The original bug is that the validation substream opens a few milliseconds before the collation substream. However, the bridge before this PR doesn't distinguish between "validation open" and "collation open". All it knows about and reports is "open".

During the few milliseconds between validation opening and collation opening, the collation code detects this "open" from the bridge (well, it's a bit more complicated, but I'm not familiar with the code and don't remember the details) and tries to send a Distribute on the collation substream, which fails because it isn't open yet.

After this original bug was fixed by properly distinguishing between collation and validation, the second bug is that we didn't open the validation substream with anyone anymore. The code before this PR, when it receives a connect_to_validators(collation) simply connects to everyone both with collation and validation. After fixing this, my understanding (again, might be wrong) was that we realized that there isn't actually any code that does connect_to_validations(validation) anywhere.

ordian · 2021-02-08T08:02:52Z

After this original bug was fixed by properly distinguishing between collation and validation, the second bug is that we didn't open the validation substream with anyone anymore. The code before this PR, when it receives a connect_to_validators(collation) simply connects to everyone both with collation and validation. After fixing this, my understanding (again, might be wrong) was that we realized that there isn't actually any code that does connect_to_validations(validation) anywhere.

Yes, it looks like previously, collator issued connect_to_validators(collation) and collate_to_validators(validation)` requests and validators were distributing the PoV to each other over the collator after receiving it from the collator.

I think maybe there was an implicit expectation that if the peers are "connected", they also have /polkadot/validation/1 substream open. But in reality we need to explicitly open this substream.

eskimor · 2021-02-08T10:34:50Z

I don't quite follow why Distribute, at least with the current code, needs to connect to anybody.

Distribute places the PoV into a mapping for the relay-parent and then we advertise the PoV hashes to anyone who connects, after we get their view-update and see that they're on the same chains as us. Then they'll request the PoV.

The problem is: Nobody connects. The problem as I see it, is that nobody issues a FetchPoV, because no validator except the ones connected to the collator actually know anything about the PoV. The ones that know about it, know it because they got it via the collation protocol, so they do not need to fetch it. It seems FetchPoV is at least initially the only code path where we actually connect to the validation protocol. Without being connected we also won't get any view updates, which would tell us who wants what.

I am not sure yet, where there would be a better place to make sure we are actually connected to relevant validators. Making sure we are connected in distribute, kinda makes sense. "Ok we have something to distribute, maybe we should make sure we are connected to someone, so they can tell us whether they want it."

Also now, he PoV distribution as a subsystem makes sure it is connected, which seems sensible.

ordian · 2021-02-08T10:41:36Z

I think what would be nice is issuing background connection requests once in a while once we know who the relevant peers are (we have multiple layers of caching of the requests, so issuing it twice should not be a problem). This also goes hand in hand with the dormant peers idea.

rphmeier · 2021-02-08T15:41:40Z

The problem as I see it, is that nobody issues a FetchPoV, because no validator except the ones connected to the collator actually know anything about the PoV.

Wouldn't this indicate a problem in statement distribution? Validators are meant to issue FetchPoV based on what they've observed in Seconded statements.

eskimor · 2021-02-08T16:48:56Z

Wouldn't this indicate a problem in statement distribution? Validators are meant to issue FetchPoV based on what they've observed in Seconded statements.

I think, I checked that, but ended nowhere. But now that you say it, it sounds awfully likely that something is wrong there. Hence, issue. I am currently right in the middle of something (availability distribution), but can have a look at #2400 tomorrow.

* master: Implement Approval Voting Subsystem (#2112) Introduce PerPeerSet utility that allows to segrate based on PeerSet (#2420) [CI] Move check_labels to github actions (#2415) runtime: set equivocation report longevity (#2404) Companion for #7936: Migrate pallet-balances to pallet attribute macro (#2331) Corrected Physical (#2414) validator_discovery: cache by (Hash, ParaId) (#2402) Enable wasmtime caching for PVF (companion for #8057) (#2387) Use construct_runtime in tests, remove default PalletInfo impl (#2409) validator_discovery: pass PeerSet to the request (#2372) guide: more robust approval counting procedure (#2378) Publish rococo on every push to `rococo-v1` branch (#2388) Bump trie-db from 0.22.2 to 0.22.3 (#2344) Send view to new peers (#2392)

validator_discovery: pass PeerSet to the request

6c4de0d

ordian added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 3, 2021

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 3, 2021

ordian marked this pull request as ready for review February 3, 2021 14:07

github-actions bot 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 3, 2021

tomaka approved these changes Feb 3, 2021

View reviewed changes

eskimor approved these changes Feb 3, 2021

View reviewed changes

ordian added 3 commits February 3, 2021 15:42

validator_discovery: track PeerSet of connected peers

45f48c0

validator_discovery: fix tests

0d6a1d7

validator_discovery: fix long line

70edb52

rphmeier approved these changes Feb 3, 2021

View reviewed changes

some fixes

b94a870

ordian added 3 commits February 4, 2021 10:31

merge master

96a3b03

* master: Diagnostics quality of life improvements (#2375) Generic request/response infrastructure for Polkadot (#2352) Fix bug in statement table (#2369)

some validator_discovery logs

4f3abc9

merge master

b1268aa

* master: Unused variable can be ignored (#2381) End multiplexer stream once one of its inputs end. (#2380) Cargo build step needs pre requisites not mentioned (#2379)

eskimor reviewed Feb 5, 2021

View reviewed changes

node/network/bridge/src/validator_discovery.rs Outdated Show resolved Hide resolved

ordian and others added 6 commits February 5, 2021 11:33

log validator discovery request

92d18e1

merge master

50c87c6

* master: node: pass local authorship info to the transaction pool (#2385) Explicit Para Lifecycle w/ Upgrades and Downgrades (#2354) Cancel Proxy Type (#2334) Cargo.lock: Update to libp2p-swarm v0.27.2 (#2384)

Also connect to validators on DistributePoV.

75f7de6

Merge branch 'ao-pass-peerset-to-connecttovalidators-request' of gith…

b280f25

…ub.com:paritytech/polkadot into ao-pass-peerset-to-connecttovalidators-request

validator_discovery: store the whole state per peer_set

2c99ba9

bump spec versions in kusama, polkadot and westend

2496006

paritytech-ci added the D2-breaksapi label Feb 5, 2021

eskimor and others added 7 commits February 5, 2021 16:26

Correcting doc.

48d1331

Merge branch 'ao-pass-peerset-to-connecttovalidators-request' of gith…

51698b9

…ub.com:paritytech/polkadot into ao-pass-peerset-to-connecttovalidators-request

validator_discovery: bump channel capacity

6436086

pov-distribution: some cleanup

f91823c

this should fix the test, but it does not

8e9a9d6

I just got some brain damage while fixing this

119240b

Why are you even reading this???

merge master

82b6c36

* master: bump spec versions in kusama, polkadot and westend (#2391)

eskimor reviewed Feb 5, 2021

View reviewed changes

pepyakin reviewed Feb 5, 2021

View reviewed changes

ordian added 2 commits February 5, 2021 20:58

wrap long line

d428ce3

address some review nits

911f36d

rphmeier reviewed Feb 6, 2021

View reviewed changes

merge

e61f384

* master: Send view to new peers (#2392)

ordian merged commit 57eb9d1 into master Feb 8, 2021

ordian deleted the ao-pass-peerset-to-connecttovalidators-request branch February 8, 2021 07:58

eskimor mentioned this pull request Feb 8, 2021

Statement distribution likely does not connect properly to the validator peer set. #2400

Closed

ordian mentioned this pull request Feb 8, 2021

validator_discovery: cache by (Hash, ParaId) #2402

Merged

pepyakin mentioned this pull request Feb 10, 2021

Missed Declare from the collator #2362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validator_discovery: pass PeerSet to the request #2372

validator_discovery: pass PeerSet to the request #2372

ordian commented Feb 3, 2021

ordian commented Feb 3, 2021

tomaka commented Feb 3, 2021 •

edited

Loading

eskimor left a comment

rphmeier commented Feb 3, 2021

rphmeier left a comment

ordian commented Feb 3, 2021

rphmeier commented Feb 3, 2021

ordian commented Feb 3, 2021

ordian commented Feb 5, 2021

eskimor Feb 5, 2021

ordian Feb 5, 2021

eskimor Feb 8, 2021

ordian Feb 8, 2021

ordian Feb 8, 2021

eskimor Feb 8, 2021

pepyakin Feb 5, 2021

ordian Feb 5, 2021

pepyakin Feb 5, 2021

ordian Feb 5, 2021

bkchr Feb 5, 2021

ordian Feb 5, 2021

bkchr Feb 6, 2021

pepyakin Feb 8, 2021

eskimor Feb 8, 2021

ordian Feb 8, 2021

rphmeier left a comment •

edited

Loading

ordian commented Feb 6, 2021

tomaka commented Feb 6, 2021 •

edited

Loading

ordian commented Feb 8, 2021

eskimor commented Feb 8, 2021 •

edited

Loading

ordian commented Feb 8, 2021 •

edited

Loading

rphmeier commented Feb 8, 2021

eskimor commented Feb 8, 2021 •

edited

Loading

validator_discovery: pass PeerSet to the request #2372

validator_discovery: pass PeerSet to the request #2372

Conversation

ordian commented Feb 3, 2021

ordian commented Feb 3, 2021

tomaka commented Feb 3, 2021 • edited Loading

eskimor left a comment

Choose a reason for hiding this comment

rphmeier commented Feb 3, 2021

rphmeier left a comment

Choose a reason for hiding this comment

ordian commented Feb 3, 2021

rphmeier commented Feb 3, 2021

ordian commented Feb 3, 2021

ordian commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier left a comment • edited Loading

Choose a reason for hiding this comment

ordian commented Feb 6, 2021

tomaka commented Feb 6, 2021 • edited Loading

ordian commented Feb 8, 2021

eskimor commented Feb 8, 2021 • edited Loading

ordian commented Feb 8, 2021 • edited Loading

rphmeier commented Feb 8, 2021

eskimor commented Feb 8, 2021 • edited Loading

tomaka commented Feb 3, 2021 •

edited

Loading

rphmeier left a comment •

edited

Loading

tomaka commented Feb 6, 2021 •

edited

Loading

eskimor commented Feb 8, 2021 •

edited

Loading

ordian commented Feb 8, 2021 •

edited

Loading

eskimor commented Feb 8, 2021 •

edited

Loading