Fix data availability checker race condition causing partial data columns to be served over RPC #7961

jimmygchen · 2025-08-28T06:59:53Z

Issue Addressed

Partially resolves #6439, an simpler alternative to #7931.

Race condition occurs when RPC data columns arrives after a block has been imported and removed from the DA checker:

Block becomes available via gossip
RPC columns arrive and pass fork choice check (block hasn't been imported)
Block import completes (removing block from DA checker)
RPC data columns finish verification and get imported into DA checker

This causes two issues:

Partial data serving: Already imported components get re-inserted, potentially causing LH to serve incomplete data
State cache misses: Leads to state reconstruction, holding the availability cache write lock longer and increasing race likelihood

Proposed Changes

Never manually remove pending components from DA checker. Components are only removed via LRU eviction as finality advances. This makes sure we don't run into the issue described above.
Use get instead of pop when recovering the executed block, this prevents cache misses in race condition. This should reduce the likelihood of the race condition
Refactor DA checker to drop write lock as soon as components are added. This should also reduce the likelihood of the race condition

Trade-offs:

This solution eliminates a few nasty race conditions while allowing simplicity, with the cost of allowing block re-import (already existing).

The increase in memory in DA checker can be partially offset by a reduction in block cache size if this really comes an issue (as we now serve recent blocks from DA checker).

mergify · 2025-08-28T08:18:42Z

Some required checks have failed. Could you please take a look @jimmygchen? 🙏

shane-moore · 2025-08-31T22:22:06Z

cool that this will simplify concurrency management for the DA checker

as a follow-up, it might be good to monitor memory usage in grafana after deploying to a devnet as to keep an eye on it

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

…ghthouse into fix-da-checker-race-take-3

jimmygchen · 2025-09-01T23:25:43Z

beacon_node/beacon_chain/src/data_availability_checker.rs

-        self.availability_cache
-            .remove_pending_components(block_root)
-    }
-


This reconstruction error makes the assumption that if the block root doesnt exist, then block is already imported, which is no longer correct. Update error message.

lighthouse/beacon_node/beacon_chain/src/data_availability_checker.rs

Lines 590 to 595 in a134d43

// Check indices from cache again to make sure we don't publish components we've already received.

let Some(existing_column_indices) = self.cached_data_column_indexes(block_root) else {

return Ok(DataColumnReconstructionResult::RecoveredColumnsNotImported(

"block already imported",

));

};

Fixed in 4d8525a. Now returns an error as it's an unexpected scenario.

michaelsproul · 2025-09-02T05:54:37Z

beacon_node/beacon_chain/src/data_availability_checker.rs

I think we should also update this comment in this file:

/// Under normal conditions, the cache should only store the current pending block, but could /// occasionally spike to 2-4 for various reasons e.g. components arriving late, but would very /// rarely go above this, unless there are many concurrent forks.

Now we expect this cache to basically be full all the time

Updated in 4d8525a, and also reduced the cache size from 64 to 32.

michaelsproul

This looks safe to me, and I like the more "idempotent" design style of keeping components rather than removing them.

I still feel like our caches could do with an overhaul and simplification, but that can wait.

…umns to be served over RPC (sigp#7961) Partially resolves sigp#6439, an simpler alternative to sigp#7931. Race condition occurs when RPC data columns arrives after a block has been imported and removed from the DA checker: 1. Block becomes available via gossip 2. RPC columns arrive and pass fork choice check (block hasn't been imported) 3. Block import completes (removing block from DA checker) 4. RPC data columns finish verification and get imported into DA checker This causes two issues: 1. **Partial data serving**: Already imported components get re-inserted, potentially causing LH to serve incomplete data 2. **State cache misses**: Leads to state reconstruction, holding the availability cache write lock longer and increasing race likelihood ### Proposed Changes 1. Never manually remove pending components from DA checker. Components are only removed via LRU eviction as finality advances. This makes sure we don't run into the issue described above. 2. Use `get` instead of `pop` when recovering the executed block, this prevents cache misses in race condition. This should reduce the likelihood of the race condition 3. Refactor DA checker to drop write lock as soon as components are added. This should also reduce the likelihood of the race condition **Trade-offs:** This solution eliminates a few nasty race conditions while allowing simplicity, with the cost of allowing block re-import (already existing). The increase in memory in DA checker can be partially offset by a reduction in block cache size if this really comes an issue (as we now serve recent blocks from DA checker).

…r` (#8045) This PR consolidates the `reqresp_pre_import_cache` into the `data_availability_checker` for the following reasons: - the `reqresp_pre_import_cache` suffers from the same TOCTOU bug we had with `data_availability_checker` earlier, and leads to unbounded memory leak, which we have observed over the last 6 months on some nodes. - the `reqresp_pre_import_cache` is no longer necessary, because we now hold blocks in the `data_availability_checker` for longer since (#7961), and recent blocks can be served from the DA checker. This PR also maintains the following functionalities - Serving pre-executed blocks over RPC, and they're now served from the `data_availability_checker` instead. - Using the cache for de-duplicating lookup requests. Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>

Another attempt to fix race condition with a simplified approach.

dd23d6f

jimmygchen mentioned this pull request Aug 28, 2025

Fix race condition causing data columns getting inserted into DA checker after block is imported #7931

Closed

jimmygchen added ready-for-review The code is ready for review v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky labels Aug 28, 2025

jimmygchen force-pushed the fix-da-checker-race-take-3 branch from 83de4dc to b8a32de Compare August 28, 2025 07:18

Add docs and fix test

b801675

jimmygchen force-pushed the fix-da-checker-race-take-3 branch from b8a32de to b801675 Compare August 28, 2025 07:44

jimmygchen changed the title ~~Another attempt to fix race condition with a simplified approach~~ Another attempt to fix race condition with a simpler approach Aug 29, 2025

jimmygchen commented Sep 1, 2025

View reviewed changes

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs Outdated Show resolved Hide resolved

jimmygchen and others added 5 commits September 1, 2025 13:53

Remove CachedComponents as it's no longer needed / used.

b864c3f

Drop the write lock before calling make_available.

ce6d1cb

Merge branch 'unstable' into fix-da-checker-race-take-3

d429abf

Clean up span after block is made available.

6ec51e4

Merge branch 'fix-da-checker-race-take-3' of github.com:jimmygchen/li…

1e57727

…ghthouse into fix-da-checker-race-take-3

jimmygchen changed the title ~~Another attempt to fix race condition with a simpler approach~~ Fix data availability checker race condition causing partial data columns to be served over RPC Sep 1, 2025

jimmygchen added the bug Something isn't working label Sep 1, 2025

Merge branch 'unstable' into fix-da-checker-race-take-3

f2ef156

jimmygchen commented Sep 1, 2025

View reviewed changes

michaelsproul reviewed Sep 2, 2025

View reviewed changes

michaelsproul approved these changes Sep 2, 2025

View reviewed changes

Update comments and reconstruction error.

4d8525a

michaelsproul approved these changes Sep 2, 2025

View reviewed changes

jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 2, 2025

mergify bot merged commit eef02af into sigp:unstable Sep 2, 2025
37 checks passed

jimmygchen deleted the fix-da-checker-race-take-3 branch September 2, 2025 21:28

jimmygchen mentioned this pull request Sep 5, 2025

Same block getting imported multiple times due to race condition #6439

Closed

jimmygchen mentioned this pull request Sep 15, 2025

Consolidate reqresp_pre_import_cache into data_availability_checker #8045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix data availability checker race condition causing partial data columns to be served over RPC #7961

Fix data availability checker race condition causing partial data columns to be served over RPC #7961

Uh oh!

jimmygchen commented Aug 28, 2025 •

edited

Loading

Uh oh!

mergify bot commented Aug 28, 2025

Uh oh!

shane-moore commented Aug 31, 2025

Uh oh!

Uh oh!

jimmygchen Sep 1, 2025

Uh oh!

jimmygchen Sep 2, 2025

Uh oh!

michaelsproul Sep 2, 2025

Uh oh!

jimmygchen Sep 2, 2025

Uh oh!

michaelsproul left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Check indices from cache again to make sure we don't publish components we've already received.
	let Some(existing_column_indices) = self.cached_data_column_indexes(block_root) else {
	return Ok(DataColumnReconstructionResult::RecoveredColumnsNotImported(
	"block already imported",
	));
	};

Fix data availability checker race condition causing partial data columns to be served over RPC #7961

Fix data availability checker race condition causing partial data columns to be served over RPC #7961

Uh oh!

Conversation

jimmygchen commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

mergify bot commented Aug 28, 2025

Uh oh!

shane-moore commented Aug 31, 2025

Uh oh!

Uh oh!

jimmygchen Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimmygchen commented Aug 28, 2025 •

edited

Loading