close
Skip to content

More robust wait for block. - #899

Merged
ZanCorDX merged 4 commits into
developfrom
dx/more-robust-wait-for-block
Mar 20, 2026
Merged

More robust wait for block.#899
ZanCorDX merged 4 commits into
developfrom
dx/more-robust-wait-for-block

Conversation

@ZanCorDX

Copy link
Copy Markdown
Collaborator

📝 Summary

Some new bug in reth (between 1.9.3 and 1.11.3) generated a rece condition that would give the last block but then when asked for it it's not there.
Now wait_for_block_header waits until the deadline no matter the errors it gets.

✅ I have completed the following steps:

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

@ZanCorDX
ZanCorDX requested a review from dvush as a code owner March 20, 2026 13:35
@claude

claude Bot commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ZanCorDX's task in 1m 19s —— View job


PR Review: More robust wait for block

  • Read changed file and diff
  • Analyze code changes
  • Post review feedback

Summary

The refactoring to extract try_to_read_block_header and retry on all errors until the deadline is a reasonable approach for the reth race condition described. The core logic change — catching errors instead of propagating them — achieves the stated goal.

Issues found

  1. Unnecessary asynctry_to_read_block_header has no .await points; all provider calls are synchronous. It should be a regular fn to avoid the overhead of an unnecessary Future. (inline comment)

  2. Typo in log message"Failed to_read_block_header" should be "Failed to read block header". (inline comment)

  3. Double logging per retry — Both the caller (line 491) and try_to_read_block_header (line 520) log at info on every "not found" iteration, producing two log lines per poll cycle. One should be removed or demoted to debug. (inline comment)

  4. Silent swallowing of provider errors — The new code retries on all errors, including potential DB/provider failures that the old code would propagate immediately. This is acceptable for the race condition fix, but consider logging these at warn rather than info so genuine provider issues are visible in production.


Comment thread crates/rbuilder/src/live_builder/mod.rs Outdated
Comment thread crates/rbuilder/src/live_builder/mod.rs Outdated
match try_to_read_block_header(block, parent_hash, payload_id, provider).await {
Ok(header) => return Ok(header),
Err(err) => {
info!(?err, "Failed to_read_block_header");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: "Failed to_read_block_header" — looks like a copy-paste from the function name. Should be "Failed to read block header".

Also, logging provider errors (e.g. DB issues) at info level may make them hard to notice in production. Consider warn here since these are errors that were previously propagated as failures.

Suggested change
info!(?err, "Failed to_read_block_header");
info!(?err, "Failed to read block header");

let current_parent_hash = provider
.header_by_number(block.checked_sub(1).unwrap_or(1))?
.map(|h| h.hash_slow());
info!(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When the header is not found (the else branch below), this logs at info level with details, and then the caller also logs the returned error at info level (line 491). This produces two log lines per retry iteration for the common "not found yet" case. Consider removing one of the two log sites or making the inner one debug to reduce noise.

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@claude

claude Bot commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ZanCorDX's task in 1m 10s —— View job


PR Review: More robust wait for block

  • Read changed file and diff
  • Analyze code changes
  • Post review feedback

Summary

Good progress from the previous review — try_to_read_block_header was correctly changed from async fn to fn. However, three issues remain:

  1. Compile error (blocking): The call site at line 488 still has .await on the now-synchronous try_to_read_block_header. Since eyre::Result<Header> doesn't implement Future, this won't compile. (inline comment)

  2. Typo (minor): "Failed to_read_block_header" at line 491 — underscore instead of space. (inline comment)

  3. Double logging (minor): Both the caller (line 491) and try_to_read_block_header (line 520-526) log at info on every "not found" iteration. (inline comment)


Comment thread crates/rbuilder/src/live_builder/mod.rs Outdated
sleep_duration = None;
} else {
sleep_duration = Some(time_to_sleep.try_into().unwrap());
match try_to_read_block_header(block, parent_hash, payload_id, provider).await {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: .await on a non-async function. try_to_read_block_header was correctly changed to a regular fn (line 505), but this call site still has .await. Since eyre::Result<Header> does not implement Future, this should be a compile error.

Suggested change
match try_to_read_block_header(block, parent_hash, payload_id, provider).await {
match try_to_read_block_header(block, parent_hash, payload_id, provider) {

Comment thread crates/rbuilder/src/live_builder/mod.rs Outdated
match try_to_read_block_header(block, parent_hash, payload_id, provider).await {
Ok(header) => return Ok(header),
Err(err) => {
info!(?err, "Failed to_read_block_header");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo still present from previous review: "Failed to_read_block_header" should be "Failed to read block header".

Suggested change
info!(?err, "Failed to_read_block_header");
info!(?err, "Failed to read block header");

?current_parent_hash,
payload_id,
"Payload parent header not found, trying again"
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double logging is still present: this info! fires on every "not found" iteration, and the caller at line 491 also logs the returned error at info. Consider removing one or demoting this inner log to debug! to reduce noise.

@claude

claude Bot commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ZanCorDX's task in 46s —— View job


PR Review: More robust wait for block

  • Read changed file and diff
  • Analyze code changes
  • Post review feedback

Summary

The previous issues (.await on non-async fn, typo) are all fixed in this push. The refactoring is clean — try_to_read_block_header is correctly a synchronous fn, the loop structure is simpler, and the retry-on-all-errors behavior matches the stated goal of working around the reth race condition.

Remaining issue

  1. Double logging per retry — still present. When the header isn't found, try_to_read_block_header logs at info with full context (lines 520-526), then returns an error, and the caller also logs that error at info (line 491). This produces two log lines per poll cycle. (inline comment)

match try_to_read_block_header(block, parent_hash, payload_id, provider) {
Ok(header) => return Ok(header),
Err(err) => {
info!(?err, "try_to_read_block_header failed");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double logging on "not found" iterations is still present. When the header isn't found, try_to_read_block_header logs at info with full context (line 520-526), then returns an error, and this line logs the error again at info. This produces two log lines per poll cycle for the common case.

Consider removing this outer log (the inner one has all the useful details), or demoting it to debug:

Suggested change
info!(?err, "try_to_read_block_header failed");
debug!(?err, "try_to_read_block_header failed");

@ZanCorDX
ZanCorDX merged commit c66f845 into develop Mar 20, 2026
10 checks passed
@ZanCorDX
ZanCorDX deleted the dx/more-robust-wait-for-block branch March 20, 2026 14:35
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