Improve `isDataAvailable` checks #7575

tbenr · 2023-10-04T16:45:12Z

~~This PR is mainly focused on making sure that commitments coming from block match the commitments contained in the BlobSidecars~~

Updated strategy by moving away extra checks from isDataAvailable method to a dedicated method in miscHelper.

Since we check that blobsSidecar content are consistent with the block before calling isDataAvailable, we can now pass them only to perform data availability.

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

LGTM

zilm13 · 2023-10-04T17:34:11Z

...spec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java

+                  "Blob sidecar slot %s does not match block slot %s",
+                  blobSidecar.getSlot(),
+                  slot);
+              checkArgument(


You could also add parent root check (but it will require changes in interface)

I started thinking that these checks could leave in the BlockBlobSidecarsTracker. If the tracker guarantees that block and sidecars field are consistent, we can then have this method only dedicated to KZG check, which is the original intention. The good thing on discarding inconsistent blobSidecars at BlockBlobSidecarsTracker level is that we could try to refetch them early.

thinking more about it, an early refetch might be undesirable here. If the proposer is doing something strange, is better to do nothing, skip the block import, vote on previous head, and then lookup only if next block builds on top of this (which might be not the case most times, unless next proposer is under control of the same malicious\buggy entity)

so ForkChoiceBlobSidecarsAvailabilityChecker::validateBatch might be a better place to have this checks

I decided to move the validation back to miscHelper but on a dedicated method. This simplified the tests in ForkChoiceBlobSidecarsAvailabilityCheckerTest

zilm13 · 2023-10-04T17:39:42Z

...spec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java

-    return kzg.verifyBlobKzgProofBatch(blobs, kzgCommitments, proofs);
+    checkArgument(
+        blobSidecars.size() == kzgCommitmentsFromBlock.size(),
+        "Blob sidecars count %s does not match KZG commitments count %s",


maybe match block KZG commitments to be more clear

would this be more helpful to report if we had the actual block slot or something associating it to the block in the message?
It'll be very hard for anyone to understand what this relates to because there's nothing tying it into data we can understand, so from the perspective of a node op, this will be a 'useless message' as it stands.

rolfyone · 2023-10-05T01:20:51Z

...spec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java

+              checkArgument(
+                  kzgCommitmentsFromBlock.get(index).equals(blobSidecar.getKZGCommitment()),
+                  "Blob sidecar KZG commitment %s does not match block KZG commitment %s",
+                  blobSidecar.getKZGCommitment(),
+                  kzgCommitmentsFromBlock.get(index));


similar to above, adding slot at a minimum would be helpful

move all blobSidecar-block sanity checks in the availability checker. Fun with tests

…into improve-isDataAvailable-checks

.../src/test/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDenebTest.java

zilm13 · 2023-10-10T17:30:16Z

...spec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java

-      final Bytes32 beaconBlockRoot,
-      final List<KZGCommitment> kzgCommitments,
-      final List<BlobSidecar> blobSidecars) {
+  public boolean isDataAvailable(final List<BlobSidecar> blobSidecars) {


What I don't like here, both methods, this and validateBlobSidecarsAgainstBlock could be called now with part of blobs, which is not clear. Next, isDataAvailable is a spec thing, so I expect it will verify it completely. Plus the last piece of puzzle, that we call this method with blobSidecars with indexes matching all indexes of kzgCommitments from block is somewhere else in the 3rd place.

I'm not sure what's the best solution to make this better. My suggestion (not the best one, need to think more on this) is:

rename this method, so it's actually 2nd round of verification

add comments to both methods that they could be used for partial verification

add actual isDataAvailable(kzgCommitmentsFromBlock, verifiedBlobKzgCommitments) here, which will finalize the action by just equal matching 2 lists

or better isDataAvailable(block, verifiedBlobIdentifiers)
return IntRange.of(0, block.getKzgCommitments.size()).map(index -> verifiedBlobIdentifiers.contains(new BlobIdentifier(block.getRoot, index))).filter(false).findFirst().isEmpty()

I agree that it isn't clear that those methods can be called with partial blobs. I'm thinking if it is better to go back to single method.

Consider that ForkChoiceBlobSidecarsAvailabilityChecker::computeFinalValidationResult is the method performing the checks on the final set.

Yeah, yeah, I understand it's happened there but we'd better have it here it and by my understanding it should be called isDataAvailable (for block, all data)

zilm13 · 2023-10-10T17:31:20Z

ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java

+
+  public List<BlobSidecar> randomBlobSidecarsForBlock(
+      final SignedBeaconBlock block,
+      BiFunction<Integer, RandomBlobSidecarBuilder, RandomBlobSidecarBuilder> modifier) {


boring final

zilm13 · 2023-10-10T17:31:27Z

ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java

-    final int numberOfKzgCommitments =
+  public List<SignedBlobSidecar> randomSignedBlobSidecarsForBlock(
+      final SignedBeaconBlock block,
+      BiFunction<Integer, RandomBlobSidecarBuilder, RandomBlobSidecarBuilder> modifier) {


another one

zilm13 · 2023-10-11T19:51:14Z

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

+   * @return
+   */
+  private BlobSidecarsAndValidationResult performCompletenessValidation(
+      final BlobSidecarsAndValidationResult additionalBlobSidecarsAndValidationResult) {


it's probably not additional, all instead, especially if it could pass completeness check

zilm13 · 2023-10-11T19:52:55Z

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

+          .verifyBlobSidecarCompleteness(
+              additionalBlobSidecarsAndValidationResult.getBlobSidecars(),
+              kzgCommitmentsFromBlockSupplier.get());
+    } catch (final IllegalArgumentException ex) {


It's not clear by verifyBlobSidecarCompleteness signature that we should catch IllegalArgumentException from it

zilm13

Maybe worth adding a test especially on completeness?

zilm13 · 2023-10-11T20:09:34Z

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


    if (!blobSidecars.isEmpty()) {
-      return validateBatch(blobSidecars, kzgCommitments);
+      return validateBatch(blobSidecars);


We are not doing completeness check in this case or I missed it.

yes, good catch, I wasn't clearly in a good shape!

zilm13 · 2023-10-17T12:13:57Z

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

-      return validateBatch(blobSidecars, kzgCommitments);
+      final BlobSidecarsAndValidationResult blobSidecarsAndValidationResult =
+          validateBatch(blobSidecars);
+      performCompletenessValidation(blobSidecarsAndValidationResult);


You ignore the result of completeness check, there should be probably return on this line

performCompletenessValidation is designed to throw only

It has the line return BlobSidecarsAndValidationResult.NOT_REQUIRED;
Unless it's not void return I assume we care on return

you're right.

zilm13 · 2023-10-17T12:19:39Z

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

+              blobSidecarsAndValidationResult.getBlobSidecars(),
+              kzgCommitmentsFromBlockSupplier.get());
+    } catch (final IllegalArgumentException ex) {
+      checkArgument(


I have some concerns on checkArgument vs checkState here, anyway we are already not super consistent with it in code. The more important is that we throw it all the way up, while we have BlobSidecarsAndValidationResult.invalidResult(blobSidecars, throwable) which looks better in this case. Or we could at least add a comment that this could throw. Though I like .invalidResult more.

zilm13

LGTM

improve-isDataAvailable-checks

9de14bc

zilm13 approved these changes Oct 4, 2023

View reviewed changes

rolfyone reviewed Oct 5, 2023

View reviewed changes

tbenr added 5 commits October 6, 2023 18:52

change isDataAvailable signature

e516cea

move all blobSidecar-block sanity checks in the availability checker. Fun with tests

Merge branch 'master' into improve-isDataAvailable-checks

c326d5e

update a comment

7b454d0

Merge remote-tracking branch 'origin/improve-isDataAvailable-checks' …

e884f3e

…into improve-isDataAvailable-checks

simplified solution

21e46b3

github-advanced-security bot found potential problems Oct 10, 2023

View reviewed changes

.../src/test/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDenebTest.java Fixed Show fixed Hide fixed

tbenr added 2 commits October 10, 2023 14:47

comment and CodeQL

bedcf74

Merge branch 'master' into improve-isDataAvailable-checks

9bae8a2

zilm13 reviewed Oct 10, 2023

View reviewed changes

tbenr added 2 commits October 11, 2023 17:07

tmp

a1a15cb

update verifyBlobSidecarCompleteness

a1086d2

zilm13 reviewed Oct 11, 2023

View reviewed changes

tbenr added 2 commits October 16, 2023 15:54

add tests, fuzzers, and finals

bb5abbe

Merge branch 'master' into improve-isDataAvailable-checks

194f481

tbenr mentioned this pull request Oct 16, 2023

Add BlobSidecarsByRangeListenerWrapper with BlobSidecar p2p verification #7576

Merged

2 tasks

zilm13 reviewed Oct 17, 2023

View reviewed changes

tbenr added 2 commits October 17, 2023 15:17

apply improvements

e9c8edb

Merge branch 'master' into improve-isDataAvailable-checks

2084294

zilm13 approved these changes Oct 17, 2023

View reviewed changes

tbenr enabled auto-merge (squash) October 17, 2023 13:30

tbenr merged commit 55b2d87 into Consensys:master Oct 17, 2023
4 checks passed

tbenr deleted the improve-isDataAvailable-checks branch October 17, 2023 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `isDataAvailable` checks #7575

Improve `isDataAvailable` checks #7575

tbenr commented Oct 4, 2023 •

edited

Loading

zilm13 left a comment

zilm13 Oct 4, 2023 •

edited

Loading

tbenr Oct 5, 2023

tbenr Oct 6, 2023

tbenr Oct 6, 2023

tbenr Oct 10, 2023

zilm13 Oct 4, 2023 •

edited

Loading

rolfyone Oct 5, 2023

tbenr Oct 5, 2023

rolfyone Oct 5, 2023

zilm13 Oct 10, 2023 •

edited

Loading

zilm13 Oct 10, 2023

tbenr Oct 11, 2023

zilm13 Oct 11, 2023 •

edited

Loading

zilm13 Oct 10, 2023

zilm13 Oct 10, 2023

zilm13 Oct 11, 2023

zilm13 Oct 11, 2023

zilm13 left a comment

zilm13 Oct 11, 2023

tbenr Oct 16, 2023

zilm13 Oct 17, 2023

tbenr Oct 17, 2023

zilm13 Oct 17, 2023

tbenr Oct 17, 2023

zilm13 Oct 17, 2023

zilm13 left a comment

Improve isDataAvailable checks #7575

Improve isDataAvailable checks #7575

Conversation

tbenr commented Oct 4, 2023 • edited Loading

Documentation

Changelog

zilm13 left a comment

Choose a reason for hiding this comment

zilm13 Oct 4, 2023 • edited Loading

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 Oct 4, 2023 • edited Loading

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 Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 Oct 11, 2023 • edited Loading

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

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

Improve `isDataAvailable` checks #7575

Improve `isDataAvailable` checks #7575

tbenr commented Oct 4, 2023 •

edited

Loading

zilm13 Oct 4, 2023 •

edited

Loading

zilm13 Oct 4, 2023 •

edited

Loading

zilm13 Oct 10, 2023 •

edited

Loading

zilm13 Oct 11, 2023 •

edited

Loading