Blob sidecars availability checker #7081

tbenr · 2023-04-26T15:49:02Z

Block processor won't deal with data availability anymore. Everything will be done in ForkChoice.
Some minor changes to BlobsSidecarPool (including fix to cleanly ignore non-deneb blocks)
BlockManager now knows about BlobsSidecarPool:
- feeds validated blocks into it
- removes successfully imported blocks from it
- removes invalid blocks from it
BlobSidecarManager implementation
- now interacts with FutureItems<SignedBlobSidecar> and BlobsSidecarPool
- implements the createAvailabilityChecker method.
ForkChoiceBlobSidecarsAvailabilityChecker implementation:
- leverages the BlockBlobSidecarsTracker two implement a two stage async blobs validation
- the final verification step waits the tracker to complete with a timeout set as 1\3 of the slot time

Fixes #6848
Fixes #7080
Fixes #6673
Fixes #7110

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

zilm13 · 2023-04-29T10:41:08Z

...transition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManagerImpl.java

@@ -79,16 +93,32 @@ public BlobSidecarManagerImpl(
  public SafeFuture<InternalValidationResult> validateAndImportBlobSidecar(


I think we should find some better word for this and next method. We are not importing BlobSidecars here. validateAndPrepare maybe. Don't like either but import is definitely not a word for new flow from my view

yeah... I was lazily naming it like the block counterpart but now is time to find a good one

zilm13

Looks great, most nit comments only about naming. I like the way ForkChoiceBlobSidecarsAvailabilityChecker was refactored, there are some blocks now it's made from and it's much easier to read it. Just maybe some naming could be improved. I've not checked ForkChoiceBlobSidecarsAvailabilityCheckerTest to complete review yet, will check tomorrow

.../beaconchain/src/main/java/tech/pegasys/teku/services/beaconchain/BeaconChainController.java

zilm13 · 2023-05-02T19:30:47Z

...transition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManagerImpl.java

+              invalidBlobSidecarRoots.put(
+                  signedBlobSidecar.getBlobSidecar().hashTreeRoot(), result);
+              break;
+            default:


It's very counterintuitive way to handle IGNORE. I think it's better to do it in direct way

zilm13 · 2023-05-02T19:38:13Z

...transition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarManagerImpl.java

  }

  @Override
  public void onSlot(final UInt64 slot) {
    validatedPendingBlobs.headMap(slot.minusMinZero(1)).clear();
+
+    blobSidecarPool.onSlot(slot);


maybe it's better to subscribe blobSidecarPool and futureBlobSidecars on onSlot events on creation in BeaconChainController like we do for all others?

I was doing the same way PendingPool , which i think is a way to enforce that pools get updated before the manager performs it's things, while in the other way all onSlot are independent and run in parallel.

zilm13 · 2023-05-02T19:43:23Z

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java

@@ -377,20 +378,20 @@ private BlockImportResult importBlockAndState(
          payloadResult.getFailureCause().orElseThrow());
    }

-    if (blobsSidecarAndValidationResult.isFailure()) {
+    if (blobSidecarsAndValidationResult.isFailure()) {
      LOG.error(
          "blobsSidecar validation result: {}",


Maybe make some custom toString() for blobSidecarsAndValidationResult to make this logging line and next two more useful?

.../tech/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityChecker.java

zilm13 · 2023-05-02T19:58:01Z

.../tech/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityChecker.java

+    final List<KZGCommitment> kzgCommitmentsToValidate;
+    final List<BlobSidecar> blobSidecarsToValidate;
+
+    final boolean performCompleteValidation = blockBlobSidecarsTracker.isCompleted();


I'm a bit worried we are not synschronized between this and getting some blobSidecars array. Isnt' we?

I think it isn't a problem because the tracker becomes completed only when is guaranteed to expose the full set of blobSidecar. The tracker is "add only" so it isn't possible that in the meantime a blob disappears.

.../tech/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityChecker.java

...h/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityCheckerTest.java

UT update started

fix tests compilation apply suggestions on the AvailabilityChecker

Improve pool for the manager usage other suggestions

Pool not failing on non deneb blocks

zilm13

LGTM

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarPool.java

...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImpl.java

fix method name improve comment

tbenr force-pushed the blob_sidecars_availability_checker branch 2 times, most recently from 842dcea to 25cfdaf Compare April 28, 2023 15:40

zilm13 reviewed Apr 29, 2023

View reviewed changes

tbenr force-pushed the blob_sidecars_availability_checker branch from 5871a86 to 97c3ca4 Compare May 2, 2023 17:04

tbenr marked this pull request as ready for review May 2, 2023 17:04

tbenr marked this pull request as draft May 2, 2023 17:05

zilm13 reviewed May 2, 2023

View reviewed changes

tbenr force-pushed the blob_sidecars_availability_checker branch from 97c3ca4 to 3a79ac6 Compare May 3, 2023 16:44

zilm13 reviewed May 3, 2023

View reviewed changes

...h/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityCheckerTest.java Outdated Show resolved Hide resolved

tbenr added 12 commits May 4, 2023 19:30

initial commit

3501960

availability checker validation implementation

4f54f8c

improvements

545dcd6

UT update started

rename to testVerifyingEmptyBatch

bcf4b36

additional improvements

5bcbf60

various improvements and tests

53db5c8

final

0d6288f

cleanup tests

1cd2179

fix tests compilation apply suggestions on the AvailabilityChecker

Improve Manager method names

7be1b10

Improve pool for the manager usage other suggestions

spotlessly

a0fa57d

spotlessly

e190d16

set interaction with BlockManager and BlobSidecarPool

517c136

tbenr force-pushed the blob_sidecars_availability_checker branch from 7ae5ce2 to 517c136 Compare May 4, 2023 17:30

implement BlobSidecarManager test

08d531a

Pool not failing on non deneb blocks

tbenr marked this pull request as ready for review May 5, 2023 09:16

tbenr added 2 commits May 5, 2023 11:16

Merge branch 'master' into blob_sidecars_availability_checker

1cd2100

add createAvailabilityChecker tests

4cf29d2

zilm13 approved these changes May 5, 2023

View reviewed changes

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobSidecarPool.java Outdated Show resolved Hide resolved

...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImpl.java Outdated Show resolved Hide resolved

tbenr added 2 commits May 5, 2023 17:15

fix createTracker

65e37c7

fix method name improve comment

still a bug in the comment

a712371

tbenr enabled auto-merge (squash) May 5, 2023 15:24

tbenr merged commit 5795062 into Consensys:master May 5, 2023

tbenr deleted the blob_sidecars_availability_checker branch May 5, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blob sidecars availability checker #7081

Blob sidecars availability checker #7081

tbenr commented Apr 26, 2023 •

edited

Loading

zilm13 Apr 29, 2023

tbenr May 3, 2023

zilm13 left a comment

zilm13 May 2, 2023

zilm13 May 2, 2023

tbenr May 3, 2023

zilm13 May 2, 2023

zilm13 May 2, 2023

tbenr May 3, 2023

zilm13 left a comment

		@@ -79,16 +93,32 @@ public BlobSidecarManagerImpl(
		public SafeFuture<InternalValidationResult> validateAndImportBlobSidecar(

Blob sidecars availability checker #7081

Blob sidecars availability checker #7081

Conversation

tbenr commented Apr 26, 2023 • edited Loading

Documentation

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 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

zilm13 left a comment

Choose a reason for hiding this comment

tbenr commented Apr 26, 2023 •

edited

Loading