Stm error handling with anyhow #2765

curiecrypt · 2025-11-05T12:43:14Z

Content

This PR includes the changes needed to adapt the error handling of mithril-stm to the best practices described here.

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
- All check jobs of the CI have succeeded
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested
Documentation
- No new TODOs introduced

Comments

TODO: Remove unused/unnecessary error types.

Issue(s)

Relates to #2764

github-actions · 2025-11-05T12:51:04Z

Test Results

4 files ±0 168 suites ±0 22m 18s ⏱️ -38s
2 210 tests +3 2 210 ✅ +3 0 💤 ±0 0 ❌ ±0
6 895 runs +8 6 895 ✅ +8 0 💤 ±0 0 ❌ ±0

Results for commit a1ee8c1. ± Comparison against base commit 2ad7d5e.

This pull request removes 12 and adds 15 tests. Note that renamed tests count towards both.

mithril-aggregator ‑ database::query::epoch_settings::update_epoch_settings::tests::test_update_epoch_settings
mithril-aggregator ‑ runtime::runner::tests::test_update_epoch_settings
mithril-aggregator ‑ services::epoch_service::tests::update_epoch_settings_insert_future_epoch_settings_in_the_store
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::register_signer
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_aggregator_features
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_epoch_settings
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::register_signer
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_aggregator_features
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_epoch_settings
mithril-signer::era_switch ‑ test_extensions::certificate_handler::tests::register_signer
…

mithril-aggregator ‑ database::query::epoch_settings::insert_or_ignore_epoch_settings::tests::test_cant_replace_existing_value
mithril-aggregator ‑ database::query::epoch_settings::insert_or_ignore_epoch_settings::tests::test_insert_epoch_setting_in_empty_db
mithril-aggregator ‑ database::repository::epoch_settings_store::tests::save_epoch_settings_does_not_replace_existing_value_in_database
mithril-aggregator ‑ services::epoch_service::tests::inform_epoch_compute_allowed_discriminants_from_intersection_of_aggregation_network_config_and_configured_discriminants
mithril-aggregator ‑ services::epoch_service::tests::inform_epoch_insert_registration_epoch_settings_in_the_store
mithril-aggregator ‑ services::network_configuration_provider::tests::get_stored_configuration_with_stored_value_returns_them
mithril-aggregator ‑ services::network_configuration_provider::tests::get_stored_configuration_without_stored_value_fallback_to_configuration_value
mithril-aggregator ‑ services::network_configuration_provider::tests::test_get_network_configuration_retrieve_configurations_for_aggregation_next_aggregation_and_registration
mithril-common ‑ messages::epoch_settings::tests::test_current_json_deserialized_into_message_supported_until_open_api_0_1_55
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::fake_aggregator::tests::register_signer
…

♻️ This comment has been updated with latest results.

jpraynaud · 2025-11-17T11:42:08Z

mithril-common/src/crypto_helper/codec/binary.rs

    impl TryFromBytes for SingleSignature {
        fn try_from_bytes(bytes: &[u8]) -> StdResult<Self> {
-            Self::from_bytes::<D>(bytes).map_err(|e| e.into())
+            Self::from_bytes::<D>(bytes).map_err(|e| anyhow!(e))


This should be enough here and in other occurrences in this module:

Suggested change

Self::from_bytes::<D>(bytes).map_err(|e| anyhow!(e))

Self::from_bytes::<D>(bytes)

jpraynaud · 2025-11-17T11:54:30Z

mithril-stm/README.md

+    Err(error) => assert!(
+        matches!(
+            error.downcast_ref::<AggregationError>(),
+            Some(AggregationError::NotEnoughSignatures { .. })
+                | Some(AggregationError::UsizeConversionInvalid)
+                | Some(AggregationError::UnsupportedProofSystem { .. })
+        ),
+        "Unexpected error: {error}"
+    ),


We miss all the prints and additional asserts which were there before, maybe this is a bit better:

Suggested change

Err(error) => assert!(

matches!(

error.downcast_ref::<AggregationError>(),

Some(AggregationError::NotEnoughSignatures { .. })

| Some(AggregationError::UsizeConversionInvalid)

| Some(AggregationError::UnsupportedProofSystem { .. })

),

"Unexpected error: {error}"

),

Err(error) => match error.downcast_ref::<AggregationError>() {

Some(AggregationError::NotEnoughSignatures(n, k)) => {

println!("Not enough signatures");

assert!(n < params.k && k == params.k)

},

Some(AggregationError::UsizeConversionInvalid) => {

println!("Invalid usize conversion");

},

Some(AggregationError::UnsupportedProofSystem(aggregate_signature_type)) => {

println!("Unsupported proof system: {:?}", aggregate_signature_type);

},

None => {

println!("Unexpected error during aggregation: {:?}", error);

}

},

jpraynaud · 2025-11-17T11:59:56Z

mithril-stm/src/aggregate_signature/basic_verifier.rs


-        BlsSignature::verify_aggregate(msg.to_vec().as_slice(), &vks, &sigs)?;
+        BlsSignature::verify_aggregate(msg.to_vec().as_slice(), &vks, &sigs)
+            .with_context(|| "Basic verifier failed in multisignature verification.")?;


Maybe this is a bit more clear:

Suggested change

.with_context(|| "Basic verifier failed in multisignature verification.")?;

.with_context(|| "Basic verifier failed during BLS aggregate signature verification.")?;

jpraynaud · 2025-11-17T12:01:49Z

mithril-stm/src/aggregate_signature/mod.rs

-                    unreachable!(),
-                Err(AggregationError::UnsupportedProofSystem(_)) =>
-                    unreachable!(),
+                Err(error) => { assert!(


Same comment as above about missing asserts?

jpraynaud · 2025-11-17T12:05:54Z

mithril-stm/src/aggregate_signature/proof/concatenation.rs

+        .with_context(
+            || "Failed to aggregate unique signatures during selection for the k indices.",


This would be more clear, WDYT?

Suggested change

.with_context(

|| "Failed to aggregate unique signatures during selection for the k indices.",

.with_context(

|| "There are not enough signatures to reach the quorum",

jpraynaud · 2025-11-17T12:11:47Z

mithril-stm/src/key_registration.rs

-                    Err(RegisterError::KeyInvalid(a)) => {
-                        assert_eq!(fake_it, 0);
-                        assert!(a.verify_proof_of_possession().is_err());
+                    Err(error) => { assert!(


Same remark here for the missing assertions.

jpraynaud · 2025-11-17T12:15:58Z

mithril-stm/tests/stm_basic.rs

+        Err(error) => assert!(
+            matches!(
+                error.downcast_ref::<AggregationError>(),
+                Some(AggregationError::NotEnoughSignatures { .. })
+            ),
+            "Unexpected error: {error}"
+        ),


Same remark for missing asserts

jpraynaud · 2025-11-17T12:16:14Z

mithril-stm/tests/stm_protocol.rs

+        Err(error) => assert!(
+            matches!(
+                error.downcast_ref::<AggregationError>(),
+                Some(AggregationError::NotEnoughSignatures { .. })
+                    | Some(AggregationError::UsizeConversionInvalid)
+                    | Some(AggregationError::UnsupportedProofSystem { .. })
+            ),
+            "Unexpected error: {error}"
+        ),


Same remark for missing asserts

stm error and result types defined

b2ff616

curiecrypt added 2 commits November 5, 2025 15:55

sorting

41fbcfb

remove generics from error enums

344f3c3

curiecrypt temporarily deployed to testing-preview November 6, 2025 14:32 — with GitHub Actions Inactive

curiecrypt added 12 commits November 7, 2025 15:08

aggregate signature with StmResult

ff75c7a

merkle tree with StmResult

bcd20e4

participant with StmResult

a7364ed

single signature with StmResult

678a747

key reg with StmResult

0b3b96f

bls sig StmResult

328a1d2

revising tests

751b7bd

stm tests run with anyhow

b2e3a49

contexts added

64aad5f

rest of the library adjusted

9a42242

error froms removed

a2ea280

resolve unmatched errors in common

a1ee8c1

curiecrypt marked this pull request as ready for review November 17, 2025 09:04

curiecrypt requested a review from jpraynaud November 17, 2025 09:05

jpraynaud reviewed Nov 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stm error handling with anyhow #2765

Stm error handling with anyhow #2765

Uh oh!

curiecrypt commented Nov 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

jpraynaud Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Self::from_bytes::<D>(bytes).map_err(\|e\| anyhow!(e))
	Self::from_bytes::<D>(bytes)

-    Err(error) => assert!(
-        matches!(
-            error.downcast_ref::<AggregationError>(),
-            Some(AggregationError::NotEnoughSignatures { .. })
-                | Some(AggregationError::UsizeConversionInvalid)
-                | Some(AggregationError::UnsupportedProofSystem { .. })
-        ),
-        "Unexpected error: {error}"
-    ),
+    Err(error) => match error.downcast_ref::<AggregationError>() {
+        Some(AggregationError::NotEnoughSignatures(n, k)) => {
+            println!("Not enough signatures");
+            assert!(n < params.k && k == params.k)
+        },
+        Some(AggregationError::UsizeConversionInvalid) => {
+            println!("Invalid usize conversion");
+        },
+        Some(AggregationError::UnsupportedProofSystem(aggregate_signature_type)) => {
+            println!("Unsupported proof system: {:?}", aggregate_signature_type);
+        },
+        None => {
+            println!("Unexpected error during aggregation: {:?}", error);
+        }
+    },

	.with_context(\|\| "Basic verifier failed in multisignature verification.")?;
	.with_context(\|\| "Basic verifier failed during BLS aggregate signature verification.")?;

		.with_context(
		\|\| "Failed to aggregate unique signatures during selection for the k indices.",

Stm error handling with anyhow #2765

Are you sure you want to change the base?

Stm error handling with anyhow #2765

Uh oh!

Conversation

curiecrypt commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Content

Pre-submit checklist

Comments

Issue(s)

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

curiecrypt commented Nov 5, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading