From 9c18af7c6b7fa117fd1ed5a2d2d8f3a287faadae Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 08:06:17 +1000 Subject: [PATCH 01/12] Round change manager implementation --- .../ibft/statemachine/RoundChangeManager.java | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java new file mode 100644 index 0000000000..281ec927fe --- /dev/null +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -0,0 +1,131 @@ +/* + * 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.statemachine; + +import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangeCertificate; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData; +import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidator; +import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidator.MessageValidatorFactory; +import tech.pegasys.pantheon.ethereum.core.Address; + +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +/** + * Responsible for handling all RoundChange messages received for a given block height + * (theoretically, RoundChange messages for a older height should have been previously discarded, + * and messages for a future round should have been buffered). + * + *

If enough RoundChange messages all targeting a given round are received (and this node is the + * proposer for said round) - a newRound message is sent, and a new round should be started by the + * controlling class. + */ +public class RoundChangeManager { + + public static class RoundChangeStatus { + private final int quorumSize; + private final List> receivedMessages = Lists.newArrayList(); + private boolean actioned = false; + + public RoundChangeStatus(final int quorumSize) { + this.quorumSize = quorumSize; + } + + public void addMessage(final SignedData msg) { + if (!actioned) { + receivedMessages.add(msg); + } + } + + public boolean roundChangeReady() { + return receivedMessages.size() >= quorumSize && !actioned; + } + + public RoundChangeCertificate createRoundChangeCertificate() { + if (roundChangeReady()) { + actioned = true; + return new RoundChangeCertificate(receivedMessages); + } else { + throw new IllegalStateException("Unable to create RoundChangeCertificate at this time."); + } + } + } + + private static final Logger LOG = LogManager.getLogger(); + private final Map roundChangeCache = + Maps.newHashMap(); + private final Collection

validators; + private final int quorumSize; + private final long chainHeight; + private final MessageValidatorFactory messageValidityFactory; + + public RoundChangeManager( + final int quorumSize, + final long currentChainHeight, + final IbftFinalState finalState, + final MessageValidatorFactory messageValidityFactory) { + this.quorumSize = quorumSize; + this.validators = finalState.getValidators(); + this.messageValidityFactory = messageValidityFactory; + this.chainHeight = currentChainHeight; + } + + public Optional appendRoundChangeMessage( + final SignedData msg) { + // validate the received msg + final ConsensusRoundIdentifier msgTargetRound = msg.getPayload().getRoundChangeIdentifier(); + final RoundChangeMessageValidator messageValidator = + new RoundChangeMessageValidator( + messageValidityFactory, validators, quorumSize, chainHeight); + if (messageValidator.validateMessage(msg)) { + LOG.info("RoundChange message was invalid."); + return Optional.empty(); + } + if (!roundChangeCache.containsKey(msgTargetRound)) { + roundChangeCache.put(msgTargetRound, new RoundChangeStatus(quorumSize)); + } + final RoundChangeStatus roundChangeStatus = roundChangeCache.get(msgTargetRound); + roundChangeStatus.addMessage(msg); + if (roundChangeStatus.roundChangeReady()) { + return Optional.of(roundChangeStatus.createRoundChangeCertificate()); + } + return Optional.empty(); + } + + /* Called when a given round has completed/timed-out such that msgs cached can be removed */ + public void discardCompletedRound(final ConsensusRoundIdentifier completedRoundIdentifier) { + List cacheKeysToRemove = Lists.newArrayList(); + for (final ConsensusRoundIdentifier msgRoundId : roundChangeCache.keySet()) { + if (isAnEarlierOrEqualRound(msgRoundId, completedRoundIdentifier)) { + cacheKeysToRemove.add(msgRoundId); + } + } + for (final ConsensusRoundIdentifier msgRoundId : cacheKeysToRemove) { + roundChangeCache.remove(msgRoundId); + } + } + + private boolean isAnEarlierOrEqualRound( + final ConsensusRoundIdentifier left, final ConsensusRoundIdentifier right) { + return left.getRoundNumber() <= right.getRoundNumber(); + } +} From 0bc8af5f6b42c81be5149fc76a314ac987207aa7 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 13:06:38 +1000 Subject: [PATCH 02/12] Tests for RoundChangeManager and fixes for both usage and bugs --- .../ibft/statemachine/IbftFinalState.java | 2 +- .../ibft/statemachine/RoundChangeManager.java | 33 ++-- .../statemachine/RoundChangeManagerTest.java | 159 ++++++++++++++++++ 3 files changed, 180 insertions(+), 14 deletions(-) create mode 100644 consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java index 8912800d15..f85137bed4 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java @@ -44,7 +44,7 @@ public class IbftFinalState { private final BlockHeaderValidator ibftContextBlockHeaderValidator; public IbftFinalState( - final VoteTally validatorProvider, + final ValidatorProvider validatorProvider, final KeyPair nodeKeys, final Address localAddress, final ProposerSelector proposerSelector, diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 281ec927fe..a7c27702b2 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -12,7 +12,11 @@ */ package tech.pegasys.pantheon.consensus.ibft.statemachine; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Sets; +import java.util.Set; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; +import tech.pegasys.pantheon.consensus.ibft.IbftHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangeCertificate; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData; @@ -43,7 +47,10 @@ public class RoundChangeManager { public static class RoundChangeStatus { private final int quorumSize; - private final List> receivedMessages = Lists.newArrayList(); + + @VisibleForTesting + final Map> receivedMessages = Maps.newHashMap(); + private boolean actioned = false; public RoundChangeStatus(final int quorumSize) { @@ -52,7 +59,7 @@ public RoundChangeStatus(final int quorumSize) { public void addMessage(final SignedData msg) { if (!actioned) { - receivedMessages.add(msg); + receivedMessages.put(msg.getSender(), msg); } } @@ -63,7 +70,7 @@ public boolean roundChangeReady() { public RoundChangeCertificate createRoundChangeCertificate() { if (roundChangeReady()) { actioned = true; - return new RoundChangeCertificate(receivedMessages); + return new RoundChangeCertificate(receivedMessages.values()); } else { throw new IllegalStateException("Unable to create RoundChangeCertificate at this time."); } @@ -71,22 +78,22 @@ public RoundChangeCertificate createRoundChangeCertificate() { } private static final Logger LOG = LogManager.getLogger(); - private final Map roundChangeCache = + @VisibleForTesting + final Map roundChangeCache = Maps.newHashMap(); private final Collection
validators; private final int quorumSize; - private final long chainHeight; + private final long sequenceNumber; private final MessageValidatorFactory messageValidityFactory; public RoundChangeManager( - final int quorumSize, - final long currentChainHeight, - final IbftFinalState finalState, + final long sequenceNumber, + final Collection
validators, final MessageValidatorFactory messageValidityFactory) { - this.quorumSize = quorumSize; - this.validators = finalState.getValidators(); + this.validators = validators; this.messageValidityFactory = messageValidityFactory; - this.chainHeight = currentChainHeight; + this.sequenceNumber = sequenceNumber; + this.quorumSize = IbftHelpers.calculateRequiredValidatorQuorum(validators.size()); } public Optional appendRoundChangeMessage( @@ -95,8 +102,8 @@ public Optional appendRoundChangeMessage( final ConsensusRoundIdentifier msgTargetRound = msg.getPayload().getRoundChangeIdentifier(); final RoundChangeMessageValidator messageValidator = new RoundChangeMessageValidator( - messageValidityFactory, validators, quorumSize, chainHeight); - if (messageValidator.validateMessage(msg)) { + messageValidityFactory, validators, quorumSize, sequenceNumber); + if (!messageValidator.validateMessage(msg)) { LOG.info("RoundChange message was invalid."); return Optional.empty(); } diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java new file mode 100644 index 0000000000..b4d5cdb58b --- /dev/null +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -0,0 +1,159 @@ +package tech.pegasys.pantheon.consensus.ibft.statemachine; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.junit.Before; +import org.junit.Test; +import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; +import tech.pegasys.pantheon.consensus.ibft.IbftContext; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData; +import tech.pegasys.pantheon.consensus.ibft.validation.MessageValidator; +import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair; +import tech.pegasys.pantheon.ethereum.ProtocolContext; +import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain; +import tech.pegasys.pantheon.ethereum.core.Address; +import tech.pegasys.pantheon.ethereum.core.BlockHeader; +import tech.pegasys.pantheon.ethereum.core.Util; +import tech.pegasys.pantheon.ethereum.db.WorldStateArchive; +import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator; + +public class RoundChangeManagerTest { + + private RoundChangeManager manager; + + private final KeyPair proposerKey = KeyPair.generate(); + private final KeyPair validator1Key = KeyPair.generate(); + private final KeyPair validator2Key = KeyPair.generate(); + private final KeyPair nonValidatorKey = KeyPair.generate(); + + private final ConsensusRoundIdentifier ri1 = new ConsensusRoundIdentifier(2, 1); + private final ConsensusRoundIdentifier ri2 = new ConsensusRoundIdentifier(2, 2); + private final ConsensusRoundIdentifier ri3 = new ConsensusRoundIdentifier(2, 3); + + @Before + public void setup() { + List
validators = Lists.newArrayList(); + + validators.add(Util.publicKeyToAddress(proposerKey.getPublicKey())); + validators.add(Util.publicKeyToAddress(validator1Key.getPublicKey())); + validators.add(Util.publicKeyToAddress(validator2Key.getPublicKey())); + + final ProtocolContext protocolContext = + new ProtocolContext<>( + mock(MutableBlockchain.class), mock(WorldStateArchive.class), mock(IbftContext.class)); + + @SuppressWarnings("unchecked") + BlockHeaderValidator headerValidator = + (BlockHeaderValidator) mock(BlockHeaderValidator.class); + BlockHeader parentHeader = mock(BlockHeader.class); + + Map messageValidators = Maps.newHashMap(); + + messageValidators.put( + ri1, + new MessageValidator( + validators, + Util.publicKeyToAddress(proposerKey.getPublicKey()), + ri1, + headerValidator, + protocolContext, + parentHeader)); + + messageValidators.put( + ri2, + new MessageValidator( + validators, + Util.publicKeyToAddress(validator1Key.getPublicKey()), + ri2, + headerValidator, + protocolContext, + parentHeader)); + + messageValidators.put( + ri3, + new MessageValidator( + validators, + Util.publicKeyToAddress(validator2Key.getPublicKey()), + ri3, + headerValidator, + protocolContext, + parentHeader)); + + manager = new RoundChangeManager(2, validators, messageValidators::get); + } + + private SignedData makeRoundChangeMessage(KeyPair key, ConsensusRoundIdentifier round) { + MessageFactory messageFactory = new MessageFactory(key); + return messageFactory.createSignedRoundChangePayload(round, Optional.empty()); + } + + @Test + public void rejectsInvalidRoundChangeMessage() { + SignedData roundChangeData = makeRoundChangeMessage(nonValidatorKey, ri1); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.roundChangeCache.get(ri1)).isNull(); + } + + @Test + public void acceptsValidRoundChangeMessage() { + SignedData roundChangeData = makeRoundChangeMessage(proposerKey, ri2); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); + } + + @Test + public void doesntAcceptDuplicateValidRoundChangeMessage() { + SignedData roundChangeData = makeRoundChangeMessage(proposerKey, ri2); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); + } + + @Test + public void becomesReadyAtThreshold() { + SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, + ri2); + SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, + ri2); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).satisfies( + Optional::isPresent); + } + + @Test + public void doesntReadyWithDifferentRounds() { + SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, + ri2); + SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, + ri3); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).isEqualTo(Optional.empty()); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); + assertThat(manager.roundChangeCache.get(ri3).receivedMessages.size()).isEqualTo(1); + } + + @Test + public void discardsPreviousRounds() { + SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, + ri1); + SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, + ri2); + SignedData roundChangeDataValidator2 = makeRoundChangeMessage(validator2Key, + ri3); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator2)).isEqualTo(Optional.empty()); + manager.discardCompletedRound(ri1); + assertThat(manager.roundChangeCache.get(ri1)).isNull(); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); + assertThat(manager.roundChangeCache.get(ri3).receivedMessages.size()).isEqualTo(1); + } +} From 5f898c38f1aa97a71175824379bd7d121b2e5ae3 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 13:34:28 +1000 Subject: [PATCH 03/12] spotless --- .../ibft/statemachine/IbftFinalState.java | 1 - .../ibft/statemachine/RoundChangeManager.java | 9 +-- .../statemachine/RoundChangeManagerTest.java | 81 ++++++++++++------- 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java index f85137bed4..0fbd7a2820 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftFinalState.java @@ -15,7 +15,6 @@ import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.calculateRequiredValidatorQuorum; import tech.pegasys.pantheon.consensus.common.ValidatorProvider; -import tech.pegasys.pantheon.consensus.common.VoteTally; import tech.pegasys.pantheon.consensus.ibft.BlockTimer; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftContext; diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index a7c27702b2..f5b158b20a 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -12,9 +12,6 @@ */ package tech.pegasys.pantheon.consensus.ibft.statemachine; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.Sets; -import java.util.Set; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangeCertificate; @@ -29,6 +26,7 @@ import java.util.Map; import java.util.Optional; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import org.apache.logging.log4j.LogManager; @@ -78,9 +76,10 @@ public RoundChangeCertificate createRoundChangeCertificate() { } private static final Logger LOG = LogManager.getLogger(); + @VisibleForTesting - final Map roundChangeCache = - Maps.newHashMap(); + final Map roundChangeCache = Maps.newHashMap(); + private final Collection
validators; private final int quorumSize; private final long sequenceNumber; diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java index b4d5cdb58b..74f36bf831 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -1,15 +1,20 @@ +/* + * 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.statemachine; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; -import com.google.common.collect.Lists; -import com.google.common.collect.Maps; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import org.junit.Before; -import org.junit.Test; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftContext; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory; @@ -25,6 +30,15 @@ import tech.pegasys.pantheon.ethereum.db.WorldStateArchive; import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import org.junit.Before; +import org.junit.Test; + public class RoundChangeManagerTest { private RoundChangeManager manager; @@ -90,7 +104,8 @@ public void setup() { manager = new RoundChangeManager(2, validators, messageValidators::get); } - private SignedData makeRoundChangeMessage(KeyPair key, ConsensusRoundIdentifier round) { + private SignedData makeRoundChangeMessage( + final KeyPair key, final ConsensusRoundIdentifier round) { MessageFactory messageFactory = new MessageFactory(key); return messageFactory.createSignedRoundChangePayload(round, Optional.empty()); } @@ -119,38 +134,44 @@ public void doesntAcceptDuplicateValidRoundChangeMessage() { @Test public void becomesReadyAtThreshold() { - SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, - ri2); - SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, - ri2); - assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).satisfies( - Optional::isPresent); + SignedData roundChangeDataProposer = + makeRoundChangeMessage(proposerKey, ri2); + SignedData roundChangeDataValidator1 = + makeRoundChangeMessage(validator1Key, ri2); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)) + .isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)) + .satisfies(Optional::isPresent); } @Test public void doesntReadyWithDifferentRounds() { - SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, - ri2); - SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, - ri3); - assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).isEqualTo(Optional.empty()); + SignedData roundChangeDataProposer = + makeRoundChangeMessage(proposerKey, ri2); + SignedData roundChangeDataValidator1 = + makeRoundChangeMessage(validator1Key, ri3); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)) + .isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)) + .isEqualTo(Optional.empty()); assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); assertThat(manager.roundChangeCache.get(ri3).receivedMessages.size()).isEqualTo(1); } @Test public void discardsPreviousRounds() { - SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, - ri1); - SignedData roundChangeDataValidator1 = makeRoundChangeMessage(validator1Key, - ri2); - SignedData roundChangeDataValidator2 = makeRoundChangeMessage(validator2Key, - ri3); - assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)).isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)).isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator2)).isEqualTo(Optional.empty()); + SignedData roundChangeDataProposer = + makeRoundChangeMessage(proposerKey, ri1); + SignedData roundChangeDataValidator1 = + makeRoundChangeMessage(validator1Key, ri2); + SignedData roundChangeDataValidator2 = + makeRoundChangeMessage(validator2Key, ri3); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)) + .isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)) + .isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator2)) + .isEqualTo(Optional.empty()); manager.discardCompletedRound(ri1); assertThat(manager.roundChangeCache.get(ri1)).isNull(); assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); From 0c5a37089034fc9e33543bdf2f65886ff753b326 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 14:20:20 +1000 Subject: [PATCH 04/12] changed roundchange discard to use a filter --- .../ibft/statemachine/RoundChangeManager.java | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index f5b158b20a..878ed8fe78 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -46,6 +46,7 @@ public class RoundChangeManager { public static class RoundChangeStatus { private final int quorumSize; + // Store only 1 round change per round per validator @VisibleForTesting final Map> receivedMessages = Maps.newHashMap(); @@ -119,15 +120,7 @@ public Optional appendRoundChangeMessage( /* Called when a given round has completed/timed-out such that msgs cached can be removed */ public void discardCompletedRound(final ConsensusRoundIdentifier completedRoundIdentifier) { - List cacheKeysToRemove = Lists.newArrayList(); - for (final ConsensusRoundIdentifier msgRoundId : roundChangeCache.keySet()) { - if (isAnEarlierOrEqualRound(msgRoundId, completedRoundIdentifier)) { - cacheKeysToRemove.add(msgRoundId); - } - } - for (final ConsensusRoundIdentifier msgRoundId : cacheKeysToRemove) { - roundChangeCache.remove(msgRoundId); - } + roundChangeCache.entrySet().removeIf(entry -> isAnEarlierOrEqualRound(entry.getKey(), completedRoundIdentifier)); } private boolean isAnEarlierOrEqualRound( From 29b894bac9b50b4a3905e0f42221a3aa26876048 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 14:22:22 +1000 Subject: [PATCH 05/12] change empty checks to isEmpty() --- .../ibft/statemachine/RoundChangeManagerTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java index 74f36bf831..7ce4680fcb 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -113,22 +113,22 @@ private SignedData makeRoundChangeMessage( @Test public void rejectsInvalidRoundChangeMessage() { SignedData roundChangeData = makeRoundChangeMessage(nonValidatorKey, ri1); - assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); assertThat(manager.roundChangeCache.get(ri1)).isNull(); } @Test public void acceptsValidRoundChangeMessage() { SignedData roundChangeData = makeRoundChangeMessage(proposerKey, ri2); - assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); } @Test public void doesntAcceptDuplicateValidRoundChangeMessage() { SignedData roundChangeData = makeRoundChangeMessage(proposerKey, ri2); - assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); } From c33aaf9ac6b556f81d33ba7a4fc487ba39b06aec Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 14:34:03 +1000 Subject: [PATCH 06/12] refactoring roundChangeManager to make smaller methods --- .../ibft/statemachine/RoundChangeManager.java | 51 ++++++++++++------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 878ed8fe78..9f25dcfdb5 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -27,7 +27,6 @@ import java.util.Optional; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -81,44 +80,62 @@ public RoundChangeCertificate createRoundChangeCertificate() { @VisibleForTesting final Map roundChangeCache = Maps.newHashMap(); - private final Collection
validators; private final int quorumSize; - private final long sequenceNumber; - private final MessageValidatorFactory messageValidityFactory; + private final RoundChangeMessageValidator roundChangeMessageValidator; public RoundChangeManager( final long sequenceNumber, final Collection
validators, final MessageValidatorFactory messageValidityFactory) { - this.validators = validators; - this.messageValidityFactory = messageValidityFactory; - this.sequenceNumber = sequenceNumber; this.quorumSize = IbftHelpers.calculateRequiredValidatorQuorum(validators.size()); + this.roundChangeMessageValidator = + new RoundChangeMessageValidator( + messageValidityFactory, validators, quorumSize, sequenceNumber); } + /** + * Adds the round message to this manager and return a certificate if it passes the threshold + * @param msg The signed round change message to add + * @return Empty if the round change threshold hasn't been hit, otherwise a round change certificate + */ public Optional appendRoundChangeMessage( final SignedData msg) { + // validate the received msg - final ConsensusRoundIdentifier msgTargetRound = msg.getPayload().getRoundChangeIdentifier(); - final RoundChangeMessageValidator messageValidator = - new RoundChangeMessageValidator( - messageValidityFactory, validators, quorumSize, sequenceNumber); - if (!messageValidator.validateMessage(msg)) { + if (! isMessageValid(msg)) { LOG.info("RoundChange message was invalid."); return Optional.empty(); } + + RoundChangeStatus roundChangeStatus = storeRoundChangeMessage(msg); + + if (roundChangeStatus.roundChangeReady()) { + return Optional.of(roundChangeStatus.createRoundChangeCertificate()); + } + + return Optional.empty(); + } + + private boolean isMessageValid(final SignedData msg) { + return roundChangeMessageValidator.validateMessage(msg); + } + + private RoundChangeStatus storeRoundChangeMessage(final SignedData msg) { + final ConsensusRoundIdentifier msgTargetRound = msg.getPayload().getRoundChangeIdentifier(); + if (!roundChangeCache.containsKey(msgTargetRound)) { roundChangeCache.put(msgTargetRound, new RoundChangeStatus(quorumSize)); } + final RoundChangeStatus roundChangeStatus = roundChangeCache.get(msgTargetRound); roundChangeStatus.addMessage(msg); - if (roundChangeStatus.roundChangeReady()) { - return Optional.of(roundChangeStatus.createRoundChangeCertificate()); - } - return Optional.empty(); + return roundChangeStatus; } - /* Called when a given round has completed/timed-out such that msgs cached can be removed */ + /** + * Clears old rounds from storage that have been superseded by a given round + * @param completedRoundIdentifier round identifier that has been identified as superseded + */ public void discardCompletedRound(final ConsensusRoundIdentifier completedRoundIdentifier) { roundChangeCache.entrySet().removeIf(entry -> isAnEarlierOrEqualRound(entry.getKey(), completedRoundIdentifier)); } From 5e92f7b9caef57c2640af4d1a732c60ac01aac72 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 14:35:48 +1000 Subject: [PATCH 07/12] test naming --- .../consensus/ibft/statemachine/RoundChangeManagerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java index 7ce4680fcb..05aca326d0 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -145,7 +145,7 @@ public void becomesReadyAtThreshold() { } @Test - public void doesntReadyWithDifferentRounds() { + public void doesntReachReadyWhenSuppliedWithDifferentRounds() { SignedData roundChangeDataProposer = makeRoundChangeMessage(proposerKey, ri2); SignedData roundChangeDataValidator1 = From 391868ffc45b4003f27ba8134f4623293ec56393 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 14:42:44 +1000 Subject: [PATCH 08/12] spotless --- .../ibft/statemachine/RoundChangeManager.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 9f25dcfdb5..5217340cc2 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -22,7 +22,6 @@ import tech.pegasys.pantheon.ethereum.core.Address; import java.util.Collection; -import java.util.List; import java.util.Map; import java.util.Optional; @@ -95,14 +94,16 @@ public RoundChangeManager( /** * Adds the round message to this manager and return a certificate if it passes the threshold + * * @param msg The signed round change message to add - * @return Empty if the round change threshold hasn't been hit, otherwise a round change certificate + * @return Empty if the round change threshold hasn't been hit, otherwise a round change + * certificate */ public Optional appendRoundChangeMessage( final SignedData msg) { // validate the received msg - if (! isMessageValid(msg)) { + if (!isMessageValid(msg)) { LOG.info("RoundChange message was invalid."); return Optional.empty(); } @@ -134,10 +135,13 @@ private RoundChangeStatus storeRoundChangeMessage(final SignedData isAnEarlierOrEqualRound(entry.getKey(), completedRoundIdentifier)); + roundChangeCache + .entrySet() + .removeIf(entry -> isAnEarlierOrEqualRound(entry.getKey(), completedRoundIdentifier)); } private boolean isAnEarlierOrEqualRound( From 5f0c7e8a0cdcd77be2fa945c4a43887a43f5b0c4 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Tue, 11 Dec 2018 15:34:42 +1000 Subject: [PATCH 09/12] refactor to use built in collection functions --- .../consensus/ibft/statemachine/RoundChangeManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 5217340cc2..1c2388c879 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -124,12 +124,12 @@ private boolean isMessageValid(final SignedData msg) { private RoundChangeStatus storeRoundChangeMessage(final SignedData msg) { final ConsensusRoundIdentifier msgTargetRound = msg.getPayload().getRoundChangeIdentifier(); - if (!roundChangeCache.containsKey(msgTargetRound)) { - roundChangeCache.put(msgTargetRound, new RoundChangeStatus(quorumSize)); - } + final RoundChangeStatus roundChangeStatus = + roundChangeCache.computeIfAbsent( + msgTargetRound, ignored -> new RoundChangeStatus(quorumSize)); - final RoundChangeStatus roundChangeStatus = roundChangeCache.get(msgTargetRound); roundChangeStatus.addMessage(msg); + return roundChangeStatus; } From 7045ca4dafe8816b801d371434f4cf1c704c055e Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Thu, 13 Dec 2018 10:10:29 +1000 Subject: [PATCH 10/12] Update consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java Co-Authored-By: Errorific --- .../consensus/ibft/statemachine/RoundChangeManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 1c2388c879..45cd34d29f 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -108,7 +108,7 @@ public Optional appendRoundChangeMessage( return Optional.empty(); } - RoundChangeStatus roundChangeStatus = storeRoundChangeMessage(msg); + final RoundChangeStatus roundChangeStatus = storeRoundChangeMessage(msg); if (roundChangeStatus.roundChangeReady()) { return Optional.of(roundChangeStatus.createRoundChangeCertificate()); From 161b2389e1b2d86daf9d89b101da892905371eb2 Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Thu, 13 Dec 2018 10:10:38 +1000 Subject: [PATCH 11/12] Update consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java Co-Authored-By: Errorific --- .../pantheon/consensus/ibft/statemachine/RoundChangeManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 45cd34d29f..8649ce47d7 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -102,7 +102,6 @@ public RoundChangeManager( public Optional appendRoundChangeMessage( final SignedData msg) { - // validate the received msg if (!isMessageValid(msg)) { LOG.info("RoundChange message was invalid."); return Optional.empty(); From 4a34c94353567a9f69b7ef8a7825f1d33b178455 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Thu, 13 Dec 2018 10:33:15 +1000 Subject: [PATCH 12/12] Test added that set is frozen after actioning --- .../statemachine/RoundChangeManagerTest.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java index 05aca326d0..a69752137f 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -140,8 +140,7 @@ public void becomesReadyAtThreshold() { makeRoundChangeMessage(validator1Key, ri2); assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)) .isEqualTo(Optional.empty()); - assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1)) - .satisfies(Optional::isPresent); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1).isPresent()).isTrue(); } @Test @@ -177,4 +176,21 @@ public void discardsPreviousRounds() { assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); assertThat(manager.roundChangeCache.get(ri3).receivedMessages.size()).isEqualTo(1); } + + @Test + public void stopsAcceptingMessagesAfterReady() { + SignedData roundChangeDataProposer = + makeRoundChangeMessage(proposerKey, ri2); + SignedData roundChangeDataValidator1 = + makeRoundChangeMessage(validator1Key, ri2); + SignedData roundChangeDataValidator2 = + makeRoundChangeMessage(validator2Key, ri2); + assertThat(manager.appendRoundChangeMessage(roundChangeDataProposer)) + .isEqualTo(Optional.empty()); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator1).isPresent()).isTrue(); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(2); + assertThat(manager.appendRoundChangeMessage(roundChangeDataValidator2)) + .isEqualTo(Optional.empty()); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(2); + } }