From 023445ff6b4b601e2770ba4ffa8814a2914b8a09 Mon Sep 17 00:00:00 2001 From: "stefan.pingel@consensys.net" Date: Fri, 25 Oct 2024 12:51:51 +1000 Subject: [PATCH] small fixes Signed-off-by: stefan.pingel@consensys.net --- .../peertask/task/GetBodiesFromPeerTask.java | 20 ++++----- .../tasks/CompleteBlocksWithPeerTask.java | 8 +--- .../task/GetBodiesFromPeerTaskTest.java | 42 ++++++++++------- .../tasks/CompleteBlocksWithPeerTaskTest.java | 45 +++++++++---------- 4 files changed, 58 insertions(+), 57 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTask.java index 3688cab107d..a73a4520445 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTask.java @@ -25,14 +25,12 @@ import org.hyperledger.besu.ethereum.eth.messages.GetBlockBodiesMessage; import org.hyperledger.besu.ethereum.mainnet.BodyValidation; import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; -import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import java.util.ArrayList; import java.util.List; import java.util.function.Predicate; -import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -42,19 +40,19 @@ public class GetBodiesFromPeerTask implements PeerTask> { private static final Logger LOG = LoggerFactory.getLogger(GetBodiesFromPeerTask.class); private final List blockHeaders; - private final Supplier currentProtocolSpecSupplier; private final ProtocolSchedule protocolSchedule; private final long requiredBlockchainHeight; private final List blocks = new ArrayList<>(); + private final boolean isPoS; public GetBodiesFromPeerTask( - final List blockHeaders, - final Supplier currentProtocolSpecSupplier, - final ProtocolSchedule protocolSchedule) { + final List blockHeaders, final ProtocolSchedule protocolSchedule) { + if (blockHeaders == null || blockHeaders.isEmpty()) { + throw new IllegalArgumentException("Block headers must not be empty"); + } this.blockHeaders = blockHeaders; - this.currentProtocolSpecSupplier = currentProtocolSpecSupplier; this.protocolSchedule = protocolSchedule; this.requiredBlockchainHeight = @@ -62,6 +60,7 @@ public GetBodiesFromPeerTask( .mapToLong(BlockHeader::getNumber) .max() .orElse(BlockHeader.GENESIS_BLOCK_NUMBER); + this.isPoS = protocolSchedule.getByBlockHeader(blockHeaders.getLast()).isPoS(); } @Override @@ -105,8 +104,7 @@ public List parseResponse(final MessageData messageData) private boolean blockBodyMatchesBlockHeader( final BlockBody blockBody, final BlockHeader blockHeader) { // this method validates that the block body matches the block header by calculating the roots - // of the block body - // and comparing them to the roots in the block header + // of the block body and comparing them to the roots in the block header if (!BodyValidation.transactionsRoot(blockBody.getTransactions()) .equals(blockHeader.getTransactionsRoot())) { return false; @@ -142,9 +140,7 @@ private boolean blockBodyMatchesBlockHeader( @Override public Predicate getPeerRequirementFilter() { return (ethPeer) -> - ethPeer.getProtocolName().equals(getSubProtocol().getName()) - && (currentProtocolSpecSupplier.get().isPoS() - || ethPeer.chainState().getEstimatedHeight() >= requiredBlockchainHeight); + isPoS || ethPeer.chainState().getEstimatedHeight() >= requiredBlockchainHeight; } @Override diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTask.java index dfc3405b8da..6c86ee7af78 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTask.java @@ -47,7 +47,7 @@ public class CompleteBlocksWithPeerTask { private final Block[] result; private final int resultSize; private int nextIndex = 0; - private int remainingBlocks = 0; + private int remainingBlocks; public CompleteBlocksWithPeerTask( final ProtocolSchedule protocolSchedule, @@ -97,11 +97,7 @@ public List getBlocks() { .setMessage("Requesting {} bodies from peer") .addArgument(headersToGet.size()) .log(); - final GetBodiesFromPeerTask task = - new GetBodiesFromPeerTask( - headersToGet, - () -> protocolSchedule.getByBlockHeader(headersToGet.getLast()), - protocolSchedule); + final GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(headersToGet, protocolSchedule); final PeerTaskExecutorResult> executionResult = peerTaskExecutor.execute(task); if (executionResult.responseCode() == PeerTaskExecutorResponseCode.SUCCESS && executionResult.result().isPresent()) { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTaskTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTaskTest.java index 463ea97908e..21b9ac59772 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTaskTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetBodiesFromPeerTaskTest.java @@ -33,6 +33,7 @@ import org.hyperledger.besu.ethereum.eth.messages.EthPV62; import org.hyperledger.besu.ethereum.eth.messages.GetBlockBodiesMessage; import org.hyperledger.besu.ethereum.mainnet.BodyValidation; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.rlp.BytesValueRLPInput; @@ -45,6 +46,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -60,10 +62,21 @@ public class GetBodiesFromPeerTaskTest { public static final List TRANSACTION_LIST = List.of(TX); public static final BlockBody BLOCK_BODY = new BlockBody(TRANSACTION_LIST, Collections.emptyList(), Optional.empty(), Optional.empty()); + private static ProtocolSchedule protocolSchedule; + + @BeforeAll + public static void setup() { + protocolSchedule = mock(ProtocolSchedule.class); + final ProtocolSpec protocolSpec = mock(ProtocolSpec.class); + when(protocolSpec.isPoS()).thenReturn(true); + when(protocolSchedule.getByBlockHeader(Mockito.any())).thenReturn(protocolSpec); + } @Test public void testGetSubProtocol() { - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(Collections.emptyList(), null, null); + + GetBodiesFromPeerTask task = + new GetBodiesFromPeerTask(List.of(mockBlockHeader(0)), protocolSchedule); Assertions.assertEquals(EthProtocol.get(), task.getSubProtocol()); } @@ -71,7 +84,7 @@ public void testGetSubProtocol() { public void testGetRequestMessage() { GetBodiesFromPeerTask task = new GetBodiesFromPeerTask( - List.of(mockBlockHeader(1), mockBlockHeader(2), mockBlockHeader(3)), null, null); + List.of(mockBlockHeader(1), mockBlockHeader(2), mockBlockHeader(3)), protocolSchedule); MessageData messageData = task.getRequestMessage(); GetBlockBodiesMessage getBlockBodiesMessage = GetBlockBodiesMessage.readFrom(messageData); @@ -93,13 +106,15 @@ public void testGetRequestMessage() { @Test public void testParseResponseWithNullResponseMessage() { - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(Collections.emptyList(), null, null); - Assertions.assertThrows(InvalidPeerTaskResponseException.class, () -> task.parseResponse(null)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> new GetBodiesFromPeerTask(Collections.emptyList(), protocolSchedule)); } @Test public void testParseResponseForInvalidResponse() { - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(List.of(mockBlockHeader(1)), null, null); + GetBodiesFromPeerTask task = + new GetBodiesFromPeerTask(List.of(mockBlockHeader(1)), protocolSchedule); // body does not match header BlockBodiesMessage bodiesMessage = BlockBodiesMessage.create(List.of(BLOCK_BODY)); @@ -113,7 +128,7 @@ public void testParseResponse() throws InvalidPeerTaskResponseException { getNonEmptyBlockHeaderMock(BodyValidation.transactionsRoot(TRANSACTION_LIST).toString()); GetBodiesFromPeerTask task = - new GetBodiesFromPeerTask(List.of(nonEmptyBlockHeaderMock), null, null); + new GetBodiesFromPeerTask(List.of(nonEmptyBlockHeaderMock), protocolSchedule); final BlockBodiesMessage blockBodiesMessage = BlockBodiesMessage.create(List.of(BLOCK_BODY)); @@ -129,32 +144,27 @@ public void testGetPeerRequirementFilter() { BlockHeader blockHeader2 = mockBlockHeader(2); BlockHeader blockHeader3 = mockBlockHeader(3); - ProtocolSpec protocolSpec = Mockito.mock(ProtocolSpec.class); - Mockito.when(protocolSpec.isPoS()).thenReturn(false); - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask( - List.of(blockHeader1, blockHeader2, blockHeader3), () -> protocolSpec, null); + List.of(blockHeader1, blockHeader2, blockHeader3), protocolSchedule); - EthPeer failForIncorrectProtocol = mockPeer("incorrectProtocol", 5); - EthPeer failForShortChainHeight = mockPeer("incorrectProtocol", 1); EthPeer successfulCandidate = mockPeer(EthProtocol.NAME, 5); - Assertions.assertFalse(task.getPeerRequirementFilter().test(failForIncorrectProtocol)); - Assertions.assertFalse(task.getPeerRequirementFilter().test(failForShortChainHeight)); Assertions.assertTrue(task.getPeerRequirementFilter().test(successfulCandidate)); } @Test public void testIsSuccessForPartialSuccess() { - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(Collections.emptyList(), null, null); + GetBodiesFromPeerTask task = + new GetBodiesFromPeerTask(List.of(mockBlockHeader(1)), protocolSchedule); Assertions.assertFalse(task.isSuccess(Collections.emptyList())); } @Test public void testIsSuccessForFullSuccess() { - GetBodiesFromPeerTask task = new GetBodiesFromPeerTask(Collections.emptyList(), null, null); + GetBodiesFromPeerTask task = + new GetBodiesFromPeerTask(List.of(mockBlockHeader(1)), protocolSchedule); final List blockHeaders = List.of(mock(Block.class)); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTaskTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTaskTest.java index ed6a451df28..d94dd7615b3 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTaskTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksWithPeerTaskTest.java @@ -63,10 +63,10 @@ public void shouldReturnEmptyBlock() { CompleteBlocksWithPeerTask completeBlocksWithPeerTask = new CompleteBlocksWithPeerTask(protocolSchedule, List.of(blockHeader), peerTaskExecutor); - assertThat(completeBlocksWithPeerTask.getBlocks()).isNotEmpty(); - assertThat(completeBlocksWithPeerTask.getBlocks().size()).isEqualTo(1); - assertThat(BlockHeader.hasEmptyBlock(completeBlocksWithPeerTask.getBlocks().get(0).getHeader())) - .isTrue(); + final List blocks = completeBlocksWithPeerTask.getBlocks(); + assertThat(blocks).isNotEmpty(); + assertThat(blocks.size()).isEqualTo(1); + assertThat(BlockHeader.hasEmptyBlock(blocks.get(0).getHeader())).isTrue(); verify(peerTaskExecutor, Mockito.never()).execute(any()); } @@ -120,9 +120,10 @@ public void shouldReturnNonEmptyBlock() { new CompleteBlocksWithPeerTask( protocolSchedule, List.of(nonEmptyBlockHeaderMock), peerTaskExecutor); - assertThat(completeBlocksWithPeerTask.getBlocks()).isNotEmpty(); - assertThat(completeBlocksWithPeerTask.getBlocks().size()).isEqualTo(1); - assertThat(completeBlocksWithPeerTask.getBlocks().get(0)).isEqualTo(block); + final List blocks = completeBlocksWithPeerTask.getBlocks(); + assertThat(blocks).isNotEmpty(); + assertThat(blocks.size()).isEqualTo(1); + assertThat(blocks.get(0)).isEqualTo(block); } @Test @@ -150,14 +151,13 @@ public void shouldReturnBlocksInRightOrderWhenEmptyAndNonEmptyBlocksRequested() emptyBlockHeaderMock), peerTaskExecutor); - assertThat(completeBlocksWithPeerTask.getBlocks()).isNotEmpty(); - assertThat(completeBlocksWithPeerTask.getBlocks().size()).isEqualTo(4); - assertThat(completeBlocksWithPeerTask.getBlocks().get(0)).isEqualTo(block1); - assertThat(BlockHeader.hasEmptyBlock(completeBlocksWithPeerTask.getBlocks().get(1).getHeader())) - .isTrue(); - assertThat(completeBlocksWithPeerTask.getBlocks().get(2)).isEqualTo(block3); - assertThat(BlockHeader.hasEmptyBlock(completeBlocksWithPeerTask.getBlocks().get(3).getHeader())) - .isTrue(); + final List blocks = completeBlocksWithPeerTask.getBlocks(); + assertThat(blocks).isNotEmpty(); + assertThat(blocks.size()).isEqualTo(4); + assertThat(blocks.get(0)).isEqualTo(block1); + assertThat(BlockHeader.hasEmptyBlock(blocks.get(1).getHeader())).isTrue(); + assertThat(blocks.get(2)).isEqualTo(block3); + assertThat(BlockHeader.hasEmptyBlock(blocks.get(3).getHeader())).isTrue(); } @Test @@ -188,14 +188,13 @@ public void shouldRequestMoreBodiesUntilFinished() { emptyBlockHeaderMock), peerTaskExecutor); - assertThat(completeBlocksWithPeerTask.getBlocks()).isNotEmpty(); - assertThat(completeBlocksWithPeerTask.getBlocks().size()).isEqualTo(4); - assertThat(completeBlocksWithPeerTask.getBlocks().get(0)).isEqualTo(block1); - assertThat(BlockHeader.hasEmptyBlock(completeBlocksWithPeerTask.getBlocks().get(1).getHeader())) - .isTrue(); - assertThat(completeBlocksWithPeerTask.getBlocks().get(2)).isEqualTo(block3); - assertThat(BlockHeader.hasEmptyBlock(completeBlocksWithPeerTask.getBlocks().get(3).getHeader())) - .isTrue(); + final List blocks = completeBlocksWithPeerTask.getBlocks(); + assertThat(blocks).isNotEmpty(); + assertThat(blocks.size()).isEqualTo(4); + assertThat(blocks.get(0)).isEqualTo(block1); + assertThat(BlockHeader.hasEmptyBlock(blocks.get(1).getHeader())).isTrue(); + assertThat(blocks.get(2)).isEqualTo(block3); + assertThat(BlockHeader.hasEmptyBlock(blocks.get(3).getHeader())).isTrue(); } private static ProtocolSchedule getProtocolScheduleMock() {