Signature verification by diegomrsantos · Pull Request #215 · sigp/anchor

diegomrsantos · 2025-03-28T13:30:40Z

Issue Addressed

Proposed Changes

This pull request adds signature verification and refactors the validation logic by introducing a centralized ValidationContext and updates several message validation functions and tests to use this new context. Key changes include:

Replacing multiple function parameters with a single ValidationContext struct in partial signature and consensus message verification.
Integrating OpenSSL-based signature verification into the validation flows.
Updating tests to reflect the new ValidationContext usage and enhanced signature verification.

Copilot

Pull Request Overview

This PR refactors the SSV message validation flow by introducing a new ValidationContext and integrating signature verification using OpenSSL. Key changes include:

Replacing multiple function parameters with a unified ValidationContext across validation functions.
Adding signature verification logic using OpenSSL in the consensus message validation.
Updating tests and documentation to align with the new ValidationContext approach.

Reviewed Changes

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

Show a summary per file

File	Description
anchor/message_validator/src/partial_signature.rs	Refactored usage of parameters to use ValidationContext in partial signature validation.
anchor/message_validator/src/lib.rs	Introduced the ValidationContext struct and updated operator public key retrieval.
anchor/message_validator/src/consensus_state.rs	Enhanced documentation and minor refactoring in operator and signer state handling.
anchor/message_validator/src/consensus_message.rs	Integrated ValidationContext into consensus message validation and added signature verification using OpenSSL.
anchor/message_validator/Cargo.toml	Added OpenSSL dependency and cleaned up dev-dependencies.

Comments suppressed due to low confidence (2)

anchor/message_validator/src/consensus_state.rs:205

Review the use of 'into()' on the slice of OperatorId; ensure that the conversion to the type used in 'seen_signers' is correct and functions as expected.

self.seen_signers.contains(&operators.into())

anchor/message_validator/src/consensus_message.rs:267

[nitpick] Consider aligning the naming of the parameter 'operators_pks' with the field 'operators_pk' in ValidationContext for consistency.

if signatures.len() != operators_pks.len() {

# Conflicts: # anchor/client/src/config.rs # anchor/message_validator/src/consensus_message.rs # anchor/message_validator/src/consensus_state.rs # anchor/message_validator/src/lib.rs # anchor/message_validator/src/partial_signature.rs # anchor/network/src/network.rs # anchor/validator_store/src/lib.rs

Copilot

Pull Request Overview

This pull request refactors the signature verification logic by introducing a centralized ValidationContext and updates several message validation functions and tests to use this new context. Key changes include:

Replacing multiple function parameters with a single ValidationContext struct in partial signature and consensus message verification.
Integrating OpenSSL-based signature verification into the validation flows.
Updating tests to reflect the new ValidationContext usage and enhanced signature verification.

Reviewed Changes

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

Show a summary per file

File	Description
anchor/message_validator/src/partial_signature.rs	Refactored validation functions to use the new ValidationContext.
anchor/message_validator/src/lib.rs	Introduced helper functions for signature verification using OpenSSL.
anchor/message_validator/src/consensus_state.rs	Updated documentation and minor code improvements.
anchor/message_validator/src/consensus_message.rs	Updated consensus message validation to use ValidationContext; added signature verification.
anchor/message_validator/Cargo.toml	Added OpenSSL dependency and removed unused dev-dependencies.

Comments suppressed due to low confidence (1)

anchor/message_validator/src/lib.rs:363

Consider including additional context, such as the operator ID if available, in the 'Signature verification failed' error message to assist in debugging.

        Ok(false) => Err(ValidationFailure::SignatureVerificationFailed {

Zacholme7

small comments, otherwise looking good!

Zacholme7 · 2025-04-01T21:00:25Z

anchor/message_validator/src/lib.rs

    Processor(#[from] ::processor::Error),
 }

+struct ValidationContext<'a> {


Are the borrows here just to reduce some clone overheads?

Exactly, wdyt?

Zacholme7 · 2025-04-01T21:09:51Z

anchor/message_validator/src/lib.rs

        }
    }

+    fn get_operator_pks(


nit: I would probably extract this into a function in state.rs instead of keeping it here.

why? It's not related to the consensus state

I meant state.rs in the database as this function deals with fetching state. But if this is the only place this is needed then there is not much point in moving it I suppose

I see, yeah it's used only here

dknopik · 2025-04-02T09:28:05Z

anchor/message_validator/src/lib.rs

-    signed_ssv_message: &SignedSSVMessage,
-    committee_info: &CommitteeInfo,
-    role: Role,
+    validation_context: &ValidationContext,


I like that the ValidationContext borrows its contents, but borrowing again here basically creates a double indirection. Wdyt about passing the context by value?

Or, alternatively, having the Context own the values and passing around a reference, which is maybe even better.

Good catch! But if it owns the values, we'll have to copy it when creating the struct. Is it necessary?

Moving the values once might have better performance than having one additional level of pointer indirection during the whole of validation. But I don't know. We can also go ahead with my first suggestion if you prefer that.

Now I remember that the reason I passed it as a ref is to avoid it being moved. Doesn't the compiler optimize away the indirections?

How would it?

If I move it to a function, it can't be used anymore

Ah, you mean the indirections. The contents of the struct aren’t mutated anywhere, so there’s no reason to keep loading them from memory many times.

diegomrsantos · 2025-04-02T15:06:11Z

@dknopik @Zacholme7 is there anything pending?

dknopik · 2025-04-02T15:08:30Z

Gonna test it on interop tomorrow, after that I think we are good to go imo

dknopik

lgtm

diegomrsantos force-pushed the signature-verification branch 3 times, most recently from 1a02e0e to 25b3396 Compare March 28, 2025 17:42

diegomrsantos added 3 commits March 31, 2025 16:34

create ValidationContext

3b1cdd2

verify sig for consensus messages

ab36341

fix verification and add tests

0e59810

diegomrsantos force-pushed the signature-verification branch from 25b3396 to 0e59810 Compare March 31, 2025 14:34

improve comments

4dc9693

diegomrsantos self-assigned this Apr 1, 2025

diegomrsantos added the validation label Apr 1, 2025

diegomrsantos requested a review from Copilot April 1, 2025 08:29

Copilot AI reviewed Apr 1, 2025

View reviewed changes

diegomrsantos added 4 commits April 1, 2025 11:21

move generate_random_rsa_public_keys to tests

0489a93

move generate_random_rsa_public_keys to tests

48e113d

move verify_message_signatures

efdc8c6

refactor verify_message_signatures

ec34b0f

diegomrsantos force-pushed the signature-verification branch from d7f0caa to ec34b0f Compare April 1, 2025 14:24

diegomrsantos and others added 4 commits April 1, 2025 16:33

format with rustfmt

9f644ab

fix tests

f0384b4

Merge branch 'unstable' into signature-verification

eb313f6

diegomrsantos marked this pull request as ready for review April 1, 2025 16:50

diegomrsantos requested a review from Copilot April 1, 2025 16:50

Copilot AI reviewed Apr 1, 2025

View reviewed changes

diegomrsantos requested review from Zacholme7, dknopik and jking-aus April 1, 2025 16:51

remove duplication in tests

eb329b9

Zacholme7 reviewed Apr 1, 2025

View reviewed changes

dknopik reviewed Apr 2, 2025

View reviewed changes

remove some indirections

74f812b

diegomrsantos requested review from Zacholme7 and dknopik April 2, 2025 14:30

diegomrsantos added the ready-for-review This PR is ready to be reviewed label Apr 2, 2025

dknopik approved these changes Apr 3, 2025

View reviewed changes

dknopik merged commit 1c495f7 into sigp:unstable Apr 3, 2025
11 checks passed

Comments

Conversation

diegomrsantos commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Zacholme7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos commented Apr 2, 2025

Uh oh!

dknopik commented Apr 2, 2025

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diegomrsantos commented Mar 28, 2025 •

edited

Loading