Break out RoundChangeCertificate validation #864
Conversation
With upcoming changes to remove the NewRound message, and place the RoundChangeCertificate in the Proposal, it was decided to break out the RoundChangeCertiifcate validation into a separate file to minimise changes during message restructuring.
| return false; | ||
| } | ||
|
|
||
| if (!roundChangeCert.getRoundChangePayloads().stream() |
There was a problem hiding this comment.
Would the RoundChangeCertificate benefit from providing this behaviour in a method, say public boolean doesRoundEqual(ConsensRoundIdentifier expectedRound), as that would make the semantics nicer in this spot.
There was a problem hiding this comment.
Just the round check (i.e. not whole validation) - yeah could, the function is more like "doAllRoundChangesMatch(ConsensusRoundIdentifier expectedRound)"
| return false; | ||
| } | ||
|
|
||
| for (final SignedData<RoundChangePayload> roundChangeMsg : |
There was a problem hiding this comment.
The RoundChangePayloadValidator only needs to be created once. And this validation check could be replaced with allMatch on the stream so could do something like if (!roundChangeCert.getRoundChangePayloads().stream()
.allMatch(roundChangeValidator::validateRoundChange)) { .. }
| this.quorum = IbftHelpers.calculateRequiredValidatorQuorum(validators.size()); | ||
| this.chainHeight = chainHeight; | ||
| } | ||
|
|
There was a problem hiding this comment.
would if make sense to just have a top level validateRoundChangeCertificate method?
There was a problem hiding this comment.
At this stage we can't - there's 2 concepts - when we make Proposal, this may be an option.
- Is the RoundChange self-consistent?
- Does this Block align with the content in the RoundChange?
With upcoming changes to remove the NewRound message, and place the RoundChangeCertificate in the Proposal, it was decided to break out the RoundChangeCertiifcate validation into a separate file to minimise changes during message restructuring.