Skip to content

Only request column reconstruction once half the columns are cached - #10561

Open
dknopik wants to merge 2 commits into
sigp:unstablefrom
dknopik:fix-reconstruction-trigger
Open

dknopik wants to merge 2 commits into
sigp:unstablefrom
dknopik:fix-reconstruction-trigger

Conversation

@dknopik

@dknopik dknopik commented Nov 10, 2026

Copy link
Copy Markdown
Member

Issue Addressed

The reprocess queue dispatches a single reconstruction attempt per block root. A gossip column requested reconstruction regardless of how many columns were cached, so an attempt could be dispatched with fewer than half of the columns. That attempt was declined and no later column scheduled another one. Past the reconstruction deadline the delay is zero, so this happened on the first column of a late block.

Proposed Changes

check_reconstruction_trigger now returns early while fewer than half of the columns are cached, so the single attempt is only spent when reconstruction is possible.

PendingComponents::num_completed_columns and
get_cached_data_columns_indices no longer build the sidecars to count complete columns, as the trigger now queries them for every gossip column.

The reprocess queue dispatches a single reconstruction attempt per block
root. A gossip column requested reconstruction regardless of how many
columns were cached, so an attempt could be dispatched with fewer than
half of the columns. That attempt was declined and no later column
scheduled another one. Past the reconstruction deadline the delay is
zero, so this happened on the first column of a late block.

`check_reconstruction_trigger` now returns early while fewer than half
of the columns are cached, so the single attempt is only spent when
reconstruction is possible.

`PendingComponents::num_completed_columns` and
`get_cached_data_columns_indices` no longer build the sidecars to count
complete columns, as the trigger now queries them for every gossip
column.
@mergify

mergify Bot commented Nov 10, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue). If it does not match the queue conditions yet, it enters the queue once it does.

  • Queue this pull request

@dapplion dapplion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice find

.chain
.cached_data_column_indexes(block_root, epoch)
.map_or(0, |indexes| indexes.len());
if cached_column_count < T::EthSpec::number_of_columns() / 2 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This repeats the threshold from check_and_set_reconstruction_started, so the two can drift. Could the reprocess queue free the root when an attempt is declined or fails instead, or could both places use one shared predicate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

added a helper and migrated usages to it

@dapplion dapplion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice find

@dknopik
dknopik requested a review from macladson as a code owner November 12, 2026 02:32

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants