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.

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

Avoid cloning block in EngineApiTreeHandler::insert_block #13698

Open
nekomoto911 opened this issue Jan 7, 2025 · 2 comments
Open

Avoid cloning block in EngineApiTreeHandler::insert_block #13698

nekomoto911 opened this issue Jan 7, 2025 · 2 comments
Assignees
Labels
C-debt Refactor of code section that is hard to understand or maintain C-enhancement New feature or request

Comments

@nekomoto911
Copy link

Describe the feature

In EngineApiTreeHandler::insert_block, it is desirable to include block in the error and return it when EngineApiTreeHandler::insert_block_inner is unsuccessful. However, insert_block_inner needs to take ownership of block, so a clone of block is made. Cloning SealedBlockWithSenders involves deep copying of transactions, receipts, etc., which is a costly operation. Since insert_block_inner typically returns OK, always performing a costly operation for a rare exceptional case is not considered best practice.

EngineApiTreeHandler::insert_block_inner appears to be called only by EngineApiTreeHandler::insert_block. So could we let insert_block_inner directly return InsertBlockErrorTwo in case of an exception to avoid cloning the block?

If you find this suggestion reasonable, please assign this issue to me. I am very expected to contribute to the project!

Additional context

No response

@nekomoto911 nekomoto911 added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Jan 7, 2025
@nekomoto911
Copy link
Author

fn insert_block(
&mut self,
block: SealedBlockWithSenders<N::Block>,
) -> Result<InsertPayloadOk2, InsertBlockErrorTwo<N::Block>> {
self.insert_block_inner(block.clone())
.map_err(|kind| InsertBlockErrorTwo::new(block.block, kind))
}

@mattsse
Copy link
Collaborator

mattsse commented Jan 7, 2025

yeah this has been on my backlog for a while,

this def needs improvement, and should be fixed by simplifying the error handling here, either by stripping the block context from InsertBlockErrorTwo or removing the map_err

@mattsse mattsse added C-debt Refactor of code section that is hard to understand or maintain and removed S-needs-triage This issue needs to be labelled labels Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain C-enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

2 participants