Integrating das branch into master by lucassaldanha · Pull Request #9458 · Consensys/teku

lucassaldanha · 2025-05-22T01:33:22Z

PR Description

Integrating das branch into master.

Fixed Issue(s)

N/A

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java

.../src/main/java/tech/pegasys/teku/networking/eth2/peers/GossipTopicDasPeerCustodyTracker.java

lucassaldanha · 2025-05-22T04:40:37Z

Everything that was mapped as TODO-fulu has now a ticket to make sure we don't miss anything. All tickets are linked as sub-tasks on our PeerDAS cleanup epic (#9434).

...src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java

.../src/main/java/tech/pegasys/teku/validator/coordinator/publisher/AbstractBlockPublisher.java

ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/util/BlobsUtil.java

...ion/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DataAvailabilitySampler.java

...ain/java/tech/pegasys/teku/statetransition/datacolumns/retriever/SimpleSidecarRetriever.java

...java/tech/pegasys/teku/statetransition/datacolumns/retriever/SimpleSidecarRetrieverTest.java

...ion/src/testFixtures/java/tech/pegasys/teku/statetransition/datacolumns/DasCustodyStand.java

zilm13 · 2025-05-23T19:53:00Z

We should add this to commit comment if squash-merged

Co-authored-by: Dmitrii Shmatko <zilm@zilm.net>
Co-authored-by: Anton Nashatyrev <anton.nashatyrev@gmail.com>
Co-authored-by: Jeroen Ost <jeroen.ost2@sony.com>
Co-authored-by: Ekaterina Riazantseva <sibkatya@gmail.com>

zilm13 · 2025-05-24T22:29:18Z

I've completed first round of review comparing it to das, looks good, nothing lost except #9408, which I guess, was not cherry-picked yet
Next round is casual review

also fixed `initDasCustody` a bit - missing finals - random anonymous code segments Signed-off-by: Paul Harris <paul.harris@consensys.net>

Signed-off-by: Paul Harris <paul.harris@consensys.net>

start on some of the codeql comments Signed-off-by: Paul Harris <paul.harris@consensys.net>

Signed-off-by: Paul Harris <paul.harris@consensys.net>

...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/DebugDataFileDumper.java

...java/tech/pegasys/teku/statetransition/datacolumns/retriever/SimpleSidecarRetrieverTest.java

lucassaldanha · 2025-05-28T05:02:30Z

...java/tech/pegasys/teku/statetransition/datacolumns/retriever/SimpleSidecarRetrieverTest.java

+    final List<TestPeer> testNodes =
+        craftNodeIds()
+            .map(
+                nodeId ->
+                    new TestPeer(stubAsyncRunner, nodeId, Duration.ofMillis(100))
+                        .currentRequestLimit(1000))
+            .limit(128)
+            .peek(node -> custodyCountSupplier.setCustomCount(node.getNodeId(), columnCount))
+            .peek(testPeerManager::connectPeer)
+            .toList();


@zilm13 this unit test performanceTest() seems a bit odd. Help me understand what we are doing here?

Signed-off-by: Paul Harris <paul.harris@consensys.net>

…r-das

Signed-off-by: Paul Harris <paul.harris@consensys.net>

zilm13

First half of the review

zilm13 · 2025-05-26T17:59:42Z

...ance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/SimpleHttpClient.java

    headers.forEach(builder::header);
    final Response response = httpClient.newCall(builder.build()).execute();
    final ResponseBody body = response.body();
    if (!response.isSuccessful()) {


nit: It's more readable when not inverted

zilm13 · 2025-05-26T18:11:41Z

beacon/sync/src/test/java/tech/pegasys/teku/beacon/sync/forward/singlepeer/PeerSyncTest.java

+
+    // Check that the sync is done and the peer was not disconnected.
+    assertThat(syncFuture).isCompleted();
+    verify(peer, never()).disconnectCleanly(any());


we could add disconnectImmediately check if paranoid also

zilm13 · 2025-05-26T18:23:29Z

beacon/sync/src/test/java/tech/pegasys/teku/beacon/sync/forward/singlepeer/PeerSyncTest.java

+
+    completeRequestWithBlocksAtSlots(blocksRequestFuture, denebSecondSlot, denebPeerSlotsAhead);
+
+    verifyBlobSidecarsAddedToPool(denebSecondSlot, slotsPerEpochMinimal, blobSidecarsBySlot);


I don't like this test. It delivers last BlobSidecar in a Fulu epoch, which shouldn't happen. Probably it's set with when but it shouldn't be like this anyway. I'd address it in a separate issue

zilm13 · 2025-05-26T18:24:04Z

beacon/sync/src/test/java/tech/pegasys/teku/beacon/sync/forward/singlepeer/PeerSyncTest.java

+    when(recentChainData.getFinalizedEpoch()).thenReturn(denebPeerFinalizedEpoch);
+
+    final UInt64 slotsPerEpochMinimal = UInt64.valueOf(8);
+    final UInt64 denebLastSlot = denebFirstSlot.plus(slotsPerEpochMinimal);


it's in FULU, not in deneb, there is even a check below

zilm13 · 2025-05-26T18:28:04Z

...main/java/tech/pegasys/teku/validator/coordinator/performance/DefaultPerformanceTracker.java

        "Checking if block {} is in historic block roots {} of the state {}",
        producedBlock,
-        state.getBlockRoots(),
+        state.getBlockRoots().hashTreeRoot(),


I think we could remove this parameter completely, not sure it makes any sense

zilm13 · 2025-05-26T20:46:22Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/executionlayer/ExecutionLayerChannel.java

+  SafeFuture<List<Optional<BlobAndProof>>> engineGetBlobAndProofs(
+      List<VersionedHash> blobVersionedHashes, UInt64 slot);
+
+  SafeFuture<List<BlobAndCellProofs>> engineGetBlobAndCellProofsList(


I don't like this interface difference. From the spec we either get all or nothing here. Maybe it should be wrapped with Optional<> too? I'd at least create an issue to revisit

zilm13 · 2025-05-26T20:48:36Z

...ava/tech/pegasys/teku/spec/logic/common/statetransition/results/FailedBlockImportResult.java

  }

+  @Override
+  public boolean isDataNotYetAvailable() {


do we need Yet here and previously in BatchImportResult?

zilm13 · 2025-05-26T20:51:17Z

...src/main/java/tech/pegasys/teku/spec/datastructures/blocks/BlockContentsWithBlobsSchema.java

+import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.Blob;
+import tech.pegasys.teku.spec.datastructures.type.SszKZGProof;
+
+public interface BlockContentsWithBlobsSchema<T extends BlockContainer>


BlockContents are always with Blobs, may worth renaming this and Signed container

zilm13 · 2025-05-26T20:57:40Z

...ec/src/test/java/tech/pegasys/teku/spec/logic/versions/fulu/helpers/MiscHelpersFuluTest.java

+  @MethodSource("getExtendedSampleCountFixtures")
+  public void getExtendedSampleCountReturnsCorrectValues(
+      final int allowedFailures, final int numberOfSamples) {
+    assertThat(miscHelpersFulu.getExtendedSampleCount(UInt64.valueOf(allowedFailures)))


I'd clarify in MiscHelpersFulu that called method that it's not actually used anymore to avoid any confusion. It was designed for lossy sampling which is not a part of the spec anymore. But called helper is still part of the spec surprisingly

zilm13 · 2025-05-26T21:00:26Z

ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/TestSpecFactory.java

+            Eth2Network.MINIMAL, capellaForkEpoch, denebForkEpoch, electraForkEpoch, fuluForkEpoch);
+    return create(config, SpecMilestone.FULU);
  }



I'd restore removed javadoc here and change ELECTRA to FULU:

// Our current config files contain FULU params. // So all specConfigs created from them will be FULU. // Here we just want to make sure that a given config supports the given milestone // (which useless in theory because they are all FULU)

zilm13

Second and final part of the review

I'm not sure on info, warn, error logging downgrade suggestions. If doesn't come to EventLogger, may be we could keep it? I've started commeinting on it but not sure now. There were several other info logs left that I had not commented (for example, in DataColumnSidecarAvailabilityChecker, GossipFailureLogger etc). They should be addressed if we decide to downgrade them all out of info.
We may need to add finals everywhere, especially finals are missing in tests and testFixtures

zilm13 · 2025-05-28T13:01:22Z

...java/tech/pegasys/teku/statetransition/datacolumns/retriever/DataColumnSidecarRetriever.java

+   * The request may complete with this exception when requested column is no more on our local
+   * canonical chain
+   */
+  class NotOnCanonicalChainException extends RuntimeException {


maybe IllegalStateException is more appropriate for it?

zilm13 · 2025-05-28T13:19:58Z

...ch/pegasys/teku/statetransition/datacolumns/util/DataColumnSidecarELRecoveryManagerImpl.java

+              .toList();
+    }
+
+    LOG.info(


not info for production, it happens on every slot

zilm13 · 2025-05-28T13:20:59Z

...ch/pegasys/teku/statetransition/datacolumns/util/DataColumnSidecarELRecoveryManagerImpl.java

+        .engineGetBlobAndCellProofsList(versionedHashes, slotAndBlockRoot.getSlot())
+        .thenAccept(
+            blobAndCellProofsList -> {
+              LOG.info("Found {} blobs", blobAndCellProofsList.size());


downgrade level please

zilm13 · 2025-05-28T13:21:06Z

...ch/pegasys/teku/statetransition/datacolumns/util/DataColumnSidecarELRecoveryManagerImpl.java

+                  versionedHashes.size(),
+                  blobAndCellProofsList.size());
+
+              LOG.info(


downgrade level please

zilm13 · 2025-05-28T13:21:12Z

...ch/pegasys/teku/statetransition/datacolumns/util/DataColumnSidecarELRecoveryManagerImpl.java

+  }
+
+  private void logLocalElBlobsLookupFailure(final Throwable error) {
+    LOG.warn("Local EL blobs lookup failed: {}", getRootCauseMessage(error));


downgrade level please

zilm13 · 2025-05-28T15:17:53Z

teku/src/main/java/tech/pegasys/teku/cli/options/InteropOptions.java

@@ -61,6 +62,14 @@ public class InteropOptions {
      description = "Represents the total number of validators in the network")
  private int interopNumberOfValidators = InteropConfig.DEFAULT_INTEROP_NUMBER_OF_VALIDATORS;

+  @Option(


Implementation of this option was dropped. We could have a separate issue for removal.

zilm13 · 2025-05-28T15:18:32Z

...ient/src/main/java/tech/pegasys/teku/validator/client/AttestationDutySchedulingStrategy.java

@@ -1,5 +1,5 @@
 /*
- * Copyright Consensys Software Inc., 2025
+ * Copyright Consensys Software Inc., 2024


Intentional change?

zilm13 · 2025-05-28T15:24:16Z

storage/src/main/java/tech/pegasys/teku/storage/server/rocksdb/RocksDbInstanceFactory.java

@@ -147,6 +147,8 @@ private static DBOptions createDBOptions(
            .setMaxBackgroundJobs(configuration.getMaxBackgroundJobs())
            .setDbWriteBufferSize(configuration.getWriteBufferCapacity())
            .setMaxOpenFiles(configuration.getMaxOpenFiles())
+            .setBytesPerSync(1_048_576L) // 1MB


both these settings should be removed as they were intentionally removed from the master and restored here by merge error

Nice catch!

zilm13 · 2025-05-28T15:35:53Z

.../beaconchain/src/main/java/tech/pegasys/teku/services/beaconchain/BeaconChainController.java

+    this.nodeId =
+        p2pNetwork
+            .getDiscoveryNodeId()
+            .orElseThrow(() -> new InvalidConfigurationException("NodeID is required"));


Maybe something more friendly to user, say "Failed to get NodeId from Discovery System"

zilm13 · 2025-05-28T15:39:15Z

...t/java/tech/pegasys/teku/statetransition/forkchoice/BlobSidecarsAvailabilityCheckerTest.java

+import tech.pegasys.teku.storage.client.RecentChainData;
+import tech.pegasys.teku.storage.store.UpdatableStore;
+
+public class BlobSidecarsAvailabilityCheckerTest {


it duplicates ForkChoiceBlobSidecarsAvailabilityCheckerTest. Remove this one, leave ForkChoiceBlobSidecarsAvailabilityCheckerTest

lucassaldanha added 3 commits May 22, 2025 12:58

DAS magic

722465c

Merge branch 'das-squashed' into master-das

54966b4

fixes

01e010f

lucassaldanha added the DO NOT MERGE Not ready to merge label May 22, 2025

github-advanced-security bot found potential problems May 22, 2025

View reviewed changes

lucassaldanha commented May 22, 2025

View reviewed changes

...m/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java Outdated Show resolved Hide resolved

lucassaldanha added 2 commits May 22, 2025 15:47

cleaning logs and unifying TODOs

dc1bf79

more log cleanning :)

3751e9f

lucassaldanha commented May 22, 2025

View reviewed changes

.../src/main/java/tech/pegasys/teku/networking/eth2/peers/GossipTopicDasPeerCustodyTracker.java Outdated Show resolved Hide resolved

Added ticket urls to TODO-fulu

7b85b7d

rolfyone reviewed May 22, 2025

View reviewed changes

...src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java Show resolved Hide resolved

lucassaldanha and others added 2 commits May 23, 2025 14:06

Merge branch 'master' into master-das

69e0c21

Fix Fulu block production (Consensys#9481)

b2c1b0f

github-advanced-security bot found potential problems May 23, 2025

View reviewed changes

jeroost and others added 8 commits May 26, 2025 10:08

Post Fulu BlobSidecars RPC deprecation (Consensys#9408)

e5dc685

Merge branch 'master' into master-das

219259c

Only creating DasPreSample when FULU is scheduled

692154e

PR comments

20ae3a3

cleanup NPE on start

9e8a3b4

also fixed `initDasCustody` a bit - missing finals - random anonymous code segments Signed-off-by: Paul Harris <paul.harris@consensys.net>

oopsie

68fe44a

Signed-off-by: Paul Harris <paul.harris@consensys.net>

cleanup

ac7e83e

start on some of the codeql comments Signed-off-by: Paul Harris <paul.harris@consensys.net>

more cleanup

a926494

Signed-off-by: Paul Harris <paul.harris@consensys.net>

github-advanced-security bot found potential problems May 26, 2025

View reviewed changes

rolfyone and others added 4 commits May 26, 2025 10:51

more cleanup

adc6662

Signed-off-by: Paul Harris <paul.harris@consensys.net>

Revert changes in EventLogger

55034f4

Merge branch 'master-das' of github.com:lucassaldanha/teku into maste…

ab28db6

…r-das

should be debug

866c035

Signed-off-by: Paul Harris <paul.harris@consensys.net>

Merge remote-tracking branch 'lucassaldanha/master-das' into master-das

a824248

zilm13 reviewed May 26, 2025

View reviewed changes

lucassaldanha marked this pull request as ready for review May 26, 2025 21:05

lucassaldanha and others added 4 commits May 27, 2025 18:57

PR comments

8750185

Add logging on DataColumnSidecars recovery failure (Consensys#9486)

15c270b

Merge branch 'master' into master-das

4367b89

CodeQL suggestions

c67fb44

zilm13 reviewed May 28, 2025

View reviewed changes

lucassaldanha added 3 commits May 29, 2025 13:49

2nd round of PR comments

cdf24b3

Merge branch 'master' into master-das

55ff4d9

Fix constructor

e55cd30

lucassaldanha removed the DO NOT MERGE Not ready to merge label May 29, 2025

lucassaldanha added 2 commits May 29, 2025 14:01

Other log downgrades

176ba3a

Decrease log

6ed6471

lucassaldanha enabled auto-merge (squash) May 29, 2025 04:35

rolfyone approved these changes May 29, 2025

View reviewed changes

lucassaldanha merged commit 7856340 into Consensys:master May 29, 2025
16 checks passed


		completeRequestWithBlocksAtSlots(blocksRequestFuture, denebSecondSlot, denebPeerSlotsAhead);

		verifyBlobSidecarsAddedToPool(denebSecondSlot, slotsPerEpochMinimal, blobSidecarsBySlot);

Comments

Conversation

lucassaldanha commented May 22, 2025

PR Description

Fixed Issue(s)

Documentation

Changelog

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lucassaldanha commented May 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zilm13 commented May 23, 2025

Uh oh!

zilm13 commented May 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Choose a reason for hiding this comment

Uh oh!

zilm13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zilm13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zilm13 left a comment •

edited

Loading

zilm13 May 28, 2025 •

edited

Loading