mirror of
https://github.com/sigp/lighthouse.git
synced 2026-06-30 03:14:25 +00:00
Fix and improve handling of empty columns after getBlobs response (#9361)
This PR fixes two issues: 1. This condition is inverted:dfb259171a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs (L1507-L1508)We are supposed to filter out incomplete columns when we DON'T have local blobs yet! 2. When the EL returns no blobs, we never store a partial in the assembler, and this code fails to publish our need to the network, as no partials are returned:dfb259171a/beacon_node/network/src/network_beacon_processor/mod.rs (L1038-L1050)The simple fix for 1 would be to invert the condition, but we can improve the flow here: Instead of not publishing anything, we can publish what we got, but not request anything. This ties into the fix for 2: After get blobs completes, we not only publish anything in the partial assembler, but also for every missing custody column in there, publish an empty column and a request for all cells. In particular: - When sending a partial message to `network`, allow specifying a request bitmap instead of hardcoding an all-ones bitmap. - For clarity and to prepare for Gloas integration, add a `PubsubPartialMessage` enum with a `DataColumnFulu` variant. - On republishing after merging a gossip column: always publish, but only request cells if local blobs are known or get blobs is disabled. This also prepares us to request only *some* cells, e.g. in cases where we are aware of the blobs that the EL is going to send us, e.g. via `engine_hasBlobs`. - Move guards in `fetch_engine_blobs_and_publish` to ensure everything works fine if there are no blobs or if get_blobs is disabled. Co-Authored-By: Daniel Knopik <daniel@dknopik.de>
This commit is contained in:
@@ -908,6 +908,7 @@ where
|
||||
let shuffling_cache_size = self.chain_config.shuffling_cache_size;
|
||||
let complete_blob_backfill = self.chain_config.complete_blob_backfill;
|
||||
let enable_partial_columns = self.chain_config.enable_partial_columns;
|
||||
let disable_get_blobs = self.chain_config.disable_get_blobs;
|
||||
|
||||
// Calculate the weak subjectivity point in which to backfill blocks to.
|
||||
let genesis_backfill_slot = if self.chain_config.genesis_backfill {
|
||||
@@ -1043,6 +1044,7 @@ where
|
||||
custody_context.clone(),
|
||||
self.spec.clone(),
|
||||
enable_partial_columns,
|
||||
disable_get_blobs,
|
||||
)
|
||||
.map_err(|e| format!("Error initializing DataAvailabilityChecker: {:?}", e))?,
|
||||
),
|
||||
|
||||
@@ -121,6 +121,7 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> {
|
||||
custody_context: Arc<CustodyContext<T::EthSpec>>,
|
||||
spec: Arc<ChainSpec>,
|
||||
enable_partial_columns: bool,
|
||||
disable_get_blobs: bool,
|
||||
) -> Result<Self, AvailabilityCheckError> {
|
||||
let inner = DataAvailabilityCheckerInner::new(
|
||||
OVERFLOW_LRU_CAPACITY,
|
||||
@@ -130,6 +131,7 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> {
|
||||
let partial_assembler = if enable_partial_columns {
|
||||
Some(Arc::new(PartialDataColumnAssembler::new(
|
||||
OVERFLOW_LRU_CAPACITY,
|
||||
disable_get_blobs,
|
||||
)))
|
||||
} else {
|
||||
None
|
||||
@@ -1432,6 +1434,7 @@ mod test {
|
||||
custody_context,
|
||||
spec,
|
||||
true,
|
||||
false,
|
||||
)
|
||||
.expect("should initialise data availability checker")
|
||||
}
|
||||
|
||||
@@ -344,7 +344,7 @@ fn mock_beacon_adapter(fork_name: ForkName, get_blobs_v3: bool) -> MockFetchBlob
|
||||
let test_runtime = TestRuntime::default();
|
||||
let spec = Arc::new(fork_name.make_genesis_spec(E::default_spec()));
|
||||
let kzg = get_kzg(&spec);
|
||||
let partial_assembler = PartialDataColumnAssembler::new(32);
|
||||
let partial_assembler = PartialDataColumnAssembler::new(32, false);
|
||||
|
||||
let mut mock_adapter = MockFetchBlobsBeaconAdapter::default();
|
||||
mock_adapter.expect_spec().return_const(spec.clone());
|
||||
|
||||
@@ -13,6 +13,9 @@ use types::data::{ColumnIndex, PartialDataColumnHeader};
|
||||
pub struct PartialDataColumnAssembler<E: EthSpec> {
|
||||
/// Cache of assemblies keyed by block root
|
||||
assemblies: RwLock<LruCache<Hash256, PartialAssembly<E>>>,
|
||||
/// Whether getBlobs is disabled. If so, always set `has_local_blobs` to true, as we will never
|
||||
/// retrieve blobs from the EL and therefore should immediately request cells from the network.
|
||||
disable_get_blobs: bool,
|
||||
}
|
||||
|
||||
/// Tracks partial columns being assembled for a single block
|
||||
@@ -43,9 +46,10 @@ pub struct PartialMergeResult<E: EthSpec> {
|
||||
}
|
||||
|
||||
impl<E: EthSpec> PartialDataColumnAssembler<E> {
|
||||
pub fn new(capacity: usize) -> Self {
|
||||
pub fn new(capacity: usize, disable_get_blobs: bool) -> Self {
|
||||
Self {
|
||||
assemblies: RwLock::new(LruCache::new(capacity)),
|
||||
disable_get_blobs,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -60,7 +64,7 @@ impl<E: EthSpec> PartialDataColumnAssembler<E> {
|
||||
|
||||
let assembly = PartialAssembly {
|
||||
header,
|
||||
has_local_blobs: false,
|
||||
has_local_blobs: self.disable_get_blobs,
|
||||
columns: HashMap::new(),
|
||||
};
|
||||
|
||||
@@ -82,7 +86,7 @@ impl<E: EthSpec> PartialDataColumnAssembler<E> {
|
||||
.entry(block_root)
|
||||
.or_insert_with(|| PartialAssembly {
|
||||
header: header.clone(),
|
||||
has_local_blobs: false,
|
||||
has_local_blobs: self.disable_get_blobs,
|
||||
columns: HashMap::new(),
|
||||
});
|
||||
|
||||
@@ -174,7 +178,7 @@ impl<E: EthSpec> PartialDataColumnAssembler<E> {
|
||||
signed_block_header: fulu.signed_block_header.clone(),
|
||||
kzg_commitments_inclusion_proof: fulu.kzg_commitments_inclusion_proof.clone(),
|
||||
}),
|
||||
has_local_blobs: false,
|
||||
has_local_blobs: self.disable_get_blobs,
|
||||
columns: Default::default(),
|
||||
});
|
||||
let prev = assembly
|
||||
@@ -367,7 +371,7 @@ mod tests {
|
||||
}
|
||||
|
||||
fn make_assembler() -> PartialDataColumnAssembler<E> {
|
||||
PartialDataColumnAssembler::new(16)
|
||||
PartialDataColumnAssembler::new(16, false)
|
||||
}
|
||||
|
||||
// -- init and get_header tests --
|
||||
|
||||
@@ -246,6 +246,7 @@ pub fn test_da_checker<E: EthSpec>(
|
||||
custody_context,
|
||||
spec,
|
||||
true,
|
||||
false,
|
||||
)
|
||||
.expect("should initialise data availability checker")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user