Synchronize access to block header (hyperledger#6143)

* Synchronize access to block header Signed-off-by: Matthew Whitehead <[email protected]> * Add 'safe' version of getBlockHeader Signed-off-by: Matthew Whitehead <[email protected]> * Move retry with lock into getChainHeadBlockHeader() Signed-off-by: Matthew Whitehead <[email protected]> * Reinstate 'final' modifier Signed-off-by: Matthew Whitehead <[email protected]> --------- Signed-off-by: Matthew Whitehead <[email protected]> Signed-off-by: Justin Florentine <[email protected]>
jflo · Nov 20, 2023 · e034abe · e034abe
1 parent 3f9f0a5
commit e034abe
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 1 deletion.
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
@@ -148,6 +148,15 @@ default boolean contains(final Hash blockHash) {
    */
   Optional<BlockHeader> getBlockHeader(Hash blockHeaderHash);
 
+  /**
+   * Safe version of {@code getBlockHeader} (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 header corresponding to this block hash.
+   */
+  Optional<BlockHeader> getBlockHeaderSafe(Hash blockHeaderHash);
+
   /**
    * Returns the block body corresponding to the given block header hash. 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
@@ -302,6 +302,16 @@ public Optional<BlockHeader> getBlockHeader(final Hash blockHeaderHash) {
         .orElseGet(() -> blockchainStorage.getBlockHeader(blockHeaderHash));
   }
 
+  @Override
+  public synchronized Optional<BlockHeader> getBlockHeaderSafe(final Hash blockHeaderHash) {
+    return blockHeadersCache
+        .map(
+            cache ->
+                Optional.ofNullable(cache.getIfPresent(blockHeaderHash))
+                    .or(() -> blockchainStorage.getBlockHeader(blockHeaderHash)))
+        .orElseGet(() -> blockchainStorage.getBlockHeader(blockHeaderHash));
+  }
+
   @Override
   public Optional<BlockBody> getBlockBody(final Hash blockHeaderHash) {
     return blockBodiesCache

diff --git a/...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java b/...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java
@@ -551,7 +551,14 @@ public Optional<Transaction> getTransactionByHash(final Hash hash) {
 
   private Optional<BlockHeader> getChainHeadBlockHeader() {
     final MutableBlockchain blockchain = protocolContext.getBlockchain();
-    return blockchain.getBlockHeader(blockchain.getChainHeadHash());
+
+    // 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 blockchain
+        .getBlockHeader(blockchain.getChainHeadHash())
+        .or(() -> blockchain.getBlockHeaderSafe(blockchain.getChainHeadHash()));
   }
 
   private boolean isLocalSender(final Address sender) {

diff --git a/...s/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java b/...s/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java
@@ -132,6 +132,11 @@ public Optional<BlockHeader> getBlockHeader(final Hash blockHeaderHash) {
     return Optional.ofNullable(hashToHeader.get(blockHeaderHash));
   }
 
+  @Override
+  public synchronized Optional<BlockHeader> getBlockHeaderSafe(final Hash blockHeaderHash) {
+    return Optional.ofNullable(hashToHeader.get(blockHeaderHash));
+  }
+
   @Override
   public Optional<BlockBody> getBlockBody(final Hash blockHeaderHash) {
     // Deterministic, but just not implemented.