message/validation: move signature verification outside validation lock - #2728
message/validation: move signature verification outside validation lock#2728nkryuchkov wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR moves RSA signature verification outside the per- Key changes:
The main correctness concern is the ordering inversion: cheap stateful guards (e.g., The consensus regression test ( Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant P as Peer
participant MV as messageValidator
participant SV as signatureVerifier
participant L as ValidationLock (per MessageID)
participant S as ValidatorState
P->>MV: handleSignedSSVMessage()
MV->>MV: validateSignedSSVMessage() [outside lock]
MV->>MV: validateSSVMessage() [outside lock]
MV->>MV: getCommitteeAndValidatorIndices() [outside lock]
MV->>MV: committeeChecks() [outside lock]
alt SSVConsensusMsgType
MV->>MV: decodeMessage + validateConsensusMessageSemantics() [outside lock]
MV->>SV: verifyConsensusMessageSignatures() [outside lock, PARALLEL possible]
SV-->>MV: ok / ErrSignatureVerification
MV->>L: withValidationLock(MessageID)
L-->>MV: lock acquired
MV->>S: validatorState()
MV->>MV: validateQBFTLogic() [inside lock]
MV->>MV: validateQBFTMessageByDutyLogic() [inside lock]
MV->>S: updateConsensusState() [inside lock]
MV->>L: unlock
else SSVPartialSignatureMsgType
MV->>MV: decode + validatePartialSignatureMessageSemantics() [outside lock]
MV->>SV: verifyPartialSignatureMessageSignature() [outside lock, PARALLEL possible]
SV-->>MV: ok / ErrSignatureVerification
MV->>L: withValidationLock(MessageID)
L-->>MV: lock acquired
MV->>S: validatorState()
MV->>MV: validatePartialSigMessagesByDutyLogic() [inside lock]
MV->>S: updatePartialSignatureState() [inside lock]
MV->>L: unlock
end
MV-->>P: decoded message / error
Last reviewed commit: "remove redundant com..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai please review it again |
| if err := mv.verifyConsensusMessageSignatures(signedSSVMessage); err != nil { | ||
| return consensusMessage, err | ||
| } | ||
|
|
||
| if err := mv.validateQBFTMessageByDutyLogic(signedSSVMessage, consensusMessage, committeeInfo, receivedAt, state); err != nil { | ||
| return consensusMessage, err | ||
| } | ||
| if err := mv.withValidationLock(ssvMessage.GetID(), func() error { | ||
| state := mv.validatorState(ssvMessage.GetID(), committeeInfo) | ||
|
|
||
| for i := range signedSSVMessage.Signatures { | ||
| operatorID := signedSSVMessage.OperatorIDs[i] | ||
| signature := signedSSVMessage.Signatures[i] | ||
| if err := mv.validateQBFTLogic(signedSSVMessage, consensusMessage, committeeInfo, receivedFrom, receivedAt, state); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := mv.signatureVerifier.VerifySignature(operatorID, ssvMessage, signature); err != nil { | ||
| e := ErrSignatureVerification | ||
| e.innerErr = fmt.Errorf("verify opid: %v signature: %w", operatorID, err) | ||
| return consensusMessage, e | ||
| if err := mv.validateQBFTMessageByDutyLogic(signedSSVMessage, consensusMessage, committeeInfo, receivedAt, state); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if err := mv.updateConsensusState(signedSSVMessage, consensusMessage, committeeInfo, receivedFrom, state); err != nil { | ||
| return mv.updateConsensusState(signedSSVMessage, consensusMessage, committeeInfo, receivedFrom, state) | ||
| }); err != nil { |
There was a problem hiding this comment.
The ordering inversion means that legitimately-signed but stale/duplicate messages now incur full RSA signature verification before being rejected by the cheap stateful checks inside the lock (e.g. ErrRoundAlreadyAdvanced, ErrDuplicatedMessage, ErrDecidedMessageWithTooFewSigners).
In the old code a replayed, round-old, or duplicate message would be rejected immediately inside the lock without ever touching the crypto path. Under the new ordering, any message that carries a valid RSA signature can force O(signers) RSA verifications before the cheap stateful rejection fires. Because an attacker only needs to replay previously-seen, legitimately-signed messages (no forgery required), this creates a more efficient CPU-exhaustion path than before.
The PR description acknowledges this ("Cheap stateful rejections may still happen after signature verification"), but it may be worth considering whether a subset of the cheapest stateful checks (e.g. ErrRoundAlreadyAdvanced) could be moved back before signature verification to provide an inexpensive early-exit for replay traffic, without reintroducing lock contention for the common case.
There was a problem hiding this comment.
but it may be worth considering whether a subset of the cheapest stateful checks (e.g.
ErrRoundAlreadyAdvanced) could be moved back before signature verification to provide an inexpensive early-exit for replay traffic, without reintroducing lock contention for the common case.
TBH I don't think this is worth the added code complexity.
| func TestConsensusSignatureVerificationOutsideValidationLock(t *testing.T) { | ||
| env := newValidationLockTestEnv(t) | ||
|
|
||
| slot := env.netCfg.FirstSlotAtEpoch(1) | ||
| signedSSVMessage := generateSignedMessage(env.ks, env.committeeIdentifier, slot) | ||
| topicID := commons.CommitteeTopicID(env.committeeID)[0] | ||
| peerID, err := libp2ptest.RandPeerID() | ||
| require.NoError(t, err) | ||
|
|
||
| validationMu := env.validator.getValidationLock(signedSSVMessage.SSVMessage.GetID()) | ||
| validationMu.Lock() | ||
| locked := true | ||
| defer func() { | ||
| if locked { | ||
| validationMu.Unlock() | ||
| } | ||
| }() | ||
|
|
||
| done := make(chan error, 1) | ||
| go func() { | ||
| _, err := env.validator.handleSignedSSVMessage(signedSSVMessage, topicID, peerID, env.netCfg.SlotStartTime(slot)) | ||
| done <- err | ||
| }() | ||
|
|
||
| select { | ||
| case <-env.validator.signatureVerifier.(*observingSignatureVerifier).called: | ||
| case <-time.After(time.Second): | ||
| t.Fatal("signature verification did not start while the validation lock was held") | ||
| } | ||
|
|
||
| select { | ||
| case err := <-done: | ||
| t.Fatalf("validation completed before the lock was released: %v", err) | ||
| default: | ||
| } | ||
|
|
||
| validationMu.Unlock() | ||
| locked = false | ||
|
|
||
| select { | ||
| case err := <-done: | ||
| require.NoError(t, err) | ||
| case <-time.After(time.Second): | ||
| t.Fatal("validation did not complete after the lock was released") | ||
| } |
There was a problem hiding this comment.
ErrSignerNotLeader can silently break the consensus test
generateSignedMessage always creates a ProposalMsgType message signed by operator 1. Once the lock is released, validateQBFTLogic checks whether operator 1 is actually the round-robin leader at that height/round:
leader := mv.roundRobinProposer(consensusMessage.Height, consensusMessage.Round, committeeInfo.committee)
if signedSSVMessage.OperatorIDs[0] != leader {
return ErrSignerNotLeader
}roundRobinProposer computes:
firstRoundIndex = height % len(committee) // height = FirstSlotAtEpoch(1) = SlotsPerEpoch
index = (firstRoundIndex + round - 1) % len(committee)
For the test to succeed, index must be 0 (so that committee[0] = 1 is returned). With Testing4SharesSet (len = 4) and TestNetwork.SlotsPerEpoch = 32, the arithmetic works out (32 % 4 == 0), but this relies on an implicit relationship between the slot chosen in the test and the committee size.
If the test slot or committee size changes the modular arithmetic fails, ErrSignerNotLeader is returned from inside the lock, and require.NoError(t, err) at line 171 fails — but the test already proved its structural correctness (the called channel was signalled and the goroutine was blocked before the lock). The assertion at line 171 is testing something unrelated to the lock behaviour and couples the test to unrelated scheduling logic.
Consider either using a CommitMsgType message (no leader check), or computing the expected leader explicitly and signing with that operator's key.
There was a problem hiding this comment.
LGTM, I would also ask @MatheusFranco99 to take a look (specifically at this concern AI bot raised) ... IMO this PR doesn't pose much of an attack-danger because we REJECT (not INGORE) those invalid-signature messages and duplicate-peer messages - so an attacker won't be able to "sustain" his attack for a meaningfully long period of time that would result in issues for SSV node in practice.
Staging Test Report
Results
Fix VerificationThe PR splits the validation flow: signature verification (CPU-intensive RSA ops) now runs before acquiring the per-MessageID validation lock, while stateful checks (QBFT logic, duty logic, validator state) remain under the lock via the new 🤖 Tested with SSV Scout automated staging deployment |
UPD: I closed #2736 because I don't like that solution |
|
Converting to draft until we make sure we need it |
| return nil | ||
| } | ||
|
|
||
| func (mv *messageValidator) verifyConsensusMessageSignatures(signedSSVMessage *spectypes.SignedSSVMessage) error { |
There was a problem hiding this comment.
The board finding (F-ssv-173) recommended three optimizations:
- Move sig verification outside the lock — this PR
- Verify multiple signatures in parallel for decided messages (up to 13 sigs × ~1.5ms = ~20ms sequential) — not addressed
- Cache recently verified (operatorID, messageHash, signature) tuples to skip re-verification on retransmitted messages — not addressed
This PR reduces lock hold time but doesn't reduce the total CPU cost of verification itself. A decided message with 4 signatures still takes ~6ms of sequential RSA verification before acquiring the lock. Consider whether 2 (parallel verification in verifyConsensusMessageSignatures via a goroutine pool) should be part of this PR or tracked as a follow-up.
There was a problem hiding this comment.
(2) and (3) depend on (1), so I want to decide on (1) first before continuing. It moves the expensive signature verification before the light checks, which might be worse than keeping the lock
Once we decide on it, (2) and (3) will be ready for implementation
There was a problem hiding this comment.
3. Cache recently verified (operatorID, messageHash, signature) tuples to skip re-verification on retransmitted messages — not addressed
libp2p probably does some basic de-duplication for us already (not sure if by messageHash or somehow else) ? Probably want to check if our own caching will even catch any such duplicates before proceeding to it
| state := mv.validatorState(ssvMessage.GetID(), committeeInfo) | ||
|
|
||
| if err := mv.validateQBFTLogic(signedSSVMessage, consensusMessage, committeeInfo, receivedFrom, receivedAt, state); err != nil { | ||
| if err := mv.verifyConsensusMessageSignatures(signedSSVMessage); err != nil { |
There was a problem hiding this comment.
What do you think about per-peer CPU amplification here? I fear a single peer could send multiple structurally-different messages with the same MsgID (varying Round, FullData, etc.) that all pass semantic validation and enter verifyConsensusMessageSignatures in parallel before the per-peer SeenMsgTypes limits kick in inside the lock. Pre-PR the outer lock capped simultaneous RSA work per MsgID at 1; now it's bounded only by libp2p's validation-worker pool. Maybe worth a follow-up: a cheap pre-verify per-peer counter keyed on (MsgID, peerID), or rely on topic scoring — could be bundled with the F-ssv-173 (2)/(3) follow-ups. Same applies to partial_validation.go verify path.
| }() | ||
|
|
||
| select { | ||
| case <-env.validator.signatureVerifier.(*observingSignatureVerifier).called: |
There was a problem hiding this comment.
Worth considering whether this test actually proves the parallel-verify claim? Seems that it asserts verify runs while the test goroutine externally holds the lock, but the operational value of the PR is that two handler goroutines for the same MsgID can both be in verify simultaneously. A test that spawns two handleSignedSSVMessage goroutines (no external lock) and asserts both reach VerifySignature before either records state would be a stronger regression guard. Could be a follow-up alongside the F-ssv-173 items.
| } | ||
|
|
||
| func (mv *messageValidator) verifyPartialSignatureMessageSignature(signedSSVMessage *spectypes.SignedSSVMessage) error { | ||
| signature := signedSSVMessage.Signatures[0] |
There was a problem hiding this comment.
Might be worth a defensive len guard or a short comment here? The [0] indexing is safe today because validateSignedSSVMessage (len≥1, equal arities) and validatePartialSignatureMessageSemantics (len==1) run before verify. A reorder in a future refactor would silently break it — a one-line // semantics above guarantees len==1 makes the invariant explicit.
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if there are no updates. Please comment if you would like to keep it open. |
5 participants
Summary
Move message signature verification out of the per-
MessageIDvalidation critical section.Previously, incoming consensus and partial-signature messages were serialized under the same per-
MessageIDmutex for the full validation flow, including RSA signature verification. This meant messages for the same validator/role could queue behind each other while spending time in crypto verification.This change keeps the existing stateful validation and state update serialized, but performs signature verification before acquiring the validation lock. That reduces lock hold time and allows signature verification to proceed in parallel for messages sharing the same
MessageID.What Changed
MessageIDvalidation lock is heldWhy
This reduces contention in the message validation pipeline during active consensus, where multiple operators send messages nearly simultaneously for the same committee/role. Expensive signature verification no longer blocks other messages from progressing to their own signature checks.
Behavioral Notes