chore(l1): add phase completion markers for snap sync validation

[ilitteri]

Motivation

The three post-sync validation steps (validate_state_root, validate_storage_root, validate_bytecodes) run after snap sync healing but don't emit ✓ phase completion markers. This makes them invisible in the docker monitor's Slack notification phase breakdown — validation time is unaccounted for in the per-phase timing report.

Description

• Add std::time::Instant timing to all three validation functions in snap_sync.rs
• Emit ✓ markers matching the format used by other phases (e.g. ✓ STATE ROOT VALIDATION complete: state root verified in HH:MM:SS)
• Register corresponding regex patterns in docker_monitor.py's PHASE_COMPLETION_PATTERNS so they appear in the Slack phase breakdown

How to Test

1. Run a snap sync with release-with-debug-assertions profile (validation only runs inside debug_assert!)
2. After sync completes, check logs for ✓ STATE ROOT VALIDATION complete:, ✓ STORAGE ROOT VALIDATION complete:, ✓ BYTECODE VALIDATION complete: markers with timing
3. Verify the docker monitor Slack notification includes the validation phases in its breakdown

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR adds timing information to validation functions in snap sync and updates the Python monitoring script. Here are the issues found:

1. Code Duplication (snap_sync.rs:702-705, 737-740, 771-774)

The elapsed time formatting is duplicated in three functions. Extract this into a helper:

fn format_duration(duration: std::time::Duration) -> String {
    let secs = duration.as_secs();
    format!("{:02}:{:02}:{:02}", secs / 3600, (secs % 3600) / 60, secs % 60)
}

2. Inconsistent Logging (snap_sync.rs:703, 738, 772)

The validation functions log completion times but use different patterns. Standardize the format string to match the Python regex expectations.

3. Regex Pattern Mismatch (docker_monitor.py:61-63)

The Python regex patterns expect a count parameter (e.g., ([\d,]+) or ([\w ]+)) but the Rust code doesn't provide this. The patterns should be:

"State Root Validation": r"✓ STATE ROOT VALIDATION complete: state root verified in (\d+:\d{2}:\d{2})",
"Storage Root Validation": r"✓ STORAGE ROOT VALIDATION complete: all storage roots verified in (\d+:\d{2}:\d{2})",
"Bytecode Validation": r"✓ BYTECODE VALIDATION complete: all bytecodes verified in (\d+:\d{2}:\d{2})",

4. Process Exit on Validation Failure

The functions call std::process::exit(1) on validation failure, which is abrupt. Consider returning a Result and handling the exit at a higher level for better testability.

5. Minor Typo (snap_sync.rs:706)

"Succesfully" should be "Successfully".

The changes are generally safe but could benefit from the refactoring above for maintainability.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Review summary
This PR only adds timing logs and regex patterns. I didn’t see correctness, security, or consensus-impacting issues in the diff. Overall looks fine.

Notes / Minor suggestions

1. crates/networking/p2p/sync/snap_sync.rs (validate_state_root, validate_storage_root, validate_bytecodes): The elapsed formatting code is duplicated 3 times. Consider a small helper (e.g., format_elapsed(start: Instant) -> String) to reduce repetition and keep formatting consistent. This is purely maintainability.
2. tooling/sync/docker_monitor.py: The new regexes use [\w ]+, which will not match punctuation (e.g., commas, colons, hyphens). Current log messages are fine, but if you ever add punctuation (e.g., “state root verified, fast path”), matching will fail. If you want it resilient, consider (.+?) in instead.

No further issues spotted.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

It seems the patterns are defined in the Python file but the actual log emission might be elsewhere. Let me look at the bigger picture of the validation logic and check the actual changes more carefully.

Now I have enough context to provide a comprehensive code review.

---

PR Review: Add phase completion markers for snap sync validation

Summary

This PR adds timing instrumentation and phase completion markers to three post-sync validation functions in snap sync. The changes are straightforward and well-motivated - they improve observability without altering validation logic.

✅ Positives

• Clear motivation: Makes validation phases visible in monitoring
• Consistent format matching existing phase markers
• Non-invasive: Only adds timing/logging, no logic changes
• Pattern registration in Python monitor aligns with Rust output

Issues Found

1. CRITICAL: Timing Only Captures Success Path (snap_sync.rs:705-712)

Bug in validate_state_root:

if validated.is_ok() {
    info!("Succesfully validated tree, {state_root} found");
    info!("✓ STATE ROOT VALIDATION complete: state root verified in {elapsed_str}");
} else {
    error!("We have failed the validation of the state tree");
    std::process::exit(1);
}

The completion marker is only emitted on success, but timing is measured before the branch. If validation fails, the elapsed time is computed but never logged. This means the docker monitor will:

• Never see a completion marker on failure
• Not account for time spent on failed validation

Suggestion: Emit timing info before std::process::exit(1) or restructure to always log elapsed time.

Same issue exists in validate_storage_root (lines 741-745) and implicitly in validate_bytecodes (line 768 exits without timing log).

---

2. Minor: Typo in Original Code (snap_sync.rs:706)

info!("Succesfully validated tree, {state_root} found");

Should be "Successfully" (double-s). Not introduced by this PR, but worth fixing.

---

3. Code Quality: Duplicate Timing Formatting Logic (multiple locations)

The time formatting is duplicated across all three functions:

let elapsed = start.elapsed();
let secs = elapsed.as_secs();
let elapsed_str = format!("{:02}:{:02}:{:02}", secs / 3600, (secs % 3600) / 60, secs % 60);

Suggestion: Extract to a helper function:

fn format_duration(duration: std::time::Duration) -> String {
    let secs = duration.as_secs();
    format!("{:02}:{:02}:{:02}", secs / 3600, (secs % 3600) / 60, secs % 60)
}

This would reduce duplication and make timing format changes easier.

---

4. Python Regex Pattern Mismatch Risk (docker_monitor.py:61-63)

The regex patterns use ([\w ]+) to capture the descriptive text:

"State Root Validation": r"✓ STATE ROOT VALIDATION complete: ([\w ]+) in (\d+:\d{2}:\d{2})",

This expects alphanumeric + spaces only, which matches current outputs:

• ✓ "state root verified"
• ✓ "all storage roots verified"
• ✓ "all bytecodes verified"

Risk: If you later add punctuation (e.g., "state root verified (123 nodes)"), the regex will break. Consider using ([^i][^n]+) or (.+?) for more flexibility, or simply [\w\s,()-]+ to allow common punctuation.

---

5. Consistency: Variable Position (snap_sync.rs:750)

In validate_bytecodes, the timing is measured at line 750 after the function declaration but before iteration starts. The other two async functions measure timing at the same relative position. ✅ This is consistent.

However, note that validate_bytecodes is synchronous while the other two are async and use spawn_blocking. The timing for validate_bytecodes includes all work inline, while the others include tokio task spawn overhead. This is acceptable but worth noting for performance analysis.

---

6. Potential Issue: Regex Group Capture Inconsistency

Other phase patterns capture count as group 1 and time as group 2:

"Bytecodes": r"✓ BYTECODES complete: ([\d,]+) bytecodes in (\d+:\d{2}:\d{2})",

The new validation patterns capture description text as group 1:

"State Root Validation": r"✓ STATE ROOT VALIDATION complete: ([\w ]+) in (\d+:\d{2}:\d{2})",

Risk: If the Python parsing code expects group 1 to always be a numeric count for statistics, this could break. Check how PHASE_COMPLETION_PATTERNS groups are used downstream in the monitor script.

---

Ethereum-Specific Considerations

✅ No consensus/EVM impact - purely observability changes
✅ No state/storage modification - read-only validation unchanged
✅ Validation logic untouched - trie validation, storage root checks, and bytecode verification remain identical

---

Recommendations

Must Fix (P0):

1. Ensure timing is logged even on validation failure paths (all three functions)

Should Fix (P1):

2. Extract duration formatting to a shared helper function
3. Verify Python monitor correctly handles non-numeric first capture group

Nice to Have (P2):

4. Fix typo: "Succesfully" → "Successfully" (snap_sync.rs:706)
5. Make regex patterns more robust to future message changes

---

Verdict

Approve with required changes. The core change is sound, but the timing measurement should account for failure paths to avoid incomplete monitoring data. The Python regex patterns should be validated against actual monitor parsing logic to ensure compatibility.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Overview

Greptile Summary

Adds phase completion markers with timing to three post-sync validation functions (validate_state_root, validate_storage_root, validate_bytecodes) and registers corresponding regex patterns in the docker monitor.

Key changes:

• Instrumented all three validation functions with std::time::Instant timing
• Added ✓ completion markers matching existing phase format (e.g., "✓ STATE ROOT VALIDATION complete: state root verified in HH:MM:SS")
• Registered three new patterns in docker_monitor.py's PHASE_COMPLETION_PATTERNS to parse these markers

The implementation is clean and consistent with existing patterns in the codebase. All timing calculations use the same HH:MM:SS format used elsewhere.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The changes are purely additive (logging and monitoring instrumentation), follow existing patterns exactly, and don't modify any core logic. The timing measurements are straightforward, regex patterns correctly match the emitted log messages, and the implementation is consistent across all three validation functions.
• No files require special attention

Important Files Changed

Filename	Overview
crates/networking/p2p/sync/snap_sync.rs	Added timing instrumentation and completion markers for three validation functions (validate_state_root, validate_storage_root, validate_bytecodes). Clean, consistent implementation following existing patterns.
tooling/sync/docker_monitor.py	Added three new regex patterns to PHASE_COMPLETION_PATTERNS dict to capture validation phase timing from logs. Patterns correctly match the new log messages.

[github-actions]

Lines of code report

Total lines added: 15
Total lines removed: 0
Total lines changed: 15

Detailed view

+------------------------------------------------+-------+------+
| File                                           | Lines | Diff |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync/snap_sync.rs | 960   | +15  |
+------------------------------------------------+-------+------+

[Copilot]

Pull request overview

Adds missing phase-completion markers for the three post-snap-sync validation steps so the docker monitor can include validation time in the Slack per-phase breakdown.

Changes:

• Time validate_state_root, validate_storage_root, and validate_bytecodes and emit ✓ ... VALIDATION complete: markers with HH:MM:SS durations.
• Register new validation completion regexes in the docker monitor so these phases appear in the Slack phase breakdown.

Reviewed changes

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

File	Description
tooling/sync/docker_monitor.py	Adds PHASE_COMPLETION_PATTERNS entries to detect the new validation completion markers and include them in Slack phase breakdowns.
crates/networking/p2p/sync/snap_sync.rs	Adds timing and emits phase completion marker logs for state root, storage root, and bytecode validation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

elapsed_str formatting logic is duplicated across the three validation functions. Consider extracting a small helper (or reusing the existing HH:MM:SS formatter used for sync phase logs in crates/networking/p2p/network.rs) so the duration format stays consistent and future changes only need to be made in one place.

[Copilot]

Typo in log message: "Succesfully" should be "Successfully".

Suggested change

	info!("Succesfully validated tree, {state_root} found");
	info!("Successfully validated tree, {state_root} found");