Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add NetworkBridgeEvent::UpdatedAuthorityIds #6227

Merged
merged 29 commits into from
Jul 21, 2023

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Nov 1, 2022

Closes: #6192

@Szegoo
Copy link
Contributor Author

Szegoo commented Nov 3, 2022

@rphmeier I would just like to confirm that I understand the issue correctly. As mentioned we want to introduce UpdatedAuthorityIds to the NetworkBridgeEvent. We should track for authority id updates inside the network bridge subsystem and dispatch the NetworkBridgeEvent::UpdatedAuthorityIds if authority ids are updated, right?

I am not sure for which network event should we check and emit the UpdatedAuthorityIds, I hope you could clarify this for me.

@Szegoo Szegoo marked this pull request as ready for review November 4, 2022 16:42
@Szegoo Szegoo requested a review from rphmeier November 4, 2022 16:48
@Szegoo
Copy link
Contributor Author

Szegoo commented Nov 10, 2022

@rphmeier Could you PTAL.

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts with my understanding:

  • Key rotations will lead to a rather quick DHT update, which means that authority discovery cache might be updated soon after. get_authority_ids_by_peer_id method should return both old authority_id and the new one after the update.
  • However, on chain the changes will take place on the new session only.

The only problem here seems to be statement-distribution maintains an additional mapping AuthorityId -> PeerId which is only updated on peer connection/disconnection, so it will not take into account session rotations.

Since it's a only becomes a problem on session rotations, we could issue this message in gossip-support on each new session.

node/network/bridge/src/rx/mod.rs Outdated Show resolved Hide resolved
@Szegoo
Copy link
Contributor Author

Szegoo commented Nov 11, 2022

@ordian I think the PR should be feasible now, if I am not wrong, I think I implemented all of the necessary logic for the UpdatedAuthorityIds to work.

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

node/network/gossip-support/src/lib.rs Outdated Show resolved Hide resolved
node/network/approval-distribution/src/lib.rs Outdated Show resolved Hide resolved
node/network/bitfield-distribution/src/lib.rs Outdated Show resolved Hide resolved
node/network/bridge/src/rx/mod.rs Outdated Show resolved Hide resolved
node/network/collator-protocol/src/collator_side/mod.rs Outdated Show resolved Hide resolved
node/subsystem-types/src/messages.rs Outdated Show resolved Hide resolved
node/subsystem-types/src/messages.rs Outdated Show resolved Hide resolved
node/network/collator-protocol/src/validator_side/mod.rs Outdated Show resolved Hide resolved
node/network/statement-distribution/src/lib.rs Outdated Show resolved Hide resolved
node/network/statement-distribution/src/lib.rs Outdated Show resolved Hide resolved
@ordian 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 Nov 11, 2022
@Szegoo Szegoo requested review from ordian and removed request for rphmeier December 17, 2022 17:17
@Szegoo
Copy link
Contributor Author

Szegoo commented Dec 17, 2022

@ordian Could you approve the PR if the changes are ok?

@Szegoo
Copy link
Contributor Author

Szegoo commented Dec 26, 2022

@bkchr Could you review this?

@ordian
Copy link
Member

ordian commented Jan 2, 2023

@Szegoo sorry for the delay! Could you merge recent master?

@ordian ordian requested a review from eskimor January 2, 2023 11:09
@Szegoo
Copy link
Contributor Author

Szegoo commented Jan 2, 2023

Could you merge recent master?

Done.

@Szegoo Szegoo requested a review from rphmeier April 12, 2023 07:58
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - some guide changes would be nice too!

Sorry, I completely forgot about this PR.

Doing this check every session is a good step. Probably all we'll need. But more frequent checks may be needed in the future.

@rphmeier rphmeier merged commit 1346281 into paritytech:master Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a NetworkBridgeEvent::UpdatedAuthorityIds
4 participants