Improve the selection of the most profitable built block (hyperledger…

…#7174) * Improve the selection of the most profitable built block Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Update CHANGELOG Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Update consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PayloadWrapper.java Co-authored-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> --------- Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: George Tebrean <george@web3labs.com>
gtebrean · Jun 26, 2024 · 6e65162 · 6e65162
1 parent 6ab3412
commit 6e65162
Show file tree

Hide file tree

Showing 18 changed files with 190 additions and 159 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@
 - Promote experimental --Xbonsai-limit-trie-logs-enabled to production-ready, --bonsai-limit-trie-logs-enabled [#7192](https://github.com/hyperledger/besu/pull/7192)
 - Promote experimental --Xbonsai-trie-logs-pruning-window-size to production-ready, --bonsai-trie-logs-pruning-window-size [#7192](https://github.com/hyperledger/besu/pull/7192)
 - `admin_nodeInfo` JSON/RPC call returns the currently active EVM version [#7127](https://github.com/hyperledger/besu/pull/7127)
+- Improve the selection of the most profitable built block [#7174](https://github.com/hyperledger/besu/pull/7174)
 
 ### Bug fixes
 - Make `eth_gasPrice` aware of the base fee market [#7102](https://github.com/hyperledger/besu/pull/7102)

diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/MergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/MergeContext.java
@@ -18,7 +18,6 @@
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.ConsensusContext;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
-import org.hyperledger.besu.ethereum.core.BlockWithReceipts;
 import org.hyperledger.besu.ethereum.core.Difficulty;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 
@@ -167,7 +166,7 @@ void fireNewUnverifiedForkchoiceEvent(
    * @param payloadId the payload identifier
    * @return the optional block with receipts
    */
-  Optional<BlockWithReceipts> retrieveBlockById(final PayloadIdentifier payloadId);
+  Optional<PayloadWrapper> retrievePayloadById(final PayloadIdentifier payloadId);
 
   /**
    * Is configured for a post-merge from genesis.

diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PayloadWrapper.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PayloadWrapper.java
@@ -15,13 +15,53 @@
 package org.hyperledger.besu.consensus.merge;
 
 import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier;
+import org.hyperledger.besu.datatypes.Wei;
+import org.hyperledger.besu.ethereum.core.BlockValueCalculator;
 import org.hyperledger.besu.ethereum.core.BlockWithReceipts;
 
-/**
- * Wrapper for payload plus extra info.
- *
- * @param payloadIdentifier Payload identifier
- * @param blockWithReceipts Block With Receipts
- */
-public record PayloadWrapper(
-    PayloadIdentifier payloadIdentifier, BlockWithReceipts blockWithReceipts) {}
+/** Wrapper for payload plus extra info. */
+public class PayloadWrapper {
+  private final PayloadIdentifier payloadIdentifier;
+  private final BlockWithReceipts blockWithReceipts;
+  private final Wei blockValue;
+
+  /**
+   * Construct a wrapper with the following fields.
+   *
+   * @param payloadIdentifier Payload identifier
+   * @param blockWithReceipts Block with receipts
+   */
+  public PayloadWrapper(
+      final PayloadIdentifier payloadIdentifier, final BlockWithReceipts blockWithReceipts) {
+    this.blockWithReceipts = blockWithReceipts;
+    this.payloadIdentifier = payloadIdentifier;
+    this.blockValue = BlockValueCalculator.calculateBlockValue(blockWithReceipts);
+  }
+
+  /**
+   * Get the block value
+   *
+   * @return block value in Wei
+   */
+  public Wei blockValue() {
+    return blockValue;
+  }
+
+  /**
+   * Get this payload identifier
+   *
+   * @return payload identifier
+   */
+  public PayloadIdentifier payloadIdentifier() {
+    return payloadIdentifier;
+  }
+
+  /**
+   * Get the block with receipts
+   *
+   * @return block with receipts
+   */
+  public BlockWithReceipts blockWithReceipts() {
+    return blockWithReceipts;
+  }
+}
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
@@ -16,20 +16,16 @@
 
 import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier;
 import org.hyperledger.besu.datatypes.Hash;
-import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.ConsensusContext;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
-import org.hyperledger.besu.ethereum.core.BlockValueCalculator;
-import org.hyperledger.besu.ethereum.core.BlockWithReceipts;
 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;
 import java.util.stream.Stream;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -45,13 +41,6 @@ public class PostMergeContext implements MergeContext {
   static final int MAX_BLOCKS_IN_PROGRESS = 12;
 
   private static final AtomicReference<PostMergeContext> singleton = new AtomicReference<>();
-
-  private static final Comparator<BlockWithReceipts> compareByGasUsedDesc =
-      Comparator.comparingLong(
-              (BlockWithReceipts blockWithReceipts) ->
-                  blockWithReceipts.getBlock().getHeader().getGasUsed())
-          .reversed();
-
   private final AtomicReference<SyncState> syncState;
   private final AtomicReference<Difficulty> terminalTotalDifficulty;
   // initial postMerge state is indeterminate until it is set:
@@ -70,7 +59,6 @@ public class PostMergeContext implements MergeContext {
   private final AtomicReference<BlockHeader> lastSafeBlock = new AtomicReference<>();
   private final AtomicReference<Optional<BlockHeader>> terminalPoWBlock =
       new AtomicReference<>(Optional.empty());
-  private final BlockValueCalculator blockValueCalculator = new BlockValueCalculator();
   private boolean isPostMergeAtGenesis;
 
   /** Instantiates a new Post merge context. */
@@ -227,66 +215,65 @@ public boolean validateCandidateHead(final BlockHeader candidateHeader) {
   }
 
   @Override
-  public void putPayloadById(final PayloadWrapper payloadWrapper) {
+  public void putPayloadById(final PayloadWrapper newPayload) {
+    final var newBlockWithReceipts = newPayload.blockWithReceipts();
+    final var newBlockValue = newPayload.blockValue();
+
     synchronized (blocksInProgress) {
-      final Optional<BlockWithReceipts> maybeCurrBestBlock =
-          retrieveBlockById(payloadWrapper.payloadIdentifier());
+      final Optional<PayloadWrapper> maybeCurrBestPayload =
+          retrievePayloadById(newPayload.payloadIdentifier());
 
-      maybeCurrBestBlock.ifPresentOrElse(
-          currBestBlock -> {
-            if (compareByGasUsedDesc.compare(payloadWrapper.blockWithReceipts(), currBestBlock)
-                < 0) {
+      maybeCurrBestPayload.ifPresent(
+          currBestPayload -> {
+            if (newBlockValue.greaterThan(currBestPayload.blockValue())) {
               LOG.atDebug()
-                  .setMessage("New proposal for payloadId {} {} is better than the previous one {}")
-                  .addArgument(payloadWrapper.payloadIdentifier())
+                  .setMessage(
+                      "New proposal for payloadId {} {} is better than the previous one {} by {}")
+                  .addArgument(newPayload.payloadIdentifier())
+                  .addArgument(() -> logBlockProposal(newBlockWithReceipts.getBlock()))
                   .addArgument(
-                      () -> logBlockProposal(payloadWrapper.blockWithReceipts().getBlock()))
-                  .addArgument(() -> logBlockProposal(currBestBlock.getBlock()))
+                      () -> logBlockProposal(currBestPayload.blockWithReceipts().getBlock()))
+                  .addArgument(
+                      () ->
+                          newBlockValue
+                              .subtract(currBestPayload.blockValue())
+                              .toHumanReadableString())
                   .log();
+
               blocksInProgress.removeAll(
-                  retrievePayloadsById(payloadWrapper.payloadIdentifier())
-                      .collect(Collectors.toUnmodifiableList()));
-              blocksInProgress.add(
-                  new PayloadWrapper(
-                      payloadWrapper.payloadIdentifier(), payloadWrapper.blockWithReceipts()));
-              logCurrentBestBlock(payloadWrapper.blockWithReceipts());
+                  streamPayloadsById(newPayload.payloadIdentifier()).toList());
+
+              logCurrentBestBlock(newPayload);
             }
-          },
-          () ->
-              blocksInProgress.add(
-                  new PayloadWrapper(
-                      payloadWrapper.payloadIdentifier(), payloadWrapper.blockWithReceipts())));
+          });
+      blocksInProgress.add(newPayload);
     }
   }
 
-  private void logCurrentBestBlock(final BlockWithReceipts blockWithReceipts) {
+  private void logCurrentBestBlock(final PayloadWrapper payloadWrapper) {
     if (LOG.isDebugEnabled()) {
-      final Block block = blockWithReceipts.getBlock();
+      final Block block = payloadWrapper.blockWithReceipts().getBlock();
       final float gasUsedPerc =
           100.0f * block.getHeader().getGasUsed() / block.getHeader().getGasLimit();
       final int txsNum = block.getBody().getTransactions().size();
-      final Wei reward = blockValueCalculator.calculateBlockValue(blockWithReceipts);
 
       LOG.debug(
           "Current best proposal for block {}: txs {}, gas used {}%, reward {}",
-          blockWithReceipts.getNumber(),
+          block.getHeader().getNumber(),
           txsNum,
           String.format("%1.2f", gasUsedPerc),
-          reward.toHumanReadableString());
+          payloadWrapper.blockValue().toHumanReadableString());
     }
   }
 
   @Override
-  public Optional<BlockWithReceipts> retrieveBlockById(final PayloadIdentifier payloadId) {
+  public Optional<PayloadWrapper> retrievePayloadById(final PayloadIdentifier payloadId) {
     synchronized (blocksInProgress) {
-      return retrievePayloadsById(payloadId)
-          .map(payloadWrapper -> payloadWrapper.blockWithReceipts())
-          .sorted(compareByGasUsedDesc)
-          .findFirst();
+      return streamPayloadsById(payloadId).max(Comparator.comparing(PayloadWrapper::blockValue));
     }
   }
 
-  private Stream<PayloadWrapper> retrievePayloadsById(final PayloadIdentifier payloadId) {
+  private Stream<PayloadWrapper> streamPayloadsById(final PayloadIdentifier payloadId) {
     return blocksInProgress.stream().filter(z -> z.payloadIdentifier().equals(payloadId));
   }
 

diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionContext.java
@@ -18,7 +18,6 @@
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.ConsensusContext;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
-import org.hyperledger.besu.ethereum.core.BlockWithReceipts;
 import org.hyperledger.besu.ethereum.core.Difficulty;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 
@@ -146,8 +145,8 @@ public void putPayloadById(final PayloadWrapper payloadWrapper) {
   }
 
   @Override
-  public Optional<BlockWithReceipts> retrieveBlockById(final PayloadIdentifier payloadId) {
-    return postMergeContext.retrieveBlockById(payloadId);
+  public Optional<PayloadWrapper> retrievePayloadById(final PayloadIdentifier payloadId) {
+    return postMergeContext.retrievePayloadById(payloadId);
   }
 
   @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
@@ -26,6 +26,7 @@
 
 import org.hyperledger.besu.consensus.merge.blockcreation.PayloadIdentifier;
 import org.hyperledger.besu.datatypes.Hash;
+import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockWithReceipts;
@@ -138,9 +139,12 @@ public void putAndRetrieveFirstPayload() {
     BlockWithReceipts mockBlockWithReceipts = createBlockWithReceipts(1, 21000, 1);
 
     PayloadIdentifier firstPayloadId = new PayloadIdentifier(1L);
-    postMergeContext.putPayloadById(new PayloadWrapper(firstPayloadId, mockBlockWithReceipts));
+    final var payloadWrapper = createPayloadWrapper(firstPayloadId, mockBlockWithReceipts, Wei.ONE);
+    postMergeContext.putPayloadById(payloadWrapper);
 
-    assertThat(postMergeContext.retrieveBlockById(firstPayloadId)).contains(mockBlockWithReceipts);
+    assertThat(postMergeContext.retrievePayloadById(firstPayloadId))
+        .map(PayloadWrapper::blockWithReceipts)
+        .contains(mockBlockWithReceipts);
   }
 
   @Test
@@ -149,10 +153,16 @@ public void puttingTwoBlocksWithTheSamePayloadIdWeRetrieveTheBest() {
     BlockWithReceipts betterBlockWithReceipts = createBlockWithReceipts(2, 11, 1);
 
     PayloadIdentifier payloadId = new PayloadIdentifier(1L);
-    postMergeContext.putPayloadById(new PayloadWrapper(payloadId, zeroTxBlockWithReceipts));
-    postMergeContext.putPayloadById(new PayloadWrapper(payloadId, betterBlockWithReceipts));
-
-    assertThat(postMergeContext.retrieveBlockById(payloadId)).contains(betterBlockWithReceipts);
+    final var zeroTxPayloadWrapper =
+        createPayloadWrapper(payloadId, zeroTxBlockWithReceipts, Wei.ZERO);
+    final var betterPayloadWrapper =
+        createPayloadWrapper(payloadId, betterBlockWithReceipts, Wei.ONE);
+    postMergeContext.putPayloadById(zeroTxPayloadWrapper);
+    postMergeContext.putPayloadById(betterPayloadWrapper);
+
+    assertThat(postMergeContext.retrievePayloadById(payloadId))
+        .map(PayloadWrapper::blockWithReceipts)
+        .contains(betterBlockWithReceipts);
   }
 
   @Test
@@ -162,25 +172,33 @@ public void puttingABlockWithTheSamePayloadIdSmallerThanAnExistingOneWeRetrieveT
     BlockWithReceipts smallBlockWithReceipts = createBlockWithReceipts(3, 5, 1);
 
     PayloadIdentifier payloadId = new PayloadIdentifier(1L);
-    postMergeContext.putPayloadById(new PayloadWrapper(payloadId, zeroTxBlockWithReceipts));
-    postMergeContext.putPayloadById(new PayloadWrapper(payloadId, betterBlockWithReceipts));
-    postMergeContext.putPayloadById(new PayloadWrapper(payloadId, smallBlockWithReceipts));
-
-    assertThat(postMergeContext.retrieveBlockById(payloadId)).contains(betterBlockWithReceipts);
+    final var zeroTxPayloadWrapper =
+        createPayloadWrapper(payloadId, zeroTxBlockWithReceipts, Wei.ZERO);
+    final var betterPayloadWrapper =
+        createPayloadWrapper(payloadId, betterBlockWithReceipts, Wei.of(2));
+    final var smallPayloadWrapper =
+        createPayloadWrapper(payloadId, smallBlockWithReceipts, Wei.ONE);
+    postMergeContext.putPayloadById(zeroTxPayloadWrapper);
+    postMergeContext.putPayloadById(betterPayloadWrapper);
+    postMergeContext.putPayloadById(smallPayloadWrapper);
+
+    assertThat(postMergeContext.retrievePayloadById(payloadId))
+        .map(PayloadWrapper::blockWithReceipts)
+        .contains(betterBlockWithReceipts);
   }
 
   @Test
   public void tryingToRetrieveANotYetPutPayloadIdReturnsEmpty() {
     PayloadIdentifier payloadId = new PayloadIdentifier(1L);
 
-    assertThat(postMergeContext.retrieveBlockById(payloadId)).isEmpty();
+    assertThat(postMergeContext.retrievePayloadById(payloadId)).isEmpty();
   }
 
   @Test
   public void tryingToRetrieveABlockPutButEvictedReturnsEmpty() {
     PayloadIdentifier evictedPayloadId = new PayloadIdentifier(0L);
 
-    assertThat(postMergeContext.retrieveBlockById(evictedPayloadId)).isEmpty();
+    assertThat(postMergeContext.retrievePayloadById(evictedPayloadId)).isEmpty();
   }
 
   @Test
@@ -209,6 +227,17 @@ public void syncStateNullShouldNotThrowWhenIsSyncingIsCalled() {
     assertThat(postMergeContext.isSyncing()).isFalse();
   }
 
+  private PayloadWrapper createPayloadWrapper(
+      final PayloadIdentifier firstPayloadId,
+      final BlockWithReceipts mockBlockWithReceipts,
+      final Wei blockValue) {
+    final var payloadWrapper = mock(PayloadWrapper.class);
+    when(payloadWrapper.payloadIdentifier()).thenReturn(firstPayloadId);
+    when(payloadWrapper.blockWithReceipts()).thenReturn(mockBlockWithReceipts);
+    when(payloadWrapper.blockValue()).thenReturn(blockValue);
+    return payloadWrapper;
+  }
+
   private static BlockWithReceipts createBlockWithReceipts(
       final int number, final long gasUsed, final int txCount) {
     Block mockBlock = mock(Block.class, RETURNS_DEEP_STUBS);