From 0dffe63c139b54943bd3fa4f8583327caaff1f0b Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Mon, 19 Aug 2024 15:20:47 +0200 Subject: [PATCH] Fix BlockchainQueries::gasPrice when chain head block body still not fully persisted (#7482) * Fix BlockchainQueries::gasPrice when chain head block body still not fully persisted Signed-off-by: Fabio Di Fabio --- CHANGELOG.md | 1 + .../ethereum/api/query/BlockchainQueries.java | 33 ++++++++++------- .../internal/methods/EthGasPriceTest.java | 35 ++++++++++--------- .../besu/ethereum/chain/Blockchain.java | 9 +++++ .../ethereum/chain/DefaultBlockchain.java | 10 +++++- .../besu/evmtool/t8n/T8nBlockchain.java | 5 +++ .../ReferenceTestBlockchain.java | 5 +++ 7 files changed, 68 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8945cdd1ee5..1ad5678b086 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Bug fixes - Fix tracing in precompiled contracts when halting for out of gas [#7318](https://github.com/hyperledger/besu/issues/7318) - Correctly release txpool save and restore lock in case of exceptions [#7473](https://github.com/hyperledger/besu/pull/7473) +- Fix for `eth_gasPrice` could not retrieve block error [#7482](https://github.com/hyperledger/besu/pull/7482) ## 24.8.0 diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java index 16b683ed712..6e0f0e3e35c 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java @@ -60,6 +60,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.LongStream; +import java.util.stream.Stream; import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.units.bigints.UInt256; @@ -976,27 +977,35 @@ public Optional getAndMapWorldState( } public Wei gasPrice() { - final long blockHeight = headBlockNumber(); - final var chainHeadHeader = blockchain.getChainHeadHeader(); + final Block chainHeadBlock = blockchain.getChainHeadBlock(); + final var chainHeadHeader = chainHeadBlock.getHeader(); + final long blockHeight = chainHeadHeader.getNumber(); + final var nextBlockProtocolSpec = protocolSchedule.getForNextBlockHeader(chainHeadHeader, System.currentTimeMillis()); final var nextBlockFeeMarket = nextBlockProtocolSpec.getFeeMarket(); + final Wei[] gasCollection = - LongStream.rangeClosed( - Math.max(0, blockHeight - apiConfig.getGasPriceBlocks() + 1), blockHeight) - .mapToObj( - l -> - blockchain - .getBlockByNumber(l) - .map(Block::getBody) - .map(BlockBody::getTransactions) - .orElseThrow( - () -> new IllegalStateException("Could not retrieve block #" + l))) + Stream.concat( + LongStream.range( + Math.max(0, blockHeight - apiConfig.getGasPriceBlocks() + 1), blockHeight) + .mapToObj( + l -> + blockchain + .getBlockByNumber(l) + .orElseThrow( + () -> + new IllegalStateException( + "Could not retrieve block #" + l))), + Stream.of(chainHeadBlock)) + .map(Block::getBody) + .map(BlockBody::getTransactions) .flatMap(Collection::stream) .filter(t -> t.getGasPrice().isPresent()) .map(t -> t.getGasPrice().get()) .sorted() .toArray(Wei[]::new); + return (gasCollection == null || gasCollection.length == 0) ? gasPriceLowerBound(chainHeadHeader, nextBlockFeeMarket) : UInt256s.max( diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGasPriceTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGasPriceTest.java index 54e17cb21cb..5d6aba76942 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGasPriceTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGasPriceTest.java @@ -17,7 +17,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -61,7 +63,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mock; -import org.mockito.internal.verification.VerificationModeFactory; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -106,8 +107,8 @@ public void shouldReturnMinValueWhenNoTransactionsExist() { final JsonRpcResponse actualResponse = method.response(request); assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse); - verify(blockchain).getChainHeadBlockNumber(); - verify(blockchain, VerificationModeFactory.times(100)).getBlockByNumber(anyLong()); + verify(blockchain).getChainHeadBlock(); + verify(blockchain, times(99)).getBlockByNumber(anyLong()); verifyNoMoreInteractions(blockchain); } @@ -127,8 +128,7 @@ public void shouldReturnBaseFeeAsMinValueOnGenesis() { final JsonRpcResponse actualResponse = method.response(request); assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse); - verify(blockchain).getChainHeadBlockNumber(); - verify(blockchain, VerificationModeFactory.times(1)).getBlockByNumber(anyLong()); + verify(blockchain).getChainHeadBlock(); verifyNoMoreInteractions(blockchain); } @@ -146,8 +146,8 @@ public void shouldReturnMedianWhenTransactionsExist() { final JsonRpcResponse actualResponse = method.response(request); assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse); - verify(blockchain).getChainHeadBlockNumber(); - verify(blockchain, VerificationModeFactory.times(100)).getBlockByNumber(anyLong()); + verify(blockchain).getChainHeadBlock(); + verify(blockchain, times(99)).getBlockByNumber(anyLong()); verifyNoMoreInteractions(blockchain); } @@ -165,8 +165,8 @@ public void shortChainQueriesAllBlocks() { final JsonRpcResponse actualResponse = method.response(request); assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse); - verify(blockchain).getChainHeadBlockNumber(); - verify(blockchain, VerificationModeFactory.times(81)).getBlockByNumber(anyLong()); + verify(blockchain).getChainHeadBlock(); + verify(blockchain, times(80)).getBlockByNumber(anyLong()); verifyNoMoreInteractions(blockchain); } @@ -252,8 +252,7 @@ public void ethGasPriceAtGenesis( final JsonRpcResponse actualResponse = method.response(request); assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse); - verify(blockchain).getChainHeadBlockNumber(); - verify(blockchain, VerificationModeFactory.times(1)).getBlockByNumber(anyLong()); + verify(blockchain).getChainHeadBlock(); verifyNoMoreInteractions(blockchain); } @@ -328,12 +327,14 @@ private void mockBlockchain( blocksByNumber.put(i, createFakeBlock(i, txsNum, baseFee)); } - when(blockchain.getChainHeadBlockNumber()).thenReturn(chainHeadBlockNumber); - when(blockchain.getBlockByNumber(anyLong())) - .thenAnswer( - invocation -> Optional.of(blocksByNumber.get(invocation.getArgument(0, Long.class)))); - - when(blockchain.getChainHeadHeader()) + when(blockchain.getChainHeadBlock()).thenReturn(blocksByNumber.get(chainHeadBlockNumber)); + if (chainHeadBlockNumber > 1) { + when(blockchain.getBlockByNumber(anyLong())) + .thenAnswer( + invocation -> Optional.of(blocksByNumber.get(invocation.getArgument(0, Long.class)))); + } + lenient() + .when(blockchain.getChainHeadHeader()) .thenReturn(blocksByNumber.get(chainHeadBlockNumber).getHeader()); } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/Blockchain.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/Blockchain.java index 3c76ba69f5c..49b6fea33e1 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/Blockchain.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/Blockchain.java @@ -167,6 +167,15 @@ default boolean contains(final Hash blockHash) { */ Optional getBlockBody(Hash blockHeaderHash); + /** + * Safe version of {@code getBlockBody} (it should take any locks necessary to ensure any block + * updates that might be taking place have been completed first) + * + * @param blockHeaderHash The hash of the block whose header we want to retrieve. + * @return The block body corresponding to this block hash. + */ + Optional getBlockBodySafe(Hash blockHeaderHash); + /** * Given a block's hash, returns the list of transaction receipts associated with this block's * transactions. Associated block is not necessarily on the canonical chain. diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java index 04eaaa6a353..5849f3e676c 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java @@ -299,7 +299,10 @@ public BlockHeader getChainHeadHeader() { @Override public Block getChainHeadBlock() { - return new Block(chainHeader, blockchainStorage.getBlockBody(chainHeader.getHash()).get()); + return new Block( + chainHeader, + getBlockBody(chainHeader.getHash()) + .orElseGet(() -> getBlockBodySafe(chainHeader.getHash()).get())); } @Override @@ -337,6 +340,11 @@ public Optional getBlockBody(final Hash blockHeaderHash) { .orElseGet(() -> blockchainStorage.getBlockBody(blockHeaderHash)); } + @Override + public synchronized Optional getBlockBodySafe(final Hash blockHeaderHash) { + return getBlockBody(blockHeaderHash); + } + @Override public Optional> getTxReceipts(final Hash blockHeaderHash) { return transactionReceiptsCache diff --git a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/t8n/T8nBlockchain.java b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/t8n/T8nBlockchain.java index 5e31464ee61..c5905967fc2 100644 --- a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/t8n/T8nBlockchain.java +++ b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/t8n/T8nBlockchain.java @@ -145,6 +145,11 @@ public Optional getBlockBody(final Hash blockHeaderHash) { throw new UnsupportedOperationException(); } + @Override + public synchronized Optional getBlockBodySafe(final Hash blockHeaderHash) { + return getBlockBody(blockHeaderHash); + } + @Override public Optional> getTxReceipts(final Hash blockHeaderHash) { // Deterministic, but just not implemented. diff --git a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java index 3773c8d1f05..8f03a01caa7 100644 --- a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java +++ b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java @@ -142,6 +142,11 @@ public Optional getBlockBody(final Hash blockHeaderHash) { throw new UnsupportedOperationException(); } + @Override + public synchronized Optional getBlockBodySafe(final Hash blockHeaderHash) { + return getBlockBody(blockHeaderHash); + } + @Override public Optional> getTxReceipts(final Hash blockHeaderHash) { // Deterministic, but just not implemented.