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

Improve isDataAvailable checks #7575

Merged
merged 14 commits into from
Oct 17, 2023
Prev Previous commit
Next Next commit
change isDataAvailable signature
move all blobSidecar-block sanity checks in the availability checker.
Fun with tests
  • Loading branch information
tbenr committed Oct 6, 2023
commit e516cea719f190159733935dca4e52effde2c9af
Original file line number Diff line number Diff line change
@@ -347,11 +347,7 @@ public boolean isExecutionEnabled(final BeaconState genericState, final BeaconBl
* Performs data availability by validating the blobs against the kzg commitments from the block.
* It will also check the slot and blockRoot consistency.
*/
public boolean isDataAvailable(
final UInt64 slot,
final Bytes32 beaconBlockRoot,
final List<KZGCommitment> kzgCommitmentsFromBlock,
final List<BlobSidecar> blobSidecars) {
public boolean isDataAvailable(final List<BlobSidecar> blobSidecars) {
return false;
}

Original file line number Diff line number Diff line change
@@ -13,15 +13,12 @@

package tech.pegasys.teku.spec.logic.versions.deneb.helpers;

import static com.google.common.base.Preconditions.checkArgument;
import static tech.pegasys.teku.spec.config.SpecConfigDeneb.VERSIONED_HASH_VERSION_KZG;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.stream.IntStream;
import org.apache.tuweni.bytes.Bytes;
import org.apache.tuweni.bytes.Bytes32;
import tech.pegasys.teku.infrastructure.crypto.Hash;
import tech.pegasys.teku.infrastructure.ssz.SszList;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -62,48 +59,24 @@ private static KZG initKZG(final SpecConfigDeneb config) {
/**
* <a
* href="https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/fork-choice.md#is_data_available">is_data_available</a>
*
* <p>In Deneb we don't need to retrieve data, everything is already available via blob sidecars.
*/
@Override
public boolean isDataAvailable(
final UInt64 slot,
final Bytes32 beaconBlockRoot,
final List<KZGCommitment> kzgCommitmentsFromBlock,
final List<BlobSidecar> blobSidecars) {
checkArgument(
blobSidecars.size() == kzgCommitmentsFromBlock.size(),
"Blob sidecars count %s does not match KZG commitments count %s",
blobSidecars.size(),
kzgCommitmentsFromBlock.size());
public boolean isDataAvailable(final List<BlobSidecar> blobSidecars) {
Copy link
Contributor

@zilm13 zilm13 Oct 10, 2023

Choose a reason for hiding this comment

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

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:

  1. rename this method, so it's actually 2nd round of verification
  2. add comments to both methods that they could be used for partial verification
  3. add actual isDataAvailable(kzgCommitmentsFromBlock, verifiedBlobKzgCommitments) here, which will finalize the action by just equal matching 2 lists

Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@zilm13 zilm13 Oct 11, 2023

Choose a reason for hiding this comment

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

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)


final List<Bytes> blobs = new ArrayList<>();
final List<KZGProof> proofs = new ArrayList<>();
final List<KZGCommitment> kzgCommitments = new ArrayList<>();

IntStream.range(0, blobSidecars.size())
.forEach(
index -> {
final BlobSidecar blobSidecar = blobSidecars.get(index);

checkArgument(
slot.equals(blobSidecar.getSlot()),
"Blob sidecar slot %s does not match block slot %s",
blobSidecar.getSlot(),
slot);
checkArgument(
beaconBlockRoot.equals(blobSidecar.getBlockRoot()),
"Blob sidecar block root %s does not match block root %s",
blobSidecar.getBlockRoot(),
beaconBlockRoot);
checkArgument(
kzgCommitmentsFromBlock.get(index).equals(blobSidecar.getKZGCommitment()),
"Blob sidecar KZG commitment %s does not match block KZG commitment %s",
blobSidecar.getKZGCommitment(),
kzgCommitmentsFromBlock.get(index));

blobs.add(blobSidecar.getBlob().getBytes());
proofs.add(blobSidecar.getKZGProof());
});

return kzg.verifyBlobKzgProofBatch(blobs, kzgCommitmentsFromBlock, proofs);
blobSidecars.forEach(
blobSidecar -> {
blobs.add(blobSidecar.getBlob().getBytes());
proofs.add(blobSidecar.getKZGProof());
kzgCommitments.add(blobSidecar.getKZGCommitment());
});

return kzg.verifyBlobKzgProofBatch(blobs, kzgCommitments, proofs);
}

/**
Original file line number Diff line number Diff line change
@@ -20,16 +20,12 @@
import net.jqwik.api.ForAll;
import net.jqwik.api.From;
import net.jqwik.api.Property;
import org.apache.tuweni.bytes.Bytes32;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.kzg.KZGCommitment;
import tech.pegasys.teku.spec.config.SpecConfigDeneb;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.propertytest.suppliers.SpecSupplier;
import tech.pegasys.teku.spec.propertytest.suppliers.blobs.versions.deneb.BlobSidecarSupplier;
import tech.pegasys.teku.spec.propertytest.suppliers.type.Bytes32Supplier;
import tech.pegasys.teku.spec.propertytest.suppliers.type.KZGCommitmentSupplier;
import tech.pegasys.teku.spec.propertytest.suppliers.type.UInt64Supplier;

public class MiscHelpersDenebPropertyTest {
private final SpecConfigDeneb specConfig =
@@ -42,12 +38,9 @@ public class MiscHelpersDenebPropertyTest {

@Property(tries = 100)
void fuzzIsDataAvailable(
@ForAll(supplier = UInt64Supplier.class) final UInt64 slot,
@ForAll(supplier = Bytes32Supplier.class) final Bytes32 beaconBlockRoot,
@ForAll final List<@From(supplier = KZGCommitmentSupplier.class) KZGCommitment> commitments,
@ForAll final List<@From(supplier = BlobSidecarSupplier.class) BlobSidecar> blobSidecars) {
try {
miscHelpers.isDataAvailable(slot, beaconBlockRoot, commitments, blobSidecars);
miscHelpers.isDataAvailable(blobSidecars);
} catch (Exception e) {
assertThat(e).isInstanceOf(IllegalArgumentException.class);
}
Original file line number Diff line number Diff line change
@@ -14,19 +14,14 @@
package tech.pegasys.teku.spec.logic.versions.deneb.helpers;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static tech.pegasys.teku.spec.config.SpecConfigDeneb.VERSIONED_HASH_VERSION_KZG;

import java.util.List;
import org.apache.tuweni.bytes.Bytes32;
import org.junit.jupiter.api.Test;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.kzg.KZGCommitment;
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.TestSpecFactory;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.logic.versions.deneb.types.VersionedHash;
import tech.pegasys.teku.spec.util.DataStructureUtil;

class MiscHelpersDenebTest {

@@ -36,90 +31,10 @@ class MiscHelpersDenebTest {
Bytes32.fromHexString(
"0x391610cf24e7c540192b80ddcfea77b0d3912d94e922682f3b286eee041e6f76"));

private static final KZGCommitment KZG_COMMITMENT =
KZGCommitment.fromHexString(
"0xb09ce4964278eff81a976fbc552488cb84fc4a102f004c87179cb912f49904d1e785ecaf5d184522a58e9035875440ef");

private final Spec spec = TestSpecFactory.createMinimalDeneb();
protected final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec);
private final MiscHelpersDeneb miscHelpersDeneb =
new MiscHelpersDeneb(spec.getGenesisSpecConfig().toVersionDeneb().orElseThrow());

@Test
public void isDataAvailable_shouldThrowIfCommitmentsDontMatch() {
final BlobSidecar blobSidecar =
dataStructureUtil
.createRandomBlobSidecarBuilder()
.slot(UInt64.ONE)
.blockRoot(Bytes32.ZERO)
.build();

assertThatThrownBy(
() ->
miscHelpersDeneb.isDataAvailable(
UInt64.valueOf(1), Bytes32.ZERO, List.of(KZG_COMMITMENT), List.of(blobSidecar)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Blob sidecar KZG commitment")
.hasMessageContaining("does not match block KZG commitment");
}

@Test
public void isDataAvailable_shouldThrowIfSlotDoesntMatch() {
final BlobSidecar blobSidecar =
dataStructureUtil
.createRandomBlobSidecarBuilder()
.slot(UInt64.valueOf(2))
.blockRoot(Bytes32.ZERO)
.build();

assertThatThrownBy(
() ->
miscHelpersDeneb.isDataAvailable(
UInt64.ONE, Bytes32.ZERO, List.of(KZG_COMMITMENT), List.of(blobSidecar)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Blob sidecar slot")
.hasMessageContaining("does not match block slot");
}

@Test
public void isDataAvailable_shouldThrowIfBlobSidecarsAndBlockCommitmentsHaveDifferentSizes() {
final BlobSidecar blobSidecar =
dataStructureUtil
.createRandomBlobSidecarBuilder()
.slot(UInt64.ONE)
.blockRoot(Bytes32.ZERO)
.build();

assertThatThrownBy(
() ->
miscHelpersDeneb.isDataAvailable(
UInt64.ONE,
Bytes32.ZERO,
List.of(KZG_COMMITMENT, KZG_COMMITMENT),
List.of(blobSidecar)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Blob sidecars count")
.hasMessageContaining("does not match KZG commitments count");
}

@Test
public void isDataAvailable_shouldThrowIfBlockRootDoesntMatch() {
final BlobSidecar blobSidecar =
dataStructureUtil
.createRandomBlobSidecarBuilder()
.slot(UInt64.ONE)
.blockRoot(Bytes32.fromHexString("0x01"))
.build();

assertThatThrownBy(
() ->
miscHelpersDeneb.isDataAvailable(
UInt64.ONE, Bytes32.ZERO, List.of(KZG_COMMITMENT), List.of(blobSidecar)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Blob sidecar block root")
.hasMessageContaining("does not match block root");
}

@Test
public void versionedHash() {
final VersionedHash actual =
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@
import java.util.List;
import java.util.Optional;
import java.util.Random;
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
@@ -2123,24 +2124,37 @@ public Bytes randomBlobBytes() {
}

public List<BlobSidecar> randomBlobSidecarsForBlock(final SignedBeaconBlock block) {
return randomSignedBlobSidecarsForBlock(block).stream()
return randomBlobSidecarsForBlock(block, (__, builder) -> builder);
}

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

Choose a reason for hiding this comment

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

boring final

return randomSignedBlobSidecarsForBlock(block, modifier).stream()
.map(SignedBlobSidecar::getBlobSidecar)
.collect(toList());
}

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

Choose a reason for hiding this comment

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

another one

final SszList<SszKZGCommitment> blobKzgCommitments =
BeaconBlockBodyDeneb.required(block.getBeaconBlock().orElseThrow().getBody())
.getBlobKzgCommitments()
.size();
return IntStream.range(0, numberOfKzgCommitments)
.getBlobKzgCommitments();

return IntStream.range(0, blobKzgCommitments.size())
.mapToObj(
index ->
createRandomBlobSidecarBuilder()
.slot(block.getSlot())
.blockRoot(block.getRoot())
.index(UInt64.valueOf(index))
.buildSigned())
index -> {
final RandomBlobSidecarBuilder builder =
createRandomBlobSidecarBuilder()
.slot(block.getSlot())
.blockRoot(block.getRoot())
.blockParentRoot(block.getParentRoot())
.kzgCommitment(blobKzgCommitments.get(index).getBytes())
.index(UInt64.valueOf(index));

return modifier.apply(index, builder).buildSigned();
})
.collect(toList());
}

Original file line number Diff line number Diff line change
@@ -31,6 +31,7 @@
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock;
import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
import tech.pegasys.teku.spec.datastructures.blocks.SlotAndBlockRoot;
import tech.pegasys.teku.spec.datastructures.blocks.blockbody.versions.deneb.BeaconBlockBodyDeneb;
@@ -45,7 +46,7 @@ public class BlockBlobSidecarsTracker {
private final SlotAndBlockRoot slotAndBlockRoot;
private final UInt64 maxBlobsPerBlock;

private final AtomicReference<Optional<BeaconBlockBodyDeneb>> blockBody =
private final AtomicReference<Optional<BeaconBlock>> block =
new AtomicReference<>(Optional.empty());

private final NavigableMap<UInt64, BlobSidecar> blobSidecars = new ConcurrentSkipListMap<>();
@@ -80,8 +81,8 @@ public SafeFuture<Void> getCompletionFuture() {
return newCompletionFuture;
}

public Optional<BeaconBlockBodyDeneb> getBlockBody() {
return blockBody.get();
public Optional<BeaconBlock> getBlock() {
return block.get();
}

public boolean containsBlobSidecar(final BlobIdentifier blobIdentifier) {
@@ -91,9 +92,9 @@ public boolean containsBlobSidecar(final BlobIdentifier blobIdentifier) {
}

public Stream<BlobIdentifier> getMissingBlobSidecars() {
final Optional<BeaconBlockBodyDeneb> body = blockBody.get();
if (body.isPresent()) {
return UInt64.range(UInt64.ZERO, UInt64.valueOf(body.get().getBlobKzgCommitments().size()))
final Optional<Integer> blockCommitmentsCount = getBlockKzgCommitmentsCount();
if (blockCommitmentsCount.isPresent()) {
return UInt64.range(UInt64.ZERO, UInt64.valueOf(blockCommitmentsCount.get()))
.filter(blobIndex -> !blobSidecars.containsKey(blobIndex))
.map(blobIndex -> new BlobIdentifier(slotAndBlockRoot.getBlockRoot(), blobIndex));
}
@@ -109,10 +110,10 @@ public Stream<BlobIdentifier> getMissingBlobSidecars() {
}

public Stream<BlobIdentifier> getUnusedBlobSidecarsForBlock() {
final Optional<BeaconBlockBodyDeneb> body = blockBody.get();
checkState(body.isPresent(), "Block must me known to call this method");
final Optional<Integer> blockCommitmentsCount = getBlockKzgCommitmentsCount();
checkState(blockCommitmentsCount.isPresent(), "Block must me known to call this method");

final UInt64 firstUnusedIndex = UInt64.valueOf(body.get().getBlobKzgCommitments().size());
final UInt64 firstUnusedIndex = UInt64.valueOf(blockCommitmentsCount.get());
return UInt64.range(firstUnusedIndex, maxBlobsPerBlock)
.map(blobIndex -> new BlobIdentifier(slotAndBlockRoot.getBlockRoot(), blobIndex));
}
@@ -150,9 +151,7 @@ public int blobSidecarsCount() {

public boolean setBlock(final SignedBeaconBlock block) {
checkArgument(block.getSlotAndBlockRoot().equals(slotAndBlockRoot), "Wrong block");
final Optional<BeaconBlockBodyDeneb> oldBlock =
blockBody.getAndSet(
Optional.of(BeaconBlockBodyDeneb.required(block.getMessage().getBody())));
final Optional<BeaconBlock> oldBlock = this.block.getAndSet(block.getBeaconBlock());
if (oldBlock.isPresent()) {
return false;
}
@@ -196,10 +195,13 @@ public void setFetchTriggered() {
}

private boolean areBlobsComplete() {
return blockBody
return getBlockKzgCommitmentsCount().map(count -> blobSidecars.size() >= count).orElse(false);
}

private Optional<Integer> getBlockKzgCommitmentsCount() {
return block
.get()
.map(b -> blobSidecars.size() >= b.getBlobKzgCommitments().size())
.orElse(false);
.map(b -> BeaconBlockBodyDeneb.required(b.getBody()).getBlobKzgCommitments().size());
}

/**
@@ -252,7 +254,7 @@ private void printDebugTimings(final Map<UInt64, Long> debugTimings) {
public String toString() {
return MoreObjects.toStringHelper(this)
.add("slotAndBlockRoot", slotAndBlockRoot)
.add("isBlockBodyPresent", blockBody.get().isPresent())
.add("isBlockPresent", block.get().isPresent())
.add("isCompleted", isCompleted())
.add("fetchTriggered", fetchTriggered)
.add(
Loading