Added Block Status Table #8134
Added Block Status Table #8134
Conversation
ethDreamer
force-pushed
the
block-status-table
branch
from
30f8ec1 to
0db9d67
Compare
|
Basic FunctionalityWhenever a component of a block is seen from the network: if block_status_table.insert_pending(component.block_hash) {
// block inserted with status PENDING
}
match da_cache.insert(component) {
Availability::Available(block) => {
if block_status_table.compare_and_swap_status(
block.hash,
BlockStatus::Pending,
BlockStatus::Importing, // post gloas it will be BlockStatus::Executing
) {
// proceed with importing the block into fork-choice
}
else {
// someone beat you to it; you're done
}
}
Availability::MissingComponents(..) => // you're done
}any thread that needs to know the status of a block can subscribe for events: let Some((status, mut rx)) = block_status_table.subscribe(block_hash) else {
// block doesn't exist
}
if status.is_terminal() {
// evaluate what to do based on status
}
while let Ok(status) = rx.recv().await {
if status.is_terminal() {
// decide what to do based on status
}
} |
| /// The underlying concurrent hash map storing block status as AtomicU8 | ||
| table: DashMap<Hash256, AtomicU8>, | ||
| /// Broadcast channels for notifying subscribers of status changes | ||
| notifiers: DashMap<Hash256, broadcast::Sender<BlockStatus>>, | ||
| /// Counter for tracking the number of entries (for metrics/debugging) | ||
| entry_count: AtomicUsize, |
There was a problem hiding this comment.
Should be the initial design be the simplest approach possible?
pub enum BlockStatus {
Pending,
Executing(broadcast::Sender<BlockStatus>),
Invalid,
Importing(broadcast::Sender<BlockStatus>),
Imported.
}
pub struct BlockStatusTable {
table: DashMap<Hash256, BlockStatus>,
}We can optimize later with more dashmaps if we happen to observe and measure performance degradation due to lock contention
There was a problem hiding this comment.
i have changed the design to use a single dashmap and I think the code is both simpler and more efficient - let me know what you think! :D
| return Err(BlockError::BlobNotRequired(blob.slot())); | ||
| } | ||
|
|
||
| // TODO: there appears to be no check on the parent block being in fork choice here.. |
There was a problem hiding this comment.
It's checked here
| // TODO: if somehow this blob is older than finalization, we need to not insert it - where is this checked? | ||
| let status = self | ||
| .block_status_table | ||
| .get_status_or_insert_pending(block_root, slot) |
| ) { | ||
| Ok(_) => { | ||
| let result = self.import_available_block(block).await; | ||
| if let Ok(AvailabilityProcessingStatus::Imported(_)) = result.as_ref() { |
There was a problem hiding this comment.
This is equivalent to a match with a default statement, which have been very problematic in the past as we modify the variants. Can you do a full match on result?
| BlockStatusTableError::InvalidStatusTransition { .. } => { | ||
| Err(BlockError::DuplicateFullyImported(block_root)) | ||
| } |
There was a problem hiding this comment.
Not true, this error will trigger if the current status is != Pending. If that status happens to be Invalid you will return DuplicateFullyImported which is incorrect
| Err(BlockError::DuplicateFullyImported(block_root)) | ||
| } | ||
| // other errors should not occur | ||
| e => Err(BlockError::InternalError(format!( |
There was a problem hiding this comment.
Avoid default match statement, just match explicitly the variants BlockNotFound / InvalidStatus. Are you sure BlockNotFound can never happen?
ethDreamer
force-pushed
the
block-status-table
branch
from
17628d2 to
012f0a5
Compare
| /// Block (or blob) has been seen | ||
| Seen = 0, | ||
| /// Someone is running process_block() on the block | ||
| Processing = 1, |
There was a problem hiding this comment.
As part of process_block, the block can be executing and importing, are these statuses overlapping?
3 participants
Issue Addressed
This is useful for managing lock contention and will be really useful for gloas.