Document and adjust alarm delivery precedence over network messages #318
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
- Coverage 84.99% 84.85% -0.14%
==========================================
Files 14 14
Lines 1486 1486
==========================================
- Hits 1263 1261 -2
- Misses 139 140 +1
- Partials 84 85 +1 |
Document the implicit assumption that the delivery of alarms to a participant must take precedence over broadcast messages. Adjust simulation message queue to respect the alarm ordering. Fixes #316
masih
force-pushed
the
masih/sim-alarm-precedence
branch
from
93e1983 to
787e8ef
Compare
|
This is hard or almost impossible to guarantee in an environment where signing is async and can fail. I will open an issue about it. |
Kubuxu
approved these changes
Jun 7, 2024
Even for alarms? Alarms have no signing. right? |
|
Ahh, wait, I might have miss-read it, I assumed broadcast have precedence, I can for sure make the alarms take precedence. On the other hand, in a real system, there is a possibility of a message arriving just before the alarm but IDK if it will be an issue. |
This was referenced Jun 10, 2024
masih
added a commit
that referenced
this pull request
Jun 11, 2024
The gpbft implementation implicitly assumes that broadcast of `CONVERGE` messages to self are delivered immediately. In practice this assumption does not hold because of the complexity in deferred signing and async message delivery. The changes here relax this assumption by explicitly notifying the local converge state that the self participant has begun the `CONVERGE` step, providing self proposal and justification for the proposal. The code then considers the given data whenever search in converge state does not bear any results, caused by asynchronous message delivery. Further, the code ignores the self converge value once at least one broadcast message is received. Fixes #316 Reverts #318 Relates to #103 (comment)
masih
added a commit
that referenced
this pull request
Jun 11, 2024
The gpbft implementation implicitly assumes that broadcast of `CONVERGE` messages to self are delivered immediately. In practice this assumption does not hold because of the complexity in deferred signing and async message delivery. The changes here relax this assumption by explicitly notifying the local converge state that the self participant has begun the `CONVERGE` step, providing self proposal and justification for the proposal. The code then considers the given data whenever search in converge state does not bear any results, caused by asynchronous message delivery. Further, the code ignores the self converge value once at least one broadcast message is received. Additionally, the changes remove zero-latency for messages to self in simulations to make a stronger assertion that synchronous message delivery to self is no longer required (neither for `GMessage` nor alarms). Fixes #316 Reverts #318 Relates to #103 (comment)
masih
added a commit
that referenced
this pull request
Jun 11, 2024
The gpbft implementation implicitly assumes that broadcast of `CONVERGE` messages to self are delivered immediately. In practice this assumption does not hold because of the complexity in deferred signing and async message delivery. The changes here relax this assumption by explicitly notifying the local converge state that the self participant has begun the `CONVERGE` step, providing self proposal and justification for the proposal. The code then considers the given data whenever search in converge state does not bear any results, caused by asynchronous message delivery. Further, the code ignores the self converge value once at least one broadcast message is received. Additionally, the changes remove zero-latency for messages to self in simulations to make a stronger assertion that synchronous message delivery to self is no longer required (neither for `GMessage` nor alarms). Fixes #316 Reverts #318 Relates to #103 (comment)
anorth
pushed a commit
that referenced
this pull request
Jun 11, 2024
The gpbft implementation implicitly assumes that broadcast of `CONVERGE` messages to self are delivered immediately. In practice this assumption does not hold because of the complexity in deferred signing and async message delivery. The changes here relax this assumption by explicitly notifying the local converge state that the self participant has begun the `CONVERGE` step, providing self proposal and justification for the proposal. The code then considers the given data whenever search in converge state does not bear any results, caused by asynchronous message delivery. Further, the code ignores the self converge value once at least one broadcast message is received. Additionally, the changes remove zero-latency for messages to self in simulations to make a stronger assertion that synchronous message delivery to self is no longer required (neither for `GMessage` nor alarms). Fixes #316 Reverts #318 Relates to #103 (comment)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 11, 2024
…ly (#334) * Relax the assumption of receiving own `CONVERGE` messages synchronously The gpbft implementation implicitly assumes that broadcast of `CONVERGE` messages to self are delivered immediately. In practice this assumption does not hold because of the complexity in deferred signing and async message delivery. The changes here relax this assumption by explicitly notifying the local converge state that the self participant has begun the `CONVERGE` step, providing self proposal and justification for the proposal. The code then considers the given data whenever search in converge state does not bear any results, caused by asynchronous message delivery. Further, the code ignores the self converge value once at least one broadcast message is received. Additionally, the changes remove zero-latency for messages to self in simulations to make a stronger assertion that synchronous message delivery to self is no longer required (neither for `GMessage` nor alarms). Fixes #316 Reverts #318 Relates to #103 (comment) * Adjust naming and comments. --------- Co-authored-by: Alex North <445306+anorth@users.noreply.github.com>
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.
Document the implicit assumption that the delivery of alarms to a participant must take precedence over broadcast messages.
Adjust simulation message queue to respect the alarm ordering.
Fixes #316