crates/chain/tests/block_production/unstake_refund.rs (1)

1022-1022: heavy3_ prefix is still unexplained — prior review concern unresolved.

The numeric suffix introduced by renaming heavy_unpledge_and_unstake_concurrent_success_flow → heavy3_unpledge_and_unstake_concurrent_success_flow is still undocumented. All other tests in this file continue to use plain heavy_ prefixes. If heavy3_ denotes a diagnostic/timeout tier associated with #[diag_slow], a brief doc-comment above the function would clarify the convention for future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unstake_refund.rs` at line 1022, The test
function heavy3_unpledge_and_unstake_concurrent_success_flow has an unexplained
numeric prefix "heavy3_"—add a concise doc-comment immediately above the
function that documents the naming convention (e.g., that "heavy"/"heavyN"
denotes test weight/diagnostic/timeout tier and that "3" maps to the
#[diag_slow] or specific timeout/diagnostic level used) so future maintainers
understand the suffix; reference the function name
heavy3_unpledge_and_unstake_concurrent_success_flow and, if relevant, mention
the association with #[diag_slow] and any expected timeout/behavior in that
single-line doc-comment.

crates/chain/tests/multi_node/mempool_tests.rs (1)

1924-1925: ⚠️ Potential issue | 🟡 Minor

slow_heavy3_test_evm_gossip still uses #[tokio::test] despite setting RUST_LOG and calling initialize_tracing().

This is unchanged from the previously flagged concern — the attribute should be #[test_log::test(tokio::test)].

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/multi_node/mempool_tests.rs` around lines 1924 - 1925, The
test function slow_heavy3_test_evm_gossip is still annotated with #[tokio::test]
even though you set RUST_LOG and call initialize_tracing(); replace the
attribute with #[test_log::test(tokio::test)] on the slow_heavy3_test_evm_gossip
function so the test harness initializes logging/tracing properly when running
under Tokio.

crates/actors/src/services.rs (1)

228-237: 🧹 Nitpick | 🔵 Trivial

() return type still collapses three distinct exit causes.

All three exit paths — idle gap elapsed, deadline exceeded, channel closed — return (), making it impossible for call sites to distinguish a healthy idle from an unexpected channel close or a deadline overrun. This was raised in a previous review on the predecessor function.

♻️ Proposed fix — discriminated return type

+#[derive(Debug, PartialEq, Eq)]
+pub enum BroadcastIdleResult {
+    /// No events arrived for the full `idle` window.
+    Idle,
+    /// The outer deadline was exceeded before the idle window could elapse.
+    Deadline,
+    /// The channel was closed.
+    Closed,
+}

 pub async fn wait_until_broadcast_idle<T: Clone>(
     rx: &mut tokio::sync::broadcast::Receiver<T>,
     idle: std::time::Duration,
     deadline: tokio::time::Instant,
-) {
+) -> BroadcastIdleResult {
     loop {
         match tokio::time::timeout_at(deadline, tokio::time::timeout(idle, rx.recv())).await {
             Ok(Ok(Ok(_))) => continue,
             Ok(Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(_)))) => continue,
-            Ok(Ok(Err(tokio::sync::broadcast::error::RecvError::Closed))) => break,
-            Ok(Err(_)) => break,
-            Err(_) => break,
+            Ok(Ok(Err(tokio::sync::broadcast::error::RecvError::Closed))) => {
+                return BroadcastIdleResult::Closed;
+            }
+            Ok(Err(_)) => return BroadcastIdleResult::Idle,
+            Err(_) => return BroadcastIdleResult::Deadline,
         }
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/actors/src/services.rs` around lines 228 - 237, The loop handling
tokio::time::timeout_at(deadline, tokio::time::timeout(idle, rx.recv()))
currently collapses three distinct exit causes into unit () — change the
function to return a discriminated enum (e.g., Idle, DeadlineExceeded,
ChannelClosed or similar) instead of () and have each match arm return the
appropriate variant (map Ok(Ok(Ok(_))) => Idle, Err(_) from timeout_at =>
DeadlineExceeded, Ok(Ok(Err(Closed))) => ChannelClosed, and preserve Lagged as
Idle/continue or a distinct variant if desired); update callers to handle the
new enum. Ensure to define the enum near the function and update match arms that
now use tokio::time::timeout_at, tokio::time::timeout, rx.recv(), and other
referenced symbols so exit causes are distinguishable.

crates/vdf/src/vdf.rs (1)

84-86: ⚠️ Potential issue | 🟠 Major

Cancellation can still be starved during fast-forward drain.

The while let Ok(...) loop at line 90 drains all pending fast-forward messages without re-checking shutdown_token.is_cancelled(). Under sustained fast-forward traffic, shutdown could be delayed indefinitely.

🛠️ Proposed fix

-    loop {
+    'vdf_loop: loop {
         if shutdown_token.is_cancelled() {
             tracing::info!("VDF loop: shutdown token cancelled, exiting");
             break;
         }

         // check for VDF fast forward step
         while let Ok(traced_ff_step) = fast_forward_receiver.try_recv() {
+            if shutdown_token.is_cancelled() {
+                tracing::info!("VDF loop: shutdown token cancelled during fast-forward drain, exiting");
+                break 'vdf_loop;
+            }
             let (proposed_ff_step, _entered) = traced_ff_step.into_inner();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vdf/src/vdf.rs` around lines 84 - 86, The fast-forward drain loop (the
while let Ok(...) loop in vdf.rs that consumes fast-forward messages) can starve
shutdown because it does not re-check shutdown_token.is_cancelled() while
draining; modify the loop so it checks shutdown_token.is_cancelled() between
iterations (or incorporate the token into the loop condition) and break
immediately if cancelled, ensuring the VDF loop exits promptly even under
sustained fast-forward traffic.

crates/chain/tests/block_production/unpledge_refund.rs (2)

697-709: ⚠️ Potential issue | 🟡 Minor

Inconsistent error propagation: .await? vs .await.expect("…").

Line 702 uses .await? while other equivalent wait_for_block_at_height sites (lines 88, 212, 799) use .await.expect("…") with descriptive messages. The .expect form provides better CI diagnostics.

♻️ Proposed fix

     genesis_node
         .wait_for_block_at_height(inclusion_block.height, seconds_to_wait)
-        .await?;
+        .await
+        .expect("genesis should sync peer-mined inclusion block");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unpledge_refund.rs` around lines 697 -
709, Replace the fallible `.await?` calls with `.await.expect("...")` to match
the rest of the test and improve CI diagnostics: after calling
`peer_node.mine_block().await?` use
`genesis_node.wait_for_block_at_height(...).await.expect("timed out waiting for
genesis to receive mined block via gossip")` and likewise change the
`genesis_node.wait_for_reth_marker(...).await?` to `.await.expect("timed out
waiting for genesis Reth marker for mined block")`; locate these changes around
the calls to `peer_node.mine_block`, `genesis_node.wait_for_block_at_height`,
and `genesis_node.wait_for_reth_marker` and use descriptive messages consistent
with other occurrences.

---

85-89: ⚠️ Potential issue | 🟡 Minor

Misleading variable name: inclusion_block_peer is mined by genesis, not the peer.

genesis_node.mine_block() is called on line 85, but the variable name and .expect message suggest the block comes from a peer. This was flagged in a previous review and remains unaddressed.

🔤 Proposed rename

-    let inclusion_block_peer = genesis_node.mine_block().await?;
+    let inclusion_block_mined = genesis_node.mine_block().await?;
     genesis_node
-        .wait_for_block_at_height(inclusion_block_peer.height, seconds_to_wait)
+        .wait_for_block_at_height(inclusion_block_mined.height, seconds_to_wait)
         .await
-        .expect("genesis should sync peer-mined inclusion block");
+        .expect("genesis should finish processing inclusion block");
     let inclusion_block = genesis_node
-        .get_block_by_height(inclusion_block_peer.height)
+        .get_block_by_height(inclusion_block_mined.height)
         .await?;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unpledge_refund.rs` around lines 85 - 89,
The variable name inclusion_block_peer is misleading because the block is
produced by genesis_node.mine_block(); rename it to something like
inclusion_block or genesis_mined_block and update the assertion message in the
wait_for_block_at_height().expect(...) call (currently "genesis should sync
peer-mined inclusion block") to reflect that the block was mined by genesis
(e.g., "genesis should sync its mined inclusion block"); change references to
inclusion_block_peer to the new name and ensure tests continue to compile and
run.

crates/actors/src/block_tree_service.rs (1)

183-190: ⚠️ Potential issue | 🟠 Major

Drain loop still aborts early for non-BlockValidationFinished messages during shutdown.

Skipping BlockValidationFinished is a good step, but BlockPreValidated messages can also fail during shutdown (e.g., ValidationServiceUnreachable when the validation service channel is already closed at line 381). The ? on line 189 will abort the drain and bubble an error out of start().

Consider logging and continuing for all message types during the drain, not just BlockValidationFinished:

🛠️ Proposed fix

         while let Ok(traced) = self.msg_rx.try_recv() {
             let (msg, parent_span) = traced.into_parts();
             match msg {
                 // Skip: talks to downstream services (reth FCU, migration) that
                 // may already be stopped during shutdown.
                 BlockTreeServiceMessage::BlockValidationFinished { .. } => {
                     debug!("Skipping BlockValidationFinished during shutdown drain");
                 }
-                msg => self.inner.handle_message(msg, parent_span).await?,
+                msg => {
+                    if let Err(e) = self.inner.handle_message(msg, parent_span).await {
+                        debug!("Error handling message during shutdown drain: {:?}", e);
+                    }
+                }
             }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/actors/src/block_tree_service.rs` around lines 183 - 190, The drain
loop in start() currently only skips BlockValidationFinished and awaits
self.inner.handle_message(msg, parent_span).await? which will propagate any
error (e.g., ValidationServiceUnreachable for BlockPreValidated) and abort the
drain; change the logic so that inside the match over BlockTreeServiceMessage
you call self.inner.handle_message(msg, parent_span).await but do not use the ?
— instead match on the Result, log the error with context (including the message
variant) and continue the loop; ensure BlockValidationFinished remains skipped
and that errors from handle_message (for example when handling
BlockPreValidated) are logged and ignored during shutdown rather than returned.

.claude/skills/debugging-flaky-tests/SKILL.md (1)

185-185: ⚠️ Potential issue | 🟡 Minor

Parameter name chunk is misleading — actual argument is a boolean.

The past review noted this: actual call sites pass a bool (e.g., wait_for_block_in_index(migration_height, false, seconds_to_wait)). The parameter should reflect this.

📝 Proposed fix

-| `wait_for_block_in_index(h, chunk, secs)` | Polling DB (1s) | Need block in persistent index |
+| `wait_for_block_in_index(h, include_txs, secs)` | Polling DB (1s) | Need block in persistent index |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/debugging-flaky-tests/SKILL.md at line 185, The parameter
name chunk in function wait_for_block_in_index(h, chunk, secs) is misleading
because callers pass a boolean; rename the parameter to something like
expect_persistent (or is_persistent) and update the function signature/type
hints, its docstring, all internal logic that treats it as a boolean, and every
call site that passes a boolean (e.g., wait_for_block_in_index(migration_height,
false, seconds_to_wait)) to use the new name; ensure tests and any comments are
updated accordingly to reflect the boolean semantic.

crates/macros/diag-slow/src/lib.rs (1)

101-112: 🧹 Nitpick | 🔵 Trivial

Comment on the async move branch still mentions "state diagnostics" — misleading.

The async move branch is taken when state_expr is absent, so the comment "ownership transfer doesn't conflict with state diagnostics borrowing captured variables" references a feature that does not apply in this branch. The correct explanation is that move is safe here precisely because there is no state_expr needing to borrow outer captures.

📝 Proposed fix

     } else {
         quote! {
-            // `move` is safe here: ownership transfer doesn't conflict with
-            // state diagnostics borrowing captured variables.
+            // No `state_expr`: no diagnostic closure needs to borrow outer
+            // captures, so `async move` safely transfers ownership into the
+            // future and avoids lifetime issues.
             let __diag_fut = async move `#body`;
         }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/macros/diag-slow/src/lib.rs` around lines 101 - 112, The comment on
the `async move` branch is misleading because that branch is executed when
`state_expr` is None; update the comment around `fut_decl` for the `else` branch
(the `let __diag_fut = async move `#body`;` case) to state that `move` is safe
because there is no `state_expr` that would need to borrow captured variables
(i.e., ownership transfer is fine when `state_expr` is absent), and remove or
replace the reference to "state diagnostics" borrowing captured variables to
accurately reflect the absence of `state_expr`.

crates/macros/diag-slow/Cargo.toml (1)

1-19: LGTM — quote is now a workspace dependency as previously recommended.

The Cargo.toml is correctly configured: proc-macro = true, syn = { workspace = true, features = ["full"] }, and quote = { workspace = true } now that the root workspace declares quote = "1.0".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/macros/diag-slow/Cargo.toml` around lines 1 - 19, No change required:
the Cargo.toml for the irys-macros-diag-slow crate is correctly
configured—ensure proc-macro = true remains in the [lib] section and the
dependencies keep syn = { workspace = true, features = ["full"] } and quote = {
workspace = true } so the crate uses the workspace-declared quote = "1.0".

crates/chain/tests/utils.rs (1)

1447-1461: ⚠️ Potential issue | 🟠 Major

Handle RecvError::Lagged as recoverable in reorg waits.

Line 1458 returns an error on lag, which can fail tests even though the receiver can continue. This should continue waiting until timeout/closed instead.

💡 Suggested fix

-        let timeout_duration = Duration::from_secs(seconds_to_wait as u64);
-        match tokio::time::timeout(timeout_duration, reorg_rx.recv()).await {
-            Ok(Ok(reorg_event)) => {
-                info!(
-                    "Reorg detected: {} blocks in old fork, {} in new fork, fork at height {}, new tip: {}",
-                    reorg_event.old_fork.len(),
-                    reorg_event.new_fork.len(),
-                    reorg_event.fork_parent.height,
-                    reorg_event.new_tip
-                );
-                Ok(reorg_event)
-            }
-            Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped))) => Err(eyre::eyre!(
-                "Reorg broadcast receiver lagged and skipped {} events",
-                skipped
-            )),
-            Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
-                Err(eyre::eyre!("Reorg broadcast channel closed"))
-            }
-            Err(_) => {
-                let current_tip = self.get_max_difficulty_block().block_hash;
-                let state = self.diag_wait_state().await;
-                Err(eyre::eyre!(
-                    "Timeout: No reorg event received within {} seconds (start_tip={}, current_tip={}). State: {}",
-                    seconds_to_wait,
-                    start_tip,
-                    current_tip,
-                    state
-                ))
-            }
-        }
+        let deadline = Instant::now() + Duration::from_secs(seconds_to_wait as u64);
+        loop {
+            let remaining = deadline.saturating_duration_since(Instant::now());
+            if remaining.is_zero() {
+                let current_tip = self.get_max_difficulty_block().block_hash;
+                let state = self.diag_wait_state().await;
+                return Err(eyre::eyre!(
+                    "Timeout: No reorg event received within {} seconds (start_tip={}, current_tip={}). State: {}",
+                    seconds_to_wait,
+                    start_tip,
+                    current_tip,
+                    state
+                ));
+            }
+            match tokio::time::timeout(remaining, reorg_rx.recv()).await {
+                Ok(Ok(reorg_event)) => {
+                    info!(
+                        "Reorg detected: {} blocks in old fork, {} in new fork, fork at height {}, new tip: {}",
+                        reorg_event.old_fork.len(),
+                        reorg_event.new_fork.len(),
+                        reorg_event.fork_parent.height,
+                        reorg_event.new_tip
+                    );
+                    return Ok(reorg_event);
+                }
+                Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped))) => {
+                    warn!("Reorg receiver lagged; skipped {skipped} events, continuing");
+                    continue;
+                }
+                Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
+                    return Err(eyre::eyre!("Reorg broadcast channel closed"));
+                }
+                Err(_) => continue,
+            }
+        }

tokio::sync::broadcast::Receiver::recv RecvError::Lagged semantics: is Lagged recoverable and should receiver continue waiting?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/utils.rs` around lines 1447 - 1461, The test currently
treats tokio::sync::broadcast::error::RecvError::Lagged as a terminal error when
awaiting reorg_rx.recv(), causing spurious failures; change the handling in the
match around tokio::time::timeout(timeout_duration, reorg_rx.recv()).await so
that when you get RecvError::Lagged(skipped) you log or warn (include skipped)
and then continue waiting (i.e., do not return Err, loop until a real event,
timeout, or closed), leaving the Ok(Ok(reorg_event)) and other error branches
intact; look for the match that calls reorg_rx.recv() and adjust the Lagged arm
to be recoverable.

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

crates/chain/tests/block_production/unpledge_refund.rs (2)

87-89: ⚠️ Potential issue | 🟡 Minor

Rename misleading inclusion block wording.

This block is mined via genesis_node.mine_block(), so the inclusion_block_peer naming and "peer-mined" expectation text are inaccurate and can mislead failure triage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unpledge_refund.rs` around lines 87 - 89,
The test's variable and failure message are misleading: change the variable name
inclusion_block_peer to reflect it was mined by the genesis node (e.g.,
inclusion_block_genesis) and update the expect() message passed to
wait_for_block_at_height to say "genesis-mined inclusion block" (or similar)
instead of "peer-mined inclusion block"; update all references to
inclusion_block_peer in this test (including the .height access and any other
uses) to the new name to keep names consistent with genesis_node.mine_block().

---

699-702: ⚠️ Potential issue | 🟡 Minor

Add contextual failure message for this wait call.

This wait_for_block_at_height(...).await? path is still inconsistent with nearby call sites that use .expect("...") context, which makes timeout diagnosis harder in CI logs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unpledge_refund.rs` around lines 699 -
702, Replace the fallible .await? on genesis_node.wait_for_block_at_height with
an explicit panic message so timeouts show context; call
wait_for_block_at_height(inclusion_block.height,
seconds_to_wait).await.expect(...) and include the height
(inclusion_block.height) and timeout (seconds_to_wait) in the message to make CI
failures clear (e.g. "genesis node did not receive block at height {height}
within {secs}s").

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

crates/chain/src/chain.rs (1)

216-222: ⚠️ Potential issue | 🟠 Major

Align the reth stop timeout with the ServiceSet shutdown budget.

Line [1272] can take ~10s × service_count, but Line [216] is capped by RETH_THREAD_STOP_TIMEOUT (Line [405], 60s). With the current service list size, normal shutdown can exceed 60s and produce spurious timeout errors.

Suggested fix

+/// Timeout for ServiceSet graceful shutdown aggregation.
+const SERVICE_SET_TIMEOUT: Duration = Duration::from_secs(120);
 /// Timeout for the reth thread to complete its full shutdown sequence.
-/// Budget: send(5s) + actor(25s) + service_set(~10 services × 10s) + task_manager(10s).
-/// Using 60s to cover typical case with margin; worst-case depends on service count.
-const RETH_THREAD_STOP_TIMEOUT: Duration = Duration::from_secs(60);
+/// Budget: send(5s) + actor(25s) + service_set(120s) + task_manager(10s) + margin.
+const RETH_THREAD_STOP_TIMEOUT: Duration = Duration::from_secs(170);

-                    service_set.graceful_shutdown().await;
+                    match tokio::time::timeout(SERVICE_SET_TIMEOUT, service_set.graceful_shutdown()).await {
+                        Ok(()) => debug!("ServiceSet graceful shutdown complete"),
+                        Err(_) => warn!("ServiceSet graceful shutdown timed out after {SERVICE_SET_TIMEOUT:?}"),
+                    }

Also applies to: 390-405, 1270-1272

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/src/chain.rs` around lines 216 - 222, The fixed
RETH_THREAD_STOP_TIMEOUT is causing spurious timeouts because shutdown can take
~10s × service_count; replace uses of RETH_THREAD_STOP_TIMEOUT in the
tokio::time::timeout calls (the rx match in chain.rs and the other occurrences
around 390-405 and 1270-1272) with the ServiceSet shutdown budget calculation
(e.g., obtain the ServiceSet shutdown_budget or compute per_service_timeout *
service_count — the same value used for ServiceSet shutdown) so the reth stop
wait uses the same total timeout as the ServiceSet shutdown budget.

crates/chain/tests/api/hardfork_tests.rs (1)

164-170: ⚠️ Potential issue | 🟡 Minor

Remove duplicated scanned/skipped values from the debug event.

scanned and skipped are already structured fields; interpolating them again in the message duplicates the same data.

Suggested fix

     tracing::debug!(
         ?tx_id,
         scanned,
         skipped,
         max_height,
-        "tx not found in {scanned} blocks ({skipped} heights failed to load)"
+        "tx not found in scanned blocks"
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/api/hardfork_tests.rs` around lines 164 - 170, The
tracing::debug call currently interpolates scanned and skipped both as
structured fields and again inside the message string; remove that duplication
by leaving scanned and skipped as structured fields (they are already passed as
?scanned and ?skipped) and change the message to not include {scanned} or
{skipped} (e.g. "tx not found in scanned blocks (some heights failed to load)")
in the tracing::debug invocation that includes ?tx_id, ?scanned, ?skipped,
?max_height.

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

crates/chain/tests/block_production/unstake_refund.rs (1)

1100-1100: 🧹 Nitpick | 🔵 Trivial

heavy3_ prefix is still unexplained.

The rename from heavy_unpledge_and_unstake_concurrent_success_flow to heavy3_unpledge_and_unstake_concurrent_success_flow introduces a numeric prefix that differs from all other heavy_ tests in this file, and there is no inline comment or doc note explaining the convention (e.g., test tier, #[diag_slow] level). Either rename back to the established pattern or document what heavy3_ signifies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/block_production/unstake_refund.rs` at line 1100, The test
function heavy3_unpledge_and_unstake_concurrent_success_flow uses an unexplained
numeric prefix; either revert the name to the established
heavy_unpledge_and_unstake_concurrent_success_flow or add a one-line comment or
docstring above heavy3_unpledge_and_unstake_concurrent_success_flow explaining
the meaning of the "3" (e.g., test tier or diag_slow level) so naming is
consistent and discoverable; update any references to the test name if you
rename the function and ensure any test attributes like #[diag_slow] are
mentioned in the comment.

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

crates/chain/tests/api/hardfork_tests.rs (1)

165-170: ⚠️ Potential issue | 🟡 Minor

Remove duplicated scanned/skipped values in the debug log.

Line 169 interpolates {scanned} and {skipped} while those fields are already emitted structurally, so the event records them twice.

♻️ Proposed fix

     tracing::debug!(
         ?tx_id,
         scanned,
         skipped,
         max_height,
-        "tx not found in {scanned} blocks ({skipped} heights failed to load)"
+        "tx not found in searched blocks"
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/api/hardfork_tests.rs` around lines 165 - 170, The
debug/event log currently emits scanned and skipped both as structured fields
and again via string interpolation in the message; edit the log call that
includes tx_id, scanned, skipped, max_height and the message "tx not found in
{scanned} blocks ({skipped} heights failed to load)" to remove the "{scanned}"
and "{skipped}" interpolations from the message so those values are only emitted
once as structured fields (leave tx_id and max_height usage intact).

crates/chain/tests/utils.rs (2)

1446-1483: ⚠️ Potential issue | 🟠 Major

RecvError::Lagged is still treated as fatal in wait_for_reorg_subscribed.

Line 1464 returns an error on Lagged, which is a recoverable condition — the receiver is repositioned to the oldest retained message. Under CI load this causes spurious failures. Wrap in a loop, log the lag, and continue waiting until the deadline.

Suggested fix

     async fn wait_for_reorg_subscribed(
         &self,
         mut reorg_rx: tokio::sync::broadcast::Receiver<ReorgEvent>,
         seconds_to_wait: usize,
         start_tip: H256,
     ) -> eyre::Result<ReorgEvent> {
-        let timeout_duration = Duration::from_secs(seconds_to_wait as u64);
-        match tokio::time::timeout(timeout_duration, reorg_rx.recv()).await {
-            Ok(Ok(reorg_event)) => {
-                info!(
-                    "Reorg detected: {} blocks in old fork, {} in new fork, fork at height {}, new tip: {}",
-                    reorg_event.old_fork.len(),
-                    reorg_event.new_fork.len(),
-                    reorg_event.fork_parent.height,
-                    reorg_event.new_tip
-                );
-                Ok(reorg_event)
-            }
-            Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped))) => Err(eyre::eyre!(
-                "Reorg broadcast receiver lagged and skipped {} events",
-                skipped
-            )),
-            Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
-                Err(eyre::eyre!("Reorg broadcast channel closed"))
-            }
-            Err(_) => {
-                let current_tip = self.get_max_difficulty_block().block_hash;
-                let state = self.diag_wait_state().await;
-                Err(eyre::eyre!(
-                    "Timeout: No reorg event received within {} seconds (start_tip={}, current_tip={}). State: {}",
-                    seconds_to_wait,
-                    start_tip,
-                    current_tip,
-                    state
-                ))
+        let deadline = tokio::time::Instant::now()
+            + Duration::from_secs(seconds_to_wait as u64);
+        loop {
+            let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
+            match tokio::time::timeout(remaining, reorg_rx.recv()).await {
+                Ok(Ok(reorg_event)) => {
+                    info!(
+                        "Reorg detected: {} blocks in old fork, {} in new fork, fork at height {}, new tip: {}",
+                        reorg_event.old_fork.len(),
+                        reorg_event.new_fork.len(),
+                        reorg_event.fork_parent.height,
+                        reorg_event.new_tip
+                    );
+                    return Ok(reorg_event);
+                }
+                Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped))) => {
+                    warn!("Reorg receiver lagged; skipped {skipped} events, continuing");
+                    continue;
+                }
+                Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
+                    return Err(eyre::eyre!("Reorg broadcast channel closed"));
+                }
+                Err(_) => {
+                    let current_tip = self.get_max_difficulty_block().block_hash;
+                    let state = self.diag_wait_state().await;
+                    return Err(eyre::eyre!(
+                        "Timeout: No reorg event received within {} seconds (start_tip={}, current_tip={}). State: {}",
+                        seconds_to_wait,
+                        start_tip,
+                        current_tip,
+                        state
+                    ));
+                }
             }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/utils.rs` around lines 1446 - 1483, In
wait_for_reorg_subscribed, treat
tokio::sync::broadcast::error::RecvError::Lagged as non-fatal by wrapping the
recv call in a loop that retries until the timeout_duration elapses: on
RecvError::Lagged(skipped) log the skipped count (using info/debug) and continue
waiting, on RecvError::Closed return the existing error, and on successful
Ok(reorg_event) return it; ensure the timeout uses the original deadline so
repeated Lagged occurrences don’t reset the timer and use the existing symbols
reorg_rx, timeout_duration, seconds_to_wait, start_tip,
get_max_difficulty_block, and diag_wait_state to preserve current
timeout/diagnostic behavior.

---

2249-2272: wait_for_tx_included still swallows all errors indiscriminately.

Line 2263 catches every Err(_) as a retry, masking channel/service failures that would never self-resolve. Only "not found" errors should be retried; genuine service failures should propagate immediately for better diagnostics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/chain/tests/utils.rs` around lines 2249 - 2272, wait_for_tx_included
is currently treating every Err from get_storage_tx_header_from_mempool as a
transient “retry”, which hides permanent service/channel failures; change the
error handling so you only retry when the error represents “not found” (or
whatever concrete NotFound variant your storage API returns) and immediately
return Err for all other errors. Locate wait_for_tx_included and the call to
get_storage_tx_header_from_mempool(tx_id) and replace the blanket Err(_) arm
with logic that inspects the error (e.g., pattern-match on the concrete error
enum or use an is_not_found helper) — if it’s a not-found case continue polling,
otherwise propagate the error (return Err(e)) so real failures surface. Ensure
the returned error preserves the original error value/type instead of swallowing
it.

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro