Discard IBFT messages not targetting future chain height (#1575)

This is a stronger check than in the past, which potentially allowed for a the IBFT system to handle messages for the current round, even if a block had already been imported (but had not yet received the new-block event) - which in turn, could lead to a network fork in a adversarial environment.

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
pull/1592/head
Trent Mohay 4 years ago committed by GitHub
parent 0dd90d016d
commit a5df0fde2d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 4
      CHANGELOG.md
  2. 41
      consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/tests/LocalNodeIsProposerTest.java
  3. 5
      consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/tests/LocalNodeNotProposerTest.java
  4. 44
      consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftChainObserver.java
  5. 17
      consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java
  6. 90
      consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/IbftChainObserverTest.java
  7. 77
      consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftControllerTest.java

@ -5,6 +5,10 @@
### Additions and Improvements
* Added support for batched requests in WebSockets. [#1583](https://github.com/hyperledger/besu/pull/1583)
### Bug Fixes
* Ibft2 will discard any received messages targetting a chain height <= current head - this resolves some corner cases in system correctness directly following block import. [#1575](https://github.com/hyperledger/besu/pull/1575)
## 20.10.1
### Additions and Improvements

@ -19,6 +19,8 @@ import static org.hyperledger.besu.consensus.ibft.support.IntegrationTestHelpers
import org.hyperledger.besu.consensus.ibft.ConsensusRoundIdentifier;
import org.hyperledger.besu.consensus.ibft.ibftevent.BlockTimerExpiry;
import org.hyperledger.besu.consensus.ibft.ibftevent.NewChainHead;
import org.hyperledger.besu.consensus.ibft.ibftevent.RoundExpiry;
import org.hyperledger.besu.consensus.ibft.messagewrappers.Commit;
import org.hyperledger.besu.consensus.ibft.messagewrappers.Proposal;
import org.hyperledger.besu.consensus.ibft.payload.MessageFactory;
@ -110,4 +112,43 @@ public class LocalNodeIsProposerTest {
assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(1);
peers.verifyNoMessagesReceived();
}
@Test
public void nodeDoesNotSendRoundChangeIfRoundTimesOutAfterBlockImportButBeforeNewBlock() {
peers.verifyMessagesReceived(expectedTxProposal);
peers.getNonProposing(0).injectCommit(roundId, expectedProposedBlock.getHash());
assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(0);
peers.verifyNoMessagesReceived();
peers.getNonProposing(1).injectCommit(roundId, expectedProposedBlock.getHash());
assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(1);
peers.verifyNoMessagesReceived();
context.getController().handleRoundExpiry(new RoundExpiry(roundId));
peers.verifyNoMessagesReceived();
context
.getController()
.handleNewBlockEvent(new NewChainHead(expectedProposedBlock.getHeader()));
peers.verifyNoMessagesReceived();
}
@Test
public void nodeDoesNotSendCommitMessageAfterBlockIsImportedAndBeforeNewBlockEvent() {
peers.verifyMessagesReceived(expectedTxProposal);
peers.getNonProposing(0).injectCommit(roundId, expectedProposedBlock.getHash());
assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(0);
peers.verifyNoMessagesReceived();
peers.getNonProposing(1).injectCommit(roundId, expectedProposedBlock.getHash());
assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(1);
peers.verifyNoMessagesReceived();
peers.getNonProposing(0).injectPrepare(roundId, expectedProposedBlock.getHash());
peers.verifyNoMessagesReceived();
peers.getNonProposing(1).injectPrepare(roundId, expectedProposedBlock.getHash());
peers.verifyNoMessagesReceived();
peers.getNonProposing(2).injectPrepare(roundId, expectedProposedBlock.getHash());
peers.verifyNoMessagesReceived();
}
}

@ -138,7 +138,7 @@ public class LocalNodeNotProposerTest {
@Test
public void
fullQuorumOfCommitMessagesReceivedThenProposalImportsBlockCommitSentAfterFinalPrepare() {
canImportABlockIfSufficientCommitsReceivedWithoutPreparesAndThatNoPacketsSentAfterImport() {
peers.commit(roundId, blockToPropose.getHash());
peers.verifyNoMessagesReceived();
assertThat(context.getCurrentChainHeight()).isEqualTo(0);
@ -148,7 +148,6 @@ public class LocalNodeNotProposerTest {
assertThat(context.getCurrentChainHeight()).isEqualTo(1);
peers.getNonProposing(0).injectPrepare(roundId, blockToPropose.getHash());
peers.verifyMessagesReceived(expectedTxCommit);
assertThat(context.getCurrentChainHeight()).isEqualTo(1);
peers.verifyNoMessagesReceived();
}
}

@ -1,44 +0,0 @@
/*
* Copyright 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.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.consensus.ibft;
import org.hyperledger.besu.consensus.ibft.ibftevent.NewChainHead;
import org.hyperledger.besu.ethereum.chain.BlockAddedEvent;
import org.hyperledger.besu.ethereum.chain.BlockAddedObserver;
/**
* Blockchain observer that adds {@link NewChainHead} events to the event queue when a new block is
* added to the chain head
*/
public class IbftChainObserver implements BlockAddedObserver {
private final IbftEventQueue queue;
public IbftChainObserver(final IbftEventQueue queue) {
this.queue = queue;
}
@Override
public void onBlockAdded(final BlockAddedEvent event) {
switch (event.getEventType()) {
case HEAD_ADVANCED:
queue.add(new NewChainHead(event.getBlock().getHeader()));
break;
default:
throw new IllegalStateException(
String.format("Unexpected BlockAddedEvent received: %s", event.getEventType()));
}
}
}

@ -89,6 +89,7 @@ public class IbftController {
private void handleMessage(final Message message) {
final MessageData messageData = message.getData();
switch (messageData.getCode()) {
case IbftV2.PROPOSAL:
consumeMessage(
@ -129,6 +130,15 @@ public class IbftController {
private <P extends IbftMessage<?>> void consumeMessage(
final Message message, final P ibftMessage, final Consumer<P> handleMessage) {
LOG.trace("Received IBFT {} message", ibftMessage.getClass().getSimpleName());
// Discard all messages which target the BLOCKCHAIN height (which SHOULD be 1 less than
// the currentHeightManager, but CAN be the same directly following import).
if (ibftMessage.getRoundIdentifier().getSequenceNumber()
<= blockchain.getChainHeadBlockNumber()) {
LOG.debug("Discarding a message which targets a height not above current chain height.");
return;
}
if (processMessage(ibftMessage, message)) {
gossiper.send(message);
handleMessage.accept(ibftMessage);
@ -180,6 +190,13 @@ public class IbftController {
}
public void handleRoundExpiry(final RoundExpiry roundExpiry) {
// Discard all messages which target the BLOCKCHAIN height (which SHOULD be 1 less than
// the currentHeightManager, but CAN be the same directly following import).
if (roundExpiry.getView().getSequenceNumber() <= blockchain.getChainHeadBlockNumber()) {
LOG.debug("Discarding a round-expiry which targets a height not above current chain height.");
return;
}
if (isMsgForCurrentHeight(roundExpiry.getView())) {
currentHeightManager.roundExpired(roundExpiry);
} else {

@ -1,90 +0,0 @@
/*
* Copyright 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.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.consensus.ibft;
import static java.util.Collections.emptyList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import org.hyperledger.besu.consensus.ibft.ibftevent.IbftEvent;
import org.hyperledger.besu.consensus.ibft.ibftevent.NewChainHead;
import org.hyperledger.besu.ethereum.chain.BlockAddedEvent;
import org.hyperledger.besu.ethereum.core.Address;
import org.hyperledger.besu.ethereum.core.Block;
import org.hyperledger.besu.ethereum.core.BlockBody;
import org.hyperledger.besu.ethereum.core.BlockHeader;
import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
import org.hyperledger.besu.ethereum.core.Hash;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
public class IbftChainObserverTest {
@Test
public void newChainHeadHeaderEventIsAddedToTheQueue() {
final IbftEventQueue mockQueue = mock(IbftEventQueue.class);
final BlockAddedEvent mockBlockAddedEvent = mock(BlockAddedEvent.class);
final IbftChainObserver ibftChainObserver = new IbftChainObserver(mockQueue);
final BlockHeader header =
new BlockHeaderTestFixture()
.number(1234)
.coinbase(Address.ECREC)
.parentHash(Hash.EMPTY_LIST_HASH)
.buildHeader();
final Block block = new Block(header, new BlockBody(emptyList(), emptyList()));
when(mockBlockAddedEvent.getEventType()).thenReturn(BlockAddedEvent.EventType.HEAD_ADVANCED);
when(mockBlockAddedEvent.getBlock()).thenReturn(block);
ibftChainObserver.onBlockAdded(mockBlockAddedEvent);
final ArgumentCaptor<IbftEvent> ibftEventArgumentCaptor =
ArgumentCaptor.forClass(IbftEvent.class);
verify(mockQueue).add(ibftEventArgumentCaptor.capture());
assertThat(ibftEventArgumentCaptor.getValue() instanceof NewChainHead).isTrue();
assertThat(((NewChainHead) ibftEventArgumentCaptor.getValue()).getNewChainHeadHeader())
.isEqualTo(header);
}
@Test(expected = IllegalStateException.class)
public void exceptionIsThrownWhenEventTypeIsFork() {
final IbftEventQueue mockQueue = mock(IbftEventQueue.class);
final BlockAddedEvent mockBlockAddedEvent = mock(BlockAddedEvent.class);
when(mockBlockAddedEvent.getEventType()).thenReturn(BlockAddedEvent.EventType.FORK);
final IbftChainObserver ibftChainObserver = new IbftChainObserver(mockQueue);
ibftChainObserver.onBlockAdded(mockBlockAddedEvent);
}
@Test(expected = IllegalStateException.class)
public void exceptionIsThrownWhenEventTypeIsChainReorg() {
final IbftEventQueue mockQueue = mock(IbftEventQueue.class);
final BlockAddedEvent mockBlockAddedEvent = mock(BlockAddedEvent.class);
when(mockBlockAddedEvent.getEventType()).thenReturn(BlockAddedEvent.EventType.CHAIN_REORG);
final IbftChainObserver ibftChainObserver = new IbftChainObserver(mockQueue);
ibftChainObserver.onBlockAdded(mockBlockAddedEvent);
}
}

@ -87,8 +87,9 @@ public class IbftControllerTest {
@Mock private MessageTracker messageTracker;
private final Address validator = Address.fromHexString("0x0");
private final Address unknownValidator = Address.fromHexString("0x2");
private final ConsensusRoundIdentifier futureRoundIdentifier = new ConsensusRoundIdentifier(2, 0);
private ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(0, 0);
private final ConsensusRoundIdentifier futureRoundIdentifier = new ConsensusRoundIdentifier(5, 0);
private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(4, 0);
private final ConsensusRoundIdentifier pastRoundIdentifier = new ConsensusRoundIdentifier(3, 0);
@Mock private IbftGossip ibftGossip;
@Mock private FutureMessageBuffer futureMessageBuffer;
private IbftController ibftController;
@ -96,15 +97,17 @@ public class IbftControllerTest {
@Before
public void setup() {
when(blockChain.getChainHeadHeader()).thenReturn(chainHeadBlockHeader);
when(blockChain.getChainHeadBlockNumber()).thenReturn(3L);
when(blockHeightManagerFactory.create(any())).thenReturn(blockHeightManager);
when(ibftFinalState.getValidators()).thenReturn(ImmutableList.of(validator));
when(chainHeadBlockHeader.getNumber()).thenReturn(1L);
when(chainHeadBlockHeader.getNumber()).thenReturn(3L);
when(chainHeadBlockHeader.getHash()).thenReturn(Hash.ZERO);
when(blockHeightManager.getParentBlockHeader()).thenReturn(chainHeadBlockHeader);
when(blockHeightManager.getChainHeight()).thenReturn(4L); // one great than blockchain
when(nextBlock.getNumber()).thenReturn(2L);
when(nextBlock.getNumber()).thenReturn(5L);
when(ibftFinalState.isLocalNodeValidator()).thenReturn(true);
when(messageTracker.hasSeenMessage(any())).thenReturn(false);
@ -134,22 +137,22 @@ public class IbftControllerTest {
@Test
public void startsNewBlockHeightManagerAndReplaysFutureMessages() {
final ConsensusRoundIdentifier roundIdentifierHeight3 = new ConsensusRoundIdentifier(3, 0);
final ConsensusRoundIdentifier roundIdentifierHeight6 = new ConsensusRoundIdentifier(6, 0);
setupPrepare(futureRoundIdentifier, validator);
setupProposal(roundIdentifierHeight3, validator);
setupProposal(roundIdentifierHeight6, validator);
setupCommit(futureRoundIdentifier, validator);
setupRoundChange(futureRoundIdentifier, validator);
final List<Message> height2Msgs =
newArrayList(prepareMessage, commitMessage, roundChangeMessage);
when(blockHeightManager.getChainHeight()).thenReturn(2L);
when(futureMessageBuffer.retrieveMessagesForHeight(2L)).thenReturn(height2Msgs);
when(blockHeightManager.getChainHeight()).thenReturn(5L);
when(futureMessageBuffer.retrieveMessagesForHeight(5L)).thenReturn(height2Msgs);
constructIbftController();
ibftController.start();
verify(futureMessageBuffer).retrieveMessagesForHeight(2L);
verify(futureMessageBuffer, never()).retrieveMessagesForHeight(3L);
verify(futureMessageBuffer).retrieveMessagesForHeight(5L);
verify(futureMessageBuffer, never()).retrieveMessagesForHeight(6L);
verify(blockHeightManagerFactory).create(chainHeadBlockHeader);
verify(blockHeightManager, atLeastOnce()).getChainHeight();
verify(blockHeightManager, never()).handleProposalPayload(proposal);
@ -168,11 +171,11 @@ public class IbftControllerTest {
setupCommit(futureRoundIdentifier, validator);
setupRoundChange(futureRoundIdentifier, validator);
when(futureMessageBuffer.retrieveMessagesForHeight(2L))
when(futureMessageBuffer.retrieveMessagesForHeight(5L))
.thenReturn(
ImmutableList.of(prepareMessage, proposalMessage, commitMessage, roundChangeMessage))
.thenReturn(emptyList());
when(blockHeightManager.getChainHeight()).thenReturn(2L);
when(blockHeightManager.getChainHeight()).thenReturn(5L);
constructIbftController();
ibftController.start();
@ -181,7 +184,7 @@ public class IbftControllerTest {
verify(blockHeightManagerFactory).create(nextBlock);
verify(blockHeightManager, atLeastOnce()).getChainHeight();
verify(futureMessageBuffer, times(2)).retrieveMessagesForHeight(2L);
verify(futureMessageBuffer, times(2)).retrieveMessagesForHeight(5L);
verify(blockHeightManager).handleProposalPayload(proposal);
verify(ibftGossip).send(proposalMessage);
verify(blockHeightManager).handlePreparePayload(prepare);
@ -275,7 +278,6 @@ public class IbftControllerTest {
@Test
public void roundChangeForCurrentHeightIsPassedToBlockHeightManager() {
roundIdentifier = new ConsensusRoundIdentifier(0, 1);
setupRoundChange(roundIdentifier, validator);
constructIbftController();
ibftController.start();
@ -290,36 +292,31 @@ public class IbftControllerTest {
@Test
public void proposalForPastHeightIsDiscarded() {
setupProposal(roundIdentifier, validator);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
setupProposal(pastRoundIdentifier, validator);
verifyNotHandledAndNoFutureMsgs(new IbftReceivedMessageEvent(proposalMessage));
}
@Test
public void prepareForPastHeightIsDiscarded() {
setupPrepare(roundIdentifier, validator);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
setupPrepare(pastRoundIdentifier, validator);
verifyNotHandledAndNoFutureMsgs(new IbftReceivedMessageEvent(prepareMessage));
}
@Test
public void commitForPastHeightIsDiscarded() {
setupCommit(roundIdentifier, validator);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
setupCommit(pastRoundIdentifier, validator);
verifyNotHandledAndNoFutureMsgs(new IbftReceivedMessageEvent(commitMessage));
}
@Test
public void roundChangeForPastHeightIsDiscarded() {
setupRoundChange(roundIdentifier, validator);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
setupRoundChange(pastRoundIdentifier, validator);
verifyNotHandledAndNoFutureMsgs(new IbftReceivedMessageEvent(roundChangeMessage));
}
@Test
public void roundExpiryForPastHeightIsDiscarded() {
final RoundExpiry roundExpiry = new RoundExpiry(roundIdentifier);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
final RoundExpiry roundExpiry = new RoundExpiry(pastRoundIdentifier);
constructIbftController();
ibftController.start();
ibftController.handleRoundExpiry(roundExpiry);
@ -329,8 +326,7 @@ public class IbftControllerTest {
@Test
public void blockTimerForPastHeightIsDiscarded() {
final BlockTimerExpiry blockTimerExpiry = new BlockTimerExpiry(roundIdentifier);
when(blockHeightManager.getChainHeight()).thenReturn(1L);
final BlockTimerExpiry blockTimerExpiry = new BlockTimerExpiry(pastRoundIdentifier);
constructIbftController();
ibftController.start();
ibftController.handleBlockTimerExpiry(blockTimerExpiry);
@ -365,25 +361,25 @@ public class IbftControllerTest {
@Test
public void proposalForFutureHeightIsBuffered() {
setupProposal(futureRoundIdentifier, validator);
verifyHasFutureMessages(2L, proposalMessage);
verifyHasFutureMessages(futureRoundIdentifier.getSequenceNumber(), proposalMessage);
}
@Test
public void prepareForFutureHeightIsBuffered() {
setupPrepare(futureRoundIdentifier, validator);
verifyHasFutureMessages(2L, prepareMessage);
verifyHasFutureMessages(futureRoundIdentifier.getSequenceNumber(), prepareMessage);
}
@Test
public void commitForFutureHeightIsBuffered() {
setupCommit(futureRoundIdentifier, validator);
verifyHasFutureMessages(2L, commitMessage);
verifyHasFutureMessages(futureRoundIdentifier.getSequenceNumber(), commitMessage);
}
@Test
public void roundChangeForFutureHeightIsBuffered() {
setupRoundChange(futureRoundIdentifier, validator);
verifyHasFutureMessages(2L, roundChangeMessage);
verifyHasFutureMessages(futureRoundIdentifier.getSequenceNumber(), roundChangeMessage);
}
@Test
@ -405,6 +401,27 @@ public class IbftControllerTest {
verify(messageTracker).addSeenMessage(proposalMessageData);
}
@Test
public void messagesWhichAreAboveHeightManagerButBelowBlockChainLengthAreDiscarded() {
// NOTE: for this to occur, the system would need to be synchronising - i.e. blockchain is
// moving up faster than ibft loop is handling NewBlock messages
final long blockchainLength = 10L;
final long blockHeightManagerTargettingBlock = 6L;
final long messageHeight = 8L;
setupProposal(new ConsensusRoundIdentifier(messageHeight, 0), validator);
when(blockChain.getChainHeadHeader()).thenReturn(chainHeadBlockHeader);
when(blockChain.getChainHeadBlockNumber()).thenReturn(blockchainLength);
when(blockHeightManagerFactory.create(any())).thenReturn(blockHeightManager);
when(blockHeightManager.getChainHeight()).thenReturn(blockHeightManagerTargettingBlock);
constructIbftController();
ibftController.start();
ibftController.handleMessageEvent(new IbftReceivedMessageEvent(proposalMessage));
verify(futureMessageBuffer, never()).addMessage(anyLong(), any());
verify(blockHeightManager, never()).handleProposalPayload(any());
}
private void verifyNotHandledAndNoFutureMsgs(final IbftReceivedMessageEvent msg) {
constructIbftController();
ibftController.start();

Loading…
Cancel
Save