refactor(stm): update PoP, protocol parameters and decoding functions - #3431
Conversation
Test Results 5 files ± 0 209 suites ±0 1h 31m 39s ⏱️ - 1h 42m 11s Results for commit 83390c5. ± Comparison against base commit bc9d8c1. This pull request removes 60 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
7f5d023 to
98489af
Compare
hjeljeli32
left a comment
There was a problem hiding this comment.
Added 2 additional comments.
e3fcba1 to
b5d4db6
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of mithril-stm around registration/PoP enforcement and protocol parameter validation, and hardens several legacy byte decoders by switching to checked arithmetic to avoid panics/overflows during deserialization.
Changes:
- Added checked offset arithmetic in multiple legacy decoders (Merkle tree / commitments, concatenation proof, single signature).
- Validated
phi_fis within(0, 1]and propagated errors through lottery/eligibility paths. - Tightened key-registration APIs to ensure external callers must go through PoP-verifying registration paths (plus associated visibility/documentation updates).
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mithril-stm/tests/test_extensions/protocol_phase.rs | Update test registration flow to use KeyRegistration::register() (PoP-verifying). |
| mithril-stm/src/protocol/single_signature/signature.rs | Harden single-signature legacy decoding with checked offsets; adjust tests for new encapsulation. |
| mithril-stm/src/protocol/single_signature/signature_registered_party.rs | Restrict access to registered-party payload inside signature wrapper; update tests accordingly. |
| mithril-stm/src/protocol/participant/initializer.rs | Use ClosedKeyRegistration accessor instead of direct field access. |
| mithril-stm/src/protocol/mod.rs | Stop publicly re-exporting RegistrationEntry; keep it crate-visible where needed. |
| mithril-stm/src/protocol/key_registration/registration_entry.rs | Make RegistrationEntry crate-private and remove serde derives to prevent unverified external construction. |
| mithril-stm/src/protocol/key_registration/register.rs | Make register_by_entry crate-private; encapsulate ClosedKeyRegistration fields and provide crate accessor(s). |
| mithril-stm/src/protocol/key_registration/mod.rs | Narrow RegistrationEntry re-export to pub(crate). |
| mithril-stm/src/protocol/key_registration/closed_registration_entry.rs | Make ClosedRegistrationEntry::new private to ensure construction flows through verified paths. |
| mithril-stm/src/protocol/error.rs | Silence private_interfaces warning on a public error carrying a now-private type. |
| mithril-stm/src/proof_system/halo2_snark/mod.rs | Update call sites to use ClosedKeyRegistration accessor. |
| mithril-stm/src/proof_system/halo2_snark/eligibility.rs | Add (0, 1] validation for phi_f and tests for invalid values. |
| mithril-stm/src/proof_system/halo2_snark/aggregate_key.rs | Use ClosedKeyRegistration accessor for total stake. |
| mithril-stm/src/proof_system/concatenation/single_signature.rs | Propagate phi_f validation errors from lottery checks. |
| mithril-stm/src/proof_system/concatenation/signer.rs | Make lottery check fallible (StmResult) and propagate errors. |
| mithril-stm/src/proof_system/concatenation/proof.rs | Harden concatenation-proof legacy decoding with checked offsets. |
| mithril-stm/src/proof_system/concatenation/eligibility.rs | Validate phi_f and make is_lottery_won fallible; add tests for invalid values. |
| mithril-stm/src/proof_system/concatenation/aggregate_key.rs | Use ClosedKeyRegistration accessor for total stake. |
| mithril-stm/src/membership_commitment/merkle_tree/tree.rs | Harden MerkleTree legacy decoding with checked arithmetic; adjust lint-handling around (de)serialization helpers. |
| mithril-stm/src/membership_commitment/merkle_tree/commitment.rs | Harden batch commitment verification arithmetic with checked ops. |
| mithril-stm/src/lib.rs | Update public docs/imports to reflect new registration API (no public RegistrationEntry). |
| mithril-stm/README.md | Update README example to use KeyRegistration::register(). |
| mithril-stm/examples/key_registration.rs | Update example to use KeyRegistration::register(). |
| mithril-stm/CHANGELOG.md | Add a new release entry describing the refactor/validation/decoder hardening. |
| mithril-stm/benches/stm.rs | Update benches to use KeyRegistration::register(). |
| mithril-stm/benches/size_benches.rs | Update benches to use KeyRegistration::register(). |
83d45a6 to
36b7b29
Compare
There was a problem hiding this comment.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
95c67c0 to
e150f0b
Compare
…h path verification
eb86804 to
883bec1
Compare
28ac44a to
83390c5
Compare
Content
This PR includes an update to the handling of the proof of possession of BLS keys, a modification of the checks done on the protocol parameters and a modification to the decoding function of the merkle tree path and concatenation proof.
Changes
phi_fis within (0, 1] before use in lottery/eligibility computationsKeyRegistration::register_by_entrytopub(crate)so external registration must go throughregister(), which always verifies proof of possessionClosedKeyRegistrationfields private with apub(crate)constructorRegistrationEntry/ClosedRegistrationEntry(types, constructors, methods) topub(crate)and dropped RegistrationEntry's unused Serialize/DeserializePre-submit checklist
Comments
Issue(s)