Keep PendingComponents in da_checker during import_block

[dapplion]

Issue Addressed

Extends

• Ensure lookup sync checks caches correctly #5840

Sync lookups relay on having an accurate picture of block and blobs processing status to perform well (not re-download, not get stuck)

Sync lookup makes the following assumption:

• if block in da_checker = block is known and valid
• else if block in processing_cache = block is known but not valid (yet)

These assumptions are incorrect. During block import, a block is no longer in the da_checker, but it's still in the processing_cache. Because we remove the block from the da_checker when the da_checker imports the last component of the block.

The current sequence of events is

- process_block_with_early_caching
  - ..                                            <<< insert into reqresp_pre_import_cache
  - process_block
    - check_block_availability_and_import
      - put_pending_executed_block
        - ..                                      <<< insert into da_checker
        - (if available) 
          - ..                                    >>> remove from da_checker
      - process_availability
        - import_available_block
          - import_block
            - fork_choice.on_block
  - ..                                            >>> remove from reqresp_pre_import_cache

We should delay removal from the da_checker until after completing import_block

Proposed Changes

Delay removal from the da_checker until after completing import_block. New sequence:

- process_block_with_early_caching
  - ..                                            <<< insert into reqresp_pre_import_cache
  - process_block
    - check_block_availability_and_import
      - put_pending_executed_block
        - ..                                      <<< insert into da_checker
        - (if available) 
      - process_availability
        - import_available_block
          - import_block
            - fork_choice.on_block
          - ..                                    >>> remove from da_checker
  - ..                                            >>> remove from reqresp_pre_import_cache

Ideally we should remove from da_checker after removing from reqresp_pre_import_cache. However, import_block is called from many places (see call graph below), so it would make the logic more convoluted.

import_block
- import_available_block
  - process_block
    - process_chain_segment
    - process_block_with_early_caching
  - process_availability
    - check_block_availability_and_import
      - process_block
    - check_gossip_blob_availability_and_import
    - check_rpc_blob_availability_and_import

[jimmygchen]

Suggested change

	/// Removes and returns the pending_components corresponding to
	/// the `block_root` or `None` if it does not exist
	/// Removes the `pending_components` corresponding to the `block_root`.

[dapplion]

@ethDreamer plz need help with the tests, quite lost on why they are broken. overflow_cache_test_maintenance appears to have random behaviour, with the cache having different sizes on each run and breaking the assertion cache disk should have the rest

[jimmygchen]

We're cloning pending components for a second time here and I'm not sure if it's necessary.

If the pending component is newly created in this function, it won't immediately become available. So this means the pending components would always be in the overflow_store if it becomes available here right?

[jimmygchen]

Oh never mind, we do need this as we're mutating a clone!

[ethDreamer]

You don't need to modify pop_pending_components to get_pending_components. Instead you should keep it as is. Because the write lock is held the whole time, there is no difference between:

lock = get_write_lock()
item = cache.remove()
item.mutate()
cache.insert(item)
drop(lock)

and

lock = get_write_lock()
item = cache.get()
item.mutate()
cache.update(item)
drop(lock)

You can just re-insert the item even when the components are complete (just like you did above) and then remove them with the same pop_pending_components method.

[jimmygchen]

I think there might be an rare edge case scenario here where:

1. we load the PendingComponents from memory
2. we update it and try to put it back, however in_memory is at capacity and it goes to the disk.
3. next time we want to update it, it loads from memory which would give us the outdated component

In this case I think it would just delay availability as we would do another lookup right? I think we might be able to avoid this altogether if we check if the components is already in memory before we attempt to use the overflow store?

Another one that's probably less of an issue:

1. We load the PendingComponents, not found in memory, so it returned from store
2. We update it and put it back to memory, now we have two versions one in memory and one on disk
3. Now if the in memory one somehow gets pruned first, then we end up loading an old component from disk.

I think this is unlikely under normal network condition as the LRU cache's default capacity is 1024.

[jimmygchen]

overflow_cache_test_maintenance appears to have random behaviour, with the cache having different sizes on each run and breaking the assertion cache disk should have the rest

I think this may be due to the randomness in the mock execution layer:

lighthouse/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs

Lines 672 to 674 in 000a4fd

	// get random number between 0 and Max Blobs
	let mut rng = self.rng.lock();
	let num_blobs = rng.gen::<usize>() % (E::max_blobs_per_block() + 1);

Not sure if there's any reason to use random number of blobs here - perhaps we can make this deterministic while covering all blob counts per block?

[ethDreamer]

Working on a PR to update the tests for the new logic. Will release shortly..

[ethDreamer]

okay got the tests fixed:
dapplion#34

[realbigsean]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at cb32807