test(e2e): de-flake test_restarts_observer basefee assertion

[bdimitrov-netzine]

Summary

send_and_confirm asserted the basefee collector grew by an exact per-tx amount (current + current/fee_index). Under EIP-1559 the base fee drifts block-to-block, so consecutive transfers don't contribute a byte-identical fee — making the check pass or fail depending on timing (the intermittent "Expected a basefee increment!" nightly failures).

Relax it to assert only a strict increase: the transfer paid a base fee into the collector. get_balance_above_with_retry already waits for the balance to exceed the prior value, so this is deterministic.

PR description:

Summary

• De-flake the test_restarts_observer nightly e2e test by relaxing an overly-strict basefee assertion in send_and_confirm.
• The old check required the basefee collector balance to grow by an exact computed amount (current_basefee + current_basefee / fee_index). Under EIP-1559, the base fee
  drifts block-to-block, so this exact match doesn't hold on every run, causing intermittent "Expected a basefee increment!" failures in the nightly suite.
• The assertion now only requires a strict increase in the basefee collector's balance, which is what actually matters (the transfer paid some base fee).
  get_balance_above_with_retry already polls until the balance exceeds the prior value, so this check is deterministic.

Test plan

• cargo build -p e2e-tests (or equivalent) to confirm it compiles
• Run test_restarts_observer locally/in CI a few times to confirm no regressions and the flake is gone

[github-actions]

Claude finished @bdimitrov-netzine's task in 1m 20s —— View job

---

Code Review

• Read changed file and diff
• Analyze correctness and potential issues
• Post review feedback

---

Summary

The fix correctly addresses the root cause: the original assertion required the basefee collector to grow by an exact per-tx computed amount (current_basefee + current_basefee / fee_index), which is non-deterministic under EIP-1559 base fee drift. Relaxing to a strict-increase assertion is the right call.

The change is minimal, the variable rename (bal → new_basefee) improves clarity, and the error message now surfaces both values for easier debugging.

---

Observation: the explicit post-retry guard is dead code

crates/testing/e2e-tests/tests/it/restarts.rs:97-107

let new_basefee =
    get_balance_above_with_retry(node_test, &basefee_address.to_string(), current_basefee)?;
//                                                                         ^^^^^^^^^^^^^^
// returns Ok(bal) only when bal > current_basefee; otherwise returns Err → propagated by ?

if fee_index > 0 && new_basefee <= current_basefee {  // ← can never be true

get_balance_above_with_retry (line 808) polls until bal > above and returns Ok(bal) in that case; if the poll times out, it returns Err(...). The ? on line 98 means execution only continues when new_basefee > current_basefee is already guaranteed — so new_basefee <= current_basefee is unreachable in the success path.

This isn't a bug (the real safeguard is the retry function), but the guard reads like an active assertion when it is in fact dead code. This could mislead a future reader into thinking the check fires under some condition.

Options:

1. Remove the if block entirely, relying solely on get_balance_above_with_retry to enforce the invariant (makes the semantics explicit). Fix this →
2. Keep as-is and add a comment making the redundancy intentional (defensive check).

Additionally, after this change fee_index is only consumed inside the dead-code branch, so the parameter no longer affects test behaviour. That's worth a follow-up if the goal is to eventually drop fee_index entirely from send_and_confirm.

---

Minor nit

The multi-line comment at lines 99–103 is good context, but its last sentence ("this check is deterministic") is slightly inaccurate now that the check it refers to (new_basefee <= current_basefee) is unreachable. The determinism guarantee comes entirely from get_balance_above_with_retry, not from the subsequent if.

---

Overall: The fix is correct and the flake will be resolved. The dead-code observation above is worth addressing for long-term clarity, but is not a blocker.

[github-actions]

Security Scan - Code

Severity: HIGH, CRITICAL

No vulnerabilities found

View scan results

Report Summary

┌─────────────────────────────────────────────┬────────────┬─────────────────┬───────────────────┐
│                   Target                    │    Type    │ Vulnerabilities │ Misconfigurations │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ Cargo.lock                                  │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/Cargo.lock      │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/fuzz/Cargo.lock │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/state-sum/Cargo.lock                    │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/tps/package-lock.json                   │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ rayls-contracts/package-lock.json           │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/chaos-network/Dockerfile                │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-network/Dockerfile               │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-replay/Dockerfile                │ dockerfile │        -        │         0         │
└─────────────────────────────────────────────┴────────────┴─────────────────┴───────────────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)

[github-actions]

Claude finished @bdimitrov-netzine's task in 1m 23s —— View job

---

Code Review

• Read CLAUDE.md for repo-specific instructions (none found)
• Read changed file and diff
• Analyzed get_balance_above_with_retry semantics and all send_and_confirm call sites
• Post review feedback

---

Summary

The core fix is correct: the exact-increment assertion bal != current_basefee + current_basefee / fee_index is genuinely flaky under EIP-1559 (base fee drifts block-to-block), and relaxing to a strict-increase check resolves the intermittent nightly failures. The variable rename (bal → new_basefee) and the improved error message are both good.

That said, there are two issues worth resolving before merge.

---

Issue 1: Dead-code guard (previously flagged, still present)

crates/testing/e2e-tests/tests/it/restarts.rs, lines 97–107

let new_basefee =
    get_balance_above_with_retry(node_test, &basefee_address.to_string(), current_basefee)?;
// ↑ returns Ok(bal) ONLY when bal > current_basefee; otherwise Err → propagated by ?

if fee_index > 0 && new_basefee <= current_basefee {   // ← unreachable in the success path
    error!(...);
    return Err(...);
}

get_balance_above_with_retry (line 808) polls until bal > above and returns Ok(bal) on success, Err(...) on timeout. The ? means: if the retry loop exits with bal <= current_basefee, an Err is already returned before this if is evaluated. The guard therefore cannot fire on the success path — it is dead code.

Recommended fix: remove the if block and its comment entirely. The retry function is the actual enforcer. Fix this →

---

Issue 2: fee_index is now a phantom parameter

Once the dead-code guard is removed, fee_index has no effect anywhere in send_and_confirm. The docstring (lines 64–68) still says:

fee_index is the 0-based ordinal of this transfer within the test (0 for the first, 1 for the second, …) and is used only for the cumulative basefee assertion

That assertion no longer exists. All six call sites pass different values (0, 1, 2), but they have zero behavioural impact.

Recommended follow-up: remove fee_index from the function signature and all call sites, and update the docstring. This is a small but important cleanup — a parameter that does nothing is misleading and maintenance debt. Fix this →

---

Minor nit

The multi-line comment at lines 99–103 ends with the phrase "this check is deterministic." That refers to the if guard that follows — which is dead code. The determinism guarantee comes from get_balance_above_with_retry, not from the subsequent if. The comment should be removed along with the guard.

---

Overall: The flake fix is correct and will work. Issues 1 and 2 are both cleanup, not blockers for correctness — but dead code and unused parameters compound over time, so I'd suggest addressing them in this PR or an immediate follow-up.