Finish DA check on 50% columns #10248

zilm13 · 2025-12-17T22:57:44Z

PR Description

As user could quit at any moment during forward sync, this PR requires following prerequisites:

enabling reworked DasCustodyBackfiller by default
revisiting DasCustodyBackfiller's first run checks as not fully completed slots could be in a big range

Looks working:

2025-12-17 23:29:30.882 INFO  - Check is finished for 13176947(0x062c7b2808cd05112d6b507efd95e5bc84ee4464612e74408530032706868b9e), missing 64
2025-12-17 23:29:41.104 INFO  - Block 13176947(0x062c7b2808cd05112d6b507efd95e5bc84ee4464612e74408530032706868b9e) has been imported
2025-12-17 23:29:43.708 INFO  - Full check is finished for 13176947(0x062c7b2808cd05112d6b507efd95e5bc84ee4464612e74408530032706868b9e), missing 0
2025-12-17 23:29:47.049 INFO  - Removed fetch completed 13176947(0x062c7b2808cd05112d6b507efd95e5bc84ee4464612e74408530032706868b9e)

2025-12-17 23:29:41.407 INFO  - Check is finished for 13176949(0x01eeaa1de04a3bdc2c6a0ca080db0479aefff7b409f2a3e1c759b4a49ee9839f), missing 63
2025-12-17 23:29:43.495 INFO  - Block 13176949(0x01eeaa1de04a3bdc2c6a0ca080db0479aefff7b409f2a3e1c759b4a49ee9839f) has been imported
2025-12-17 23:29:43.749 INFO  - Full check is finished for 13176949(0x01eeaa1de04a3bdc2c6a0ca080db0479aefff7b409f2a3e1c759b4a49ee9839f), missing 0
2025-12-17 23:29:47.049 INFO  - Removed fetch completed 13176949(0x01eeaa1de04a3bdc2c6a0ca080db0479aefff7b409f2a3e1c759b4a49ee9839f)

Fixed Issue(s)

Main part of #9938

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.

Note

Introduces optional early completion of data availability after 50% of columns, controlled by a new P2P config and CLI flag, with sampler/tracker logic and tests updated.

Data Availability Sampling (DAS):
- Add halfColumnsSamplingCompletionEnabled to DasSamplerBasic and pass to DataColumnSamplingTracker to allow early completion when half of columns are sampled (Fulu-derived column count/2).
- Replace rpcFetchScheduled with rpcFetchInProgress; adjust scheduling/reset logic and first-seen handling.
- Update slot-pruning to only remove fully sampled trackers; incomplete ones complete exceptionally when outdated/imported.
- Reference tests construct DasSamplerBasic with early-completion disabled.
Tracker Enhancements (DataColumnSamplingTracker):
- Track fullySampled and optional earlyCompletionRequirementCount; implement partial completion on threshold; expose getMissingColumnIdentifiers().
Config/CLI Wiring:
- Add columnsDataAvailabilityHalfCheckEnabled to P2PConfig (default false) and builder method; expose getter.
- Add hidden CLI flag --Xcolumns-data-availability-half-check-enabled; plumb into P2PConfig.
- BeaconChainController passes the flag to DasSamplerBasic.
Tests:
- Extend/adjust DAS sampler and tracker unit tests for early completion, in-progress flag, and pruning behavior.
- Add CLI option tests for default and toggle behavior.

^{Written by Cursor Bugbot for commit 0aa89ba. This will update automatically on new commits. Configure here.}

zilm13 · 2025-12-18T14:51:59Z

the decision is instead of blocking:

add hidden command line flag to turn feature on
disable by default
merge today, add issue for blockers, fix it later

tbenr

some questions first

tbenr · 2025-12-18T15:54:24Z

...n/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DataColumnSamplingTracker.java

    if (!removed) {
      LOG.debug("Column {} was already marked as received, origin: {}", columnIdentifier, origin);
      return false;
+    } else {


remove else if we return in the main branch

tbenr · 2025-12-18T15:56:39Z

...n/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DataColumnSamplingTracker.java

+      if (completionColumnCount().isPresent()
+          && !completionFuture.isDone()
+          && (samplingRequirement.size() - missingColumns().size())
+              >= completionColumnCount.get()) {


too much noise here, can we have a dedicated method to check this condition?

tbenr · 2025-12-18T16:04:58Z

...n/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DataColumnSamplingTracker.java

    if (missingColumns.isEmpty()) {
      LOG.debug(
-          "Sampling complete for slot {} root {} via column {} received via {}",
+          "Fetching complete for slot {} root {} via column {} received via {}",


I disagree with the renaming here.
We are still in the context of Sampling. We maybe in a "partial sampling" case or "full sampling case".

tbenr · 2025-12-18T16:05:49Z

...n/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DataColumnSamplingTracker.java

    final Set<UInt64> missingColumns = ConcurrentHashMap.newKeySet(samplingRequirement.size());
    missingColumns.addAll(samplingRequirement);
+    final SafeFuture<List<UInt64>> completionFuture = new SafeFuture<>();
+    final SafeFuture<List<UInt64>> fetchCompletionFuture = new SafeFuture<>();


why do we need this new future? which flow will be attached to it?

let's talk about it, I don't understand you

tbenr · 2025-12-18T16:33:19Z

...etransition/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasic.java

+                  tracker
+                      .completionFuture()
+                      .completeExceptionally(new RuntimeException("DAS sampling expired"));
                  return true;


i'm worried about the possibility we discard the tracker but there is still fetchCompletionFuture not completed, with even some pending requests still pending to complete the full sampling.

I've added completion to fetchCompletionFuture(), I'm not sure it's needed because there are no consumers for it which is different to completionFuture(). Anyway, let's have it.

well if we want to represent it with a SafeFuture we must maintain it correctly, otherwise we can make it as an AtomicBoolean and problem solved.

...nsition/src/test/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasicTest.java

This reverts commit 74ffa44.

cursor · 2025-12-20T09:20:10Z

...etransition/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasic.java


-              return false;
+              // cleanup only if fully sampled
+              return tracker.fullySampled().get();


Memory leak for orphaned blocks with early completion

The new cleanup logic in onSlot doesn't remove trackers for orphaned blocks that received early completion. When a block's slot becomes finalized but the block itself is not in the chain (orphaned due to reorg), and the tracker has completionFuture done from early completion but fullySampled is false, the tracker will never be cleaned up. The old logic removed any tracker with a done completionFuture when the slot was finalized or block imported. The new logic falls through to tracker.fullySampled().get() which returns false, keeping the tracker forever. These orphaned trackers accumulate because the DasCustodyBackfiller won't complete sampling for blocks not in the canonical chain.

until we complete all flows around this feature, this is actually true. The feature is turned off by default due to this (and the fact that we can't currently guarantee we complete the sampling and thus set fullySampled to true)

cursor · 2025-12-20T10:23:07Z

...etransition/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasic.java

+                  tracker
+                      .completionFuture()
+                      .completeExceptionally(
+                          new RuntimeException("DAS sampling expired while slot finalized"));


Race condition can prematurely remove partially completed trackers

There's a race condition between checking completionFuture().isDone() and calling completeExceptionally(). If another thread completes the future with partial completion (via isCompletedEarly()) between these two operations, completeExceptionally() becomes a no-op but the code still returns true, removing the tracker. For partial completions, fullySampled is not set to true, so the tracker should remain in the map to allow background downloading of remaining columns. The return value of completeExceptionally() could be used to detect this race.

tbenr

LGTM but one cursor's comment is important

tbenr · 2025-12-21T17:38:24Z

...etransition/src/main/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasic.java

+                      .completionFuture()
+                      .completeExceptionally(
+                          new RuntimeException("DAS sampling expired while slot finalized"));
                  return true;


i think we can even remove this return true

Finish DA check on 50% columns

c8883e5

zilm13 added the blocked by another PR/issue 💔 This issue or pull request is blocked by another label Dec 17, 2025

zilm13 added 3 commits December 18, 2025 01:20

fix reference tests + future column count changes

57f86b4

Merge branch 'master' into 50-percent-import

0b61b60

Add flag, disable by default + tests

62119d4

zilm13 marked this pull request as ready for review December 18, 2025 14:50

zilm13 removed the blocked by another PR/issue 💔 This issue or pull request is blocked by another label Dec 18, 2025

tbenr reviewed Dec 18, 2025

View reviewed changes

zilm13 and others added 5 commits December 18, 2025 23:57

feedback addressed

d51252c

my version

c7c819b

my version fix

891271f

fix tests

832ae1c

Merge branch 'master' into 50-percent-import

5467d5b

cursor bot reviewed Dec 19, 2025

View reviewed changes

...nsition/src/test/java/tech/pegasys/teku/statetransition/datacolumns/DasSamplerBasicTest.java Outdated Show resolved Hide resolved

zilm13 added 3 commits December 19, 2025 18:54

fix test and logic

74ffa44

Revert "fix test and logic"

48d5243

This reverts commit 74ffa44.

fix logic and test

c0741af

cursor bot reviewed Dec 20, 2025

View reviewed changes

Fix logic and add test for completed not imported

0aa89ba

cursor bot reviewed Dec 20, 2025

View reviewed changes

tbenr approved these changes Dec 21, 2025

View reviewed changes

Finish DA check on 50% columns #10248

Are you sure you want to change the base?

Finish DA check on 50% columns #10248

Conversation

zilm13 commented Dec 17, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Fixed Issue(s)

Documentation

Changelog

Uh oh!

zilm13 commented Dec 18, 2025

Uh oh!

tbenr 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!

Uh oh!

cursor bot Dec 20, 2025

Choose a reason for hiding this comment

Memory leak for orphaned blocks with early completion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Dec 20, 2025

Choose a reason for hiding this comment

Race condition can prematurely remove partially completed trackers

Uh oh!

tbenr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zilm13 commented Dec 17, 2025 •

edited by cursor bot

Loading