message/validation: refactor validation flow to per-message actors

[nkryuchkov]

Summary
Refactor the message validation pipeline to use per-MessageID actors instead of per-MessageID mutex locking.

This PR is a part of F-ssv-164 and depends on #2728.

The goal is to explore a different concurrency model for validation on critical paths where multiple messages for the same validator/role arrive close together.

Motivation
The current lock-based validation flow has a hard tradeoff:

• keep all validation under one lock and serialize expensive signature verification
• or move signature verification outside the lock and accept more subtle lock-splitting behavior

This PR explores a third option:

• assign mutable validation state ownership to a per-MessageID actor
• run cheap stateful checks on that actor before signature verification
• run signature verification outside the actor
• re-enter the actor to recheck and commit state

This preserves cheap stale/duplicate filtering before signature verification while allowing signature verification to proceed concurrently.

What Changed

• Replaced per-MessageID mutex coordination with a per-MessageID actor/mailbox model
• Added a new actor implementation in message/validation/validation_actor.go
• Updated message/validation/validation.go to cache and manage actors instead of locks
• Routed consensus validation through the actor flow in message/validation/consensus_validation.go
• Routed partial-signature validation through the actor flow in message/validation/partial_validation.go
• Replaced lock-specific concurrency tests with actor-specific concurrency tests in message/validation/validation_actor_test.go

Behavioral Model
For a given MessageID, validation now follows this shape:

1. decode and semantic validation
2. actor-owned stateful precheck
3. signature verification outside the actor
4. actor-owned recheck and state commit

This means:

• mutable validation state is still serialized per MessageID
• duplicate/stale guards still happen before signature verification
• signature verification is no longer serialized with state ownership

[nkryuchkov]

This is intentionally opened as a draft because it changes the validation concurrency model in a critical path. The main purpose is to get team feedback on whether this architecture is desirable before committing to it.

[codecov]

Codecov Report

❌ Patch coverage is 75.19380% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.5%. Comparing base (653f8eb) to head (aeb5a65).

Files with missing lines	Patch %	Lines
message/validation/validation_actor.go	61.4%	25 Missing and 2 partials ⚠️
message/validation/validation.go	84.3%	3 Missing and 2 partials ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

[nkryuchkov]

@greptileai please review this PR

[greptile-apps]

Greptile Summary

This PR replaces the per-MessageID mutex model in the message validation pipeline with a per-MessageID actor/mailbox model, allowing signature verification to run concurrently while keeping all mutable state access serialized. The behavioral change is accurate: cheap stateful checks (duplicate/stale detection) happen before expensive signature verification, and a re-check of those guards runs inside the actor after verification completes before committing state.

Key changes:

• validation_actor.go — new validationActor type with an inbox channel, submit/stop/run/drainPending lifecycle, and a verifyAndResubmit helper that responds with errValidationActorClosed when the actor has been evicted.
• validation.go — validationLockCache / mutex replaced by validationActors TTL cache; OnEviction calls stop() on evicted actors; withValidationActor wires the three-phase (precheck → verify → commit) flow.
• consensus_validation.go / partial_validation.go — validation logic split into precheck, verify, and commit closures routed through the actor.
• validation_actor_test.go — new concurrency tests assert that two in-flight messages for the same key proceed through signature verification in parallel and that exactly one commits; a regression test confirms errValidationActorClosed maps to ValidationIgnore.
• validation_lock_test.go — deleted; its lock-based tests are fully superseded.

The lifecycle mechanics are sound: active tracks in-flight submit() goroutines so stop() can wait for inbox writes to complete before closing stopCh, and drainPending / verifyAndResubmit's fallback path ensure no caller blocks indefinitely. Previous thread concerns about goroutine leaks, the post-drain write race, and errValidationActorClosed classification have all been addressed in this revision.

Confidence Score: 4/5

• PR is safe to merge; the two remaining comments are non-blocking P2 observations about eviction-thread latency and potential metric double-counting.
• All critical lifecycle issues raised in prior review rounds (goroutine leaks, post-drain write race, ValidationIgnore classification) are resolved in this revision. The actor model is correctly implemented and well-tested. Two minor P2 items remain: the synchronous stop() call in the eviction handler (which can briefly hold the eviction thread under load) and the double execution of precheck closures that may emit metrics/logs twice. Neither blocks correctness.
• Pay attention to message/validation/validation_actor.go (double precheck execution side-effects) and message/validation/validation.go (synchronous stop() in eviction callback).

Important Files Changed

Filename	Overview
message/validation/validation_actor.go	New actor implementation. Lifecycle management (stop/submit/drainPending) is well-thought-out; the mutex-protected stopped flag and active counter correctly coordinate shutdown. The double precheck execution in the validationVerified path is intentional but may double-emit metrics/logs.
message/validation/validation.go	Replaces per-MessageID mutex with per-MessageID actor cache. getValidationActor and withValidationActor are clean. The synchronous stop() call in the OnEviction handler is a minor latency concern under load; all other logic looks correct.
message/validation/consensus_validation.go	Cleanly routes consensus validation through the actor's precheck/verify/commit split. Semantic validation still runs before the actor, which is correct.
message/validation/partial_validation.go	Mirrors the consensus refactor for partial-signature messages. The signer variable is correctly captured before entering the actor closure.
message/validation/validation_actor_test.go	New actor-specific concurrency tests. Previous concerns about shared message pointers are addressed — each goroutine receives its own independently constructed message via the newSignedMessage() closure. The TestValidationActorClosedIsIgnored regression test for ValidationIgnore classification is a good addition.
message/validation/validation_lock_test.go	Deleted file — the lock-based concurrency tests it contained are superseded by the new actor tests in validation_actor_test.go.

Sequence Diagram

sequenceDiagram
    participant C as Caller (handleSignedSSVMessage)
    participant VA as withValidationActor
    participant A as validationActor.run()
    participant V as verifyAndResubmit goroutine

    C->>VA: submit validationRequest (precheck, verify, commit)
    VA->>A: inbox ← validationRequest
    A->>A: validatorState(key)
    A->>A: precheck(state)
    alt precheck fails
        A-->>C: respond(err)
    else precheck passes
        A->>V: go verifyAndResubmit(req)
        V->>V: req.verify() [sig verification, concurrent]
        V->>A: inbox ← validationVerified{req, err}
        alt verify failed
            A-->>C: respond(verify err)
        else verify passed
            A->>A: validatorState(key) [re-fetch]
            A->>A: precheck(state) [recheck for duplicates]
            alt recheck fails (e.g. duplicate committed)
                A-->>C: respond(recheck err)
            else recheck passes
                A->>A: commit(state)
                A-->>C: respond(nil)
            end
        end
    end

Loading

_{Reviews (4): Last reviewed commit: "code review comments [3]" | Re-trigger Greptile}

[nkryuchkov]

@greptileai please review this again

[nkryuchkov]

@greptileai please review this again

[nkryuchkov]

@greptileai please review this again

[greptile-apps]

Synchronous stop() in eviction callback may delay TTL eviction thread

stop() blocks until a.active reaches zero, which requires all in-flight submit() goroutines to finish writing to the inbox. In the common case this is fast (inbox is buffered, write is near-instant), but if the inbox is at capacity when eviction fires, stop() will wait for run() to drain at least one slot before the blocked submit() callers can unblock and decrement active.

Calling stop() synchronously in the eviction handler ties up whatever internal TTL-cache goroutine performs eviction for this entire window. Under high load, many concurrent submitters for the evicted actor could measurably delay TTL processing for other MessageIDs.

A simple improvement is to move the call off the eviction thread:

mv.validationActors.OnEviction(func(_ context.Context, _ ttlcache.EvictionReason, item *ttlcache.Item[spectypes.MessageID, *validationActor]) {
    go item.Value().stop()
})

stop() is idempotent (guarded by stopOnce), so calling it asynchronously is safe.

[nkryuchkov]

@greptileai I’m keeping stop() synchronous in the eviction callback for now. This is an eviction-path concern rather than a steady-state path, and actors are only evicted after 2 epochs with Touch() on every lookup, so eviction should be relatively rare. More importantly, keeping stop() inline preserves a simpler actor lifetime model: once eviction runs, shutdown is completed before the callback returns. I don’t think moving it to a goroutine buys enough to justify making actor teardown more implicit.

[greptile-apps]

That's a reasonable call. The 2-epoch TTL with Touch() on lookup means evictions only happen for idle MessageIDs, so the frequency argument holds. And the synchronous teardown does keep the lifetime model clearer - once the eviction callback returns, the actor is fully stopped, no background cleanup to track.

The blocking risk is real but bounded: worst case is waiting for buffered inbox to drain, which should be fast unless the actor is genuinely stuck. If you see eviction delays show up in profiling or metrics later, you can always revisit, but keeping it simple now makes sense.

[greptile-apps]

precheck runs twice per successfully verified message — metrics/logging side-effects would be doubled

In the validationVerified path the actor re-executes msg.request.precheck(state) as a re-check after signature verification. This is the correct behavioral model described in the PR, but it means that any metrics recording or structured log output inside the precheck closure (e.g. inside validateQBFTLogic / validateQBFTMessageByDutyLogic / validatePartialSigMessagesByDutyLogic) will be emitted twice for every message that passes the initial precheck and reaches the commit stage.

If those helper functions currently emit observability data as side-effects, the double execution will inflate counters and produce duplicate log entries. It's worth auditing those callers, or renaming/documenting this parameter to make the double-invocation contract explicit (e.g. calling the second argument recheck rather than precheck).