Add withdrawals to PayloadIdentifier to avoid collisions (#5321)

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

CHANGELOG.md

@@ -28,6 +28,7 @@
 - Make QBFT validator smart contract mode work with london fork [#5249](https://github.com/hyperledger/besu/issues/5249)
 - Try to connect to EthStats server by default with ssl followed by non-ssl. [#5301](https://github.com/hyperledger/besu/pull/5301)
 - Allow --miner-extra-data to be used in Proof-of-Stake block production [#5291](https://github.com/hyperledger/besu/pull/5291)
+- Add withdrawals to payloadId calculation to avoid collisions [#5321](https://github.com/hyperledger/besu/pull/5321) 
 
 ### Download Links
 

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/11_shanghai_prepare_payload.json

@@ -21,7 +21,7 @@
           },
           {
             "index": "0x1",
-            "validatorIndex": "0x0",
+            "validatorIndex": "0x1",
             "address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
             "amount": "0x2"
           }
@@ -39,7 +39,7 @@
         "latestValidHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
         "validationError": null
       },
-      "payloadId": "0x0065bd32d4f95410"
+      "payloadId": "0x0065bd329c251cd9"
     }
   },
   "statusCode" : 200

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/12_shanghai_prepare_payload_replay.json

@@ -0,0 +1,46 @@
+{
+  "request": {
+    "jsonrpc": "2.0",
+    "method": "engine_forkchoiceUpdatedV2",
+    "params": [
+      {
+        "headBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "safeBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "finalizedBlockHash": "0x0000000000000000000000000000000000000000000000000000000000000000"
+      },
+      {
+        "timestamp": "0x10",
+        "prevRandao": "0x0000000000000000000000000000000000000000000000000000000000000000",
+        "suggestedFeeRecipient": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
+        "withdrawals": [
+          {
+            "index": "0x0",
+            "validatorIndex": "0x0",
+            "address": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
+            "amount": "0x3"
+          },
+          {
+            "index": "0x1",
+            "validatorIndex": "0x1",
+            "address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
+            "amount": "0x4"
+          }
+        ]
+      }
+    ],
+    "id": 67
+  },
+  "response": {
+    "jsonrpc": "2.0",
+    "id": 67,
+    "result": {
+      "payloadStatus": {
+        "status": "VALID",
+        "latestValidHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "validationError": null
+      },
+      "payloadId": "0x0065bd329c251d59"
+    }
+  },
+  "statusCode" : 200
+}

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/12_shanghai_get_payload.json → acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/13_shanghai_get_payload.json

@@ -3,7 +3,7 @@
     "jsonrpc": "2.0",
     "method": "engine_getPayloadV2",
     "params": [
-      "0x0065bd32d4f95410"
+      "0x0065bd329c251d59"
     ],
     "id": 67
   },
@@ -14,7 +14,7 @@
       "executionPayload": {
         "parentHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
         "feeRecipient": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
-        "stateRoot": "0x7a6f2e03f2348dc75731e6e767c97a88da50430748a030874e27f7c3fac3d49d",
+        "stateRoot": "0xc8c8e840369eac89a610bfe2ec21fcdee4c9c43bec4876f0129fcd4b5311f6dd",
         "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
         "prevRandao": "0x0000000000000000000000000000000000000000000000000000000000000000",
         "gasLimit": "0x1c9c380",
@@ -28,17 +28,17 @@
             "index": "0x0",
             "validatorIndex": "0x0",
             "address": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
-            "amount": "0x1"
+            "amount": "0x3"
           },
           {
             "index": "0x1",
-            "validatorIndex": "0x0",
+            "validatorIndex": "0x1",
             "address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
-            "amount": "0x2"
+            "amount": "0x4"
           }
         ],
         "blockNumber": "0x2",
-        "blockHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
+        "blockHash": "0x4a7b0e390d320a07150daa9c3b9dddfc6b7f6ca7d7a2971ef2f53ac1f29f24dc",
         "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
       },
       "blockValue": "0x0"

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/14_shanghai_execute_payload.json → acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/15_shanghai_execute_payload.json

@@ -24,13 +24,13 @@
           },
           {
             "index": "0x1",
-            "validatorIndex": "0x0",
+            "validatorIndex": "0x1",
             "address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
             "amount": "0x2"
           }
         ],
         "blockNumber": "0x2",
-        "blockHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
+        "blockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
         "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
       }
     ],
@@ -41,7 +41,7 @@
     "id": 67,
     "result": {
       "status": "VALID",
-      "latestValidHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
+      "latestValidHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
       "validationError": null
     }
   },

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/16_shanghai_update_forkchoice.json

@@ -0,0 +1,28 @@
+{
+  "request": {
+    "jsonrpc": "2.0",
+    "method": "engine_forkchoiceUpdatedV2",
+    "params": [
+      {
+        "headBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
+        "safeBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
+        "finalizedBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705"
+      },
+      null
+    ],
+    "id": 67
+  },
+  "response": {
+    "jsonrpc": "2.0",
+    "id": 67,
+    "result": {
+      "payloadStatus": {
+        "status": "VALID",
+        "latestValidHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
+        "validationError": null
+      },
+      "payloadId": null
+    }
+  },
+  "statusCode": 200
+}

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

@@ -252,7 +252,7 @@ public PayloadIdentifier preparePayload(
 
     final PayloadIdentifier payloadIdentifier =
         PayloadIdentifier.forPayloadParams(
-            parentHeader.getBlockHash(), timestamp, prevRandao, feeRecipient);
+            parentHeader.getBlockHash(), timestamp, prevRandao, feeRecipient, withdrawals);
 
     if (blockCreationTask.containsKey(payloadIdentifier)) {
       LOG.debug(

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

@@ -17,8 +17,12 @@
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.datatypes.Quantity;
+import org.hyperledger.besu.ethereum.core.Withdrawal;
 
 import java.math.BigInteger;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Optional;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonValue;
@@ -50,24 +54,37 @@ public PayloadIdentifier(final Long payloadId) {
   }
 
   /**
-   * Create payload identifier for payload params.
+   * Create payload identifier for payload params. This is a deterministic hash of all payload
+   * parameters that aims to avoid collisions
    *
    * @param parentHash the parent hash
    * @param timestamp the timestamp
    * @param prevRandao the prev randao
    * @param feeRecipient the fee recipient
+   * @param withdrawals the withdrawals
    * @return the payload identifier
    */
   public static PayloadIdentifier forPayloadParams(
       final Hash parentHash,
       final Long timestamp,
       final Bytes32 prevRandao,
-      final Address feeRecipient) {
+      final Address feeRecipient,
+      final Optional<List<Withdrawal>> withdrawals) {
+
     return new PayloadIdentifier(
         timestamp
             ^ ((long) parentHash.toHexString().hashCode()) << 8
             ^ ((long) prevRandao.toHexString().hashCode()) << 16
-            ^ ((long) feeRecipient.toHexString().hashCode()) << 24);
+            ^ ((long) feeRecipient.toHexString().hashCode()) << 24
+            ^ (long)
+                withdrawals
+                    .map(
+                        ws ->
+                            ws.stream()
+                                .sorted(Comparator.comparing(Withdrawal::getIndex))
+                                .map(Withdrawal::hashCode)
+                                .reduce(1, (a, b) -> a ^ (b * 31)))
+                    .orElse(0));
   }
 
   @Override

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

@@ -14,14 +14,19 @@
  */
 package org.hyperledger.besu.consensus.merge.blockcreation;
 
+import static java.util.Collections.emptyList;
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 
 import org.hyperledger.besu.datatypes.Address;
+import org.hyperledger.besu.datatypes.GWei;
 import org.hyperledger.besu.datatypes.Hash;
+import org.hyperledger.besu.ethereum.core.Withdrawal;
 
 import java.util.List;
+import java.util.Optional;
 
 import org.apache.tuweni.bytes.Bytes32;
+import org.apache.tuweni.units.bigints.UInt64;
 import org.junit.Test;
 
 public class PayloadIdentifierTest {
@@ -43,8 +48,92 @@ public void serializesToEvenHexRepresentation() {
   public void conversionCoverage() {
     var idTest =
         PayloadIdentifier.forPayloadParams(
-            Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42"));
+            Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42"), Optional.empty());
     assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest);
     assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest);
   }
+
+  @Test
+  public void differentWithdrawalAmountsYieldDifferentHash() {
+    final List<Withdrawal> withdrawals1 =
+        List.of(
+            new Withdrawal(
+                UInt64.valueOf(100),
+                UInt64.valueOf(1000),
+                Address.fromHexString("0x1"),
+                GWei.of(1)),
+            new Withdrawal(
+                UInt64.valueOf(200),
+                UInt64.valueOf(2000),
+                Address.fromHexString("0x2"),
+                GWei.of(2)));
+    final List<Withdrawal> withdrawals2 =
+        List.of(
+            new Withdrawal(
+                UInt64.valueOf(100),
+                UInt64.valueOf(1000),
+                Address.fromHexString("0x1"),
+                GWei.of(3)),
+            new Withdrawal(
+                UInt64.valueOf(200),
+                UInt64.valueOf(2000),
+                Address.fromHexString("0x2"),
+                GWei.of(4)));
+    final Bytes32 prevRandao = Bytes32.random();
+    var idForWithdrawals1 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals1));
+    var idForWithdrawals2 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals2));
+    assertThat(idForWithdrawals1).isNotEqualTo(idForWithdrawals2);
+  }
+
+  @Test
+  public void differentOrderedWithdrawalsYieldSameHash() {
+    final List<Withdrawal> withdrawals1 =
+        List.of(
+            new Withdrawal(
+                UInt64.valueOf(100),
+                UInt64.valueOf(1000),
+                Address.fromHexString("0x1"),
+                GWei.of(1)),
+            new Withdrawal(
+                UInt64.valueOf(200),
+                UInt64.valueOf(2000),
+                Address.fromHexString("0x2"),
+                GWei.of(2)));
+    final List<Withdrawal> withdrawals2 =
+        List.of(
+            new Withdrawal(
+                UInt64.valueOf(200),
+                UInt64.valueOf(2000),
+                Address.fromHexString("0x2"),
+                GWei.of(2)),
+            new Withdrawal(
+                UInt64.valueOf(100),
+                UInt64.valueOf(1000),
+                Address.fromHexString("0x1"),
+                GWei.of(1)));
+    final Bytes32 prevRandao = Bytes32.random();
+    var idForWithdrawals1 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals1));
+    var idForWithdrawals2 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals2));
+    assertThat(idForWithdrawals1).isEqualTo(idForWithdrawals2);
+  }
+
+  @Test
+  public void emptyOptionalAndEmptyListWithdrawalsYieldDifferentHash() {
+    final Bytes32 prevRandao = Bytes32.random();
+    var idForWithdrawals1 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.empty());
+    var idForWithdrawals2 =
+        PayloadIdentifier.forPayloadParams(
+            Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(emptyList()));
+    assertThat(idForWithdrawals1).isNotEqualTo(idForWithdrawals2);
+  }
 }