authority-discovery: Support multiple authority ids per peer id #10259

bkchr · 2021-11-13T19:06:32Z

An peer id can be mapped to multiple authority ids, because an authority id is a session key that
could be changed every session. Before this pr the internal authority discovery cache assumed that
each authority id can only be mapped to one peer id. However, this isn't true since we changed the
default implementation of the authority discovery to combine the current and next session authorities.

Closes: #7461

polkadot companion: paritytech/polkadot#4295

skip check-dependent-cumulus

An peer id can be mapped to multiple authority ids, because an authority id is a session key that could be changed every session. Before this pr the internal authority discovery cache assumed that each authority id can only be mapped to one peer id. However, this isn't true since we changed the default implementation of the authority discovery to combine the current and next session authorities.

…covery-multiple-peer-ids

client/authority-discovery/src/worker/addr_cache.rs

ordian · 2021-11-13T21:39:13Z

Needs a polkadot companion

client/authority-discovery/src/worker/addr_cache.rs

client/authority-discovery/src/worker.rs

…covery-multiple-peer-ids

ordian

Looks better now :)

client/authority-discovery/src/worker/addr_cache.rs

ordian · 2021-11-14T13:06:09Z

client/authority-discovery/src/worker/addr_cache.rs

-			};
+	pub fn insert(&mut self, authority_id: AuthorityId, addresses: Vec<Multiaddr>) {
+		let addresses = addresses.into_iter().collect::<HashSet<_>>();
+		let peer_ids = addresses_to_peer_ids(&addresses);


maybe we should ensure the len is 1 here? See the comment above

I actually meant exactly one

Hmm, I mean you could change your peerid and then for some time you would occur with multiple peer ids in the dht?

Even if someone runs two nodes, we can not know this for sure.

But if you're putting a new record, won't it override the old one? They are keyed by AuthorityId(s).

Even if someone runs two nodes, we can not know this for sure.

yeah, I wrote this in the comment above:

Technically, one could copy an AuthorityId to a validator with different PeerId and change other session keys. But that's a really contrived scenario.

Even if there are two records (old and new) floating around, we'll never see a mix of a few records in one call. We either see one or another (or one after another). And each record should contain Multiaddrs corresponding to one peer. Otherwise, a warning might be warranted -- this is either an equivocation symptom or misbehavior.

Even if the current implementation does not end up with multiple PeerId-s, does it really affect security if the same AuthorityId is used on multiple peers? That would need copying the ephemeral private key of the session from one peer to another, right?

An authority that has put multiple PeerIds into its own record can do harm to noone but itself, by not properly connecting to everyone an missing on approval voting rewards (and if there are many such authorities, it may slightly delay finality). But I think we should detect and report this and not silently ignore.

So are you okay if finding multiple PeerId-s for an AuthorityId is detected and logged, but not forbidden?

But I think we should detect and report this and not silently ignore.

Done

Co-authored-by: Andronik Ordian <write@reusable.software>

client/authority-discovery/src/worker/addr_cache.rs

wigy-opensource-developer

It is nice to see how separated these mappings in AddrCache were from all other code. Hopefully other pieces of code that had an assumption about the multiplicity of this relation would be compile-time broken by these changes.

client/authority-discovery/src/worker/addr_cache.rs

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Authority discovery before had a fixed mapping from `PeerId` to `AuthorityId`. This wasn't correct, as a `PeerId` can actually map to multiple `AuthorityId`s. The linked Substrate pr fixes this. paritytech/substrate#10259

ordian

One last thing...

client/authority-discovery/src/lib.rs

client/authority-discovery/src/service.rs

client/authority-discovery/src/worker/addr_cache.rs

Co-authored-by: Andronik Ordian <write@reusable.software>

…com:paritytech/substrate into bkchr-authority-discovery-multiple-peer-ids

eskimor · 2021-11-15T15:20:25Z

client/authority-discovery/src/lib.rs

 	/// See [`Service::get_authority_id_by_peer_id`].
-	GetAuthorityIdByPeerId(PeerId, oneshot::Sender<Option<AuthorityId>>),
+	GetAuthorityIdByPeerId(PeerId, oneshot::Sender<Option<HashSet<AuthorityId>>>),


Isn't the Option redundant here?

Depends on how you see it. None tells you that the peer id is unknown.

We would also know that, if the set is empty - no? Like if there is an entry for the PeerId the set should not be empty, otherwise, why would we keep the entry?

ordian · 2021-11-16T14:18:28Z

companion needs fixing and we're good to go

bkchr · 2021-11-17T09:51:04Z

bot merge force

paritytech-processbot · 2021-11-17T09:51:13Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4295

bkchr · 2021-11-17T09:53:17Z

bot merge force

paritytech-processbot · 2021-11-17T09:53:25Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4295

ordian · 2021-11-17T09:54:47Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4295

The error message is wrong, it expects 2 approvals on companion

* Substrate companion: Authority discovery multiple peer ids Authority discovery before had a fixed mapping from `PeerId` to `AuthorityId`. This wasn't correct, as a `PeerId` can actually map to multiple `AuthorityId`s. The linked Substrate pr fixes this. paritytech/substrate#10259 * Update node/network/availability-distribution/src/requester/mod.rs * Update node/network/collator-protocol/src/validator_side/mod.rs * Update node/network/statement-distribution/src/tests.rs * Update guide * Adapt to Substrate pr * Update Substrate

…tytech#10259) * authority-discovery: Support multiple authority ids per peer id An peer id can be mapped to multiple authority ids, because an authority id is a session key that could be changed every session. Before this pr the internal authority discovery cache assumed that each authority id can only be mapped to one peer id. However, this isn't true since we changed the default implementation of the authority discovery to combine the current and next session authorities. * Review feedback * Update client/authority-discovery/src/worker/addr_cache.rs Co-authored-by: Andronik Ordian <write@reusable.software> * Early return on no peer ids * Update client/authority-discovery/src/worker/addr_cache.rs Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Update types in comment * FMT * Add warning * Update client/authority-discovery/src/worker/addr_cache.rs Co-authored-by: Andronik Ordian <write@reusable.software> * Feedback Co-authored-by: Andronik Ordian <write@reusable.software> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

bkchr added 2 commits November 13, 2021 20:01

Merge remote-tracking branch 'origin/master' into bkchr-authority-dis…

3074710

…covery-multiple-peer-ids

bkchr requested review from tomaka, eskimor and ordian November 13, 2021 19:06

ordian reviewed Nov 13, 2021

View reviewed changes

client/authority-discovery/src/worker/addr_cache.rs Outdated Show resolved Hide resolved

ordian reviewed Nov 13, 2021

View reviewed changes

client/authority-discovery/src/worker/addr_cache.rs Outdated Show resolved Hide resolved

ordian reviewed Nov 13, 2021

View reviewed changes

client/authority-discovery/src/worker.rs Outdated Show resolved Hide resolved

bkchr added 2 commits November 13, 2021 23:54

Review feedback

45b388a

Merge remote-tracking branch 'origin/master' into bkchr-authority-dis…

c4f78da

…covery-multiple-peer-ids

ordian reviewed Nov 14, 2021

View reviewed changes

bkchr and others added 2 commits November 14, 2021 14:31

Update client/authority-discovery/src/worker/addr_cache.rs

75c4b44

Co-authored-by: Andronik Ordian <write@reusable.software>

Early return on no peer ids

960e2ff

bkchr requested a review from wigy-opensource-developer November 15, 2021 08:29

tomaka approved these changes Nov 15, 2021

View reviewed changes

client/authority-discovery/src/worker/addr_cache.rs Outdated Show resolved Hide resolved

wigy-opensource-developer approved these changes Nov 15, 2021

View reviewed changes

client/authority-discovery/src/worker/addr_cache.rs Outdated Show resolved Hide resolved

bkchr and others added 2 commits November 15, 2021 12:54

Update client/authority-discovery/src/worker/addr_cache.rs

358bd85

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Update types in comment

e4d76f7

ordian approved these changes Nov 15, 2021

View reviewed changes

bkchr mentioned this pull request Nov 15, 2021

Substrate companion: Authority discovery multiple peer ids paritytech/polkadot#4295

Merged

bkchr added 2 commits November 15, 2021 21:39

FMT

1e87690

Add warning

24bb824

ordian reviewed Nov 15, 2021

View reviewed changes

client/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved

client/authority-discovery/src/service.rs Outdated Show resolved Hide resolved

client/authority-discovery/src/worker/addr_cache.rs Outdated Show resolved Hide resolved

Update client/authority-discovery/src/worker/addr_cache.rs

7feed91

Co-authored-by: Andronik Ordian <write@reusable.software>

bkchr added 2 commits November 16, 2021 11:28

Feedback

bd9423a

Merge branch 'bkchr-authority-discovery-multiple-peer-ids' of github.…

4779981

…com:paritytech/substrate into bkchr-authority-discovery-multiple-peer-ids

eskimor approved these changes Nov 16, 2021

View reviewed changes

bkchr merged commit 0baa586 into master Nov 17, 2021

bkchr deleted the bkchr-authority-discovery-multiple-peer-ids branch November 17, 2021 09:53

bkchr modified the milestone: Polkadot v0.9.13 Nov 25, 2021

ghzlatarev mentioned this pull request Dec 6, 2021

[Manta] Integrate v0.9.13 upstream changes Manta-Network/Manta#294

Merged

23 tasks

ordian mentioned this pull request Dec 28, 2021

bridge: add all PeerIds of an AuthorityId to the gossip topology paritytech/polkadot#4621

Closed

ordian mentioned this pull request Mar 21, 2022

Reduce network bandwidth, improve parablock times: optimize approval-distribution paritytech/polkadot#5164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authority-discovery: Support multiple authority ids per peer id #10259

authority-discovery: Support multiple authority ids per peer id #10259

bkchr commented Nov 13, 2021 •

edited

Loading

ordian commented Nov 13, 2021

ordian left a comment

ordian Nov 14, 2021

bkchr Nov 14, 2021

ordian Nov 14, 2021

bkchr Nov 15, 2021

ordian Nov 15, 2021

ordian Nov 15, 2021 •

edited

Loading

wigy-opensource-developer Nov 15, 2021

ordian Nov 15, 2021 •

edited

Loading

wigy-opensource-developer Nov 15, 2021

bkchr Nov 15, 2021

wigy-opensource-developer left a comment •

edited

Loading

ordian left a comment

eskimor Nov 15, 2021

bkchr Nov 17, 2021

eskimor Nov 17, 2021

ordian commented Nov 16, 2021

bkchr commented Nov 17, 2021

paritytech-processbot bot commented Nov 17, 2021

bkchr commented Nov 17, 2021

paritytech-processbot bot commented Nov 17, 2021

ordian commented Nov 17, 2021

authority-discovery: Support multiple authority ids per peer id #10259

authority-discovery: Support multiple authority ids per peer id #10259

Conversation

bkchr commented Nov 13, 2021 • edited Loading

ordian commented Nov 13, 2021

ordian left a 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

ordian Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wigy-opensource-developer left a comment • edited Loading

Choose a reason for hiding this comment

ordian left a 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

ordian commented Nov 16, 2021

bkchr commented Nov 17, 2021

paritytech-processbot bot commented Nov 17, 2021

bkchr commented Nov 17, 2021

paritytech-processbot bot commented Nov 17, 2021

ordian commented Nov 17, 2021

bkchr commented Nov 13, 2021 •

edited

Loading

ordian Nov 15, 2021 •

edited

Loading

ordian Nov 15, 2021 •

edited

Loading

wigy-opensource-developer left a comment •

edited

Loading