From 5dea2488ac290b29afac8fc275d9879bbd5cc1ae Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Wed, 28 Sep 2022 18:51:12 +0200 Subject: [PATCH 1/7] Return the best block built until now instead of the last one Signed-off-by: Fabio Di Fabio --- .../consensus/merge/PostMergeContext.java | 37 +++++++++++-- .../merge/blockcreation/MergeCoordinator.java | 9 ++-- .../blockcreation/MergeMiningCoordinator.java | 2 +- .../blockcreation/PayloadIdentifier.java | 14 ++++- .../blockcreation/TransitionCoordinator.java | 4 +- .../consensus/merge/PostMergeContextTest.java | 53 ++++++++++++++++--- .../blockcreation/PayloadIdentifierTest.java | 6 ++- .../engine/EngineForkchoiceUpdatedTest.java | 6 ++- .../methods/engine/EngineGetPayloadTest.java | 9 +++- 9 files changed, 117 insertions(+), 23 deletions(-) diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java index 6923419ff8f..580fc4229a8 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java @@ -23,6 +23,8 @@ import org.hyperledger.besu.ethereum.eth.sync.state.SyncState; import org.hyperledger.besu.util.Subscribers; +import java.util.Comparator; +import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; @@ -36,6 +38,9 @@ public class PostMergeContext implements MergeContext { private static final AtomicReference singleton = new AtomicReference<>(); + private static final Comparator compareByGasUsedDesc = + Comparator.comparingLong((Block block) -> block.getHeader().getGasUsed()).reversed(); + private final AtomicReference syncState; private final AtomicReference terminalTotalDifficulty; // initial postMerge state is indeterminate until it is set: @@ -198,14 +203,38 @@ public boolean validateCandidateHead(final BlockHeader candidateHeader) { @Override public void putPayloadById(final PayloadIdentifier payloadId, final Block block) { - var priorsById = retrieveTuplesById(payloadId).collect(Collectors.toUnmodifiableList()); - blocksInProgress.add(new PayloadTuple(payloadId, block)); - priorsById.stream().forEach(blocksInProgress::remove); + synchronized (blocksInProgress) { + final List prevBlockTuples = + retrieveTuplesById(payloadId).collect(Collectors.toUnmodifiableList()); + + if (prevBlockTuples.isEmpty()) { + blocksInProgress.add(new PayloadTuple(payloadId, block)); + return; + } + + final Block currBestBlock = + Stream.concat( + Stream.of(block), + prevBlockTuples.stream().map(payloadTuple -> payloadTuple.block)) + .sorted(compareByGasUsedDesc) + .findFirst() + .get(); + + if (currBestBlock.getHash().equals(block.getHash())) { + prevBlockTuples.forEach(blocksInProgress::remove); + blocksInProgress.add(new PayloadTuple(payloadId, block)); + } + } } @Override public Optional retrieveBlockById(final PayloadIdentifier payloadId) { - return retrieveTuplesById(payloadId).map(tuple -> tuple.block).findFirst(); + synchronized (blocksInProgress) { + return retrieveTuplesById(payloadId) + .map(tuple -> tuple.block) + .sorted(compareByGasUsedDesc) + .findFirst(); + } } private Stream retrieveTuplesById(final PayloadIdentifier payloadId) { diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java index 3e75367cb87..315bbeca97d 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java @@ -175,17 +175,18 @@ public void changeTargetGasLimit(final Long newTargetGasLimit) { public PayloadIdentifier preparePayload( final BlockHeader parentHeader, final Long timestamp, - final Bytes32 random, + final Bytes32 prevRandao, final Address feeRecipient) { final PayloadIdentifier payloadIdentifier = - PayloadIdentifier.forPayloadParams(parentHeader.getBlockHash(), timestamp); + PayloadIdentifier.forPayloadParams( + parentHeader.getBlockHash(), timestamp, prevRandao, feeRecipient); final MergeBlockCreator mergeBlockCreator = this.mergeBlockCreator.forParams(parentHeader, Optional.ofNullable(feeRecipient)); // put the empty block in first final Block emptyBlock = - mergeBlockCreator.createBlock(Optional.of(Collections.emptyList()), random, timestamp); + mergeBlockCreator.createBlock(Optional.of(Collections.emptyList()), prevRandao, timestamp); Result result = validateBlock(emptyBlock); if (result.blockProcessingOutputs.isPresent()) { @@ -202,7 +203,7 @@ public PayloadIdentifier preparePayload( result.errorMessage); } - tryToBuildBetterBlock(timestamp, random, payloadIdentifier, mergeBlockCreator); + tryToBuildBetterBlock(timestamp, prevRandao, payloadIdentifier, mergeBlockCreator); return payloadIdentifier; } diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java index d83d16eb2c3..a09a2ac4eaa 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java @@ -33,7 +33,7 @@ public interface MergeMiningCoordinator extends MiningCoordinator { PayloadIdentifier preparePayload( final BlockHeader parentHeader, final Long timestamp, - final Bytes32 random, + final Bytes32 prevRandao, final Address feeRecipient); @Override diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifier.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifier.java index db647704644..a8568fa560e 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifier.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifier.java @@ -14,6 +14,7 @@ */ package org.hyperledger.besu.consensus.merge.blockcreation; +import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.plugin.data.Quantity; @@ -21,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonValue; +import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.units.bigints.UInt64; public class PayloadIdentifier implements Quantity { @@ -36,8 +38,16 @@ public PayloadIdentifier(final Long payloadId) { this.val = UInt64.valueOf(Math.abs(payloadId)); } - public static PayloadIdentifier forPayloadParams(final Hash parentHash, final Long timestamp) { - return new PayloadIdentifier(((long) parentHash.toHexString().hashCode()) ^ timestamp); + public static PayloadIdentifier forPayloadParams( + final Hash parentHash, + final Long timestamp, + final Bytes32 prevRandao, + final Address feeRecipient) { + return new PayloadIdentifier( + timestamp + ^ ((long) parentHash.toHexString().hashCode()) << 8 + ^ ((long) prevRandao.toHexString().hashCode()) << 16 + ^ ((long) feeRecipient.toHexString().hashCode()) << 24); } @Override diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java index e8d968e17ea..4bd465e81da 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java @@ -127,9 +127,9 @@ public void changeTargetGasLimit(final Long targetGasLimit) { public PayloadIdentifier preparePayload( final BlockHeader parentHeader, final Long timestamp, - final Bytes32 random, + final Bytes32 prevRandao, final Address feeRecipient) { - return mergeCoordinator.preparePayload(parentHeader, timestamp, random, feeRecipient); + return mergeCoordinator.preparePayload(parentHeader, timestamp, prevRandao, feeRecipient); } @Override diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java index 5f022df5f0e..b0bef634ca9 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.when; import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Difficulty; @@ -131,6 +132,7 @@ public void candidateHeadIsInvalidIfBeforeFinalizedBlock() { @Test public void putAndRetrieveFirstPayload() { Block mockBlock = mock(Block.class); + PayloadIdentifier firstPayloadId = new PayloadIdentifier(1L); postMergeContext.putPayloadById(firstPayloadId, mockBlock); @@ -138,14 +140,53 @@ public void putAndRetrieveFirstPayload() { } @Test - public void puttingTwoBlocksWithTheSamePayloadIdWeRetrieveTheLatter() { - Block mockBlock1 = mock(Block.class); - Block mockBlock2 = mock(Block.class); + public void puttingTwoBlocksWithTheSamePayloadIdWeRetrieveTheBest() { + BlockHeader zeroTxBlockHeader = mock(BlockHeader.class); + when(zeroTxBlockHeader.getGasUsed()).thenReturn(0L); + Block zeroTxBlock = mock(Block.class); + when(zeroTxBlock.getHeader()).thenReturn(zeroTxBlockHeader); + + Hash betterBlockHash = Hash.fromHexStringLenient("0xBE51"); + BlockHeader betterBlockHeader = mock(BlockHeader.class); + when(betterBlockHeader.getGasUsed()).thenReturn(11L); + Block betterBlock = mock(Block.class); + when(betterBlock.getHeader()).thenReturn(betterBlockHeader); + when(betterBlock.getHash()).thenReturn(betterBlockHash); + + PayloadIdentifier payloadId = new PayloadIdentifier(1L); + postMergeContext.putPayloadById(payloadId, zeroTxBlock); + postMergeContext.putPayloadById(payloadId, betterBlock); + + assertThat(postMergeContext.retrieveBlockById(payloadId)).contains(betterBlock); + } + + @Test + public void puttingABlockWithTheSamePayloadIdSmallerThanAnExistingOneWeRetrieveTheBest() { + BlockHeader zeroTxBlockHeader = mock(BlockHeader.class); + when(zeroTxBlockHeader.getGasUsed()).thenReturn(0L); + Block zeroTxBlock = mock(Block.class); + when(zeroTxBlock.getHeader()).thenReturn(zeroTxBlockHeader); + + Hash betterBlockHash = Hash.fromHexStringLenient("0xBE51"); + BlockHeader betterBlockHeader = mock(BlockHeader.class); + when(betterBlockHeader.getGasUsed()).thenReturn(11L); + Block betterBlock = mock(Block.class); + when(betterBlock.getHeader()).thenReturn(betterBlockHeader); + when(betterBlock.getHash()).thenReturn(betterBlockHash); + + Hash smallBlockHash = Hash.fromHexStringLenient("0x5011"); + BlockHeader smallBlockHeader = mock(BlockHeader.class); + when(smallBlockHeader.getGasUsed()).thenReturn(5L); + Block smallBlock = mock(Block.class); + when(smallBlock.getHeader()).thenReturn(smallBlockHeader); + when(smallBlock.getHash()).thenReturn(smallBlockHash); + PayloadIdentifier payloadId = new PayloadIdentifier(1L); - postMergeContext.putPayloadById(payloadId, mockBlock1); - postMergeContext.putPayloadById(payloadId, mockBlock2); + postMergeContext.putPayloadById(payloadId, zeroTxBlock); + postMergeContext.putPayloadById(payloadId, betterBlock); + postMergeContext.putPayloadById(payloadId, smallBlock); - assertThat(postMergeContext.retrieveBlockById(payloadId)).contains(mockBlock2); + assertThat(postMergeContext.retrieveBlockById(payloadId)).contains(betterBlock); } @Test diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java index 9203db85d4d..1c19ead4e33 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java @@ -16,10 +16,12 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Hash; import java.util.List; +import org.apache.tuweni.bytes.Bytes32; import org.junit.Test; public class PayloadIdentifierTest { @@ -39,7 +41,9 @@ public void serializesToEvenHexRepresentation() { @Test public void conversionCoverage() { - var idTest = PayloadIdentifier.forPayloadParams(Hash.ZERO, 1337L); + var idTest = + PayloadIdentifier.forPayloadParams( + Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42")); assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest); assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest); } diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java index e19fa6de2b7..f2351d48853 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java @@ -227,7 +227,11 @@ public void shouldReturnValidWithoutFinalizedWithPayload() { Bytes32.fromHexStringLenient("0xDEADBEEF").toHexString(), Address.ECREC.toString()); var mockPayloadId = - PayloadIdentifier.forPayloadParams(mockHeader.getHash(), payloadParams.getTimestamp()); + PayloadIdentifier.forPayloadParams( + mockHeader.getHash(), + payloadParams.getTimestamp(), + payloadParams.getPrevRandao(), + payloadParams.getSuggestedFeeRecipient()); when(mergeCoordinator.preparePayload( mockHeader, payloadParams.getTimestamp(), payloadParams.getPrevRandao(), Address.ECREC)) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadTest.java index 4695bbf4c7e..0d82d196e3f 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadTest.java @@ -21,6 +21,7 @@ import org.hyperledger.besu.consensus.merge.MergeContext; import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier; +import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; @@ -55,7 +56,8 @@ public class EngineGetPayloadTest { private static final Vertx vertx = Vertx.vertx(); private static final BlockResultFactory factory = new BlockResultFactory(); private static final PayloadIdentifier mockPid = - PayloadIdentifier.forPayloadParams(Hash.ZERO, 1337L); + PayloadIdentifier.forPayloadParams( + Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42")); private static final BlockHeader mockHeader = new BlockHeaderTestFixture().prevRandao(Bytes32.random()).buildHeader(); private static final Block mockBlock = @@ -99,7 +101,10 @@ public void shouldReturnBlockForKnownPayloadId() { @Test public void shouldFailForUnknownPayloadId() { - var resp = resp(PayloadIdentifier.forPayloadParams(Hash.ZERO, 0L)); + var resp = + resp( + PayloadIdentifier.forPayloadParams( + Hash.ZERO, 0L, Bytes32.random(), Address.fromHexString("0x42"))); assertThat(resp).isInstanceOf(JsonRpcErrorResponse.class); verify(engineCallListener, times(1)).executionEngineCalled(); } From 2fd6cb44174deea338e5234d97a327e75c02b28a Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Wed, 28 Sep 2022 18:58:01 +0200 Subject: [PATCH 2/7] Log forkchoice parameters at debug level Signed-off-by: Fabio Di Fabio --- .../methods/engine/EngineForkchoiceUpdated.java | 2 ++ .../parameters/EngineForkchoiceUpdatedParameter.java | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java index 244d4773799..1796ca63db1 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java @@ -72,6 +72,8 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) final Optional maybePayloadAttributes = requestContext.getOptionalParameter(1, EnginePayloadAttributesParameter.class); + LOG.debug("Forkchoice parameters {}", forkChoice); + Optional maybeFinalizedHash = Optional.ofNullable(forkChoice.getFinalizedBlockHash()) .filter(finalized -> !finalized.isZero()); diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/EngineForkchoiceUpdatedParameter.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/EngineForkchoiceUpdatedParameter.java index b6d70420fe5..7b3e50a07e1 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/EngineForkchoiceUpdatedParameter.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/EngineForkchoiceUpdatedParameter.java @@ -47,4 +47,14 @@ public EngineForkchoiceUpdatedParameter( this.headBlockHash = headBlockHash; this.safeBlockHash = safeBlockHash; } + + @Override + public String toString() { + return "headBlockHash=" + + headBlockHash + + ", safeBlockHash=" + + safeBlockHash + + ", finalizedBlockHash=" + + finalizedBlockHash; + } } From 64c2222a3ba591f8dae693dbb570642a53dae989 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Wed, 28 Sep 2022 20:49:49 +0200 Subject: [PATCH 3/7] Log current best block proposal at debug level Signed-off-by: Fabio Di Fabio --- .../consensus/merge/PostMergeContext.java | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java index 580fc4229a8..a3d377d2ec6 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.consensus.merge; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.debugLambda; + import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.ConsensusContext; @@ -32,8 +34,11 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.EvictingQueue; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class PostMergeContext implements MergeContext { + private static final Logger LOG = LoggerFactory.getLogger(PostMergeContext.class); static final int MAX_BLOCKS_IN_PROGRESS = 12; private static final AtomicReference singleton = new AtomicReference<>(); @@ -209,21 +214,26 @@ public void putPayloadById(final PayloadIdentifier payloadId, final Block block) if (prevBlockTuples.isEmpty()) { blocksInProgress.add(new PayloadTuple(payloadId, block)); - return; - } - - final Block currBestBlock = - Stream.concat( - Stream.of(block), - prevBlockTuples.stream().map(payloadTuple -> payloadTuple.block)) - .sorted(compareByGasUsedDesc) - .findFirst() - .get(); - - if (currBestBlock.getHash().equals(block.getHash())) { - prevBlockTuples.forEach(blocksInProgress::remove); - blocksInProgress.add(new PayloadTuple(payloadId, block)); + } else { + final Block currBestBlock = + Stream.concat( + Stream.of(block), + prevBlockTuples.stream().map(payloadTuple -> payloadTuple.block)) + .sorted(compareByGasUsedDesc) + .findFirst() + .get(); + + if (currBestBlock.getHash().equals(block.getHash())) { + debugLambda(LOG, ""); + prevBlockTuples.forEach(blocksInProgress::remove); + blocksInProgress.add(new PayloadTuple(payloadId, block)); + } } + debugLambda( + LOG, + "Current best proposal for payloadId {} {}", + payloadId::toHexString, + () -> retrieveBlockById(payloadId).map(bb -> logBlockProposal(bb)).orElse("N/A")); } } @@ -241,6 +251,15 @@ private Stream retrieveTuplesById(final PayloadIdentifier payloadI return blocksInProgress.stream().filter(z -> z.payloadIdentifier.equals(payloadId)); } + private String logBlockProposal(final Block block) { + return "block " + + block.toLogString() + + " gas used " + + block.getHeader().getGasUsed() + + " transactions " + + block.getBody().getTransactions().size(); + } + private static class PayloadTuple { final PayloadIdentifier payloadIdentifier; final Block block; From d35e7e5ae0271077ccf725c08da440d293c71a4c Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Wed, 28 Sep 2022 21:21:16 +0200 Subject: [PATCH 4/7] Update acceptance tests Signed-off-by: Fabio Di Fabio --- .../resources/jsonrpc/engine/test-cases/01_prepare_payload.json | 2 +- .../resources/jsonrpc/engine/test-cases/02_get_payload.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/01_prepare_payload.json b/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/01_prepare_payload.json index 543e12d3c7d..3ab6f968fbc 100644 --- a/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/01_prepare_payload.json +++ b/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/01_prepare_payload.json @@ -25,7 +25,7 @@ "latestValidHash": "0x3b8fb240d288781d4aac94d3fd16809ee413bc99294a085798a589dae51ddd4a", "validationError": null }, - "payloadId": "0x0000000021f32cc1" + "payloadId": "0x0065bd195a9b3bfb" } }, "statusCode" : 200 diff --git a/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/02_get_payload.json b/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/02_get_payload.json index dbb97e518dd..e5257090527 100644 --- a/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/02_get_payload.json +++ b/acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/02_get_payload.json @@ -3,7 +3,7 @@ "jsonrpc": "2.0", "method": "engine_getPayloadV1", "params": [ - "0x0000000021f32cc1" + "0x0065bd195a9b3bfb" ], "id": 67 }, From 43ce4b449dadebea2dde520890c3317847e333f8 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Thu, 29 Sep 2022 08:24:18 +0200 Subject: [PATCH 5/7] Remove unneeded log Signed-off-by: Fabio Di Fabio --- .../org/hyperledger/besu/consensus/merge/PostMergeContext.java | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java index a3d377d2ec6..22f4762b16f 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java @@ -224,7 +224,6 @@ public void putPayloadById(final PayloadIdentifier payloadId, final Block block) .get(); if (currBestBlock.getHash().equals(block.getHash())) { - debugLambda(LOG, ""); prevBlockTuples.forEach(blocksInProgress::remove); blocksInProgress.add(new PayloadTuple(payloadId, block)); } From 000b986aff42f914460b88437e00e48c42a3c2c6 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Thu, 29 Sep 2022 08:58:15 +0200 Subject: [PATCH 6/7] Refactoring + more debug logs Signed-off-by: Fabio Di Fabio --- .../consensus/merge/PostMergeContext.java | 40 +++++++++---------- .../consensus/merge/PostMergeContextTest.java | 7 ---- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java index 22f4762b16f..406229f2ecf 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java @@ -26,7 +26,6 @@ import org.hyperledger.besu.util.Subscribers; import java.util.Comparator; -import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; @@ -207,27 +206,26 @@ public boolean validateCandidateHead(final BlockHeader candidateHeader) { } @Override - public void putPayloadById(final PayloadIdentifier payloadId, final Block block) { + public void putPayloadById(final PayloadIdentifier payloadId, final Block newBlock) { synchronized (blocksInProgress) { - final List prevBlockTuples = - retrieveTuplesById(payloadId).collect(Collectors.toUnmodifiableList()); - - if (prevBlockTuples.isEmpty()) { - blocksInProgress.add(new PayloadTuple(payloadId, block)); - } else { - final Block currBestBlock = - Stream.concat( - Stream.of(block), - prevBlockTuples.stream().map(payloadTuple -> payloadTuple.block)) - .sorted(compareByGasUsedDesc) - .findFirst() - .get(); - - if (currBestBlock.getHash().equals(block.getHash())) { - prevBlockTuples.forEach(blocksInProgress::remove); - blocksInProgress.add(new PayloadTuple(payloadId, block)); - } - } + final Optional maybeCurrBestBlock = retrieveBlockById(payloadId); + + maybeCurrBestBlock.ifPresentOrElse( + currBestBlock -> { + if (compareByGasUsedDesc.compare(newBlock, currBestBlock) < 0) { + debugLambda( + LOG, + "New proposal for payloadId {} {} is better than the previous one {}", + payloadId::toHexString, + () -> logBlockProposal(newBlock), + () -> logBlockProposal(currBestBlock)); + blocksInProgress.removeAll( + retrieveTuplesById(payloadId).collect(Collectors.toUnmodifiableList())); + blocksInProgress.add(new PayloadTuple(payloadId, newBlock)); + } + }, + () -> blocksInProgress.add(new PayloadTuple(payloadId, newBlock))); + debugLambda( LOG, "Current best proposal for payloadId {} {}", diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java index b0bef634ca9..fe9af337ecd 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java @@ -23,7 +23,6 @@ import static org.mockito.Mockito.when; import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier; -import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Difficulty; @@ -146,12 +145,10 @@ public void puttingTwoBlocksWithTheSamePayloadIdWeRetrieveTheBest() { Block zeroTxBlock = mock(Block.class); when(zeroTxBlock.getHeader()).thenReturn(zeroTxBlockHeader); - Hash betterBlockHash = Hash.fromHexStringLenient("0xBE51"); BlockHeader betterBlockHeader = mock(BlockHeader.class); when(betterBlockHeader.getGasUsed()).thenReturn(11L); Block betterBlock = mock(Block.class); when(betterBlock.getHeader()).thenReturn(betterBlockHeader); - when(betterBlock.getHash()).thenReturn(betterBlockHash); PayloadIdentifier payloadId = new PayloadIdentifier(1L); postMergeContext.putPayloadById(payloadId, zeroTxBlock); @@ -167,19 +164,15 @@ public void puttingABlockWithTheSamePayloadIdSmallerThanAnExistingOneWeRetrieveT Block zeroTxBlock = mock(Block.class); when(zeroTxBlock.getHeader()).thenReturn(zeroTxBlockHeader); - Hash betterBlockHash = Hash.fromHexStringLenient("0xBE51"); BlockHeader betterBlockHeader = mock(BlockHeader.class); when(betterBlockHeader.getGasUsed()).thenReturn(11L); Block betterBlock = mock(Block.class); when(betterBlock.getHeader()).thenReturn(betterBlockHeader); - when(betterBlock.getHash()).thenReturn(betterBlockHash); - Hash smallBlockHash = Hash.fromHexStringLenient("0x5011"); BlockHeader smallBlockHeader = mock(BlockHeader.class); when(smallBlockHeader.getGasUsed()).thenReturn(5L); Block smallBlock = mock(Block.class); when(smallBlock.getHeader()).thenReturn(smallBlockHeader); - when(smallBlock.getHash()).thenReturn(smallBlockHash); PayloadIdentifier payloadId = new PayloadIdentifier(1L); postMergeContext.putPayloadById(payloadId, zeroTxBlock); From edf1dc24f94cc7786f244528792d64ef1f872df8 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Thu, 29 Sep 2022 15:52:30 +0200 Subject: [PATCH 7/7] Update CHANGELOG Signed-off-by: Fabio Di Fabio --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed1824aa480..8cfb98c9c36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,9 @@ ## 22.7.5 - ### Additions and Improvements +- When building a new proposal, keep the best block built until now instead of the last one [#4455](https://github.com/hyperledger/besu/pull/4455) + ### Bug Fixes ### Download Links