Basic Ibft message validators #314
Conversation
rain-on
force-pushed
the
ibft_msg_validation
branch
from
77febd1
to
722131e
Compare
rain-on
force-pushed
the
ibft_msg_validation
branch
from
722131e
to
3948d45
Compare
| return false; | ||
| } | ||
|
|
||
| if (preprepareMessage.isPresent()) { |
There was a problem hiding this comment.
This check is already being done on line 70
| this.parentHeader = parentHeader; | ||
| } | ||
|
|
||
| public boolean addPreprepareMessage( |
There was a problem hiding this comment.
Perhaps the validation part of this should be split out into something validation message to be consistent with the validations
| public boolean addPreprepareMessage( | ||
| final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) { | ||
|
|
||
| if (preprepareMessage.isPresent()) { |
There was a problem hiding this comment.
be nicer to use ifPresent pass through the preprepareMessage value so that handleSubsequentPreprepareMessage doesn't need to do a preprepareMessage.get() call
There was a problem hiding this comment.
Can't use ifPresent, as we need to be able to return (ifPresent takes a consumer) - however, can pass the value through to prevent the extraneous get().
| final IbftSignedMessageData<IbftUnsignedCommitMessageData> msg) { | ||
| final String msgType = "Commit"; | ||
|
|
||
| if (!isMessageInRoundFromValidatorAndMatchesPreprepareDigest(msg, msgType)) { |
There was a problem hiding this comment.
I think commit is only meant to validate that round number > current round number and not check the sequence number. Not sure if it matters or not.
There was a problem hiding this comment.
I'm getting the impression that the commit message's consensusroundID needs to match the associated Preprepare message - so .... take it up with Roberto.
There was a problem hiding this comment.
all good as we are discarding old commit messages
| } | ||
| IbftUnsignedPrePrepareMessageData that = (IbftUnsignedPrePrepareMessageData) o; | ||
| return Objects.equals(block, that.block) | ||
| && Objects.equals(roundIdentifier, that.roundIdentifier); |
There was a problem hiding this comment.
Should this equals just check the properties this class added and use super.equals for the parts defined in the superclass?
There was a problem hiding this comment.
Ditched this and made a custom equality function (can be dangerous using 'equals' in a heirarchy of classes.
rain-on
force-pushed
the
ibft_msg_validation
branch
2 times, most recently
from
424f5d9
to
d390eb4
Compare
| return validateDigestMatchesPreprepareBlock(msg.getUnsignedMessageData().getDigest(), msgType); | ||
| } | ||
|
|
||
| private boolean isMessageInRoundFromValidatorAndMatchesPreprepareDigest( |
There was a problem hiding this comment.
This name does look correct. The method does not perform any verification on the digest.
| final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> existingMsg, | ||
| final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> newMsg) { | ||
| if (!existingMsg.getSender().equals(newMsg.getSender())) { | ||
| LOG.debug("Received subsequent invalid Preprepare message; sender differs from original."); |
There was a problem hiding this comment.
This log message is printed only if a Pre-prepare message from a validator that is not the proposer is received after receiving a valid Pre-prepare message sent by the expected proposer. If the Pre-prepare message send by the non-proposer is received before the Pre-prepare message sent by the proposer then this log message is not displayed.
This does not appear right to me.
There was a problem hiding this comment.
Hopefully, if we receive a Preprepare from a Non-proposer, we will show an error indicating the supplied message was not from the expected proposer - and will thus be discarded.
This check can be removed, and the "not-proposer" check can catch subsequent failures.
| } | ||
|
|
||
| @Test | ||
| public void receivingAPrepareMessageBeforePrePrepareFails() { |
There was a problem hiding this comment.
The names of these tests suggest that if we receive a Prepare or Commit message before we receive a Pre-prepare message then the Prepare/Commit message should be rejected. Perhaps there is a better name for these tests.
There was a problem hiding this comment.
For the context, this is the expected behaviour - i.e. this class will state that a Prepare message is invalid if insufficient context exists to validate it.
rain-on
force-pushed
the
ibft_msg_validation
branch
from
d390eb4
to
d87e677
Compare
MessageValidator has been added which is responsible for determining the validity of a given prepare or commit message with respect to an existing Preprepare message.
It is also responsible for ensuring a supplied Preprepare message contains valid content, in line with the context of the validation (i.e. current consensus round, block is valid in the context of the existing blockchain etc).