implement engine_getBlobsV1 #7553

pinges · 2024-09-02T03:53:09Z

PR description

Implements the engine RPC engine_getBlobsV1 as described in ethereum/execution-apis#559

Fixed Issue(s)

#7445

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

...src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlobAndProofV1.java

macfarla · 2024-09-02T21:53:48Z

...java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetBlobsV1.java

+    return blobQuad;
+  }
+
+  public void onTransactionAdded(final Transaction transaction) {


idk if the RPC should be itself managing the blob cache?

I was wondering where that should live. The other option would be the TransactionPool. The BlobCache is taken care of in the TransactionPool, because these blobs are needed in case we have a reorg.
The blobMap is not needed by the TransactionPool. Here we just keep track of the blobs that we have available in our transaction pool and we have to update the map when blob transactions are added or removed from the pool.

I have moved that logic into the TransactionPool

.../org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetBlobsV1Test.java

macfarla · 2024-09-02T21:56:14Z

...kcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/PoWBlockCreatorTest.java

@@ -371,7 +372,8 @@ private TransactionPool createTransactionPool(
            mock(TransactionBroadcaster.class),
            ethContext,
            new TransactionPoolMetrics(metricsSystem),
-            poolConf);
+            poolConf,
+            new BlobCache());


@jflo can we daggerise the BlobCache or does it need other infra first?

It is kind of daggerised. It is available in BesuComponent, but I did not know how to use that in the engine RPC. Maybe @jflo can tell us!

To make this a more complete approach, the RPC and the TransactionPool would need to be daggerized as well, which would allow all intermediate references to be removed.

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

…sonrpc/internal/methods/engine/EngineGetBlobsV1Test.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com>

fab-10

LGTM, see the note on the of passing blobCache, then add a changelog entry and doc-change-required label

fab-10 · 2024-09-03T10:08:37Z

...java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetBlobsV1.java

+    if (blobQuad == null) {
+      blobQuad = blobCache.get(vh);
+    }


in theory this fallback should not be necessary, and removing the need to have the blobCache will simplify a lot the change, removing the need to pass it around, or am I missing something?

I also don't understand. It seems to me that blobMap is redundant, and this RPC should just use the same instance of the blobCache that the transactionpool does. Management of the cache contents should be ignored by the RPC and treated as a "read only" concern.

The blob cache does contain all the blobs that have been put into a block recently. These blobs are used to re-add the blob transactions in case of a reorg, as the blobs are not part of the block. After 3 epochs these blobs are removed from the cache. Blobs that are in the cache are not part of Transactions that are in the pool.
The blob map keeps track of all the blobs that are part of transactions that are in our transaction pool. These are the blobs that the CL will most likely ask for. We do keep them in the map for as long as their transactions are in the transaction pool.
@fab-10 @jflo

According to the specification of the new method, the CL should be interested in blobs that are in the txpool, and not blobs that have been already included in a block, so blobMap should be enough, because in case of a reorg a new add notification will be sent.

fab-10 · 2024-09-03T10:11:27Z

@pinges note that #7539 and #7538 are a prerequisite for this one, otherwise it will not work correctly, so please do not merge it before the other 2 are in.

jflo

Need to remove the responsibility of managing blobs from the RPC method.

jflo · 2024-09-03T14:12:15Z

...java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetBlobsV1.java

+    if (blobQuad == null) {
+      blobQuad = blobCache.get(vh);
+    }


I also don't understand. It seems to me that blobMap is redundant, and this RPC should just use the same instance of the blobCache that the transactionpool does. Management of the cache contents should be ignored by the RPC and treated as a "read only" concern.

jflo · 2024-09-03T14:13:27Z

...java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetBlobsV1.java

+ */
+public class EngineGetBlobsV1 extends ExecutionEngineJsonRpcMethod {
+
+  private final Map<VersionedHash, BlobsWithCommitments.BlobQuad> blobMap = new HashMap<>();


I think naming this something more specific would help me understand why it is necessary. As is, it reads like a synonym for blobCache, and seems like duplication.

jflo · 2024-09-03T14:23:54Z

...kcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/PoWBlockCreatorTest.java

@@ -371,7 +372,8 @@ private TransactionPool createTransactionPool(
            mock(TransactionBroadcaster.class),
            ethContext,
            new TransactionPoolMetrics(metricsSystem),
-            poolConf);
+            poolConf,
+            new BlobCache());


To make this a more complete approach, the RPC and the TransactionPool would need to be daggerized as well, which would allow all intermediate references to be removed.

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

…_getBlobV1

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

fab-10 · 2024-09-05T09:29:38Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

@@ -104,6 +107,7 @@ public class TransactionPool implements BlockAddedObserver {
  private final SaveRestoreManager saveRestoreManager = new SaveRestoreManager();
  private final Set<Address> localSenders = ConcurrentHashMap.newKeySet();
  private final EthScheduler.OrderedProcessor<BlockAddedEvent> blockAddedEventOrderedProcessor;
+  private final Map<VersionedHash, BlobsWithCommitments.BlobQuad> blobMap = new HashMap<>();


what about a more talking name, like liveBlobs or pendingBlobs?

fab-10 · 2024-09-05T09:36:23Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

+  private void mapBlobsOnTransactionAdded(
+      final org.hyperledger.besu.datatypes.Transaction transaction) {
+    final Optional<BlobsWithCommitments> maybeBlobsWithCommitments =
+        transaction.getBlobsWithCommitments();
+    if (maybeBlobsWithCommitments.isEmpty()) {
+      return;
+    }
+    final List<BlobsWithCommitments.BlobQuad> blobQuads =
+        maybeBlobsWithCommitments.get().getBlobQuads();
+    blobQuads.forEach(bq -> blobMap.put(bq.versionedHash(), bq));
+  }
+
+  private void unmapBlobsOnTransactionDropped(
+      final org.hyperledger.besu.datatypes.Transaction transaction) {
+    final Optional<BlobsWithCommitments> maybeBlobsWithCommitments =
+        transaction.getBlobsWithCommitments();
+    if (maybeBlobsWithCommitments.isEmpty()) {
+      return;
+    }
+    final List<BlobsWithCommitments.BlobQuad> blobQuads =
+        maybeBlobsWithCommitments.get().getBlobQuads();
+    blobQuads.forEach(bq -> blobMap.remove(bq.versionedHash()));
+  }
+
+  public BlobsWithCommitments.BlobQuad getBlobQuad(final VersionedHash vh) {
+    BlobsWithCommitments.BlobQuad blobQuad = blobMap.get(vh);
+    if (blobQuad == null) {
+      blobQuad = blobCache.get(vh);
+    }
+    return blobQuad;
+  }
+


it is better to have the logic here, and I am tempted to suggest to go a step further and extending the current BlobCache to manage both the confirmed blobs, as it does now, plus those pending blobs. (However not a blocking request, just a suggestion to manage blobs in a single place)

I was thinking about that as well, but it felt like these are two different concerns and I think that the "map" should be part of the Transaction pool, as it points to the blobs of transactions in the pool.

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

jflo

Much better. The only suggestion I have would be to consider using another instance of BlobCache internally for the pooled transaction, instead of just a map. That would afford some protection from unexpected cache growth, but I think trusting the add/drop tx events is a safe assumption.

fab-10 · 2024-09-05T13:34:19Z

Much better. The only suggestion I have would be to consider using another instance of BlobCache internally for the pooled transaction, instead of just a map. That would afford some protection from unexpected cache growth, but I think trusting the add/drop tx events is a safe assumption.

That's a good point, and in the related PRs that are fixing the notifications in the layered txpool, I will make sure they are reliable, but I think it is worth to export a metrics with the size of the map, so we can monitor its growth, but this could be part of a following PR.

fab-10

LGTM, just move the changelog entry to the right place, and add doc-change-required label

fab-10 · 2024-09-05T13:35:24Z

CHANGELOG.md

@@ -26,6 +26,7 @@
 - Performance optimzation for ECMUL (2 of 2) [#7543](https://github.com/hyperledger/besu/pull/7543)
 - Include current chain head block when computing `eth_maxPriorityFeePerGas` [#7485](https://github.com/hyperledger/besu/pull/7485)
 - Remove (old) documentation updates from the changelog [#7562](https://github.com/hyperledger/besu/pull/7562)
+- Add `engine_getBlobsV1` method to the Engine API [#7553](https://github.com/hyperledger/besu/pull/7553)


Need to move this to the Unreleased section

ups! Fixed!

fab-10 · 2024-09-05T13:52:42Z

FYI: task for adding the metrics #7578

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

fab-10 · 2024-09-06T07:37:29Z

CHANGELOG.md

@@ -40,7 +40,7 @@
 - Correctly drops messages that exceeds local message size limit [#5455](https://github.com/hyperledger/besu/pull/7507)
 - **DebugMetrics**: Fixed a `ClassCastException` occurring in `DebugMetrics` when handling nested metric structures. Previously, `Double` values within these structures were incorrectly cast to `Map` objects, leading to errors. This update allows for proper handling of both direct values and nested structures at the same level. Issue# [#7383](https://github.com/hyperledger/besu/pull/7383)
 - `evmtool` was not respecting the `--genesis` setting, resulting in unexpected trace results. [#7433](https://github.com/hyperledger/besu/pull/7433)
- The genesis config override `contractSizeLimit` was not wired into code size limits [#7557](https://github.com/hyperledger/besu/pull/7557)
+- The genesis config override `contractSizeLimit`q was not wired into code size limits [#7557](https://github.com/hyperledger/besu/pull/7557)


karalabe · 2024-10-02T11:11:26Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

+    }
+    final List<BlobsWithCommitments.BlobQuad> blobQuads =
+        maybeBlobsWithCommitments.get().getBlobQuads();
+    blobQuads.forEach(bq -> mapOfBlobsInTransactionPool.put(bq.versionedHash(), bq));


This can be used my a malicious transaction to corrupt the mapping by sending the same blobs as a benign tx. Then send a replacement to nuke out all blobs, and the original correct mapping disappears, making the blob non-retrievable any more.

pinges added 2 commits September 2, 2024 13:26

implement engine_getBlobsV1

5fa9138

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

merge main

0a8de78

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

macfarla reviewed Sep 2, 2024

View reviewed changes

pinges and others added 3 commits September 3, 2024 13:20

fix java doc

d9c272b

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into engine_getBlobV1

9c3f51f

Update ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/j…

c4324c3

…sonrpc/internal/methods/engine/EngineGetBlobsV1Test.java Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com>

fab-10 reviewed Sep 3, 2024

View reviewed changes

jflo requested changes Sep 3, 2024

View reviewed changes

pinges added 5 commits September 4, 2024 16:00

fix test

e944868

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into engine_getBlobV1

5d51a16

Merge branch 'engine_getBlobV1' of github.com:pinges/besu into engine…

8350a15

…_getBlobV1

Merge branch 'main' of github.com:hyperledger/besu into engine_getBlobV1

c1e1a46

move logic into transaction pool

300a035

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

fab-10 reviewed Sep 5, 2024

View reviewed changes

pinges added 2 commits September 5, 2024 19:54

changes after reviews

110d6bd

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into engine_getBlobV1

4ca1dc1

pinges requested a review from jflo September 5, 2024 10:00

pinges added 2 commits September 5, 2024 20:03

remove unused mock

cdec0d1

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

spotless

bdbd0cc

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

jflo approved these changes Sep 5, 2024

View reviewed changes

fab-10 approved these changes Sep 5, 2024

View reviewed changes

fab-10 requested changes Sep 5, 2024

View reviewed changes

pinges added 2 commits September 6, 2024 09:34

move chnagelog entry

a066ed7

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

merge main

5797063

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

pinges enabled auto-merge (squash) September 5, 2024 23:43

pinges added the doc-change-required Indicates an issue or PR that requires doc to be updated label Sep 5, 2024

fab-10 approved these changes Sep 6, 2024

View reviewed changes

pinges merged commit cf592c4 into hyperledger:main Sep 6, 2024
40 checks passed

m4sterbunny mentioned this pull request Sep 23, 2024

Documentation of engine_getBlobsV1 hyperledger/besu-docs#1700

Closed

m4sterbunny removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Sep 23, 2024

karalabe reviewed Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement engine_getBlobsV1 #7553

implement engine_getBlobsV1 #7553

pinges commented Sep 2, 2024

macfarla Sep 2, 2024

pinges Sep 3, 2024

pinges Sep 5, 2024

macfarla Sep 2, 2024

pinges Sep 3, 2024

jflo Sep 3, 2024

fab-10 left a comment

fab-10 Sep 3, 2024

jflo Sep 3, 2024

pinges Sep 4, 2024 •

edited

Loading

fab-10 Sep 4, 2024

fab-10 commented Sep 3, 2024

jflo left a comment

jflo Sep 3, 2024

jflo Sep 3, 2024

jflo Sep 3, 2024

fab-10 Sep 5, 2024

fab-10 Sep 5, 2024

pinges Sep 5, 2024

jflo left a comment

fab-10 commented Sep 5, 2024

fab-10 left a comment

fab-10 Sep 5, 2024

pinges Sep 5, 2024

fab-10 commented Sep 5, 2024

fab-10 Sep 6, 2024

karalabe Oct 2, 2024

implement engine_getBlobsV1 #7553

implement engine_getBlobsV1 #7553

Conversation

pinges commented Sep 2, 2024

PR description

Fixed Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 commented Sep 3, 2024

jflo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jflo left a comment

Choose a reason for hiding this comment

fab-10 commented Sep 5, 2024

fab-10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges Sep 4, 2024 •

edited

Loading