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

Connection keep alive mechanism broken on key changes #3038

Closed
eskimor opened this issue May 17, 2021 · 2 comments · Fixed by #3059
Closed

Connection keep alive mechanism broken on key changes #3038

eskimor opened this issue May 17, 2021 · 2 comments · Fixed by #3059
Labels
I3-bug Fails to follow expected behavior.

Comments

@eskimor
Copy link
Member

eskimor commented May 17, 2021

It seems that we keep track of connection requests by AuthorityDiscoveryId in validator discovery and we issue add/remove to peer set requests based on that here. Problem is, if an authority changed its key at the session boundary, then different AuthorityDiscoveryIds would match to the same PeerId, resulting in the connection to be dropped, although it should be maintained.

This happens as the same PeerId/address would be added and removed immediately afterwards.

@ordian is this analysis correct or am I missing something?

@eskimor eskimor added the I3-bug Fails to follow expected behavior. label May 17, 2021
@ordian
Copy link
Member

ordian commented May 17, 2021

Hmm, you're right!

However, there is also another issue with key rotations. The problem is that the authority discovery cache might remove the entry for the old AuthorityId link, which might lead to a leak instead of removal.

@eskimor
Copy link
Member Author

eskimor commented May 17, 2021

Right! Using the cache for anything else than the initial discovery is just racy and prone to bugs. That cache is not immutable, so we should not treat it as if it were.

@ordian ordian linked a pull request May 19, 2021 that will close this issue
2 tasks
@ghost ghost closed this as completed in #3059 May 20, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I3-bug Fails to follow expected behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants