Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Known authorities not updated #7131

Closed
2 tasks done
kamilsa opened this issue Jan 13, 2025 · 3 comments
Closed
2 tasks done

Known authorities not updated #7131

kamilsa opened this issue Jan 13, 2025 · 3 comments
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@kamilsa
Copy link

kamilsa commented Jan 13, 2025

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

I suspect the following code could lead to potential bug of when list of authorities not being updated:

// Make sure we don't ever work with an outdated set of authorities
// and that we do not update known_authorithies too often.
let best_hash = self.client.best_hash().await?;
if !self.known_authorities.contains_key(&record_key) &&
self.authorities_queried_at
.map(|authorities_queried_at| authorities_queried_at != best_hash)
.unwrap_or(true)
{
let authorities = self
.client
.authorities(best_hash)
.await
.map_err(|e| Error::CallingRuntime(e.into()))?
.into_iter()
.collect::<Vec<_>>();
self.known_authorities = authorities
.into_iter()
.map(|authority| (hash_authority_id(authority.as_ref()), authority))
.collect::<HashMap<_, _>>();
self.authorities_queried_at = Some(best_hash);
}

We enter if condition and update known_authorities only if known_authorities (possibly outdated) contains record_key. Therefore known_authorities might never be updated if record_key is in known_authorities.

Possible condition should look like

if !self.known_authorities.contains_key(&record_key) ||
      self.authorities_queried_at
        .map(|authorities_queried_at| authorities_queried_at != best_hash)
        .unwrap_or(true)

Or we just do not understand this code properly:)

cc @alexggh @iceseer

Steps to reproduce

Check if known_authorities is actually updated after the set of validators changed

@kamilsa kamilsa added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Jan 13, 2025
@alexggh
Copy link
Contributor

alexggh commented Jan 13, 2025

It periodically gets updated here

self.known_authorities = authorities
, and then this logic is for the situation where a genuine authority sends us a record before we had possibility to update, in that case you want to update only if you haven't already queried it at the current block(best_hash).

With your suggestion you would update it every time someone sends us a record key that we haven't seen before and that's not good because this requests are untrusted, so you don't want to let them control how often you query authorities which is a runtime call.

@alexggh
Copy link
Contributor

alexggh commented Jan 14, 2025

@kamilsa does this clarify the logic for you or am I missing something ?

@kamilsa
Copy link
Author

kamilsa commented Jan 14, 2025

Yes, it does. In KAGOME peer discovery was not working properly lately, so we started to check suspicious places in Polkadot sdk.

Thanks for explanation. I am closing the issue

@kamilsa kamilsa closed this as completed Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

No branches or pull requests

2 participants