From 5bc09d9eac2473ad3d03d20766f37941b0568bf3 Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Tue, 10 Jan 2023 18:21:29 +0100 Subject: [PATCH] apply suggestions --- .../logic/common/helpers/MiscHelpers.java | 10 +++++++ .../teku/spec/util/DataStructureUtil.java | 2 +- .../blobs/BlobsSidecarManager.java | 12 ++++---- .../blobs/BlobsSidecarManagerImpl.java | 22 ++++++++------- .../statetransition/block/BlockManager.java | 14 +++++----- .../validation/BlockValidator.java | 2 +- .../blobs/BlobsSidecarManagerTest.java | 17 +++++------ .../block/BlockManagerTest.java | 28 +++++++++---------- .../forkchoice/ForkChoiceTest.java | 4 +-- 9 files changed, 62 insertions(+), 49 deletions(-) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java index b9a9c97e0c5..f2952259571 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java @@ -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, diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java index 0ed3bca40c0..48fd7d7dbec 100644 --- a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java +++ b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java @@ -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(); diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManager.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManager.java index f82ca1a5e47..af472c9d46e 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManager.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManager.java @@ -21,13 +21,13 @@ 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) { @@ -35,11 +35,11 @@ public BlobsSidecarAvailabilityChecker createAvailabilityChecker(SignedBeaconBlo } }; - 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); } diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java index f3791930370..29e47091c19 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java @@ -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; @@ -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()) @@ -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()); @@ -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 createNewMap() { - return LimitedMap.createSynchronized(MAX_CACHED_VALIDATED_BLOBS_PER_SLOT); + return LimitedMap.createSynchronized(MAX_CACHED_VALIDATED_BLOBS_SIDECARS_PER_SLOT); } private Optional handleEmptyBlockCommitmentsChecker( diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java index 6824bd5561b..11dd6cb6176 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java @@ -193,7 +193,7 @@ private SafeFuture doImportBlock( final Optional blockImportPerformance) { return handleInvalidBlock(block) .or(() -> handleKnownBlock(block)) - .or(() -> handleBlobs(blobsSidecar, validationResult)) + .or(() -> handleBlobsSidecar(blobsSidecar, validationResult)) .orElseGet( () -> handleBlockImport(block, blockImportPerformance) @@ -240,16 +240,16 @@ private Optional> handleKnownBlock(final SignedBea SafeFuture.completedFuture(BlockImportResult.knownBlock(block, isOptimistic))); } - private Optional> handleBlobs( + private Optional> handleBlobsSidecar( Optional blobsSidecar, Optional 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(); } @@ -302,7 +302,7 @@ private SafeFuture 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( @@ -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) @@ -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); }); } diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java index e8f86fd2d76..3a4f41f003c 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java @@ -150,7 +150,7 @@ public SafeFuture 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"); } } diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerTest.java index 920312cf0b9..285e778cba7 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerTest.java @@ -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); } @@ -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()); @@ -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()); @@ -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); diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java index a5f985ef68e..e94ab0476ed 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java @@ -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 @@ -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); } } @@ -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); } } @@ -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() { diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java index 9deee675f58..9b1142b668d 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java @@ -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); @@ -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());