Fix invalid withdrawals root during backwards sync

Add withdrawalsRoot as part of BodyIdentifier Add comment to avoid new issues when adding new fields to BlockBody Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
hyperledger · Jan 15, 2023 · 5851a3e · 5851a3e
1 parent 901573c
commit 5851a3e
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 18 deletions.
diff --git a/...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java b/...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java
@@ -68,14 +68,16 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext request) {
     if (block.isPresent()) {
       var proposal = block.get();
       var proposalHeader = proposal.getHeader();
+      var maybeWithdrawals = proposal.getBody().getWithdrawals();
       infoLambda(
           LOG,
-          "Fetch block proposal by identifier: {}, hash: {}, number: {}, coinbase: {}, transaction count: {}",
+          "Fetch block proposal by identifier: {}, hash: {}, number: {}, coinbase: {}, transaction count: {}, withdrawals count: {}",
           () -> payloadId.toHexString(),
           () -> proposalHeader.getHash(),
           () -> proposalHeader.getNumber(),
           () -> proposalHeader.getCoinbase(),
-          () -> proposal.getBody().getTransactions().size());
+          () -> proposal.getBody().getTransactions().size(),
+          () -> maybeWithdrawals.isEmpty() ? "No withdraws" : maybeWithdrawals.get().size());
       debugLambda(LOG, "assembledBlock {}", () -> block.map(Block::toString).get());
       return new JsonRpcSuccessResponse(
           request.getRequest().getId(),

diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/BlockBody.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/BlockBody.java
@@ -26,8 +26,12 @@ public class BlockBody implements org.hyperledger.besu.plugin.data.BlockBody {
 
   private static final BlockBody EMPTY =
       new BlockBody(Collections.emptyList(), Collections.emptyList(), Optional.empty());
-
+  /**
+   * Adding a new field with a corresponding root hash in the block header will require a change in
+   * {@link org.hyperledger.besu.ethereum.eth.manager.task.GetBodiesFromPeerTask.BodyIdentifier }
+   */
   private final List<Transaction> transactions;
+
   private final List<BlockHeader> ommers;
   private final Optional<List<Withdrawal>> maybeWithdrawals;
 

diff --git a/...h/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTask.java b/...h/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTask.java
@@ -21,6 +21,7 @@
 import org.hyperledger.besu.ethereum.core.BlockBody;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.Transaction;
+import org.hyperledger.besu.ethereum.core.Withdrawal;
 import org.hyperledger.besu.ethereum.eth.manager.EthContext;
 import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
 import org.hyperledger.besu.ethereum.eth.manager.PendingPeerRequest;
@@ -127,43 +128,49 @@ protected Optional<List<Block>> processResponse(
     return Optional.of(blocks);
   }
 
-  private static class BodyIdentifier {
+  static class BodyIdentifier {
     private final Bytes32 transactionsRoot;
     private final Bytes32 ommersHash;
+    private final Bytes32 withdrawalsRoot;
 
-    public BodyIdentifier(final Bytes32 transactionsRoot, final Bytes32 ommersHash) {
+    public BodyIdentifier(
+        final Bytes32 transactionsRoot, final Bytes32 ommersHash, final Bytes32 withdrawalsRoot) {
       this.transactionsRoot = transactionsRoot;
       this.ommersHash = ommersHash;
+      this.withdrawalsRoot = withdrawalsRoot;
     }
 
     public BodyIdentifier(final BlockBody body) {
-      this(body.getTransactions(), body.getOmmers());
+      this(body.getTransactions(), body.getOmmers(), body.getWithdrawals().orElse(null));
     }
 
-    public BodyIdentifier(final List<Transaction> transactions, final List<BlockHeader> ommers) {
-      this(BodyValidation.transactionsRoot(transactions), BodyValidation.ommersHash(ommers));
+    public BodyIdentifier(
+        final List<Transaction> transactions,
+        final List<BlockHeader> ommers,
+        final List<Withdrawal> withdrawals) {
+      this(
+          BodyValidation.transactionsRoot(transactions),
+          BodyValidation.ommersHash(ommers),
+          withdrawals == null ? Hash.EMPTY : BodyValidation.withdrawalsRoot(withdrawals));
     }
 
     public BodyIdentifier(final BlockHeader header) {
-      this(header.getTransactionsRoot(), header.getOmmersHash());
+      this(header.getTransactionsRoot(), header.getOmmersHash(), header.getWithdrawalsRoot());
     }
 
     @Override
     public boolean equals(final Object o) {
-      if (this == o) {
-        return true;
-      }
-      if (o == null || getClass() != o.getClass()) {
-        return false;
-      }
-      final BodyIdentifier that = (BodyIdentifier) o;
+      if (this == o) return true;
+      if (o == null || getClass() != o.getClass()) return false;
+      BodyIdentifier that = (BodyIdentifier) o;
       return Objects.equals(transactionsRoot, that.transactionsRoot)
-          && Objects.equals(ommersHash, that.ommersHash);
+          && Objects.equals(ommersHash, that.ommersHash)
+          && Objects.equals(withdrawalsRoot, that.withdrawalsRoot);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(transactionsRoot, ommersHash);
+      return Objects.hash(transactionsRoot, ommersHash, withdrawalsRoot);
     }
   }
 }
diff --git a/...c/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTaskTest.java b/...c/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTaskTest.java
@@ -14,17 +14,25 @@
  */
 package org.hyperledger.besu.ethereum.eth.manager.task;
 
+import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import org.hyperledger.besu.datatypes.Address;
+import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockBody;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.core.Withdrawal;
 import org.hyperledger.besu.ethereum.eth.manager.ethtaskutils.PeerMessageTaskTest;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
+import org.apache.tuweni.units.bigints.UInt64;
+import org.junit.Test;
+
 public class GetBodiesFromPeerTaskTest extends PeerMessageTaskTest<List<Block>> {
 
   @Override
@@ -56,4 +64,21 @@ protected void assertPartialResultMatchesExpectation(
       assertThat(requestedData).contains(block);
     }
   }
+
+  @Test
+  public void assertBodyIdentifierUsesWithdrawalsToGenerateBodyIdentifiers() {
+    final Withdrawal withdrawal =
+        new Withdrawal(UInt64.ONE, UInt64.ONE, Address.fromHexString("0x1"), Wei.ONE);
+
+    // Empty body block
+    final BlockBody emptyBodyBlock = BlockBody.empty();
+    // Block with no tx, no ommers, 1 withdrawal
+    final BlockBody bodyBlockWithWithdrawal =
+        new BlockBody(emptyList(), emptyList(), Optional.of(List.of(withdrawal)));
+
+    assertThat(
+            new GetBodiesFromPeerTask.BodyIdentifier(emptyBodyBlock)
+                .equals(new GetBodiesFromPeerTask.BodyIdentifier(bodyBlockWithWithdrawal)))
+        .isFalse();
+  }
 }