Skip to content

Finish block building process even when cancel request is found #606

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

Merged
merged 3 commits into from
May 9, 2025

Conversation

ferranbt
Copy link
Collaborator

@ferranbt ferranbt commented May 1, 2025

📝 Summary

This PR introduces three changes:

  • It refactors some of the common utilities shared among the tests into a single TestHarness struct which spins the test framework with all the components. Note that it does not update the tests to use this new utility.
  • It fixes an issue with the block builder that would stop the block building process and not return any block if a cancel request was found. This happens when an FCU and a getPayload request are called to close to each other, the getPayload cancels the block building process, and getPayload waits forever for a block that will never be built. Now, the block building finishes.
  • It adds an integration test to cover this use case with the new utility.

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@Copilot Copilot AI review requested due to automatic review settings May 1, 2025 09:48
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a bug in the block building process where a cancel request prevented the process from completing by refactoring the block builder logic. Key changes include:

  • Introducing a new constructor (new_with_port) and updating function signatures to pass EngineApi by value rather than by reference.
  • Removing the early cancellation check in the payload builder so that block building always finishes.
  • Adding a new integration test and a TestHarness utility to better simulate and validate the block building process.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/op-rbuilder/src/tester/mod.rs Added new_with_port and updated BlockGenerator to use value types; added logging and refactored submit_payload with an extra no_sleep parameter.
crates/op-rbuilder/src/tester/main.rs Updated BlockGenerator instantiation to pass EngineApi by value.
crates/op-rbuilder/src/payload_builder_vanilla.rs Removed the cancelled check from execute_best_transactions call.
crates/op-rbuilder/src/integration/mod.rs Introduced TestHarness and port allocation helper along with related refactoring.
crates/op-rbuilder/src/integration/integration_test.rs Added an integration test for the edge case with close FCU/getPayload calls.
crates/op-rbuilder/Cargo.toml Added parking_lot dependency.
Comments suppressed due to low confidence (2)

crates/op-rbuilder/src/tester/mod.rs:344

  • [nitpick] Consider refactoring the submit_payload function to reduce the parameter count; bundling the delay-related parameters into a configuration struct could improve readability and maintainability.
no_sleep: bool, // TODO: Change this, too many parameters we can tweak here to put as a function arguments

crates/op-rbuilder/src/integration/integration_test.rs:363

  • Consider adding a timeout mechanism for the submit_payload call in this integration test to ensure that the test does not hang indefinitely in case of an underlying issue.
// TODO: In the fail case scenario, this hangs forever, but it should return an error

@avalonche
Copy link
Contributor

avalonche commented May 1, 2025

the getPayload cancels the block building process, and getPayload waits forever for a block that will never be built

this would only happen if fcu causes a cancel before getPayload is resolved right? why would getPayload cause a cancel?

@ferranbt ferranbt merged commit d22c29b into develop May 9, 2025
4 checks passed
@ferranbt ferranbt deleted the fix-get-payload-wait-forever branch May 9, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants