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

Two bugfixes to authority-discovery #8768

Merged
merged 1 commit into from
May 9, 2021

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented May 9, 2021

Two bugfixes:

  • When addresses is empty, we shouldn't just return at the beginning, as we need to erase the previous addresses. insert is really insert_or_replace, similar to what HashMap::insert does. Inserting Vec::new() should remove the previous value.

  • When an authority says it has a certain PeerId, we remove that PeerId from any previous authority. If this previous authority had multiple addresses (with the same PeerId) we will call self.peer_id_to_authority_id.remove(&peer_id) multiple times with the same value in peer_id. This is fine, but the debug assert just below (debug_assert!(_old_auth.is_some());) is wrong, as it will fail the second time.

Fix paritytech/polkadot#2998

@tomaka tomaka added A0-please_review Pull request needs code review. 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 May 9, 2021
@tomaka tomaka requested review from kpp, ordian and eskimor May 9, 2021 16:35
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.

The debug assertion removal is fine. But I'm not sure about empty addresses. In practice inserting empty addresses should never happen

if !remote_addresses.is_empty() {
self.addr_cache.insert(authority_id, remote_addresses);

@bkchr bkchr merged commit 0aad5b8 into paritytech:master May 9, 2021
@tomaka tomaka deleted the auth-disc-bugfixes branch May 10, 2021 07:35
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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.

panic - tokio-runtime-worker assertion failed: _old_auth.is_some()
3 participants