Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify blob sidecar availability checker #8840

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import tech.pegasys.teku.bls.BLSPublicKey;
import tech.pegasys.teku.bls.BLSSignature;
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.infrastructure.exceptions.ExceptionUtil;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.networking.eth2.peers.Eth2Peer;
import tech.pegasys.teku.networking.eth2.rpc.core.InvalidResponseException;
Expand All @@ -53,7 +52,6 @@
import tech.pegasys.teku.spec.datastructures.state.Fork;
import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
import tech.pegasys.teku.spec.logic.common.util.AsyncBLSSignatureVerifier;
import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAndValidationResult;
import tech.pegasys.teku.statetransition.blobs.BlobSidecarManager;
import tech.pegasys.teku.storage.api.StorageUpdateChannel;
import tech.pegasys.teku.storage.client.CombinedChainDataClient;
Expand Down Expand Up @@ -402,20 +400,30 @@ private void validateBlobSidecars(final SignedBeaconBlock block) {
final List<BlobSidecar> blobSidecars =
blobSidecarsBySlotToImport.getOrDefault(
block.getSlotAndBlockRoot(), Collections.emptyList());
LOG.trace("Validating {} blob sidecars for block {}", blobSidecars.size(), block.getRoot());
final BlobSidecarsAndValidationResult validationResult =
blobSidecarManager.createAvailabilityCheckerAndValidateImmediately(block, blobSidecars);

if (validationResult.isFailure()) {
final String causeMessage =
validationResult
.getCause()
.map(cause -> " (" + ExceptionUtil.getRootCauseMessage(cause) + ")")
.orElse("");

final boolean blobsBlockHeaderRootMatchBlockRoot =
blobSidecars.stream()
.allMatch(
blobSidecar -> {
final boolean signedHeaderMatchesBlock =
blobSidecar
.getSignedBeaconBlockHeader()
.hashTreeRoot()
.equals(block.hashTreeRoot());

if (signedHeaderMatchesBlock) {
// since we already validated block's signature, we can mark the blob sidecar's
// signature as validated
blobSidecar.markSignatureAsValidated();
}
return signedHeaderMatchesBlock;
});

if (!blobsBlockHeaderRootMatchBlockRoot) {
throw new IllegalArgumentException(
String.format(
"Blob sidecars validation for block %s failed: %s%s",
block.getRoot(), validationResult.getValidationResult(), causeMessage));
"Blob sidecars' signed beacon block header don't match signed block for %s",
block.toLogString()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import tech.pegasys.teku.spec.datastructures.blocks.SlotAndBlockRoot;
import tech.pegasys.teku.spec.generator.ChainBuilder;
import tech.pegasys.teku.spec.logic.common.util.AsyncBLSSignatureVerifier;
import tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAndValidationResult;
import tech.pegasys.teku.statetransition.blobs.BlobSidecarManager;
import tech.pegasys.teku.storage.api.StorageQueryChannel;
import tech.pegasys.teku.storage.api.StorageUpdateChannel;
Expand Down Expand Up @@ -117,7 +116,7 @@ public void setup() {
.map(SignedBlockAndState::getBlock)
.collect(Collectors.toList());
lastBlockInBatch = chainBuilder.getLatestBlockAndState().getBlock();
firstBlockInBatch = blockBatch.get(0);
firstBlockInBatch = blockBatch.getFirst();
blobSidecarsBatch =
chainBuilder
.streamBlobSidecars(10, 20)
Expand Down Expand Up @@ -148,8 +147,6 @@ public void setup() {

when(signatureVerifier.verify(any(), any(), anyList()))
.thenReturn(SafeFuture.completedFuture(true));
when(blobSidecarManager.createAvailabilityCheckerAndValidateImmediately(any(), anyList()))
.thenAnswer(i -> BlobSidecarsAndValidationResult.validResult(i.getArgument(1)));
}

@Test
Expand Down Expand Up @@ -202,22 +199,19 @@ public void run_returnAllBlocksAndBlobSidecarsOnFirstRequest() {
earliestBlobSidecarSlotCaptor.capture());
assertThat(blockCaptor.getValue()).containsExactlyElementsOf(blockBatch);
assertThat(blobSidecarCaptor.getValue()).isEqualTo(blobSidecarsBatch);
assertThat(earliestBlobSidecarSlotCaptor.getValue()).contains(blockBatch.get(0).getSlot());
assertThat(earliestBlobSidecarSlotCaptor.getValue()).contains(blockBatch.getFirst().getSlot());
}

@Test
public void run_failsOnBlobSidecarsValidationFailure() {
when(blobSidecarManager.isAvailabilityRequiredAtSlot(any())).thenReturn(true);
when(blobSidecarManager.createAvailabilityCheckerAndValidateImmediately(any(), anyList()))
.thenAnswer(
i ->
BlobSidecarsAndValidationResult.invalidResult(
i.getArgument(1), new IllegalStateException("oopsy")));

assertThat(peer.getOutstandingRequests()).isEqualTo(0);
final SafeFuture<BeaconBlockSummary> future = fetcher.run();
peer.completePendingRequests();

// TODO: find a way to provoke blob sidecar validation failure

assertThat(future)
.failsWithin(Duration.ZERO)
.withThrowableThat()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,8 @@ void shouldCreateBlobSidecarsForBlockContents() {
assertThat(blobSidecar.getSszKZGCommitment())
.isEqualTo(expectedCommitments.get(index));
// verify the merkle proof
assertThat(miscHelpersDeneb.verifyBlobSidecarMerkleProof(blobSidecar)).isTrue();
assertThat(miscHelpersDeneb.verifyBlobKzgCommitmentInclusionProof(blobSidecar))
.isTrue();
});
}

Expand Down Expand Up @@ -921,7 +922,8 @@ void shouldCreateBlobSidecarsForBlindedBlock(final boolean useLocalFallback) {
assertThat(blobSidecar.getSszKZGCommitment())
.isEqualTo(expectedCommitments.get(index));
// verify the merkle proof
assertThat(miscHelpersDeneb.verifyBlobSidecarMerkleProof(blobSidecar)).isTrue();
assertThat(miscHelpersDeneb.verifyBlobKzgCommitmentInclusionProof(blobSidecar))
.isTrue();
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class EventSubscriber {
private final AtomicBoolean processingQueue;
private final AsyncRunner asyncRunner;
private final AtomicLong excessiveQueueingDisconnectionTime = new AtomicLong(Long.MAX_VALUE);
private volatile AtomicInteger successiveFailureCounter = new AtomicInteger(0);
private final AtomicInteger successiveFailureCounter = new AtomicInteger(0);

public EventSubscriber(
final List<String> eventTypes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,6 @@ public SafeFuture<BlobSidecarsAndValidationResult> getAvailabilityCheckResult()
return SafeFuture.completedFuture(validateImmediately(block, blobsAndProofs));
}

@Override
public BlobSidecarsAndValidationResult validateImmediately(
final List<BlobSidecar> blobSidecars) {
throw new UnsupportedOperationException("Not available in fork choice reference tests");
}

private BlobSidecarsAndValidationResult validateImmediately(
final SignedBeaconBlock block, final BlobsAndProofs blobsAndProofs) {
final List<KZGCommitment> kzgCommitments =
Expand All @@ -125,11 +119,5 @@ private BlobSidecarsAndValidationResult validateImmediately(
};
}

@Override
public BlobSidecarsAndValidationResult createAvailabilityCheckerAndValidateImmediately(
final SignedBeaconBlock block, final List<BlobSidecar> blobSidecars) {
throw new UnsupportedOperationException("Not available in fork choice reference tests");
}

private record BlobsAndProofs(List<Blob> blobs, List<KZGProof> proofs) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ public class BlobSidecar
SignedBeaconBlockHeader,
SszBytes32Vector> {

private volatile boolean kzgValidated = false;
private volatile boolean kzgCommitmentInclusionProofValidated = false;
private volatile boolean signatureValidated = false;
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


BlobSidecar(final BlobSidecarSchema blobSidecarSchema, final TreeNode backingTreeNode) {
super(blobSidecarSchema, backingTreeNode);
}
Expand Down Expand Up @@ -139,6 +143,30 @@ public String toLogString() {
getKZGProof().toAbbreviatedString());
}

public boolean isKzgValidated() {
return kzgValidated;
}

public boolean isKzgCommitmentInclusionProofValidated() {
return kzgCommitmentInclusionProofValidated;
}

public boolean isSignatureValidated() {
return signatureValidated;
}

public void markKzgAsValidated() {
kzgValidated = true;
}

public void markKzgCommitmentInclusionProofAsValidated() {
kzgCommitmentInclusionProofValidated = true;
}

public void markSignatureAsValidated() {
signatureValidated = true;
}

@Override
public BlobSidecarSchema getSchema() {
return (BlobSidecarSchema) super.getSchema();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,20 +366,6 @@ public boolean verifyBlobKzgProofBatch(final KZG kzg, final List<BlobSidecar> bl
return false;
}

public void validateBlobSidecarsBatchAgainstBlock(
final List<BlobSidecar> blobSidecars,
final BeaconBlock block,
final List<KZGCommitment> kzgCommitmentsFromBlock) {
throw new UnsupportedOperationException("No Blob Sidecars before Deneb");
}

public void verifyBlobSidecarCompleteness(
final List<BlobSidecar> verifiedBlobSidecars,
final List<KZGCommitment> kzgCommitmentsFromBlock)
throws IllegalArgumentException {
throw new UnsupportedOperationException("No Blob Sidecars before Deneb");
}

public VersionedHash kzgCommitmentToVersionedHash(final KZGCommitment kzgCommitment) {
throw new UnsupportedOperationException("No KZGCommitments before Deneb");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@

import static tech.pegasys.teku.spec.logic.versions.deneb.blobs.BlobSidecarsAndValidationResult.NOT_REQUIRED_RESULT_FUTURE;

import java.util.List;
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.execution.ExecutionPayloadHeader;
import tech.pegasys.teku.spec.datastructures.execution.NewPayloadRequest;
import tech.pegasys.teku.spec.logic.versions.bellatrix.block.OptimisticExecutionPayloadExecutor;
Expand All @@ -35,12 +33,6 @@ public boolean initiateDataAvailabilityCheck() {
public SafeFuture<BlobSidecarsAndValidationResult> getAvailabilityCheckResult() {
return NOT_REQUIRED_RESULT_FUTURE;
}

@Override
public BlobSidecarsAndValidationResult validateImmediately(
final List<BlobSidecar> blobSidecars) {
return BlobSidecarsAndValidationResult.NOT_REQUIRED;
}
};

BlobSidecarsAvailabilityChecker NOT_REQUIRED = NOOP;
Expand All @@ -55,7 +47,4 @@ public BlobSidecarsAndValidationResult validateImmediately(
boolean initiateDataAvailabilityCheck();

SafeFuture<BlobSidecarsAndValidationResult> getAvailabilityCheckResult();

/** Perform the data availability check immediately on the provided blob sidecars */
BlobSidecarsAndValidationResult validateImmediately(List<BlobSidecar> blobSidecars);
}
Loading
Loading