Skip to content

Conversation

Zacholme7
Copy link
Member

@Zacholme7 Zacholme7 commented Sep 4, 2025

Issue Addressed

This pulls out the ssz infrastructure in #493 into its own PR. Functionally, this should change nothing but it enables hashing for the checks required in the spec tests

This uses new functions to_variable_list and to_variable_list_with_error which both take the length as a generic to check the upper bound of the data being validated. Allows us to use From while also ensuring that we reject invalid length data.

This is the base PR for all of the spec test as we must hash the messages for output comparison against go-ssv

This was referenced Sep 4, 2025
@Zacholme7 Zacholme7 marked this pull request as ready for review September 4, 2025 14:26
@Zacholme7 Zacholme7 self-assigned this Sep 4, 2025
@Zacholme7 Zacholme7 added enhancement New feature or request ready-for-review This PR is ready to be reviewed labels Sep 4, 2025
Copilot

This comment was marked as outdated.

}
}

impl TreeHash for PartialSignatureKind {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't it be derived?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I think I suggested adding support for cases like this a while ago, need to check

// Pad signature to RSA_SIGNATURE_SIZE if needed
let padded_signature = if signature.len() < RSA_SIGNATURE_SIZE {
let mut padded = vec![0; RSA_SIGNATURE_SIZE];
let padded_signature: [u8; RSA_SIGNATURE_SIZE] = if signature.len() < RSA_SIGNATURE_SIZE {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about pub type RsaSignature = [u8; RSA_SIGNATURE_SIZE];?

Copy link
Member

@diegomrsantos diegomrsantos Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for creating a type alias for [u8; RSA_SIGNATURE_SIZE]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but this was the only place we explicitly defining the type and the compiler can infer it so its not needed here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use it in other places too. But we also do vec![[0u8; RSA_SIGNATURE_SIZE]] which is actually defining elements instead of a type

Copy link
Member

@diegomrsantos diegomrsantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side, I'm good with it. Sorry for the lengthy review. As I'm not that familiar with the code, it's better if @dknopik can give it a final review.

@dknopik
Copy link
Member

dknopik commented Sep 18, 2025

Will review once tomorrow's post-release backmerge has been merged here

Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you so much!

@dknopik dknopik added ready-for-merge and removed ready-for-review This PR is ready to be reviewed labels Sep 22, 2025
@mergify mergify bot merged commit b4660eb into sigp:unstable Sep 22, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ready-for-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants