Test: QBFT messages out of order #336
Conversation
MatheusFranco99
commented
Jan 2, 2024
MatheusFranco99
commented
- QBFT Messages received out of order. Test case: one prepare and one commit are received before the proposal causing the instance not to decide.
| ControllerPostState: sc.ExpectedState, | ||
| }, | ||
| }, | ||
| ExpectedError: "could not process msg: invalid signed message: did not receive proposal for this round", |
There was a problem hiding this comment.
You know, until this day this behavior in spec confuses me...
Technically, messages on a real network may get delayed and come out of order.
Also looking again at the IBFT paper it seems that out of order messages are allowed to my understanding...
To my understanding the implementation uses the assumption that messages come in order to increase performance. This may be fine and maybe we adjusted the spec to work this way.
But it is kind of weird that we restrict out of order messages... maybe it is best to not have a test for it so that implementation can choose their behavior?
There was a problem hiding this comment.
I agree that it's a confusing behaviour.
IBFT totally allows it while QBFT implementation doesn't, as it seems to me.
In my opinion this could change. We could store messages that seems valid and only process the IBFT rule when something triggers it (e.g. storing all prepares and, once a proposal is received, we check the IBFT rule for that proposal).
There was a problem hiding this comment.
just curious if you ever saw duties fail/delay due to out of order messages
There was a problem hiding this comment.
currently I don't think we should merge this because why enforce this?
There was a problem hiding this comment.
I agree. I currently don't really see the point of this test.
There was a problem hiding this comment.
Basically, we are talking about processing messages the way you said, we did see sometimes where we get a prepare before propose and then fail to process the propose. we want to create a structure like you mentioned where we store valid looking messages that arrive and process them in the appropriate time instead of just crashing because of it.
There was a problem hiding this comment.
I'm not reviewing this (although it looks fine) for now.
There was a problem hiding this comment.
Even if eventually you will do this @y0sher
not sure this test should be merged because I am unsure if we should force implementation to accept out of order messages (even though theoretically we should)
| } | ||
| } | ||
|
|
||
| func OutOfOrderFlowStateComparison(msgs []*qbft.SignedMessage) *qbftcomparable.StateComparison { |
There was a problem hiding this comment.
Since we added the json feature we only do this for "core tests" so we pay more attention to the tests
|
@MatheusFranco99 do we have a github issue for this (no need to open one)? |
|
@GalRogozinski |
|
Closing since we don't want to enforce this |
