From fbf0e21adc44814ff484e9b5a932a97313f12072 Mon Sep 17 00:00:00 2001 From: Simon Dudley Date: Sun, 15 Jan 2023 12:40:03 +1000 Subject: [PATCH] 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 Co-authored-by: Gabriel Fukushima Signed-off-by: Gabriel Fukushima Signed-off-by: Simon Dudley --- .../methods/engine/EngineGetPayloadV2.java | 6 ++- .../besu/ethereum/core/BlockBody.java | 6 ++- .../manager/task/GetBodiesFromPeerTask.java | 37 +++++++++++-------- .../task/GetBodiesFromPeerTaskTest.java | 25 +++++++++++++ 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java index dda8cc9bc7a..cbd295b02fd 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java +++ b/ethereum/api/src/main/java/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 index 2fd7aac5c73..42826e56a16 100644 --- 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 transactions; + private final List ommers; private final Optional> maybeWithdrawals; diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTask.java index 9eb0f77f5d4..545fd21066e 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTask.java +++ b/ethereum/eth/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> 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 transactions, final List ommers) { - this(BodyValidation.transactionsRoot(transactions), BodyValidation.ommersHash(ommers)); + public BodyIdentifier( + final List transactions, + final List ommers, + final List 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/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTaskTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTaskTest.java index 3178afbe3cb..684593b636f 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBodiesFromPeerTaskTest.java +++ b/ethereum/eth/src/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> { @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(); + } }