Simplify blob sidecar availability checker #8840

tbenr · 2024-11-20T17:02:33Z

blobs get from EL are marked as valid (trust assumption on EL)

TODO:

historical sync unit tests
gossip unit tests
rpc unit tests

fixes #8740

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.

...ence-tests/src/referenceTest/java/tech/pegasys/teku/reference/ManualReferenceTestRunner.java

mehdi-aouadi · 2024-11-22T15:57:48Z

...ec/src/main/java/tech/pegasys/teku/spec/datastructures/blobs/versions/deneb/BlobSidecar.java

@@ -39,6 +39,9 @@ public class BlobSidecar
        SignedBeaconBlockHeader,
        SszBytes32Vector> {

+  private volatile boolean kzgAndInclusionProofValidated = false;
+  private volatile boolean signatureValidated = false;


I find these validation steps a bit confusing and I'm wondering if we can't simply mark the blob sidecar as valid or not (merge them) or split the kzgAndInclusionProofValidated into two validation flags: kzgValidated and inclusionProofValidated. An and in a boolean variable name is red flag to me. Or do we really need this design for some reason I'm missing?!

the reason I put kzg and inclusion proof together and signatureValidation as a separate boolean is because we can perform kzg and inclusion proof validation just having the blob itself (you don't need anything else). For the signature validation you need a prevalidated signed block OR the state to get the pubkey from and do the signature check.

mehdi-aouadi · 2024-11-22T16:19:03Z

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

+          && !blobSidecar
+              .getSignedBeaconBlockHeader()
+              .hashTreeRoot()
+              .equals(block.hashTreeRoot())) {


Isn't this the same check done twice?!

same check as which one?

I meant same as blobSidecar.isSignatureValidated()

ok but where am I doing this the second time?
I'm not sure this always checked. For instance the RPC lookups check blobs via BlobSidecarsByRootValidator.validate, which can only do kzg and inclusion proof.
So if we end up processing blobs coming from RPC source, we are not checking the block header in the blobSidecar (correct me if i'm wrong :) )

oh. I actually forgot to do the marking in BlobSidecarsByRangeListenerValidatingProxy

mmm maybe I can move those marking deeper in the stack and have them separated as you suggested

So if we end up processing blobs coming from RPC source, we are not checking the block header in the blobSidecar (correct me if i'm wrong :) )

Ok I see what you mean. One should keep in mind the whole flow to understand that...

tbenr · 2024-11-22T16:39:12Z

since i forgot to do this marking i think the way I did it is a bit error prone. I'll change it.

zilm13 · 2024-11-24T10:11:29Z

I will join Mehdi feedback:

if some validation was not performed I expect that we want to run it now
if it means that it was actually performed but failed it's confusing
I'm not sure on extending base BlobSidecar class, maybe better make some wrapper? It would be cleaner.
I'm happy with split kzg+inclusion / signature

zilm13

Have not checked tests, yet (starting)

zilm13 · 2024-12-09T17:17:26Z

beacon/sync/src/main/java/tech/pegasys/teku/beacon/sync/historical/HistoricalBatchFetcher.java

+                          .hashTreeRoot()
+                          .equals(block.hashTreeRoot());
+
+                  if (signedHeaderMatchesBlock) {


it's a bit dirty, "side-effect", I'd just mark it in separate cycle after throw. Or even better mark it first and then boolean = allmatch(isSignatureValidated).

zilm13 · 2024-12-09T17:24:53Z

beacon/sync/src/main/java/tech/pegasys/teku/beacon/sync/historical/HistoricalBatchFetcher.java

@@ -402,20 +400,30 @@ private void validateBlobSidecars(final SignedBeaconBlock block) {
    final List<BlobSidecar> blobSidecars =


Maybe validateBlobSidecarsBlockSignature as it's the only thing we are checking here? The reason why I'm so meticulous here is because we don't even check we have all required blobSidecars here (I guess it's done in other place)

I think you're right here. We rely on availability checker to verify the completeness. Need to think about if we want to continue exposing the check from the manager or do it here if it is the only place we need it.

zilm13 · 2024-12-09T17:30:58Z

...ec/src/main/java/tech/pegasys/teku/spec/datastructures/blobs/versions/deneb/BlobSidecar.java

+  }
+
+  public void markKzgAsValidated() {
+    kzgValidated = true;


Still some uncertainty in this approach. Original container is immutable, this structure is mutable for these fields. Plus we add fields which are not used in equals, changes purpose etc. So (I'm not sure) I'd think on

wrapper/keep as is

immutable/mutable

In my idea, due to the fact that the container is immutable, than we can safely have these additional flags.
So it is intentional that this fields are not part of the equality and the object must behave as these flags are not here at all. We can discuss it

Ok, just to check you are confident on it. Will not oppose.

zilm13 · 2024-12-09T17:38:58Z

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

+      blobSidecar.markKzgAsValidated();
+    }
+
+    return result;


For this and further check implementations. If check was failed we have a chance to run it twice (if called again). So may worth separate checkIsPerformed, checkIsPassed flags.

This is true but I just wanted to keep it simple.
An option would be to have the flags as Optional.

But at the end I don't think it is really relevant because we discard the invalid blobs immediately (ie via gossip\rpc) so I don't think that we ever risk to validate an invalid one twice.

Didn't think about discard, looks reasonable, but I'd add some info somewhere for clarity. I don't know, javadoc to isKzgValidated maybe.

zilm13 · 2024-12-09T17:44:56Z

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

-        // there are no available blobs so far, but we are outside the availability window. We can
-        // skip additional checks
-        return Optional.of(BlobSidecarsAndValidationResult.NOT_REQUIRED);
+    for (final BlobSidecar blobSidecar : blobSidecars) {


Could we put this check in some helper and reuse here and in HistoricalBatchFetcher.java

zilm13 · 2024-12-09T17:47:34Z

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

-    final SlotAndBlockRoot slotAndBlockRoot = blockBlobSidecarsTracker.getSlotAndBlockRoot();
-
-    final MiscHelpers miscHelpers = spec.atSlot(slotAndBlockRoot.getSlot()).miscHelpers();
+  private BlobSidecarsAndValidationResult validateCompletedBlobSidecars() {


where do we check that we have all blobSidecars for the number of commitments, I miss it?

yeh I was thinking that I've been too aggressive by removing the completeness check (which is also needed for historical sync). It's a cheap check and we should not rely on blobs tracker which rely only on blob indices to determine completeness. In general it is better that this class takes all it's responsibilities.

well.. checked again and in theory what BlockBlobSidecarsTracker is doing should be good enough. 🤔

ok, I see, so it will fail by timeout. I'd add some comment for transparency.

mehdi-aouadi reviewed Nov 22, 2024

View reviewed changes

...ence-tests/src/referenceTest/java/tech/pegasys/teku/reference/ManualReferenceTestRunner.java Outdated Show resolved Hide resolved

mehdi-aouadi reviewed Nov 22, 2024

View reviewed changes

tbenr force-pushed the simplify-BlobSidecarAvailabilityChecker branch 3 times, most recently from 0cfe5bb to 39ecb99 Compare November 26, 2024 14:43

tbenr added 14 commits December 9, 2024 16:19

tmp

2c9252c

tmp

60e6efe

tmp

1c4291c

update some tests

a873955

update some tests

8847e67

other fixes

b2f392a

fix stub for reference tests

ba9d88e

fix stub for reference tests

959e242

improvement

0d6f814

fix comparison

e16bb37

mark locally fetched blobs as validated

164982e

update BlockBlobSidecarsTrackerTest

f494afe

improve test

08334ef

change approach

e3a2ee7

tbenr force-pushed the simplify-BlobSidecarAvailabilityChecker branch from 39ecb99 to e3a2ee7 Compare December 9, 2024 16:37

tbenr added 3 commits December 9, 2024 17:51

fix property test

7fcce98

remove an unneeded test

d0b52e6

Merge branch 'master' into simplify-BlobSidecarAvailabilityChecker

ce5d599

zilm13 reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify blob sidecar availability checker #8840

Simplify blob sidecar availability checker #8840

tbenr commented Nov 20, 2024 •

edited

Loading

mehdi-aouadi Nov 22, 2024 •

edited

Loading

tbenr Nov 22, 2024

mehdi-aouadi Nov 22, 2024 •

edited

Loading

tbenr Nov 22, 2024

mehdi-aouadi Nov 22, 2024

tbenr Nov 22, 2024

tbenr Nov 22, 2024

tbenr Nov 22, 2024

mehdi-aouadi Nov 22, 2024

tbenr commented Nov 22, 2024

zilm13 commented Nov 24, 2024 •

edited

Loading

zilm13 left a comment

zilm13 Dec 9, 2024

zilm13 Dec 9, 2024

tbenr Dec 10, 2024

zilm13 Dec 9, 2024

tbenr Dec 10, 2024

zilm13 Dec 10, 2024

zilm13 Dec 9, 2024

tbenr Dec 10, 2024

zilm13 Dec 10, 2024

zilm13 Dec 9, 2024

zilm13 Dec 9, 2024

tbenr Dec 10, 2024

tbenr Dec 10, 2024 •

edited

Loading

zilm13 Dec 10, 2024

		@@ -402,20 +400,30 @@ private void validateBlobSidecars(final SignedBeaconBlock block) {
		final List<BlobSidecar> blobSidecars =

Simplify blob sidecar availability checker #8840

Are you sure you want to change the base?

Simplify blob sidecar availability checker #8840

Conversation

tbenr commented Nov 20, 2024 • edited Loading

Documentation

Changelog

mehdi-aouadi Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehdi-aouadi Nov 22, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbenr commented Nov 22, 2024

zilm13 commented Nov 24, 2024 • edited Loading

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

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

tbenr Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbenr commented Nov 20, 2024 •

edited

Loading

mehdi-aouadi Nov 22, 2024 •

edited

Loading

mehdi-aouadi Nov 22, 2024 •

edited

Loading

zilm13 commented Nov 24, 2024 •

edited

Loading

tbenr Dec 10, 2024 •

edited

Loading