Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

curiecrypt · 2025-02-19T19:17:50Z

Content

If there is any identity element in the following vectors:

let transmuted_vks: Vec<blst_p2> = vks.iter().map(vk_from_p2_affine).collect();
let transmuted_sigs: Vec<blst_p1> = signatures.iter().map(sig_to_p1).collect();

The content of the following vectors in mithril-stm/src/multi_sig.rs:

let grouped_vks = p2_affines::from(transmuted_vks.as_slice());
let grouped_sigs = p1_affines::from(transmuted_sigs.as_slice());

Becomes vectors full of identity elements.

This PR includes the changes to avoid having an identity element in signature and verification lists.

In mithril-stm/src/multi_sig.rs:

Signature::verify function is updated: If signature is an infinity value, it returns an error.
VerificationKeyPoP::check function is updated: If the verification key is an infinity value, it returns an error.
test_infinity_sig test is added.
test_infinity_vk test is added.
test_keyreg_with_infinity_vk test is added.

In mithril-stm/src/error.rs:

MultiSignatureError is updated to cover
- SignatureInfinity
- VerificationKeyInfinity
impl From<MultiSignatureError> for StmSignatureError is updated.
impl<D: Digest + FixedOutput> From<MultiSignatureError> for StmAggregateSignatureError<D> is updated.
impl From<MultiSignatureError> for CoreVerifierError is updated.
pub(crate) fn blst_err_to_mithril is updated.

Pre-submit checklist

Branch
- Tests are provided (if possible)
- Crates versions are updated (if relevant)
- CHANGELOG file is updated (if relevant)
- Commit sequence broadly makes sense
- Key commits have useful messages
PR
- No clippy warnings in the CI
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested
Documentation
- Update README file (if relevant)
- Update documentation website (if relevant)
- Add dev blog post (if relevant)

Issue(s)

Closes #2321

github-actions · 2025-02-19T19:25:48Z

Test Results

3 files ± 0 55 suites +3 11m 31s ⏱️ + 1m 7s
1 686 tests + 41 1 686 ✅ + 41 0 💤 ±0 0 ❌ ±0
2 078 runs +149 2 078 ✅ +149 0 💤 ±0 0 ❌ ±0

Results for commit cf63d31. ± Comparison against base commit 01e15ab.

This pull request removes 48 and adds 89 tests. Note that renamed tests count towards both.

mithril-aggregator ‑ database::repository::epoch_settings_store::tests::replace_cardano_signing_config_empty_values_updates_only_empty_values
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::get_certificate_pending_with_existing_certificate
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::get_certificate_pending_with_no_existing_certificate
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::remove_certificate_pending
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::save_certificate_pending_once
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::should_migrate_data_from_adapter
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::update_certificate_pending
mithril-aggregator ‑ http_server::routes::certificate_routes::tests::test_certificate_pending_get_ko_500
mithril-aggregator ‑ http_server::routes::certificate_routes::tests::test_certificate_pending_with_content_get_ok_200
mithril-aggregator ‑ http_server::routes::certificate_routes::tests::test_certificate_pending_without_content_get_ok_204
…

mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::ancillary::tests::should_compute_the_size_of_the_ancillary
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::digest_artifact_builder_return_size_of_digest_file
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::immutable::tests::should_compute_the_average_size_of_the_immutables
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::immutable::tests::should_return_an_error_when_compute_the_average_size_of_the_immutables_with_zero
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_file_size
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_file_with_file_name_starting_with_a_folder_name
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_file_with_folder_name_starting_with_a_file_name
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_folder_size
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_multi_folders_size
mithril-aggregator ‑ artifact_builder::utils::tests::test_compute_multiple_files_size
…

♻️ This comment has been updated with latest results.

mithril-stm/src/multi_sig.rs

mithril-stm/src/error.rs

Co-authored-by: Jean-Philippe Raynaud <[email protected]>

mithril-stm/src/error.rs

mithril-stm/src/multi_sig.rs

jpraynaud · 2025-03-07T17:20:47Z

mithril-stm/src/multi_sig.rs

+                let verify_aggregate = Signature::verify_aggregate(&msg, &mvks, &sigs);
+                assert!(verify_aggregate.is_ok(), "Aggregate verification {verify_aggregate:?}");
+
+                match Signature::aggregate(&mvks, &sigs) {
+                    Ok((agg_vk, agg_sig)) => {
+                        batch_msgs.push(msg.to_vec());
+                        batch_vk.push(agg_vk);
+                        batch_sig.push(agg_sig);
+                    }
+                    Err(MultiSignatureError::AggregateSignatureInvalid) => {
+                        println!("Aggregation failed.");
+                    }
+                    _ => unreachable!(),
+                }
            }
-            assert!(Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig).is_ok());
+            let batch_verify_aggregates = Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig);
+            assert!(batch_verify_aggregates.is_ok(), "{batch_verify_aggregates:?}");


This should be also rolled back as we have discussed.

jpraynaud · 2025-03-07T17:37:43Z

mithril-stm/src/multi_sig.rs

+        match self.0.validate(true) {
+            Ok(_) => blst_err_to_mithril(
+                self.0.verify(false, msg, &[], &[], &mvk.0, false),
+                Some(*self),
+                None,
+            ),
+            Err(e) => blst_err_to_mithril(e, Some(*self), None),
+        }


Maybe this would be more readable:

Suggested change

match self.0.validate(true) {

Ok(_) => blst_err_to_mithril(

self.0.verify(false, msg, &[], &[], &mvk.0, false),

Some(*self),

None,

),

Err(e) => blst_err_to_mithril(e, Some(*self), None),

}

blst_err_to_mithril(

self.0.validate(true).map_or_else(

|e| e,

|_| self.0.verify(false, msg, &[], &[], &mvk.0, false),

),

Some(*self),

None,

)

error handling done for inf values of sigs and vks

8858065

curiecrypt changed the title ~~error handling done for inf values of sigs and vks~~ Fix BLST bindings: Error handling for infinite values of sigs and vks Feb 19, 2025

curiecrypt temporarily deployed to testing-preview February 19, 2025 19:26 — with GitHub Actions Inactive

tests added

f36b695

curiecrypt temporarily deployed to testing-preview February 19, 2025 20:28 — with GitHub Actions Inactive

update crates version

90d7e2a

curiecrypt marked this pull request as ready for review February 19, 2025 20:35

curiecrypt temporarily deployed to testing-preview February 19, 2025 20:44 — with GitHub Actions Inactive

jpraynaud reviewed Feb 20, 2025

View reviewed changes

mithril-stm/src/multi_sig.rs Outdated Show resolved Hide resolved

mithril-stm/src/error.rs Outdated Show resolved Hide resolved

Update mithril-stm/src/error.rs

3f622c7

Co-authored-by: Jean-Philippe Raynaud <[email protected]>

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

curiecrypt added 2 commits February 20, 2025 16:30

key as input of blst_err_to_mithril

6f4f4a8

check expected error

1fc1077

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

mithril-stm/src/multi_sig.rs Fixed Show fixed Hide fixed

correct box usage for errors

d39e877

curiecrypt temporarily deployed to testing-preview February 20, 2025 14:24 — with GitHub Actions Inactive

error handling for multi sig tests

09087d3

curiecrypt temporarily deployed to testing-preview February 20, 2025 17:51 — with GitHub Actions Inactive

jpraynaud reviewed Feb 24, 2025

View reviewed changes

mithril-stm/src/multi_sig.rs Outdated Show resolved Hide resolved

revert serialization tests

cf63d31

curiecrypt temporarily deployed to testing-preview March 7, 2025 13:45 — with GitHub Actions Inactive

curiecrypt requested a review from jpraynaud March 7, 2025 14:15

jpraynaud reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

curiecrypt commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

jpraynaud Mar 7, 2025

jpraynaud Mar 7, 2025

Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

Are you sure you want to change the base?

Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

Conversation

curiecrypt commented Feb 19, 2025 • edited Loading

Content

Pre-submit checklist

Issue(s)

github-actions bot commented Feb 19, 2025 • edited Loading

Test Results

jpraynaud Mar 7, 2025

Choose a reason for hiding this comment

jpraynaud Mar 7, 2025

Choose a reason for hiding this comment

curiecrypt commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading