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.

Sign up for GitHub

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
Merged

Improve isDataAvailable checks #7575

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9de14bc
improve-isDataAvailable-checks
tbenr Oct 4, 2023
e516cea
change isDataAvailable signature
tbenr Oct 6, 2023
c326d5e
Merge branch 'master' into improve-isDataAvailable-checks
tbenr Oct 6, 2023
7b454d0
update a comment
tbenr Oct 6, 2023
e884f3e
Merge remote-tracking branch 'origin/improve-isDataAvailable-checks' …
tbenr Oct 6, 2023
21e46b3
simplified solution
tbenr Oct 10, 2023
bedcf74
comment and CodeQL
tbenr Oct 10, 2023
9bae8a2
Merge branch 'master' into improve-isDataAvailable-checks
tbenr Oct 10, 2023
a1a15cb
tmp
tbenr Oct 11, 2023
a1086d2
update verifyBlobSidecarCompleteness
tbenr Oct 11, 2023
bb5abbe
add tests, fuzzers, and finals
tbenr Oct 16, 2023
194f481
Merge branch 'master' into improve-isDataAvailable-checks
tbenr Oct 16, 2023
e9c8edb
apply improvements
tbenr Oct 17, 2023
2084294
Merge branch 'master' into improve-isDataAvailable-checks
tbenr Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ public boolean isExecutionEnabled(final BeaconState genericState, final BeaconBl
public boolean isDataAvailable(
final UInt64 slot,
final Bytes32 beaconBlockRoot,
final List<KZGCommitment> kzgCommitments,
final List<KZGCommitment> kzgCommitmentsFromBlock,
final List<BlobSidecar> blobSidecars) {
return false;
}
Expand Down
57 changes: 38 additions & 19 deletions ...c/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDeneb.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
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;
Expand Down Expand Up @@ -65,26 +67,43 @@ private static KZG initKZG(final SpecConfigDeneb config) {
public boolean isDataAvailable(
final UInt64 slot,
final Bytes32 beaconBlockRoot,
final List<KZGCommitment> kzgCommitments,
final List<KZGCommitment> kzgCommitmentsFromBlock,
final List<BlobSidecar> blobSidecars) {
blobSidecars.forEach(
blobSidecar -> {
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);
});
final List<Bytes> blobs =
blobSidecars.stream().map(BlobSidecar::getBlob).map(Blob::getBytes).toList();
final List<KZGProof> proofs = blobSidecars.stream().map(BlobSidecar::getKZGProof).toList();

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

@zilm13 zilm13 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

maybe match block KZG commitments to be more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right!

blobSidecars.size(),
kzgCommitmentsFromBlock.size());

final List<Bytes> blobs = new ArrayList<>();
final List<KZGProof> proofs = 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(
Copy link
Contributor

@zilm13 zilm13 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 decided to move the validation back to miscHelper but on a dedicated method. This simplified the tests in ForkChoiceBlobSidecarsAvailabilityCheckerTest

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

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

/**
Expand Down
85 changes: 85 additions & 0 deletions ...c/test/java/tech/pegasys/teku/spec/logic/versions/deneb/helpers/MiscHelpersDenebTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,19 @@
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 {

Expand All @@ -31,10 +36,90 @@ 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 =
Expand Down
18 changes: 9 additions & 9 deletions ...ch/pegasys/teku/statetransition/forkchoice/ForkChoiceBlobSidecarsAvailabilityChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class ForkChoiceBlobSidecarsAvailabilityChecker implements BlobSidecarsAv

private final SafeFuture<BlobSidecarsAndValidationResult> validationResult = new SafeFuture<>();

private final Supplier<List<KZGCommitment>> kzgCommitmentsSupplier =
private final Supplier<List<KZGCommitment>> kzgCommitmentsFromBlockSupplier =
createLazyKzgCommitmentsSupplier(this);

private final Duration waitForTrackerCompletionTimeout;
Expand Down Expand Up @@ -106,33 +106,33 @@ public SafeFuture<BlobSidecarsAndValidationResult> getAvailabilityCheckResult()
@Override
public BlobSidecarsAndValidationResult validateImmediately(final List<BlobSidecar> blobSidecars) {

final List<KZGCommitment> kzgCommitments = kzgCommitmentsSupplier.get();
final List<KZGCommitment> kzgCommitmentsFromBlock = kzgCommitmentsFromBlockSupplier.get();

if (!blobSidecars.isEmpty()) {
return validateBatch(blobSidecars, kzgCommitments);
return validateBatch(blobSidecars, kzgCommitmentsFromBlock);
}

if (isBlockOutsideDataAvailabilityWindow()) {
return BlobSidecarsAndValidationResult.NOT_REQUIRED;
}

if (kzgCommitments.isEmpty()) {
if (kzgCommitmentsFromBlock.isEmpty()) {
return BlobSidecarsAndValidationResult.validResult(List.of());
}

return BlobSidecarsAndValidationResult.NOT_AVAILABLE;
}

private BlobSidecarsAndValidationResult validateBatch(
final List<BlobSidecar> blobSidecars, final List<KZGCommitment> kzgCommitments) {
final List<BlobSidecar> blobSidecars, final List<KZGCommitment> kzgCommitmentsFromBlock) {
final SlotAndBlockRoot slotAndBlockRoot = blockBlobSidecarsTracker.getSlotAndBlockRoot();
try {
if (!spec.atSlot(slotAndBlockRoot.getSlot())
.miscHelpers()
.isDataAvailable(
slotAndBlockRoot.getSlot(),
slotAndBlockRoot.getBlockRoot(),
kzgCommitments,
kzgCommitmentsFromBlock,
blobSidecars)) {
return BlobSidecarsAndValidationResult.invalidResult(blobSidecars);
}
Expand All @@ -153,7 +153,7 @@ private BlobSidecarsAndValidationResult validateBatch(
* be completed it returns empty
*/
private Optional<BlobSidecarsAndValidationResult> validateImmediatelyAvailable() {
final List<KZGCommitment> kzgCommitmentsInBlock = kzgCommitmentsSupplier.get();
final List<KZGCommitment> kzgCommitmentsInBlock = kzgCommitmentsFromBlockSupplier.get();

final List<KZGCommitment> kzgCommitmentsToValidate;
final List<BlobSidecar> blobSidecarsToValidate;
Expand Down Expand Up @@ -266,7 +266,7 @@ private BlobSidecarsAndValidationResult computeAndValidateRemaining() {
final List<KZGCommitment> additionalKzgCommitmentsToBeValidated = new ArrayList<>();
final List<BlobSidecar> additionalBlobSidecarsToBeValidated = new ArrayList<>();

final List<KZGCommitment> kzgCommitmentsInBlock = kzgCommitmentsSupplier.get();
final List<KZGCommitment> kzgCommitmentsInBlock = kzgCommitmentsFromBlockSupplier.get();
final SortedMap<UInt64, BlobSidecar> completeBlobSidecars =
blockBlobSidecarsTracker.getBlobSidecars();

Expand Down Expand Up @@ -330,7 +330,7 @@ private BlobSidecarsAndValidationResult computeFinalValidationResult(
completeValidatedBlobSidecars.add(blobSidecar);
});

if (completeValidatedBlobSidecars.size() < kzgCommitmentsSupplier.get().size()) {
if (completeValidatedBlobSidecars.size() < kzgCommitmentsFromBlockSupplier.get().size()) {
// we haven't verified enough blobs to match the commitments present in the block
// this should never happen in practice. If it does, is likely a bug and should be fixed.
checkState(
Expand Down