From 428177f5146bc6858fdf4d6a64e83c96f3109af7 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Mon, 8 Jan 2024 09:20:34 +0000 Subject: [PATCH] Use synchronized call to access the chain head block in `eth_estimateGas` (#6345) * Use synchronized call to access the chain head block in estimateGas() Signed-off-by: Matthew Whitehead * Add error log entries when throwing internal error from estimateGas() Signed-off-by: Matthew Whitehead * Update unit tests Signed-off-by: Matthew Whitehead * Update changelog Signed-off-by: Matthew Whitehead --------- Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + .../internal/methods/AbstractEstimateGas.java | 13 +++++++++++-- .../jsonrpc/internal/methods/EthEstimateGas.java | 7 +++++++ .../internal/methods/EthCreateAccessListTest.java | 12 +++++++++--- .../internal/methods/EthEstimateGasTest.java | 11 +++++++++-- 5 files changed, 37 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6a96fa741..ea4b5f915e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Optimize RocksDB WAL files, allows for faster restart and a more linear disk space utilization [#6328](https://github.com/hyperledger/besu/pull/6328) ### Bug fixes +- INTERNAL_ERROR from `eth_estimateGas` JSON/RPC calls [#6344](https://github.com/hyperledger/besu/issues/6344) ## 23.10.3 diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AbstractEstimateGas.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AbstractEstimateGas.java index 1a45d194ef..b3de5094c8 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AbstractEstimateGas.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/AbstractEstimateGas.java @@ -23,6 +23,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType; import org.hyperledger.besu.ethereum.api.query.BlockchainQueries; +import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.mainnet.ValidationResult; import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult; @@ -48,8 +49,16 @@ public abstract class AbstractEstimateGas implements JsonRpcMethod { } protected BlockHeader blockHeader() { - final long headBlockNumber = blockchainQueries.headBlockNumber(); - return blockchainQueries.getBlockchain().getBlockHeader(headBlockNumber).orElse(null); + final Blockchain theChain = blockchainQueries.getBlockchain(); + + // Optimistically get the block header for the chain head without taking a lock, + // but revert to the safe implementation if it returns an empty optional. (It's + // possible the chain head has been updated but the block is still being persisted + // to storage/cache under the lock). + return theChain + .getBlockHeader(theChain.getChainHeadHash()) + .or(() -> theChain.getBlockHeaderSafe(theChain.getChainHeadHash())) + .orElse(null); } protected CallParameter overrideGasLimitAndPrice( diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGas.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGas.java index ea69ad9d38..9a382d6441 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGas.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGas.java @@ -32,8 +32,13 @@ import org.hyperledger.besu.evm.tracing.EstimateGasOperationTracer; import java.util.Optional; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class EthEstimateGas extends AbstractEstimateGas { + private static final Logger LOG = LoggerFactory.getLogger(EthEstimateGas.class); + public EthEstimateGas( final BlockchainQueries blockchainQueries, final TransactionSimulator transactionSimulator) { super(blockchainQueries, transactionSimulator); @@ -50,6 +55,7 @@ public class EthEstimateGas extends AbstractEstimateGas { final BlockHeader blockHeader = blockHeader(); if (blockHeader == null) { + LOG.error("Chain head block not found"); return errorResponse(requestContext, RpcErrorType.INTERNAL_ERROR); } if (!blockchainQueries @@ -70,6 +76,7 @@ public class EthEstimateGas extends AbstractEstimateGas { blockHeader, modifiedCallParams, operationTracer, isAllowExceedingBalance); if (gasUsed.isEmpty()) { + LOG.error("gasUsed is empty after simulating transaction."); return errorResponse(requestContext, RpcErrorType.INTERNAL_ERROR); } diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCreateAccessListTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCreateAccessListTest.java index ec74d82991..674617b4b5 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCreateAccessListTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCreateAccessListTest.java @@ -17,7 +17,6 @@ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -25,6 +24,7 @@ import static org.mockito.Mockito.when; import org.hyperledger.besu.datatypes.AccessListEntry; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; @@ -78,10 +78,16 @@ public class EthCreateAccessListTest { @BeforeEach public void setUp() { - when(blockchainQueries.headBlockNumber()).thenReturn(1L); when(blockchainQueries.getBlockchain()).thenReturn(blockchain); when(blockchainQueries.getWorldStateArchive()).thenReturn(worldStateArchive); - when(blockchain.getBlockHeader(eq(1L))).thenReturn(Optional.of(blockHeader)); + when(blockchain.getChainHeadHash()) + .thenReturn( + Hash.fromHexString( + "0x3f07a9c83155594c000642e7d60e8a8a00038d03e9849171a05ed0e2d47acbb3")); + when(blockchain.getBlockHeader( + Hash.fromHexString( + "0x3f07a9c83155594c000642e7d60e8a8a00038d03e9849171a05ed0e2d47acbb3"))) + .thenReturn(Optional.of(blockHeader)); when(blockHeader.getGasLimit()).thenReturn(Long.MAX_VALUE); when(blockHeader.getNumber()).thenReturn(1L); when(worldStateArchive.isWorldStateAvailable(any(), any())).thenReturn(true); diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGasTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGasTest.java index 9ac62cf8c2..44b3bb9ba0 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGasTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGasTest.java @@ -22,6 +22,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; @@ -73,10 +74,16 @@ public class EthEstimateGasTest { @BeforeEach public void setUp() { - when(blockchainQueries.headBlockNumber()).thenReturn(1L); when(blockchainQueries.getBlockchain()).thenReturn(blockchain); when(blockchainQueries.getWorldStateArchive()).thenReturn(worldStateArchive); - when(blockchain.getBlockHeader(eq(1L))).thenReturn(Optional.of(blockHeader)); + when(blockchain.getChainHeadHash()) + .thenReturn( + Hash.fromHexString( + "0x3f07a9c83155594c000642e7d60e8a8a00038d03e9849171a05ed0e2d47acbb3")); + when(blockchain.getBlockHeader( + Hash.fromHexString( + "0x3f07a9c83155594c000642e7d60e8a8a00038d03e9849171a05ed0e2d47acbb3"))) + .thenReturn(Optional.of(blockHeader)); when(blockHeader.getGasLimit()).thenReturn(Long.MAX_VALUE); when(blockHeader.getNumber()).thenReturn(1L); when(worldStateArchive.isWorldStateAvailable(any(), any())).thenReturn(true);