Relax the assumption of receiving own CONVERGE messages synchronously
#334
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
==========================================
+ Coverage 83.48% 83.59% +0.11%
==========================================
Files 15 15
Lines 1623 1634 +11
==========================================
+ Hits 1355 1366 +11
Misses 158 158
Partials 110 110
|
masih
force-pushed
the
masih/async-recieve-self-ok
branch
2 times, most recently
from
1d885d8 to
64f5861
Compare
|
Seems good to me as an approach |
gpbft/gpbft.go
Outdated
| // has begun the CONVERGE_PHASE. This notification ensures that the convergeState | ||
| // of a round does not rely on messages broadcast by a participant destined for | ||
| // itself to be delivered synchronously. See HasSelfBegunConverge. | ||
| func (c *convergeState) NotifySelfConvergeBegun(value ECChain, justification *Justification) { |
There was a problem hiding this comment.
Nit: rather than the terminology of notifications in the method names and comments, I suggest something simple and direct like SetSelfValue, HasSelfValue.
gpbft/gpbft.go
Outdated
| @@ -1281,16 +1308,34 @@ func (c *convergeState) FindMaxTicketProposal(table PowerTable) ConvergeValue { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // Check if self participant has entered CONVERGE phase. | |||
There was a problem hiding this comment.
I think we can focus the documentation here a little, but the code looks good.
I would say that if the node has received any converged messages, it finds the best ticket from those. That will include the node's own messages, once they are delivered asynchronously. Fall back to the self-value if nothing has been received yet. Note that this introduces a brief possibility of a node ignoring its own value if it has not received the ticket for it yet.
I don't think we need to talk about checks on whether the phase has started (goes with renaming the method) or make statements about what is assumed to have been sent down at this level. This is a change in focus of the comments from what other code is assumed to have done to direct statements about what this code does.
gpbft/gpbft.go
Outdated
|
|
||
| // Check if self participant has entered the CONVERGE step, and whether the chain | ||
| // matches the self proposal. This clause covers an edge-case where self | ||
| // participant has not received broadcasts about its own CONVERGE messages yet. |
There was a problem hiding this comment.
I think these comments are unnecessary. The code does what it says it will do.
sim/latency/log_normal.go
Outdated
| @@ -40,10 +40,10 @@ func NewLogNormal(seed int64, mean time.Duration) *LogNormal { | |||
| // distribution. Latency from one participant to another may be asymmetric and | |||
| // once generated remains constant for the lifetime of a simulation. | |||
| // | |||
| // Note, where from and to are the same or mean configured latency is not larger | |||
| // than zero the latency sample will always be zero. | |||
| // Note, mean configured latency is not larger than zero the latency sample will | |||
There was a problem hiding this comment.
| // Note, mean configured latency is not larger than zero the latency sample will | |
| // Note, when mean configured latency is not larger than zero the latency sample will |
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
force-pushed
the
masih/async-recieve-self-ok
branch
from
71af777 to
174bb85
Compare
anorth
force-pushed
the
masih/async-recieve-self-ok
branch
from
174bb85 to
60f23e6
Compare
The gpbft implementation implicitly assumes that broadcast of
CONVERGEmessages 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
CONVERGEstep, 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
GMessagenor alarms).Fixes #316
Reverts #318
Relates to #103 (comment)