Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(tree): remove BlockAttachment usage #10453

Merged
merged 1 commit into from
Aug 22, 2024
Merged

chore(tree): remove BlockAttachment usage #10453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions crates/blockchain-tree-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,36 @@ impl CanonicalOutcome {
}
}

/// Block inclusion can be valid, accepted, or invalid. Invalid blocks are returned as an error
/// variant.
///
/// If we don't know the block's parent, we return `Disconnected`, as we can't claim that the block
/// is valid or not.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum BlockStatus2 {
/// The block is valid and block extends canonical chain.
Valid,
/// The block may be valid and has an unknown missing ancestor.
Disconnected {
/// Current canonical head.
head: BlockNumHash,
/// The lowest ancestor block that is not connected to the canonical chain.
missing_ancestor: BlockNumHash,
},
}

/// How a payload was inserted if it was valid.
///
/// If the payload was valid, but has already been seen, [`InsertPayloadOk2::AlreadySeen(_)`] is
/// returned, otherwise [`InsertPayloadOk2::Inserted(_)`] is returned.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum InsertPayloadOk2 {
/// The payload was valid, but we have already seen it.
AlreadySeen(BlockStatus2),
/// The payload was valid and inserted into the tree.
Inserted(BlockStatus2),
}

/// From Engine API spec, block inclusion can be valid, accepted or invalid.
/// Invalid case is already covered by error, but we need to make distinction
/// between valid blocks that extend canonical chain and the ones that fork off
Expand Down
46 changes: 20 additions & 26 deletions crates/engine/tree/src/tree/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ use reth_beacon_consensus::{
};
use reth_blockchain_tree::{
error::{InsertBlockErrorKindTwo, InsertBlockErrorTwo, InsertBlockFatalError},
BlockAttachment, BlockBuffer, BlockStatus,
BlockBuffer, BlockStatus2, InsertPayloadOk2,
};
use reth_blockchain_tree_api::InsertPayloadOk;
use reth_chain_state::{
CanonicalInMemoryState, ExecutedBlock, MemoryOverlayStateProvider, NewCanonicalChain,
};
Expand Down Expand Up @@ -665,17 +664,17 @@ where
match self.insert_block_without_senders(block) {
Ok(status) => {
let status = match status {
InsertPayloadOk::Inserted(BlockStatus::Valid(_)) => {
InsertPayloadOk2::Inserted(BlockStatus2::Valid) => {
latest_valid_hash = Some(block_hash);
self.try_connect_buffered_blocks(num_hash);
PayloadStatusEnum::Valid
}
InsertPayloadOk::AlreadySeen(BlockStatus::Valid(_)) => {
InsertPayloadOk2::AlreadySeen(BlockStatus2::Valid) => {
latest_valid_hash = Some(block_hash);
PayloadStatusEnum::Valid
}
InsertPayloadOk::Inserted(BlockStatus::Disconnected { .. }) |
InsertPayloadOk::AlreadySeen(BlockStatus::Disconnected { .. }) => {
InsertPayloadOk2::Inserted(BlockStatus2::Disconnected { .. }) |
InsertPayloadOk2::AlreadySeen(BlockStatus2::Disconnected { .. }) => {
// not known to be invalid, but we don't know anything else
PayloadStatusEnum::Syncing
}
Expand Down Expand Up @@ -1404,12 +1403,7 @@ where
Ok(res) => {
debug!(target: "engine", child =?child_num_hash, ?res, "connected buffered block");
if self.is_sync_target_head(child_num_hash.hash) &&
matches!(
res,
InsertPayloadOk::Inserted(BlockStatus::Valid(
BlockAttachment::Canonical
))
)
matches!(res, InsertPayloadOk2::Inserted(BlockStatus2::Valid))
{
self.make_canonical(child_num_hash.hash);
}
Expand Down Expand Up @@ -1640,7 +1634,7 @@ where

// try to append the block
match self.insert_block(block) {
Ok(InsertPayloadOk::Inserted(BlockStatus::Valid(_))) => {
Ok(InsertPayloadOk2::Inserted(BlockStatus2::Valid)) => {
if self.is_sync_target_head(block_num_hash.hash) {
trace!(target: "engine", "appended downloaded sync target block");
// we just inserted the current sync target block, we can try to make it
Expand All @@ -1652,12 +1646,15 @@ where
trace!(target: "engine", "appended downloaded block");
self.try_connect_buffered_blocks(block_num_hash);
}
Ok(InsertPayloadOk::Inserted(BlockStatus::Disconnected { head, missing_ancestor })) => {
Ok(InsertPayloadOk2::Inserted(BlockStatus2::Disconnected {
head,
missing_ancestor,
})) => {
// block is not connected to the canonical head, we need to download
// its missing branch first
return self.on_disconnected_downloaded_block(block_num_hash, missing_ancestor, head)
}
Ok(InsertPayloadOk::AlreadySeen(_)) => {
Ok(InsertPayloadOk2::AlreadySeen(_)) => {
trace!(target: "engine", "downloaded block already executed");
}
Err(err) => {
Expand All @@ -1673,7 +1670,7 @@ where
fn insert_block_without_senders(
&mut self,
block: SealedBlock,
) -> Result<InsertPayloadOk, InsertBlockErrorTwo> {
) -> Result<InsertPayloadOk2, InsertBlockErrorTwo> {
match block.try_seal_with_senders() {
Ok(block) => self.insert_block(block),
Err(block) => Err(InsertBlockErrorTwo::sender_recovery_error(block)),
Expand All @@ -1683,18 +1680,17 @@ where
fn insert_block(
&mut self,
block: SealedBlockWithSenders,
) -> Result<InsertPayloadOk, InsertBlockErrorTwo> {
) -> Result<InsertPayloadOk2, InsertBlockErrorTwo> {
self.insert_block_inner(block.clone())
.map_err(|kind| InsertBlockErrorTwo::new(block.block, kind))
}

fn insert_block_inner(
&mut self,
block: SealedBlockWithSenders,
) -> Result<InsertPayloadOk, InsertBlockErrorKindTwo> {
) -> Result<InsertPayloadOk2, InsertBlockErrorKindTwo> {
if self.block_by_hash(block.hash())?.is_some() {
let attachment = BlockAttachment::Canonical; // TODO: remove or revise attachment
return Ok(InsertPayloadOk::AlreadySeen(BlockStatus::Valid(attachment)))
return Ok(InsertPayloadOk2::AlreadySeen(BlockStatus2::Valid))
}

let start = Instant::now();
Expand All @@ -1714,7 +1710,7 @@ where

self.state.buffer.insert_block(block);

return Ok(InsertPayloadOk::Inserted(BlockStatus::Disconnected {
return Ok(InsertPayloadOk2::Inserted(BlockStatus2::Disconnected {
head: self.state.tree_state.current_canonical_head,
missing_ancestor,
}))
Expand Down Expand Up @@ -1791,9 +1787,7 @@ where
};
self.emit_event(EngineApiEvent::BeaconConsensus(engine_event));

let attachment = BlockAttachment::Canonical; // TODO: remove or revise attachment

Ok(InsertPayloadOk::Inserted(BlockStatus::Valid(attachment)))
Ok(InsertPayloadOk2::Inserted(BlockStatus2::Valid))
}

/// Handles an error that occurred while inserting a block.
Expand Down Expand Up @@ -2176,7 +2170,7 @@ mod tests {
fn insert_block(
&mut self,
block: SealedBlockWithSenders,
) -> Result<InsertPayloadOk, InsertBlockErrorTwo> {
) -> Result<InsertPayloadOk2, InsertBlockErrorTwo> {
let execution_outcome = self.block_builder.get_execution_outcome(block.clone());
self.extend_execution_outcome([execution_outcome]);
self.tree.provider.add_state_root(block.state_root);
Expand Down Expand Up @@ -2524,7 +2518,7 @@ mod tests {
let outcome = test_harness.tree.insert_block_without_senders(sealed.clone()).unwrap();
assert_eq!(
outcome,
InsertPayloadOk::Inserted(BlockStatus::Disconnected {
InsertPayloadOk2::Inserted(BlockStatus2::Disconnected {
head: test_harness.tree.state.tree_state.current_canonical_head,
missing_ancestor: sealed.parent_num_hash()
})
Expand Down
Loading