Rollup node manager

[frisitano]

No description provided.

[frisitano]

Added comments inline.

[frisitano]

Currently all our creates are being skipped.

[frisitano]

before merge we should merge feat/add-deref-blanket-engine-api into scroll and then revert back to default on these dependency definitions.

[frisitano]

Currently blocks 2 - 5 can not be read as this dump does not encode the data using serde. It is instead sourced from the rpc response using cast. We should replace this with serde data such that we can deserialize in testing.

[frisitano]

Do we need this?

[greged93]

Looks good, I think the refactor of the Networking make the code flow clearer! Just couple of nits and questions

[greged93]

instead of custom errors you mean? I don't have a strong opinion on this, if it simplifies the code let's use eyre everywhere

[greged93]

just a remark on the engine, otherwise lgtm!

[greged93]

should we do fcs.safe_block_hash = unsafe_block_info.hash; before issuing the forkchoice?

[frisitano]

The logic below defines what is currently implemented to manage fcs. We get the current fcs from the node manager and then set the head block hash to the block hash of the unsafe block received from the network when defining the future for importing the block. Do you think we should be setting the safe block hash when we receive a new block over p2p?

rollup-node/crates/node/src/lib.rs

Lines 145 to 178 in 04a5f44

	// Send the block to the engine to validate the correctness of the block.
	let mut fcs = self.get_alloy_fcs();
	let engine = self.engine.clone();
	let future = Box::pin(async move {
	trace!(target: "scroll::node::manager", "handling block import future for block {:?}", block.hash_slow());
	// convert the block to an execution payload and update the forkchoice state
	let execution_payload: ExecutionPayload =
	ExecutionPayloadV1::from_block_slow(&block).into();
	let unsafe_block_info: BlockInfo = (&execution_payload).into();
	fcs.head_block_hash = unsafe_block_info.hash;
	// process the execution payload
	// TODO: needs testing
	let (unsafe_block_info, import_outcome) =
	match engine.handle_execution_payload(execution_payload, fcs).await {
	Err(EngineDriverError::InvalidExecutionPayload) => (
	Some(unsafe_block_info),
	Some(Err(BlockImportError::Validation(BlockValidationError::InvalidBlock))),
	),
	Ok((PayloadStatusEnum::Valid, PayloadStatusEnum::Valid)) => (
	Some(unsafe_block_info),
	Some(Ok(BlockValidation::ValidBlock {
	new_block: NewBlock {
	block,
	signature: signature.serialize_compact().to_vec().into(),
	},
	})),
	),
	_ => (None, None),
	};
	(unsafe_block_info, import_outcome.map(|result| BlockImportOutcome { peer, result }))
	});

Upon block import result if success we update the fcs of the node manager.

rollup-node/crates/node/src/lib.rs

Lines 197 to 214 in 04a5f44

	/// Handles a block import outcome.
	fn handle_block_import_outcome(
	&mut self,
	unsafe_block_info: Option<BlockInfo>,
	outcome: Option<BlockImportOutcome>,
	) {
	trace!(target: "scroll::node::manager", ?outcome, "handling block import outcome");
	// If we have an unsafe block info, update the forkchoice state.
	if let Some(unsafe_block_info) = unsafe_block_info {
	self.forkchoice_state.update_unsafe_block_info(unsafe_block_info);
	}
	// If we have an outcome, send it to the network manager.
	if let Some(outcome) = outcome {
	self.network.handle().block_import_outcome(outcome);
	}
	}

This works ok but I'm thinking we should introduce a State type that can be shared across async tasks (type State = Arc<Mutex<InnerState>>