Keep the best block built until now instead of the last one (#4455)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

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
 

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

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
   },

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;
@@ -23,6 +25,7 @@ import org.hyperledger.besu.ethereum.core.Difficulty;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 import org.hyperledger.besu.util.Subscribers;
 
+import java.util.Comparator;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
@@ -30,12 +33,18 @@ import java.util.stream.Stream;
 
 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<PostMergeContext> singleton = new AtomicReference<>();
 
+  private static final Comparator<Block> compareByGasUsedDesc =
+      Comparator.comparingLong((Block block) -> block.getHeader().getGasUsed()).reversed();
+
   private final AtomicReference<SyncState> syncState;
   private final AtomicReference<Difficulty> terminalTotalDifficulty;
   // initial postMerge state is indeterminate until it is set:
@@ -197,21 +206,57 @@ public class PostMergeContext implements MergeContext {
   }
 
   @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);
+  public void putPayloadById(final PayloadIdentifier payloadId, final Block newBlock) {
+    synchronized (blocksInProgress) {
+      final Optional<Block> 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 {} {}",
+          payloadId::toHexString,
+          () -> retrieveBlockById(payloadId).map(bb -> logBlockProposal(bb)).orElse("N/A"));
+    }
   }
 
   @Override
   public Optional<Block> 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<PayloadTuple> retrieveTuplesById(final PayloadIdentifier payloadId) {
     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;

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java

@@ -175,17 +175,18 @@ public class MergeCoordinator implements MergeMiningCoordinator, BadChainListene
   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 class MergeCoordinator implements MergeMiningCoordinator, BadChainListene
           result.errorMessage);
     }
 
-    tryToBuildBetterBlock(timestamp, random, payloadIdentifier, mergeBlockCreator);
+    tryToBuildBetterBlock(timestamp, prevRandao, payloadIdentifier, mergeBlockCreator);
 
     return payloadIdentifier;
   }

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

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 java.math.BigInteger;
 
 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 class PayloadIdentifier implements Quantity {
     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

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java

@@ -127,9 +127,9 @@ public class TransitionCoordinator extends TransitionUtils<MiningCoordinator>
   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

consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java

@@ -131,6 +131,7 @@ public class PostMergeContextTest {
   @Test
   public void putAndRetrieveFirstPayload() {
     Block mockBlock = mock(Block.class);
+
     PayloadIdentifier firstPayloadId = new PayloadIdentifier(1L);
     postMergeContext.putPayloadById(firstPayloadId, mockBlock);
 
@@ -138,14 +139,47 @@ public class PostMergeContextTest {
   }
 
   @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);
+
+    BlockHeader betterBlockHeader = mock(BlockHeader.class);
+    when(betterBlockHeader.getGasUsed()).thenReturn(11L);
+    Block betterBlock = mock(Block.class);
+    when(betterBlock.getHeader()).thenReturn(betterBlockHeader);
+
+    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);
+
+    BlockHeader betterBlockHeader = mock(BlockHeader.class);
+    when(betterBlockHeader.getGasUsed()).thenReturn(11L);
+    Block betterBlock = mock(Block.class);
+    when(betterBlock.getHeader()).thenReturn(betterBlockHeader);
+
+    BlockHeader smallBlockHeader = mock(BlockHeader.class);
+    when(smallBlockHeader.getGasUsed()).thenReturn(5L);
+    Block smallBlock = mock(Block.class);
+    when(smallBlock.getHeader()).thenReturn(smallBlockHeader);
+
     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

consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java

@@ -16,10 +16,12 @@ package org.hyperledger.besu.consensus.merge.blockcreation;
 
 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 class PayloadIdentifierTest {
 
   @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);
   }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java

@@ -72,6 +72,8 @@ public class EngineForkchoiceUpdated extends ExecutionEngineJsonRpcMethod {
     final Optional<EnginePayloadAttributesParameter> maybePayloadAttributes =
         requestContext.getOptionalParameter(1, EnginePayloadAttributesParameter.class);
 
+    LOG.debug("Forkchoice parameters {}", forkChoice);
+
     Optional<Hash> maybeFinalizedHash =
         Optional.ofNullable(forkChoice.getFinalizedBlockHash())
             .filter(finalized -> !finalized.isZero());

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/EngineForkchoiceUpdatedParameter.java

@@ -47,4 +47,14 @@ public class EngineForkchoiceUpdatedParameter {
     this.headBlockHash = headBlockHash;
     this.safeBlockHash = safeBlockHash;
   }
+
+  @Override
+  public String toString() {
+    return "headBlockHash="
+        + headBlockHash
+        + ", safeBlockHash="
+        + safeBlockHash
+        + ", finalizedBlockHash="
+        + finalizedBlockHash;
+  }
 }

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java

@@ -227,7 +227,11 @@ public class EngineForkchoiceUpdatedTest {
             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))

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadTest.java

@@ -21,6 +21,7 @@ import static org.mockito.Mockito.when;
 
 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 class EngineGetPayloadTest {
 
   @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();
   }