Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Basic Ibft message validators #314

Merged
merged 4 commits into from
Nov 28, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ public static Hash calculateDataHashForCommittedSeal(
return Hash.hash(serializeHeader(header, ibftExtraData::encodeWithoutCommitSeals));
}

public static Hash calculateDataHashForCommittedSeal(final BlockHeader header) {
final IbftExtraData ibftExtraData = IbftExtraData.decode(header.getExtraData());
return Hash.hash(serializeHeader(header, ibftExtraData::encodeWithoutCommitSeals));
}

/**
* Constructs a hash of the block header, but omits the committerSeals and sets round number to 0
* (as these change on each of the potentially circulated blocks at the current chain height).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;

import java.util.Objects;

// NOTE: Implementation of all methods of this class is still pending. This class was added to show
// how a PreparedCertificate is encoded and decoded inside a RoundChange message
public class IbftUnsignedPrePrepareMessageData extends AbstractIbftUnsignedInRoundMessageData {
Expand All @@ -36,7 +38,8 @@ public static IbftUnsignedPrePrepareMessageData readFrom(final RLPInput rlpInput

rlpInput.enterList();
final ConsensusRoundIdentifier roundIdentifier = ConsensusRoundIdentifier.readFrom(rlpInput);
final Block block = Block.readFrom(rlpInput, IbftBlockHashing::calculateHashOfIbftBlockOnChain);
final Block block =
Block.readFrom(rlpInput, IbftBlockHashing::calculateDataHashForCommittedSeal);
rlpInput.leaveList();

return new IbftUnsignedPrePrepareMessageData(roundIdentifier, block);
Expand All @@ -59,4 +62,22 @@ public Block getBlock() {
public int getMessageType() {
return TYPE;
}

@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
IbftUnsignedPrePrepareMessageData that = (IbftUnsignedPrePrepareMessageData) o;
return Objects.equals(block, that.block)
&& Objects.equals(roundIdentifier, that.roundIdentifier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this equals just check the properties this class added and use super.equals for the parts defined in the superclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditched this and made a custom equality function (can be dangerous using 'equals' in a heirarchy of classes.

}

@Override
public int hashCode() {
return Objects.hash(block);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/*
* Copyright 2018 ConsenSys AG.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/
package tech.pegasys.pantheon.consensus.ibft.validation;

import static tech.pegasys.pantheon.ethereum.mainnet.HeaderValidationMode.FULL;

import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier;
import tech.pegasys.pantheon.consensus.ibft.IbftContext;
import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.AbstractIbftUnsignedInRoundMessageData;
import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.IbftSignedMessageData;
import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.IbftUnsignedCommitMessageData;
import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.IbftUnsignedPrePrepareMessageData;
import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.IbftUnsignedPrepareMessageData;
import tech.pegasys.pantheon.ethereum.ProtocolContext;
import tech.pegasys.pantheon.ethereum.core.Address;
import tech.pegasys.pantheon.ethereum.core.Block;
import tech.pegasys.pantheon.ethereum.core.BlockHeader;
import tech.pegasys.pantheon.ethereum.core.Hash;
import tech.pegasys.pantheon.ethereum.core.Util;
import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator;

import java.util.Collection;
import java.util.Optional;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

public class MessageValidator {

private static final Logger LOG = LogManager.getLogger();

private final Collection<Address> validators;
private final Address expectedProposer;
private final ConsensusRoundIdentifier roundIdentifier;
private final BlockHeaderValidator<IbftContext> headerValidator;
private final ProtocolContext<IbftContext> protocolContext;
private final BlockHeader parentHeader;

private Optional<IbftSignedMessageData<IbftUnsignedPrePrepareMessageData>> preprepareMessage =
Optional.empty();

public MessageValidator(
final Collection<Address> validators,
final Address expectedProposer,
final ConsensusRoundIdentifier roundIdentifier,
final BlockHeaderValidator<IbftContext> headerValidator,
final ProtocolContext<IbftContext> protocolContext,
final BlockHeader parentHeader) {
this.validators = validators;
this.expectedProposer = expectedProposer;
this.roundIdentifier = roundIdentifier;
this.headerValidator = headerValidator;
this.protocolContext = protocolContext;
this.parentHeader = parentHeader;
}

public boolean addPreprepareMessage(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the validation part of this should be split out into something validation message to be consistent with the validations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done (I think?).

final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) {

if (preprepareMessage.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be nicer to use ifPresent pass through the preprepareMessage value so that handleSubsequentPreprepareMessage doesn't need to do a preprepareMessage.get() call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

return handleSubsequentPreprepareMessage(msg);
}

if (!msg.getUnsignedMessageData().getRoundIdentifier().equals(roundIdentifier)) {
LOG.info("Invalid Preprepare message, does not match current round.");
return false;
}

if (preprepareMessage.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already being done on line 70

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

LOG.info("Invalid Preprepare message, one already exists for the current round.");
return false;
}

if (!msg.getSender().equals(expectedProposer)) {
LOG.info(
"Invalid Preprepare message, was not created by the proposer expected for the "
+ "associated round.");
return false;
}

final Block proposedBlock = msg.getUnsignedMessageData().getBlock();
if (!headerValidator.validateHeader(
proposedBlock.getHeader(), parentHeader, protocolContext, FULL)) {
LOG.info("Invalid Prepare message, block did not pass header validation.");
return false;
}

preprepareMessage = Optional.of(msg);
return true;
}

private boolean handleSubsequentPreprepareMessage(
final IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> msg) {
IbftSignedMessageData<IbftUnsignedPrePrepareMessageData> existingMsg = preprepareMessage.get();

if (!existingMsg.getSender().equals(msg.getSender())) {
LOG.debug("Received subsequent invalid Preprepare message; sender differs from original.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}

final IbftUnsignedPrePrepareMessageData existingData = existingMsg.getUnsignedMessageData();
final IbftUnsignedPrePrepareMessageData newData = msg.getUnsignedMessageData();

if (!existingData.equals(newData)) {
LOG.debug("Received subsequent invalid Preprepare message; content differs from original.");
return false;
}

return true;
}

public boolean validatePrepareMessage(
final IbftSignedMessageData<IbftUnsignedPrepareMessageData> msg) {
final String msgType = "Prepare";

if (!isMessageInRoundFromValidatorAndMatchesPreprepareDigest(msg, msgType)) {
return false;
}

if (msg.getSender().equals(expectedProposer)) {
LOG.info("Illegal Prepare message; was sent by the round's proposer.");
return false;
}

return validateDigestMatchesPreprepareBlock(msg.getUnsignedMessageData().getDigest(), msgType);
}

public boolean validateCommmitMessage(
final IbftSignedMessageData<IbftUnsignedCommitMessageData> msg) {
final String msgType = "Commit";

if (!isMessageInRoundFromValidatorAndMatchesPreprepareDigest(msg, msgType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting the impression that the commit message's consensusroundID needs to match the associated Preprepare message - so .... take it up with Roberto.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good as we are discarding old commit messages

return false;
}

final Block proposedBlock = preprepareMessage.get().getUnsignedMessageData().getBlock();
final Address commitSealCreator =
Util.signatureToAddress(
msg.getUnsignedMessageData().getCommitSeal(), proposedBlock.getHash());

if (!commitSealCreator.equals(msg.getSender())) {
LOG.info("Invalid Commit message. Seal was not created by the message transmitter.");
return false;
}

return validateDigestMatchesPreprepareBlock(msg.getUnsignedMessageData().getDigest(), msgType);
}

private boolean isMessageInRoundFromValidatorAndMatchesPreprepareDigest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name does look correct. The method does not perform any verification on the digest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed - updated.

final IbftSignedMessageData<? extends AbstractIbftUnsignedInRoundMessageData> msg,
final String msgType) {

if (!msg.getUnsignedMessageData().getRoundIdentifier().equals(roundIdentifier)) {
LOG.info("Invalid {} message, does not match current round.", msgType);
return false;
}

if (!validators.contains(msg.getSender())) {
LOG.info(
"Invalid {} message, was not transmitted by a validator for the " + "associated round.",
msgType);
return false;
}

if (!preprepareMessage.isPresent()) {
LOG.info(
"Unable to validate {} message. No Preprepare message exists against "
+ "which to validate block digest.",
msgType);
return false;
}
return true;
}

private boolean validateDigestMatchesPreprepareBlock(final Hash digest, final String msgType) {
final Block proposedBlock = preprepareMessage.get().getUnsignedMessageData().getBlock();
if (!digest.equals(proposedBlock.getHash())) {
LOG.info(
"Illegal {} message, digest does not match the block in the Prepare Message.", msgType);
return false;
}
return true;
}
}
Loading