Address BLST audit

[dknopik]

Issue Addressed

After an internal audit, one major and several minor issues were found in our lagrange interpolation implementation using blst. Recall that this implementation was not in active use until now.

Proposed Changes

I will go over the issues found and the solution implemented for it.

Misuse of blst_scalar_from_uint64 🌶️

blst_scalar_from_uint64 reads four u64 instead of one (as assumed by the author). This caused the From and TryFrom implementations to read additional memory after the passed u64. This was not caught before as the crate was never used productively, and the tests triggered this in a way where the IDs ended up the same regardless, causing no apparent issue.

We now instead use blst_scalar_from_le_bytes, which allows us to explicitly choose the amount of bytes to read. Fixed in commit 25bb849.

Rename keys and msk

These variable names are unclear: while "keys" corresponds to the type, they are not actually used as keys, but coefficients in our polynomial. msk is an unclear acronym.

In commit ab74ae0, keys has been renamed to random_coefficients, and msk has been renamed to coefficients.

Check for zero input key

The coefficients must not be zero. This includes the secret as constant coefficient, and the random coefficients.

The random coefficients can not be zero, as blsts key_gen ensures that it does not generate a zero key. While it difficult for a user to even create a zero key to be split, a check was added in commit 3b72be2 to be safe.

Initialise scratch memory

We create some scratch space memory for use in blsts implementation of Pippenger's algorithm. While the memory is allowed to be uninitialised as a raw pointer is passed, we switch to initialising the Vec instead of just allocating it in commit 42ba302 for clarity.

Additional Info

Now that we are audited, this PR switches to use the single thread blst implementation by default for now, and remove the warning. Some additional error case tests have been added.

[kirk-baird]

Looks good to me.

Just 2 nit picks which you can take or leave, just a bit of a coding style.

[Copilot]

Pull Request Overview

This PR addresses security and clarity issues in the BLST‑based lagrange interpolation implementation, following an internal audit. Key changes include correcting the use of blst’s scalar conversion function, renaming variables to better reflect their roles, and adding explicit checks for zero keys and invalid thresholds.

Reviewed Changes

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

File	Description
anchor/common/bls_lagrange/src/lib.rs	Added a new error variant and updated tests to cover zero key and signature issues.
anchor/common/bls_lagrange/src/blst.rs	Switched to using blst_scalar_from_le_bytes and improved scratch memory initialization.
anchor/common/bls_lagrange/src/blsful.rs	Revised secret key extraction and error handling for signatures and secrets.
anchor/common/bls_lagrange/Cargo.toml	Changed the default feature from blsful to blst_single_thread.

Comments suppressed due to low confidence (1)

anchor/common/bls_lagrange/src/blsful.rs:60

• [nitpick] Consider replacing the numeric check with a clear boolean check (e.g., if scalar.is_zero() { ... }) to improve readability and reduce potential confusion around the unwrap_u8() call.

if scalar.is_zero().unwrap_u8() != 0 {

[dknopik]

@jking-aus I think you should take a look at this, especially as it sets blst as the active implementiation

[jking-aus]

Thanks for the audit kirk! And Daniel, must feel good to get that warning out of codebase.