Log index is counted per block, not per transaction (hyperledger#4355)

* Log index is counted per block, not per transaction

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Co-authored-by: mark-terry <mark.terry@consensys.net>

CHANGELOG.md

@@ -8,6 +8,7 @@
 - Add Mainnet to merged networks [#4463](https://github.com/hyperledger/besu/pull/4463)
 
 ### Bug Fixes
+- Fixed logIndex value returned by eth_getLogs RPC call [#4355](https://github.com/hyperledger/besu/pull/4355)
 ### Download Links
 
 ## 22.7.4

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/graphql/internal/pojoadapter/TransactionAdapter.java

@@ -21,6 +21,7 @@
 import org.hyperledger.besu.ethereum.api.query.BlockchainQueries;
 import org.hyperledger.besu.ethereum.api.query.TransactionReceiptWithMetadata;
 import org.hyperledger.besu.ethereum.api.query.TransactionWithMetadata;
+import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.LogWithMetadata;
 import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.TransactionReceipt;
@@ -168,18 +169,25 @@ public Optional<AccountAdapter> getCreatedContract(final DataFetchingEnvironment
   public List<LogAdapter> getLogs(final DataFetchingEnvironment environment) {
     final BlockchainQueries query = getBlockchainQueries(environment);
     final Hash hash = transactionWithMetadata.getTransaction().getHash();
+
+    final Optional<BlockHeader> maybeBlockHeader =
+        transactionWithMetadata.getBlockNumber().flatMap(query::getBlockHeaderByNumber);
+
+    if (maybeBlockHeader.isEmpty()) {
+      throw new RuntimeException(
+          "Cannot get block ("
+              + transactionWithMetadata.getBlockNumber()
+              + ") for transaction "
+              + transactionWithMetadata.getTransaction().getHash());
+    }
+
     final Optional<TransactionReceiptWithMetadata> maybeTransactionReceiptWithMetadata =
         query.transactionReceiptByTransactionHash(hash);
     final List<LogAdapter> results = new ArrayList<>();
     if (maybeTransactionReceiptWithMetadata.isPresent()) {
       final List<LogWithMetadata> logs =
-          LogWithMetadata.generate(
-              maybeTransactionReceiptWithMetadata.get().getReceipt(),
-              transactionWithMetadata.getBlockNumber().get(),
-              transactionWithMetadata.getBlockHash().get(),
-              hash,
-              transactionWithMetadata.getTransactionIndex().get(),
-              false);
+          query.matchingLogs(
+              maybeBlockHeader.get().getBlockHash(), transactionWithMetadata, () -> true);
       for (final LogWithMetadata log : logs) {
         results.add(new LogAdapter(log));
       }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java

@@ -47,6 +47,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -752,43 +753,33 @@ private List<LogWithMetadata> matchingLogsCached(
   public List<LogWithMetadata> matchingLogs(
       final Hash blockHash, final LogsQuery query, final Supplier<Boolean> isQueryAlive) {
     try {
-      final Optional<BlockHeader> blockHeader =
-          BackendQuery.runIfAlive(
-              "matchingLogs - getBlockHeader",
-              () -> blockchain.getBlockHeader(blockHash),
-              isQueryAlive);
+      final Optional<BlockHeader> blockHeader = getBlockHeader(blockHash, isQueryAlive);
       if (blockHeader.isEmpty()) {
         return Collections.emptyList();
       }
       // receipts and transactions should exist if the header exists, so throwing is ok.
-      final List<TransactionReceipt> receipts =
-          BackendQuery.runIfAlive(
-              "matchingLogs - getTxReceipts",
-              () -> blockchain.getTxReceipts(blockHash).orElseThrow(),
-              isQueryAlive);
-      final List<Transaction> transactions =
-          BackendQuery.runIfAlive(
-              "matchingLogs - getBlockBody",
-              () -> blockchain.getBlockBody(blockHash).orElseThrow().getTransactions(),
-              isQueryAlive);
+      final List<TransactionReceipt> receipts = getReceipts(blockHash, isQueryAlive);
+      final List<Transaction> transactions = getTransactions(blockHash, isQueryAlive);
       final long number = blockHeader.get().getNumber();
-      final boolean removed =
-          BackendQuery.runIfAlive(
-              "matchingLogs - blockIsOnCanonicalChain",
-              () -> !blockchain.blockIsOnCanonicalChain(blockHash),
-              isQueryAlive);
+      final boolean removed = getRemoved(blockHash, isQueryAlive);
+
+      final AtomicInteger logIndexOffset = new AtomicInteger();
       return IntStream.range(0, receipts.size())
           .mapToObj(
               i -> {
                 try {
                   BackendQuery.stopIfExpired(isQueryAlive);
-                  return LogWithMetadata.generate(
-                      receipts.get(i),
-                      number,
-                      blockHash,
-                      transactions.get(i).getHash(),
-                      i,
-                      removed);
+                  final List<LogWithMetadata> result =
+                      LogWithMetadata.generate(
+                          logIndexOffset.intValue(),
+                          receipts.get(i),
+                          number,
+                          blockHash,
+                          transactions.get(i).getHash(),
+                          i,
+                          removed);
+                  logIndexOffset.addAndGet(receipts.get(i).getLogs().size());
+                  return result;
                 } catch (final Exception e) {
                   throw new RuntimeException(e);
                 }
@@ -801,6 +792,47 @@ public List<LogWithMetadata> matchingLogs(
     }
   }
 
+  public List<LogWithMetadata> matchingLogs(
+      final Hash blockHash,
+      final TransactionWithMetadata transactionWithMetaData,
+      final Supplier<Boolean> isQueryAlive) {
+    if (transactionWithMetaData.getTransactionIndex().isEmpty()) {
+      throw new RuntimeException(
+          "Cannot find logs because transaction "
+              + transactionWithMetaData.getTransaction().getHash()
+              + " does not have a transaction index");
+    }
+
+    try {
+      final Optional<BlockHeader> blockHeader = getBlockHeader(blockHash, isQueryAlive);
+      if (blockHeader.isEmpty()) {
+        return Collections.emptyList();
+      }
+      // receipts and transactions should exist if the header exists, so throwing is ok.
+      final List<TransactionReceipt> receipts = getReceipts(blockHash, isQueryAlive);
+      final List<Transaction> transactions = getTransactions(blockHash, isQueryAlive);
+      final long number = blockHeader.get().getNumber();
+      final boolean removed = getRemoved(blockHash, isQueryAlive);
+
+      final int transactionIndex = transactionWithMetaData.getTransactionIndex().get();
+      final int logIndexOffset =
+          logIndexOffset(
+              transactionWithMetaData.getTransaction().getHash(), receipts, transactions);
+
+      return LogWithMetadata.generate(
+          logIndexOffset,
+          receipts.get(transactionIndex),
+          number,
+          blockHash,
+          transactions.get(transactionIndex).getHash(),
+          transactionIndex,
+          removed);
+
+    } catch (final Exception e) {
+      throw new RuntimeException(e);
+    }
+  }
+
   /**
    * Returns the world state for the corresponding block number
    *
@@ -889,4 +921,50 @@ private List<TransactionWithMetadata> formatTransactions(
   private boolean outsideBlockchainRange(final long blockNumber) {
     return blockNumber > headBlockNumber() || blockNumber < BlockHeader.GENESIS_BLOCK_NUMBER;
   }
+
+  private Boolean getRemoved(final Hash blockHash, final Supplier<Boolean> isQueryAlive)
+      throws Exception {
+    return BackendQuery.runIfAlive(
+        "matchingLogs - blockIsOnCanonicalChain",
+        () -> !blockchain.blockIsOnCanonicalChain(blockHash),
+        isQueryAlive);
+  }
+
+  private List<Transaction> getTransactions(
+      final Hash blockHash, final Supplier<Boolean> isQueryAlive) throws Exception {
+    return BackendQuery.runIfAlive(
+        "matchingLogs - getBlockBody",
+        () -> blockchain.getBlockBody(blockHash).orElseThrow().getTransactions(),
+        isQueryAlive);
+  }
+
+  private List<TransactionReceipt> getReceipts(
+      final Hash blockHash, final Supplier<Boolean> isQueryAlive) throws Exception {
+    return BackendQuery.runIfAlive(
+        "matchingLogs - getTxReceipts",
+        () -> blockchain.getTxReceipts(blockHash).orElseThrow(),
+        isQueryAlive);
+  }
+
+  private Optional<BlockHeader> getBlockHeader(
+      final Hash blockHash, final Supplier<Boolean> isQueryAlive) throws Exception {
+    return BackendQuery.runIfAlive(
+        "matchingLogs - getBlockHeader", () -> blockchain.getBlockHeader(blockHash), isQueryAlive);
+  }
+
+  private int logIndexOffset(
+      final Hash transactionHash,
+      final List<TransactionReceipt> receipts,
+      final List<Transaction> transactions) {
+    int logIndexOffset = 0;
+    for (int i = 0; i < receipts.size(); i++) {
+      if (transactions.get(i).getHash().equals(transactionHash)) {
+        break;
+      }
+
+      logIndexOffset += receipts.get(i).getLogs().size();
+    }
+
+    return logIndexOffset;
+  }
 }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/PrivacyQueries.java

@@ -26,6 +26,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import java.util.stream.LongStream;
@@ -83,16 +84,24 @@ public List<LogWithMetadata> matchingLogs(
     final long blockNumber = blockHeader.get().getNumber();
     final boolean removed = !blockchainQueries.blockIsOnCanonicalChain(blockHash);
 
+    final AtomicInteger logIndexOffset = new AtomicInteger();
     return IntStream.range(0, privateTransactionReceiptList.size())
         .mapToObj(
-            i ->
-                LogWithMetadata.generate(
-                    privateTransactionReceiptList.get(i),
-                    blockNumber,
-                    blockHash,
-                    privateTransactionMetadataList.get(i).getPrivateMarkerTransactionHash(),
-                    findPMTIndex(pmtHashList.get(i)),
-                    removed))
+            i -> {
+              final List<LogWithMetadata> result =
+                  LogWithMetadata.generate(
+                      logIndexOffset.intValue(),
+                      privateTransactionReceiptList.get(i),
+                      blockNumber,
+                      blockHash,
+                      privateTransactionMetadataList.get(i).getPrivateMarkerTransactionHash(),
+                      findPMTIndex(pmtHashList.get(i)),
+                      removed);
+
+              logIndexOffset.addAndGet(privateTransactionReceiptList.get(i).getLogs().size());
+
+              return result;
+            })
         .flatMap(Collection::stream)
         .filter(query::matches)
         .collect(Collectors.toList());

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/filter/FilterManagerLogFilterTest.java

@@ -112,7 +112,7 @@ public void shouldUseBlockHashWhenCheckingLogsForChangesForPrivateFiltersOnly()
 
     final Hash blockAddedHash = recordBlockEvents(1).get(0).getBlock().getHash();
 
-    verify(blockchainQueries, never()).matchingLogs(any(), any(), any());
+    verify(blockchainQueries, never()).matchingLogs(any(Hash.class), any(LogsQuery.class), any());
     verify(privacyQueries).matchingLogs(eq(PRIVACY_GROUP_ID), eq(blockAddedHash), eq(logsQuery()));
   }
 

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/websocket/subscription/logs/LogsSubscriptionServiceTest.java

@@ -90,8 +90,9 @@ public void singleMatchingLogEvent() {
     final List<TransactionReceipt> receipts = blockWithReceipts.getReceipts();
 
     final int txIndex = 1;
-    final int logIndex = 1;
-    final Log targetLog = receipts.get(txIndex).getLogsList().get(logIndex);
+    final int logIndexInTransaction = 1;
+    final int logIndexInBlock = 3; // third log in the block
+    final Log targetLog = receipts.get(txIndex).getLogsList().get(logIndexInTransaction);
 
     final LogsSubscription subscription = createSubscription(targetLog.getLogger());
     registerSubscriptions(subscription);
@@ -104,7 +105,8 @@ public void singleMatchingLogEvent() {
 
     assertThat(logResults).hasSize(1);
     final LogResult result = logResults.get(0);
-    assertLogResultMatches(result, block, receipts, txIndex, logIndex, false);
+    assertLogResultMatches(
+        result, block, receipts, txIndex, logIndexInTransaction, logIndexInBlock, false);
   }
 
   @Test
@@ -115,8 +117,8 @@ public void singleMatchingLogEmittedThenRemovedInReorg() {
     final List<TransactionReceipt> receipts = blockWithReceipts.getReceipts();
 
     final int txIndex = 1;
-    final int logIndex = 1;
-    final Log targetLog = receipts.get(txIndex).getLogsList().get(logIndex);
+    final int logListIndex = 1;
+    final Log targetLog = receipts.get(txIndex).getLogsList().get(logListIndex);
 
     final LogsSubscription subscription = createSubscription(targetLog.getLogger());
     registerSubscriptions(subscription);
@@ -138,9 +140,11 @@ public void singleMatchingLogEmittedThenRemovedInReorg() {
 
     assertThat(logResults).hasSize(2);
     final LogResult firstLog = logResults.get(0);
-    assertLogResultMatches(firstLog, block, receipts, txIndex, logIndex, false);
+    // third log in the block
+    assertLogResultMatches(firstLog, block, receipts, txIndex, logListIndex, 3, false);
     final LogResult secondLog = logResults.get(1);
-    assertLogResultMatches(secondLog, block, receipts, txIndex, logIndex, true);
+    // third log in the block, but was removed
+    assertLogResultMatches(secondLog, block, receipts, txIndex, logListIndex, 3, true);
   }
 
   @Test
@@ -151,8 +155,8 @@ public void singleMatchingLogEmittedThenMovedInReorg() {
     final List<TransactionReceipt> receipts = blockWithReceipts.getReceipts();
 
     final int txIndex = 1;
-    final int logIndex = 1;
-    final Log targetLog = receipts.get(txIndex).getLogsList().get(logIndex);
+    final int logListIndex = 1;
+    final Log targetLog = receipts.get(txIndex).getLogsList().get(logListIndex);
 
     final LogsSubscription subscription = createSubscription(targetLog.getLogger());
     registerSubscriptions(subscription);
@@ -181,12 +185,12 @@ public void singleMatchingLogEmittedThenMovedInReorg() {
 
     assertThat(logResults).hasSize(3);
     final LogResult originalLog = logResults.get(0);
-    assertLogResultMatches(originalLog, block, receipts, txIndex, logIndex, false);
+    assertLogResultMatches(originalLog, block, receipts, txIndex, logListIndex, 3, false);
     final LogResult removedLog = logResults.get(1);
-    assertLogResultMatches(removedLog, block, receipts, txIndex, logIndex, true);
+    assertLogResultMatches(removedLog, block, receipts, txIndex, logListIndex, 3, true);
     final LogResult updatedLog = logResults.get(2);
     assertLogResultMatches(
-        updatedLog, newBlockWithLog.getBlock(), newBlockWithLog.getReceipts(), 0, 0, false);
+        updatedLog, newBlockWithLog.getBlock(), newBlockWithLog.getReceipts(), 0, 0, 0, false);
   }
 
   @Test
@@ -218,6 +222,9 @@ public void multipleMatchingLogsEmitted() {
 
     // Verify all logs are emitted
     assertThat(logResults).hasSize(targetBlocks.size() * txCount);
+
+    final List<Integer> expectedLogIndex = Arrays.asList(0, 2);
+
     for (int i = 0; i < targetBlocks.size(); i++) {
       final BlockWithReceipts targetBlock = targetBlocks.get(i);
       for (int j = 0; j < txCount; j++) {
@@ -228,6 +235,7 @@ public void multipleMatchingLogsEmitted() {
             targetBlock.getReceipts(),
             j,
             0,
+            expectedLogIndex.get(j),
             false);
       }
     }
@@ -259,7 +267,7 @@ public void multipleSubscriptionsForSingleMatchingLog() {
 
       assertThat(logResults).hasSize(1);
       final LogResult result = logResults.get(0);
-      assertLogResultMatches(result, block, receipts, txIndex, logIndex, false);
+      assertLogResultMatches(result, block, receipts, txIndex, logIndex, 3, false);
     }
   }
 
@@ -332,10 +340,11 @@ private void assertLogResultMatches(
       final Block block,
       final List<TransactionReceipt> receipts,
       final int txIndex,
+      final int logListIndex,
       final int logIndex,
       final boolean isRemoved) {
     final Transaction expectedTransaction = block.getBody().getTransactions().get(txIndex);
-    final Log expectedLog = receipts.get(txIndex).getLogsList().get(logIndex);
+    final Log expectedLog = receipts.get(txIndex).getLogsList().get(logListIndex);
 
     assertThat(result.getLogIndex()).isEqualTo(Quantity.create(logIndex));
     assertThat(result.getTransactionIndex()).isEqualTo(Quantity.create(txIndex));