Skip to content

Commit

Permalink
apply suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@tbenr
tbenr committed Jan 10, 2023
1 parent aba7ca0 commit 5bc09d9
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 49 deletions.
10 changes: 10 additions & 0 deletions ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,16 @@ public boolean isMergeTransitionComplete(final BeaconState state) {
return false;
}

/**
* Performs data availability by validating blobs and proof from Sidecar against blobs kzg
* commitments from the block. It will also make check slot and blockRoot consistency.
*
* @param slot
* @param beaconBlockRoot
* @param kzgCommitments
* @param blobsSidecar
* @return
*/
public boolean isDataAvailable(
final UInt64 slot,
final Bytes32 beaconBlockRoot,
Expand Down
2 changes: 1 addition & 1 deletion ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1830,7 +1830,7 @@ public BlobsSidecar randomBlobsSidecar(final Bytes32 blockRoot, final UInt64 slo
}

public BlobsSidecar randomBlobsSidecar(
final Bytes32 blockRoot, final UInt64 slot, int numberOfBlobs) {
final Bytes32 blockRoot, final UInt64 slot, final int numberOfBlobs) {
final BlobsSidecarSchema blobsSidecarSchema =
SchemaDefinitionsEip4844.required(spec.getGenesisSchemaDefinitions())
.getBlobsSidecarSchema();
Expand Down
12 changes: 6 additions & 6 deletions ...transition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,25 @@ public interface BlobsSidecarManager {
BlobsSidecarManager NOOP =
new BlobsSidecarManager() {
@Override
public void storeUnconfirmedValidatedBlobs(BlobsSidecar blobsSidecar) {}
public void storeUnconfirmedValidatedBlobsSidecar(BlobsSidecar blobsSidecar) {}

@Override
public void storeUnconfirmedBlobs(BlobsSidecar blobsSidecar) {}
public void storeUnconfirmedBlobsSidecar(BlobsSidecar blobsSidecar) {}

@Override
public void discardBlobsByBlock(SignedBeaconBlock block) {}
public void discardBlobsSidecarByBlock(SignedBeaconBlock block) {}

@Override
public BlobsSidecarAvailabilityChecker createAvailabilityChecker(SignedBeaconBlock block) {
return BlobsSidecarAvailabilityChecker.NOOP;
}
};

void storeUnconfirmedValidatedBlobs(BlobsSidecar blobsSidecar);
void storeUnconfirmedValidatedBlobsSidecar(BlobsSidecar blobsSidecar);

void storeUnconfirmedBlobs(BlobsSidecar blobsSidecar);
void storeUnconfirmedBlobsSidecar(BlobsSidecar blobsSidecar);

void discardBlobsByBlock(SignedBeaconBlock block);
void discardBlobsSidecarByBlock(SignedBeaconBlock block);

BlobsSidecarAvailabilityChecker createAvailabilityChecker(SignedBeaconBlock block);
}
22 changes: 12 additions & 10 deletions ...sition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import tech.pegasys.teku.storage.client.RecentChainData;

public class BlobsSidecarManagerImpl implements BlobsSidecarManager, SlotEventsChannel {
private static final int MAX_CACHED_VALIDATED_BLOBS_PER_SLOT = 10;
private static final int MAX_CACHED_VALIDATED_BLOBS_SIDECARS_PER_SLOT = 10;
private static final Logger LOG = LogManager.getLogger();

private final Spec spec;
Expand All @@ -64,7 +64,7 @@ public BlobsSidecarManagerImpl(
}

@Override
public void storeUnconfirmedValidatedBlobs(final BlobsSidecar blobsSidecar) {
public void storeUnconfirmedValidatedBlobsSidecar(final BlobsSidecar blobsSidecar) {
// cache already validated blobs
validatedPendingBlobs
.computeIfAbsent(blobsSidecar.getBeaconBlockSlot(), __ -> createNewMap())
Expand All @@ -74,16 +74,12 @@ public void storeUnconfirmedValidatedBlobs(final BlobsSidecar blobsSidecar) {
}

@Override
public void storeUnconfirmedBlobs(final BlobsSidecar blobsSidecar) {
// remove blobs from the already validated cache
Optional.ofNullable(validatedPendingBlobs.get(blobsSidecar.getBeaconBlockSlot()))
.ifPresent(map -> map.remove(blobsSidecar.getBeaconBlockRoot()));

public void storeUnconfirmedBlobsSidecar(final BlobsSidecar blobsSidecar) {
internalStoreUnconfirmedBlobs(blobsSidecar);
}

@Override
public void discardBlobsByBlock(final SignedBeaconBlock block) {
public void discardBlobsSidecarByBlock(final SignedBeaconBlock block) {
final SlotAndBlockRoot blobsAtSlotAndBlockRoot =
new SlotAndBlockRoot(block.getSlot(), block.getRoot());

Expand Down Expand Up @@ -120,12 +116,18 @@ public void onSlot(final UInt64 slot) {
private void internalStoreUnconfirmedBlobs(final BlobsSidecar blobsSidecar) {
storageUpdateChannel
.onBlobsSidecar(blobsSidecar)
.thenRun(() -> LOG.debug("Unconfirmed BlobsSidecar stored {}", blobsSidecar))
.thenRun(
() ->
LOG.debug(
"Unconfirmed BlobsSidecar stored for {}",
() ->
new SlotAndBlockRoot(
blobsSidecar.getBeaconBlockSlot(), blobsSidecar.getBeaconBlockRoot())))
.ifExceptionGetsHereRaiseABug();
}

private Map<Bytes32, BlobsSidecar> createNewMap() {
return LimitedMap.createSynchronized(MAX_CACHED_VALIDATED_BLOBS_PER_SLOT);
return LimitedMap.createSynchronized(MAX_CACHED_VALIDATED_BLOBS_SIDECARS_PER_SLOT);
}

private Optional<BlobsSidecarAvailabilityChecker> handleEmptyBlockCommitmentsChecker(
Expand Down
14 changes: 7 additions & 7 deletions ...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ private SafeFuture<BlockImportResult> doImportBlock(
final Optional<BlockImportPerformance> blockImportPerformance) {
return handleInvalidBlock(block)
.or(() -> handleKnownBlock(block))
.or(() -> handleBlobs(blobsSidecar, validationResult))
.or(() -> handleBlobsSidecar(blobsSidecar, validationResult))
.orElseGet(
() ->
handleBlockImport(block, blockImportPerformance)
Expand Down Expand Up @@ -240,16 +240,16 @@ private Optional<SafeFuture<BlockImportResult>> handleKnownBlock(final SignedBea
SafeFuture.completedFuture(BlockImportResult.knownBlock(block, isOptimistic)));
}

private Optional<SafeFuture<BlockImportResult>> handleBlobs(
private Optional<SafeFuture<BlockImportResult>> handleBlobsSidecar(
Optional<BlobsSidecar> blobsSidecar, Optional<InternalValidationResult> validationResult) {
if (blobsSidecar.isEmpty()) {
return Optional.empty();
}
if (validationResult.map(InternalValidationResult::isAccept).orElse(false)) {
blobsSidecarManager.storeUnconfirmedValidatedBlobs(blobsSidecar.get());
blobsSidecarManager.storeUnconfirmedValidatedBlobsSidecar(blobsSidecar.get());
return Optional.empty();
}
blobsSidecarManager.storeUnconfirmedBlobs(blobsSidecar.get());
blobsSidecarManager.storeUnconfirmedBlobsSidecar(blobsSidecar.get());
return Optional.empty();
}

Expand Down Expand Up @@ -302,7 +302,7 @@ private SafeFuture<BlockImportResult> handleBlockImport(
// Trigger the fetcher in the case the coupled BeaconBlockAndBlobsSidecar
// contains a valid block but the BlobsSidecar validation fails.
// Should be similar to what we do with pendingBlocks.
blobsSidecarManager.discardBlobsByBlock(block);
blobsSidecarManager.discardBlobsSidecarByBlock(block);
break;
default:
LOG.trace(
Expand Down Expand Up @@ -330,7 +330,7 @@ private void dropInvalidBlock(

invalidBlockRoots.put(block.getMessage().hashTreeRoot(), blockImportResult);
pendingBlocks.remove(block);
blobsSidecarManager.discardBlobsByBlock(block);
blobsSidecarManager.discardBlobsSidecarByBlock(block);

pendingBlocks
.getItemsDependingOn(blockRoot, true)
Expand All @@ -340,7 +340,7 @@ private void dropInvalidBlock(
blockToDrop.getMessage().hashTreeRoot(),
BlockImportResult.FAILED_DESCENDANT_OF_INVALID_BLOCK);
pendingBlocks.remove(blockToDrop);
blobsSidecarManager.discardBlobsByBlock(block);
blobsSidecarManager.discardBlobsSidecarByBlock(block);
});
}

Expand Down
2 changes: 1 addition & 1 deletion ...transition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public SafeFuture<InternalValidationResult> validate(
if (!blockKzgCommitmentsAreValidAgainstBlobsSidecar(
block, blobsSidecar.get(), miscHelpers)) {
return reject(
"Block kzg commitments are invalid against the given blobs sidecars");
"Block kzg commitments are invalid against the given blobs sidecar");
}
}

Expand Down
17 changes: 9 additions & 8 deletions ...sition/src/test/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,17 @@ void setUp() {
}

@Test
void shouldStoreUnconfirmedValidatedBlobs() {
void shouldStoreUnconfirmedValidatedBlobsSidecar() {
final BlobsSidecar blobsSidecar = dataStructureUtil.randomBlobsSidecar();
blobsSidecarManager.storeUnconfirmedValidatedBlobs(blobsSidecar);
blobsSidecarManager.storeUnconfirmedValidatedBlobsSidecar(blobsSidecar);

verify(storageUpdateChannel).onBlobsSidecar(blobsSidecar);
}

@Test
void shouldStoreUnconfirmedBlobs() {
void shouldStoreUnconfirmedBlobsSidecar() {
final BlobsSidecar blobsSidecar = dataStructureUtil.randomBlobsSidecar();
blobsSidecarManager.storeUnconfirmedBlobs(blobsSidecar);
blobsSidecarManager.storeUnconfirmedBlobsSidecar(blobsSidecar);

verify(storageUpdateChannel).onBlobsSidecar(blobsSidecar);
}
Expand All @@ -82,7 +82,8 @@ void shouldCreatePreValidatedAvailabilityChecker() {
final SignedBeaconBlockAndBlobsSidecar blockAndBlobsSidecar =
dataStructureUtil.randomConsistentSignedBeaconBlockAndBlobsSidecar();

blobsSidecarManager.storeUnconfirmedValidatedBlobs(blockAndBlobsSidecar.getBlobsSidecar());
blobsSidecarManager.storeUnconfirmedValidatedBlobsSidecar(
blockAndBlobsSidecar.getBlobsSidecar());

final BlobsSidecarAvailabilityChecker blobsSidecarAvailabilityChecker =
blobsSidecarManager.createAvailabilityChecker(blockAndBlobsSidecar.getSignedBeaconBlock());
Expand Down Expand Up @@ -114,7 +115,7 @@ void shouldCreateValidatingAvailabilityChecker() {
final SignedBeaconBlockAndBlobsSidecar blockAndBlobsSidecar =
dataStructureUtil.randomConsistentSignedBeaconBlockAndBlobsSidecar();

blobsSidecarManager.storeUnconfirmedBlobs(blockAndBlobsSidecar.getBlobsSidecar());
blobsSidecarManager.storeUnconfirmedBlobsSidecar(blockAndBlobsSidecar.getBlobsSidecar());

final BlobsSidecarAvailabilityChecker blobsSidecarAvailabilityChecker =
blobsSidecarManager.createAvailabilityChecker(blockAndBlobsSidecar.getSignedBeaconBlock());
Expand All @@ -141,8 +142,8 @@ void shouldDiscardCachedValidatedBlobsOnSlot() {
final BlobsSidecar blobs1 = dataStructureUtil.randomBlobsSidecar(blockRoot, UInt64.ONE);
final BlobsSidecar blobs2 = dataStructureUtil.randomBlobsSidecar(blockRoot, UInt64.valueOf(2));

blobsSidecarManager.storeUnconfirmedValidatedBlobs(blobs1);
blobsSidecarManager.storeUnconfirmedValidatedBlobs(blobs2);
blobsSidecarManager.storeUnconfirmedValidatedBlobsSidecar(blobs1);
blobsSidecarManager.storeUnconfirmedValidatedBlobsSidecar(blobs2);

assertThat(blobsSidecarManager.getValidatedPendingBlobsForSlot(UInt64.ONE))
.containsEntry(blockRoot, blobs1);
Expand Down
28 changes: 14 additions & 14 deletions ...atetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,8 +661,8 @@ void onGossipWithBlobs_shouldStoreValidatedBlobs() {

blockManager.validateAndImportBlockAndBlobsSidecar(blockAndBlobsSidecar);

verify(blobsSidecarManager).storeUnconfirmedValidatedBlobs(blobsSidecar);
verify(blobsSidecarManager, never()).discardBlobsByBlock(block);
verify(blobsSidecarManager).storeUnconfirmedValidatedBlobsSidecar(blobsSidecar);
verify(blobsSidecarManager, never()).discardBlobsSidecarByBlock(block);
}

@Test
Expand All @@ -681,15 +681,15 @@ private void assertImportBlockWithResult(SignedBeaconBlock block, FailureReason
.isCompletedWithValueMatching(result -> result.getFailureReason().equals(failureReason));

if (nonStoringBlobsFailureReasons.contains(failureReason)) {
verify(blobsSidecarManager, never()).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager, never()).storeUnconfirmedBlobsSidecar(blobsSidecar);
} else {
verify(blobsSidecarManager).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager).storeUnconfirmedBlobsSidecar(blobsSidecar);
}
if (nonDiscardingBlobsFailureReasons.contains(failureReason)) {
// no discard for these reason
verify(blobsSidecarManager, never()).discardBlobsByBlock(block);
verify(blobsSidecarManager, never()).discardBlobsSidecarByBlock(block);
} else {
verify(blobsSidecarManager, atLeastOnce()).discardBlobsByBlock(block);
verify(blobsSidecarManager, atLeastOnce()).discardBlobsSidecarByBlock(block);
}
}

Expand All @@ -699,15 +699,15 @@ private void assertImportBlockWithResult(
assertThat(blockManager.importBlock(block, Optional.of(blobsSidecar)))
.isCompletedWithValueMatching(result -> result.equals(importResult));
if (nonStoringBlobsFailureReasons.contains(importResult.getFailureReason())) {
verify(blobsSidecarManager, never()).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager, never()).storeUnconfirmedBlobsSidecar(blobsSidecar);
} else {
verify(blobsSidecarManager).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager).storeUnconfirmedBlobsSidecar(blobsSidecar);
}
if (nonDiscardingBlobsFailureReasons.contains(importResult.getFailureReason())) {
// no discard for these reasons
verify(blobsSidecarManager, never()).discardBlobsByBlock(block);
verify(blobsSidecarManager, never()).discardBlobsSidecarByBlock(block);
} else {
verify(blobsSidecarManager, atLeastOnce()).discardBlobsByBlock(block);
verify(blobsSidecarManager, atLeastOnce()).discardBlobsSidecarByBlock(block);
}
}

Expand All @@ -716,16 +716,16 @@ private void assertValidateAndImportBlockRejectWithoutValidation(final SignedBea
assertThat(blockManager.validateAndImportBlock(block, Optional.of(blobsSidecar)))
.isCompletedWithValueMatching(InternalValidationResult::isReject);
verify(blockValidator, never()).validate(block, Optional.of(blobsSidecar));
verify(blobsSidecarManager, never()).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager, atLeastOnce()).discardBlobsByBlock(block);
verify(blobsSidecarManager, never()).storeUnconfirmedBlobsSidecar(blobsSidecar);
verify(blobsSidecarManager, atLeastOnce()).discardBlobsSidecarByBlock(block);
}

private void assertImportBlockSuccessfully(SignedBeaconBlock block) {
BlobsSidecar blobsSidecar = dataStructureUtil.randomBlobsSidecar();
assertThat(blockManager.importBlock(block, Optional.of(blobsSidecar)))
.isCompletedWithValueMatching(BlockImportResult::isSuccessful);
verify(blobsSidecarManager).storeUnconfirmedBlobs(blobsSidecar);
verify(blobsSidecarManager, never()).discardBlobsByBlock(block);
verify(blobsSidecarManager).storeUnconfirmedBlobsSidecar(blobsSidecar);
verify(blobsSidecarManager, never()).discardBlobsSidecarByBlock(block);
}

private UInt64 incrementSlot() {
Expand Down
4 changes: 2 additions & 2 deletions ...transition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void setup() {

forkChoice.subscribeToOptimisticHeadChangesAndUpdate(optimisticSyncStateTracker);

// bobs always available
// blobs always available
if (spec.isMilestoneSupported(SpecMilestone.EIP4844)) {
final BlobsSidecar blobsSidecar = dataStructureUtil.randomBlobsSidecar();
when(blobsSidecarAvailabilityChecker.initiateDataAvailabilityCheck()).thenReturn(true);
Expand Down Expand Up @@ -209,7 +209,7 @@ void onBlock_shouldFailIfBlobsAreNotAvailable() {
}

@Test
void onBlock_shouldFailIfBlobsAreNotRequired() {
void onBlock_shouldImportIfBlobsAreNotRequired() {
final SignedBlockAndState blockAndState = chainBuilder.generateBlockAtSlot(ONE);
storageSystem.chainUpdater().advanceCurrentSlotToAtLeast(blockAndState.getSlot());

Expand Down

0 comments on commit 5bc09d9

Please sign in to comment.