Issue A - Missing Check on Quorum for the RoundChange Justification #207
Conversation
| //TODO rename to root | ||
| Value [32]byte | ||
| //TODO rename to value |
There was a problem hiding this comment.
The rename affects other tests
after the PR is accepted I will open issues for the rename and remove the TODO comment
| if err != nil && len(test.ExpectedError) != 0 { | ||
| require.EqualError(t, err, test.ExpectedError) | ||
| return | ||
| } else { |
There was a problem hiding this comment.
not important but else isn't required since you return anyawy.
|
State Comparison will be added here after a seperate PR will add this ability to |
|
@GalRogozinski we have some real world scenario to test this out with? That will be valuable. It seems to me that LastPreparedValue is only set when received a quorum of prepare messages so in fact there won't be a situation in which we don't have the full quorum. Right? |
And yes I agree with you. Only if we allow nil proposal values (I don't think we do) maybe then the new check can somehow be important. But better to be danfy-spec compliant and defensive. |
Synopsis
The function getRoundChangeJustification in the qbft/prepare.go file does not check that the
set of constructed Prepare messages is of size quorum. This is a deviation from the QBFT formal
verification code.
Impact
The function returns a set of valid Prepare messages that is attached to a RoundChange message to
justify the round change by the operator. As specified in the QBFT code, the operator needs to check here
the size of the set. Not checking this can lead to sending around a RoundChange message that is not
accepted by the other operators. This in turn can lead to liveness issues, i.e. the honest operators send
around invalid RoundChange messages that are not accepted by other honest operators. Since the
proposer for a higher round requires quorum-many valid RoundChange messages, this can lead to a
state in which the operators do not find consensus and liveness is not reached.
Fix
Add missing check and spectest