Ensure received certificates' messages are delivered. #2696
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deuszx
reviewed
Oct 23, 2024
deuszx
reviewed
Oct 23, 2024
| @@ -3236,16 +3233,11 @@ where | |||
| &self, | |||
| remote_node: RemoteNode<P::Node>, | |||
| ) -> Result<(), ChainClientError> { | |||
| let mutex = self.state().client_mutex(); | |||
| let _guard = mutex.lock_owned().await; | |||
There was a problem hiding this comment.
TBH I don't know why we can remove it now. It protected from concurrent calls to synchronize_received_certificates_from_validator - why do we not care about that anymore?
There was a problem hiding this comment.
It also protected from at the same time doing
synchronize_received_certificates, and- processing a notification about a received certificate from a validator.
That's what can cause the issue described in #2692 and it was the only reason these were added in #2567.
With the retry_… call, this scenario should now work fine.
deuszx
approved these changes
Oct 24, 2024
deuszx
pushed a commit
that referenced
this pull request
Oct 24, 2024
* Ensure received certificates' messages are delivered. * Remove unnecessary client mutex locking. * Add a comment: Why are we retrying?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Due to #2692
process_inboxcan in some cases return without processing a message, even though a quorum of validators already had the sending certificate.The client mutex fixed this in most cases, but causes unnecessary contention.
Proposal
Ensure that the messages are delivered using
retry_pending_cross_chain_requests.Test Plan
The tests should not be flaky anymore despite the removal of the two client locks.
Release Plan
devnetbranch, thentestnetbranch, thenLinks