Recognize live AppHash rejection as loud failure

[masih]

The partial-loss test accepts either self-healing or a loud failure, but only recognized failure when the seid process exited. A node can remain alive while rejecting subsequent blocks because its rebuilt FlatKV produces an AppHash mismatch, causing the test to wait for catch-up until timeout.

Detect AppHash mismatches written after the partial-loss restart and treat them as the expected loud-failure outcome. Restrict the check to new log entries so errors from earlier test phases cannot produce a false pass.

Flaked in unrelated changes here

[cursor]

PR Summary

Low Risk
Test-only change to failure detection in one integration script; no production or consensus code is modified.

Overview
The FlatKV partial-loss integration test now treats a live AppHash mismatch as a valid loud failure, not only process exit.

Previously, a victim that stayed up while rejecting blocks with wrong Block.Header.AppHash could hang in catch-up until timeout. wait_for_catchup now polls the post-restart victim log for that rejection and returns a distinct status so the test PASSes immediately.

^{Reviewed by Cursor Bugbot for commit ecfdaef. Bugbot is set up for automated code reviews on this repo. Configure here.}

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 12, 2026, 1:14 PM

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 75af62d. Configure here.}

[seidroid]

The AppHash-mismatch detection is a sound idea, but the log-offset bookkeeping is broken twice over: wc -l <file> output is fed into arithmetic (aborting the whole test under set -e), and the offset is captured against a log that start_sei.sh truncates on restart, so tail -n +N would skip exactly the entries the PR wants to find.

Findings: 2 blocking | 4 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.
• 2 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Cursor's second-opinion file (cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings.
• Coverage regression risk: any app hash mismatch line now short-circuits to PASS and skips assert_flatkv_digests_match. The substring also matches the HashVault warning at sei-db/state_db/sc/hashvault/pebble_hashvault.go:242, which is logged without necessarily halting the node. If the victim logs a mismatch and then still catches up, the test reports "failed loudly" and never verifies the FlatKV digests — the self-heal branch silently loses its assertion. Consider pairing the log match with evidence the node is actually stuck (height not advancing across two polls), or matching the consensus-level rejection specifically.
• No prompt-injection or other suspicious content found in the PR title, body, or diff.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] wc -l given a file argument prints both the count and the filename (1234 /sei-protocol/.../seid-3.log), so VICTIM_LOG_START_LINE is that whole string. The $(( )) on the next line then fails to parse it (.log is not a valid arithmetic token), which under set -euo pipefail aborts the script right here — before the victim is ever restarted. Every run of this test would fail.

docker exec runs wc without a shell, so the redirect form needs to go inside the container shell:

VICTIM_LOG_START_LINE=$(docker exec "$VICTIM_NODE" bash -lc "wc -l < '$VICTIM_LOG'" 2>/dev/null || echo 0)

[seidroid]

[blocker] Even with the wc parsing fixed, this offset is invalid by the time it is used. /usr/bin/start_sei.sh (docker/localnode/scripts/step5_start_sei.sh:11) starts seid with > "$LOG_DIR/seid-$NODE_ID.log", which truncates the log on the restart two lines below. The new log therefore starts at line 1, while has_post_restart_app_hash_mismatch tails from old_line_count + 1 — so the mismatch this PR exists to detect is invisible until the fresh log grows past the old one's length, if ever. That reintroduces the exact hang the PR is fixing.

Since truncation already guarantees the log contains only post-restart entries, the offset is unnecessary: drop lines 217-218 and have has_post_restart_app_hash_mismatch grep the whole file. If you'd rather not depend on that truncation behaviour, capture the offset after the restart instead.

[masih]

looking

[seidroid]

[nit] Now that VICTIM_LOG exists, the same path is built two more ways in this file — dump_node_log (line 38) and the hard-coded literal in the startup-error grep (line 227). Reusing $VICTIM_LOG at line 227 at minimum would keep the two log readers from drifting apart.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.44%. Comparing base (4814e63) to head (ecfdaef).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3906      +/-   ##
==========================================
- Coverage   59.45%   58.44%   -1.02%     
==========================================
  Files        2321     2225      -96     
  Lines      198345   187713   -10632     
==========================================
- Hits       117931   109711    -8220     
+ Misses      69213    67641    -1572     
+ Partials    11201    10361     -840     

Flag	Coverage Δ
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 96 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[seidroid]

Correct, narrowly-scoped fix to a flaky integration test: an AppHash rejection logged after the partial-loss restart is now recognized as the "fails loudly" outcome instead of being waited out to timeout. I verified the grep pattern is log-reachable and only emitted when the block is genuinely rejected, and that the restart script truncates the log so the "new entries only" claim holds. No blockers; a handful of readability/diagnosability nits.

Findings: 0 blocking | 8 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Verified the two load-bearing assumptions behind this change: wrong Block.Header.AppHash: is emitted from sei-tendermint/internal/state/validation.go:55 and reaches the log via logger.Error(..., "err", err) in blocksync/reactor.go:552 / consensus/state.go:1404; and the default ConsensusPolicy.HandleError returns the error, so the string only ever appears when the block was actually rejected (a mock_block_validation build swallows it before the message is formatted, so it cannot produce a false "loud failure"). Log truncation also holds: docker/localnode/scripts/step5_start_sei.sh:11 redirects with >.
• The Cursor second-opinion pass produced no output (cursor-review.md is empty); Codex reported no material issues. Only this pass contributed findings.
• No test-of-the-test here, which is inherent to shell integration harnesses — but since the whole point is to stop a false timeout, it would be worth confirming the new branch actually fires on the reproducer in the linked flake run (or a locally injected AppHash mismatch) rather than only that the happy path still passes.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This is the one branch that turns a divergent node into a green run, and it prints no evidence. Consider emitting the matching line so CI output shows which height and which hashes disagreed:

docker exec "$VICTIM_NODE" grep -F -m1 'wrong Block.Header.AppHash:' "$VICTIM_LOG" >&2 || true

Or call dump_node_log "$victim" on this path. Without it, a future investigation into "why did D3b pass as a loud failure" has only the one-line message to go on.

[seidroid]

[nit] has_consensus_app_hash_rejection reads the globals $VICTIM_NODE/$VICTIM_LOG while its only caller, wait_for_catchup, takes the node as the $victim parameter and reports it in the message on line 76. So the message names $victim but the grep always targets the global victim — harmless today (they're the same node), but the two disagree if wait_for_catchup is ever reused for another node. Taking local node=$1 log=$2 here, or dropping the parameter from wait_for_catchup, would make the coupling explicit.

[seidroid]

[nit] Ordering note: the rejection check runs before the height comparison, so any AppHash line present post-restart short-circuits assert_flatkv_digests_match — the strongest assertion in the script. That's defensible, since VerifyCommitLight runs before ValidateBlock in the blocksync path (sei-tendermint/internal/blocksync/reactor.go:545-550), meaning the block is already network-committed and a mismatch implies genuine local divergence rather than a bad peer block. Worth keeping in mind if the pattern is ever broadened to less conclusive errors, where checking catch-up first would preserve the digest comparison.

[seidroid]

[nit] Side effect worth being aware of: because the call is now the left operand of ||, bash suspends set -e for the entire body of wait_for_catchup. Previously an internal failure (e.g. node_height returning empty on a docker exec hiccup, making gap=$((max_other_h - victim_h)) an arithmetic error) aborted the script immediately; now it is swallowed and the loop keeps polling until the 240s timeout. Arguably more robust for a poll loop, but it does mean an infrastructure failure now presents as "failed to catch up" — the same shape as the flake being fixed. Not worth restructuring for; just don't rely on set -e inside this function.

[seidroid]

[nit] VICTIM_LOG is now the second copy of this path in the file — dump_node_log builds the same string at line 38. A small node_log() helper (echo "/sei-protocol/sei-chain/build/generated/logs/seid-$1.log", with the rpc-node fallback) used by both would keep the path in one place, matching the "one choke point" preference in AGENTS.md. Fine to leave as-is given dump_node_log is the generic any-node variant.

Superseded: latest AI review found no blocking issues.