Conversation
refcell
changed the title
This stack of pull requests is managed by Graphite. Learn more about stacking. |
refcell
added
A-proof
Graphite App
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files☔ View full report in Codecov by Sentry. |
refcell
force-pushed
the
rf/bs-flush
branch
from
061409b to
68be195
Compare
refcell
force-pushed
the
rf/sb-prefix-check
branch
from
dcb49c5 to
f40a1ec
Compare
|
|
||
| /// Returns the ending timestamp for the span batch. | ||
| pub fn end_timestamp(&self) -> u64 { | ||
| self.batches.last().map_or(0, |b| b.timestamp) |
There was a problem hiding this comment.
I think we should follow the same behavior as timestamp and panic if batches is empty. Getting 0 could cause some weird UB.
| l2_safe_head: L2BlockInfo, | ||
| block_time: u64, | ||
| fetcher: &mut BF, | ||
| ) -> bool { |
There was a problem hiding this comment.
Can we use BatchValidity? There are circumstances within these checks where we're undecided, not Drop.
| Ok(block) => block, | ||
| Err(e) => { | ||
| warn!("failed to fetch L2 block number {parent_num}: {e}"); | ||
| return false; |
There was a problem hiding this comment.
The RPC fetch failing should be temporary - should not influence batch validity
| // If the span batch L1 origin check is not part of | ||
| // the canonical L1 chain, the span batch is invalid. | ||
| let starting_epoch_num = self.starting_epoch_num(); | ||
| if starting_epoch_num > parent_block.l1_origin.number + 1 { | ||
| warn!( | ||
| "batch is for future epoch too far ahead, while it has the next timestamp, so it must be invalid. starting epoch: {} | next epoch: {}", | ||
| starting_epoch_num, | ||
| parent_block.l1_origin.number + 1 | ||
| ); | ||
| return false; | ||
| } | ||
| // Check if the batch is too old. | ||
| if starting_epoch_num < parent_block.l1_origin.number { | ||
| warn!("dropped batch, epoch is too old, minimum: {:?}", parent_block.block_info.id()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Also need to check against the origin check within the batch itself: https://github.com/anton-rs/kona/blob/main/crates/derive/src/batch/span_batch/batch.rs#L167-L188
| /// A consecutive, time-centric window of L1 Blocks. | ||
| /// Every L1 origin of unsafe L2 Blocks must be included in this list. | ||
| /// If every L2 Block corresponding to a single L1 Block becomes safe, | ||
| /// the block is popped from this list. | ||
| /// If new L2 Block's L1 origin is not included in this list, fetch and | ||
| /// push it to the list. | ||
| l1_blocks: Vec<BlockInfo>, |
There was a problem hiding this comment.
I think we can re-use the l1_blocks in the BatchQueue rather than doubling it up? The batch queue already holds onto these - would it work to extend the BatchQueueProvider trait's next_batch signature to include the l1_blocks?
They get prepared directly before the call to next_batch, think that would work fine: https://github.com/anton-rs/kona/blob/main/crates/derive/src/stages/batch_queue.rs#L306-L339
2 participants
Description
Channel flushing on invalid span batch in the
BatchStream.Adds validation checks to the
SpanBatchfor holocene.