eth/eventhandler: classify remote-signer share decryption errors as malformed - #2997
Open
iurii-ssv wants to merge 3 commits into
Open
eth/eventhandler: classify remote-signer share decryption errors as malformed#2997iurii-ssv wants to merge 3 commits into
iurii-ssv wants to merge 3 commits into
Conversation
…alformed
A validator share that the signer cannot decrypt should be treated as a
malformed registry event (logged and skipped), exactly as the local key
manager already handles it. In remote-signing mode it was not:
ssvsigner declared ShareDecryptionError as an interface alias of error
(type ShareDecryptionError error), so the value the client returned on an
HTTP 422 kept its underlying *errors.errorString dynamic type. The event
handler classifies decryption failures with
errors.As(err, &ekm.ShareDecryptionError{}) against a struct type, which never
matched the remote error, so a share that failed to decrypt was handled as a
non-malformed (fatal) error instead of a skippable malformed event.
Consolidate the two identically-named ShareDecryptionError types into a single
concrete struct in the ssvsigner package, used by the client, both key managers
and the event handler, so decryption failures are classified consistently in
local and remote signing modes.
Tests: replace the ShareDecryptionError errors.As assertions that passed for
any error (the type was an interface) with ones that distinguish a decryption
error from other failures, and add coverage for the remote client -> AddShare
path.
Contributor
Greptile SummaryThe PR consolidates share-decryption failures into a concrete error type so local and remote key managers classify malformed validator events consistently.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| ssvsigner/server.go | Narrows HTTP 422 to malformed-share failures and maps internal keystore and downstream Web3Signer failures to retryable HTTP 500 responses. |
| ssvsigner/client.go | Converts add-validator HTTP 422 responses into the concrete share-decryption error while leaving other statuses on the ordinary error path. |
| ssvsigner/types.go | Replaces the error-interface alias with a concrete wrapping error that supports precise errors.As classification. |
| eth/eventhandler/handlers.go | Recognizes the shared concrete decryption error and converts it into a skippable malformed-event result. |
| ssvsigner/ekm/local_key_manager.go | Uses the shared signer-package error type for local share decryption and validation failures. |
| ssvsigner/ekm/remote_key_manager.go | Preserves the concrete decryption error through wrapping so the event handler can classify remote-signer failures correctly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
E[ValidatorAdded event] --> N[SSV node]
N --> S[ssv-signer add-validator endpoint]
S --> V{Share decrypts and validates?}
V -->|No| U[HTTP 422]
U --> D[ShareDecryptionError]
D --> M[Malformed event: log and skip]
V -->|Yes| W[Import into Web3Signer]
W --> I{Internal or downstream failure?}
I -->|Yes| F[HTTP 500]
F --> R[Retryable event-handler error]
I -->|No| O[Share registered]
Reviews (3): Last reviewed commit: "ssvsigner: tighten 422 usage and error h..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The add-validator endpoint returned 422 both for shares that cannot be decrypted/validated and for a random-password-generation failure. The node maps 422 to a malformed registry event (logged and skipped), so an internal password-generation error would drop a valid ValidatorAdded event instead of being retried. Return 500 for the password-generation failure (an internal, transient error) and keep 422 exclusively for shares that cannot be decrypted or validated. Document the 422 contract on both the server and the client.
Contributor
Author
|
@greptile pls re-review |
Server: - Reserve HTTP 422 for malformed shares only. Keystore generation and marshalling failures in keystoreJSONFromEncryptedShare happen after the share is validated, so they are internal (retryable) errors: tag them with errInternalKeystore and reply 500, matching the earlier password-generation fix. - handleWeb3SignerErr always replies 500 and no longer forwards Web3Signer's upstream status. The previous errors.As never matched (Web3Signer returns a value-typed HTTPResponseError, the check targeted a pointer), so this is behaviour-preserving; the comment now documents that a forwarded upstream 422 must not reach the node, which would misclassify it as a malformed share. Client: - Keep the transport error alongside the 422 body so a bodyless 422 still carries context. Docs: - Update the stale RemoteKeyManager.AddShare comment: a malformed share is now skipped (not crash-and-retried). Tests: - Replace the hand-rolled classification test with one that drives the real handleValidatorAdded path: a ValidatorAdded event with a valid signature but an undecryptable share must be classified as a MalformedEventError. - Enforce the slashing-protection mock expectations in the AddShare error subtests. - Use require.ErrorAs/NotErrorAs and add a 400-status row so the "non-422 must not classify" assertion exercises the 4xx boundary.
Contributor
Author
|
@greptile pls re-review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A validator share that the signer cannot decrypt should be treated as a malformed registry event (logged and skipped), exactly as the local key manager already does. In remote-signing mode it wasn't:
ssvsignerdeclaredShareDecryptionErroras an interface alias oferror, so the value the client returned on an HTTP 422 kept its underlying*errors.errorStringdynamic type, and the event handler'serrors.As(err, &ekm.ShareDecryptionError{})(a struct type) never matched. A share that failed to decrypt was therefore handled as a non-malformed error instead of a skippable malformed event, inconsistently with local signing.Change
ShareDecryptionErrortypes into a single concrete struct in thessvsignerpackage, used by the client, both key managers, and the event handler, so decryption failures are classified consistently across local and remote signing modes.Tests
ShareDecryptionErrorerrors.Asassertions that passed for any error (the type was an interface) with ones that distinguish a decryption error from other failures.AddSharepath.Compatibility
The classification fix is node-side and works with any ssv-signer version (it relies only on the existing HTTP 422 contract). The server-side 422 tightening is independent and degrades gracefully, so nodes and ssv-signer can still be upgraded independently.