Add `StaticRound` to eliminate some boilerplate when writing protocols #117

fjarri · 2025-06-07T23:31:59Z

Main changes:

Round now uses associated types for messages, payloads, and artifacts. Fixes Typed Round trait #65. Most dynamic parts (BoxedFormat, ProtocolMessagePart, serialization/deserialization of messages etc) are hidden from the user.
Evidence verification methods are moved from Protocol to corresponding Round implementors. This allows one to keep the related logic (receive_message() and evidence verification) together.

Corollary changes:

Parts of Add various protocol writing helpers from synedrion #113 are rendered unnecessary (e.g. map-downcast). Need to re-evaluate after this PR is merged.
Closes Consider renaming verify_*_is_invalid methods #105, fixes Simplify invalid message evidence verification #82 (it now happens in DynRound impl for Round types).
AssociatedData is renamed to SharedData and is now an associated type of Protocol instead of ProtocolError. This helps Uniformize associated data and entry point inputs #89, but does not close it yet.
Closes Add an extend combinator for protocols. #62 - it seems that given static rounds, an extension like that can be just as well implemented manually without manul support. Closes Add a zip combinator for protocols #61 for the same reason (plus the current system of Protocol::round_info() will make getting the correct round simply impossible in a zipped protocol)
Fixes ProtocolError mandates Display #114 (ProtocolError now uses description() instead of Display)
Closes Rename BoxedRound::new_dynamic to just new #112 (fix included in this PR)

For reviewers

misbehave was renamed to extend (with the corresponding renames for types and traits), because it seemed to me like a better wording, but it is still intended for the same purpose: misbehavior tests. Should I bring back the original name?
extend is not really a combinator, it's more of a dynamic override API. I am not sure how useful it is outside tests. Should it be moved to dev?
BoxedRound is the only "boxed" part of the API visible to user. Is there a better name for it? (e.g. RoundInfo is also "boxed", but doesn't have a Boxed prefix) I don't think it can be hidden entirely as long as we allow finalizing into several different rounds.

dvdplm · 2025-06-09T07:59:03Z

I have skimmed the code, so this is not a review.

Thoughts:

manul is likely never going to be a performance bottleneck
…so we can "afford" to prioritize readability, ergonomics, correctness and maintainability.
I think we should make opinionated choices, aiming at giving users one correct way to do what they need to do.

Given the above I think we should provide either dynamic or static rounds, but not both. When we have a concrete use case that requires both dynamic and static we can re-assess.

Is switching to only static rounds possible? It'd be interesting to see how synedrion would look with only static rounds and if there'd be less boilerplate.

fjarri · 2025-06-09T21:28:56Z

Is switching to only static rounds possible?

For protocols themselves, yes, but evidence verification is not limited to one round and needs access to the message types from the previous rounds - so the user would have to manually transform them from some untyped form to typed messages.

Although it is possible to automate deserialization (and the handling of its errors) - by making every Round type responsible for deserializing its messages and putting them in a Box<>, which the user would later downcast to a specific type. A possible error for this operation is a LocalError.

This would need some kind of "routing" of round number -> boxed round type (not the round object, since we don't need the round state for that). This may be also used to simplify evidence checking for invalid messages (#82).

coveralls · 2025-06-10T21:36:30Z

Pull Request Test Coverage Report for Build 16578768783

Details

653 of 1298 (50.31%) changed or added relevant lines in 21 files are covered.
76 unchanged lines in 11 files lost coverage.
Overall coverage decreased (-5.2%) to 66.91%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
manul/src/dev/run_sync.rs	6	8	75.0%
manul/src/session/transcript.rs	0	2	0.0%
manul/src/session/message.rs	2	8	25.0%
manul/src/session/wire_format.rs	5	11	45.45%
manul/src/protocol/dyn_evidence.rs	28	35	80.0%
manul/src/protocol/errors.rs	0	8	0.0%
manul/src/protocol/rng.rs	0	12	0.0%
manul/src/protocol/round_id.rs	22	34	64.71%
manul/src/session/session.rs	27	39	69.23%
manul/src/protocol/message.rs	8	29	27.59%

Files with Coverage Reduction	New Missed Lines	%
manul/src/dev/tokio.rs	1	96.67%
manul/src/session/transcript.rs	1	59.56%
manul/src/session/echo.rs	2	51.63%
manul/src/session/message.rs	2	89.78%
manul/src/dev/run_sync.rs	4	83.43%
manul/src/protocol/message.rs	5	61.62%
manul/src/protocol/round_id.rs	6	60.0%
manul/src/combinators/chain.rs	7	48.52%
manul/src/session/session.rs	9	79.66%
manul/src/session/tokio.rs	16	81.03%

Totals
Change from base Build 15695642792:	-5.2%
Covered Lines:	2291
Relevant Lines:	3424

💛 - Coveralls

fjarri · 2025-06-11T00:56:00Z

Current roadblock: handling protocols where some nodes do not send or do not receive messages (e.g. KeyResharing). This means that the type of the round is not enough to determine whether a message is expected, it's also the state that's needed.

In the n-of-n case the static rounds eliminate the need for verify_*_is_invalid() methods, and also allow one to split evidence checking into per-round methods, making them much easier to maintain.

dvdplm

Another partial review.

dvdplm · 2025-07-21T11:03:14Z

examples/dining_cryptographers.rs

-        Ok(())
+    fn round_info(round_id: &RoundId) -> Option<RoundInfo<DinerId, Self>> {
+        match round_id {
+            _ if round_id == 1 => Some(RoundInfo::new::<Round1>()),


_ if round_id == 1 is a bit awkward looking, and I think the PartialEq impl between RoundId and RoundNum actually makes it a bit worse. I tinkered a bit and came up with a few alternatives.

More explicit, without any changes to other code:

fn round_info(round_id: &RoundId) -> Option<RoundInfo<DinerId, Self>> { if round_id == 1 { Some(RoundInfo::new::<Round1>()) } else if round_id == 2 { Some(RoundInfo::new::<Round1>()) } else { None } }

Instead of using the PartialEq impl, add a round() method to RoundId to make it clear what we're matching on:

fn round_info(round_id: &RoundId) -> Option<RoundInfo<DinerId, Self>> { match round_id.round() { 1 => Some(RoundInfo::new::<Round1>()), 2 => Some(RoundInfo::new::<Round2>()), _ => None, } }

Same as 2 but adding a to_info() method on the Round trait so we can emphasize the round itself:

fn round_info(round_id: &RoundId) -> Option<RoundInfo<DinerId, Self>> { match round_id.round() { 1 => Some(Round1::to_info()), 2 => Some(Round2::to_info()), _ => None, } }

Adding the to_info method to the Round-trait requires adding + Sized to the bounds, which might be problematic, otherwise I think 3) is the version I prefer. Regardless of which version you prefer going with, I think the PartialEq impl between RoundNum and RoundId is "too clever" and forces the reader to go deeper into the internals than what is actually useful.

Ideally, the comparison part would be fixed by #120. I like the to_info() approach, but it's an orthogonal issue.

Options 2 and 3 implicitly drop the groups from round ID, which can lead to silent errors. Yes, technically at that point round IDs should not have groups, but if there's some bug in the code, they might.

to_info() would require a method with a default implementation in the Round trait, which I would really like to make non-overridable, but that involves some sealed trait magic.

examples/dining_cryptographers.rs

dvdplm · 2025-07-21T11:32:52Z

examples/dining_cryptographers.rs

@@ -217,6 +200,14 @@ impl Round<DinerId> for Round1 {

 impl Round<DinerId> for Round2 {
    type Protocol = DiningCryptographersProtocol;
+    type ProtocolError = NoProtocolErrors<Self>;


Naming nitpick: the associated type is in the singular, so it's a bit odd to assign a value whose name is plural. OTOH calling it "NoProtocolError" isn't great either. Not sure there is a great solution to be found here (idiomatically it should really be () but that isn't possible as we saw above).

Maybe DummyProtocolError?

A random thought I had:
I wonder how far we'd be able to get if we changed the Round trait to make the type ProtocolError take a impl core::error::Error. Maybe then we could impl Round with () as the error type when there are no errors, but then we'd need some clever trick to transform or cast the impl core::error::Error into an actual ProtocolError with all the methods and trait bounds we need.

I read through rust-lang/rust#99301 which seems to be about ways to access data from nested errors in a generic way. Seems a bit stuck though.

Maybe we can instead rename the associated type to ProtocolErrors?

But wouldn't the plural on a type name imply that it is a collection of types of protocol errors?

I tried to impl ProtocolError for core::convert::Infallible (described as "The error type for errors that can never happen." in the docs) and it kinda works if it wasn't for the ser/deser bounds on ProtocolError. That's sort of what I was trying to hint at by saying "it'd be so nice if we could use a impl core::error::Error and then – handwaves – transfom/cast to concrete error types": we could have less bounds.

Anyway, all of this is nitpicking. The code is fine as-is, modulo perhaps the name.

Maybe we can instead rename the associated type to ProtocolErrors?

This is better.

I still think it's awkward that there isn't a better way to do this but that has nothing to do with this PR.

examples/dining_cryptographers.rs

manul/benches/empty_rounds.rs

manul/src/combinators/chain.rs

manul/src/combinators/extend.rs

dvdplm · 2025-07-24T10:00:34Z

manul/src/combinators/extend.rs

+};
+
+/// An extension to a round, allowing one to extend or override its methods.
+pub trait Extension<Id>: 'static + Debug + Send + Sync + Clone {


While "Extension" works it is a bit too generic imo. It is specific to a round so what do you think about RoundExt? That would be in line with other extension traits (https://rust-lang.github.io/rfcs/0445-extension-trait-conventions.html#the-convention).

I am not sure the quoted doc applies here, since it's not an extension of the Round trait, but rather an override. But I have no problem with RoundExt per se.

Hmm, a tricky one; you're right that it's not technically an extension trait, but maybe RoundExt is still a better name?

Looking at this again I still think Extension is too generic and that we need to have "round" in the name somewhere.

RoundExt is my preference

RoundOverride is explicit and clearly states intent. It also jives well with a struct OverriddenRound to replace ExtendedRound

RoundExtension is also a possibility I guess, but it's longer and could be misunderstood into making the round itself longer in some way.

manul/src/combinators/extend.rs

dvdplm · 2025-07-24T10:15:18Z

manul/src/combinators/extend.rs

+/// A wrapper for a protocol's [`EntryPoint`], allowing registering [`Extension`] implementors
+/// to extend or override [`Round`] methods.
+#[derive(Debug)]
+pub struct Extendable<Id: PartyId, EP: EntryPoint<Id>> {


Given this is pub I think we need a less generic name. Maybe even the full ExtendableEntryPoint. I have a slight preference for Wrapped over Extendable though.

Looking at this again I think we should avoid naming structs with adjectives, given the (weakly enforced) convention is to use adjectives for traits.

My suggestion here is WrappedEntryPoint.

manul/src/dev/run_sync.rs

manul/src/combinators/chain.rs

examples/dining_cryptographers.rs

manul/src/protocol/dyn_evidence.rs

manul/src/protocol/dyn_round.rs

These methods are now internal

dvdplm · 2025-07-31T13:11:01Z

examples/dining_cryptographers.rs

@@ -217,6 +200,14 @@ impl Round<DinerId> for Round1 {

 impl Round<DinerId> for Round2 {
    type Protocol = DiningCryptographersProtocol;
+    type ProtocolError = NoProtocolErrors<Self>;


Maybe we can instead rename the associated type to ProtocolErrors?

This is better.

I still think it's awkward that there isn't a better way to do this but that has nothing to do with this PR.

dvdplm · 2025-07-31T13:14:18Z

examples/src/simple.rs

+        messages: EvidenceMessages<Id, Self::Round>,
+    ) -> std::result::Result<(), EvidenceError> {
+        let _message: Round1Message = messages.direct_message()?;
+        // Message contents would be checked here


Suggested change

// Message contents would be checked here

// Message content would be checked here

Given this is an example and is supposed to be educational it'd be good to elaborate on what checks one would do here, e.g. "check signatures, size limits, sessions id" (or whatever).

dvdplm · 2025-07-31T13:44:34Z

examples/src/simple.rs


-        let message = Round1Broadcast {
+    type Payload = Round1Payload;
+    type Artifact = ();


Despite the other comment I still struggle with this.

If I understand things correctly this should be read as "Round 1 sends a direct message but does not use an artifact". In this sense, it is perfectly idiomatic to use ().

When the new manul user later discovers that there exists a NoArtifact type, I think there'll be confusion: which should I use?

The rule is:

If the round uses direct messages but no artifact, use () here.

If the round does not use direct messages , use NoArtifact here.

If the user makes a mistake, it will be impossible to impl make_direct_message but I don't think we do enough to help them figure out that they need to come here and set the associated type properly.

There's some kind of cognitive mismatch here that I can't quite put my finger on. The artifact is not merely a piece of "Associated data created alongside a message in [Self::make_direct_message]". It is actually doing two jobs, the other being to act like a "switch" to make it possible/impossible to send direct messages.

The old signature of make_direct_message was Result<(DirectMessage, Option<Artifact>), LocalError> and I think that the Option<Artifact> made the intention more clear.

I'd have to dig in deeper to know what the consequences of removing this associated type would be, but I think this is a wart.

The Round docs hints at this with "Set to [NoArtifact] if [Self::DirectMessage] is [NoMessage].", but doesn't give a deeper understanding and does not spell out when to use () and when to use NoArtifact. I think at the very least that we need to explain this properly both in docs and examples.

The aim is to do something like

trait Round { type DirectMessage: (MessageType, ArtifactType); }

That is, the user need to either specify both of these types, or none. It is not possible to do directly in the current Rust, but kind of possible with some additional constructs, like a trait that provides an API for "splitting" a value into a message and an artifact. The problem is that this trait would have to be implemented for every direct message.

When the new manul user later discovers that there exists a NoArtifact type, I think there'll be confusion: which should I use?

There may be confusion, but at least NoArtifact is impossible to instantiate, so it can't be used.

The old signature of make_direct_message was Result<(DirectMessage, Option), LocalError>

The problem with it is that it implies that some calls may return an artifact and some not, which is not really what we want.

dvdplm · 2025-07-31T13:48:24Z

manul/benches/async_session.rs

-        Ok(Payload::new(Round1Payload))
+        message: ProtocolMessage<Id, Self>,
+    ) -> Result<Self::Payload, ReceiveError<Id, Self>> {
+        assert!(message.direct_message.0 == do_work(self.round_counter + 2));


If this was a debug_assert would it be triggered in CI you think, when we run the benchmarks one time? If yes, then we could switch to that and avoid the work here.

dvdplm · 2025-07-31T13:58:58Z

manul/src/combinators/extend.rs

+};
+
+/// An extension to a round, allowing one to extend or override its methods.
+pub trait Extension<Id>: 'static + Debug + Send + Sync + Clone {


Looking at this again I still think Extension is too generic and that we need to have "round" in the name somewhere.

RoundExt is my preference

RoundOverride is explicit and clearly states intent. It also jives well with a struct OverriddenRound to replace ExtendedRound

RoundExtension is also a possibility I guess, but it's longer and could be misunderstood into making the round itself longer in some way.

dvdplm · 2025-07-31T15:22:53Z

manul/src/session/session.rs

+        // TODO (#4): we reuse `EchoBroadcast::none()` (that means `NoMessage` in the typed round)
+        // to have a second meaning, the node not sending messages at all.


Is this TODO still valid?

dvdplm · 2025-07-31T15:23:55Z

manul/src/session/wire_format.rs

-use crate::protocol::{DeserializationError, LocalError};
+use crate::protocol::LocalError;
+
+/// An error that can be returned during deserialization error.


Suggested change

/// An error that can be returned during deserialization error.

/// An error that can be returned during deserialization.

dvdplm · 2025-07-31T15:25:58Z

manul/src/tests/partial_echo.rs

+            // TODO (#4): this branch is unreachable in the absense of bugs in the code
+            // (the method will not be called in the first place if the node does not send messages).
+            // Can it be eliminated using the type system?


If we're convinced about this, let's use unreachable!("This node does not send messages in this round. This is a bug in manul") or something.

dvdplm · 2025-07-31T15:41:21Z

manul/src/session/evidence.rs

+        let verified = self
+            .normal_broadcast
+            .clone()
+            .verify::<SP>(verifier)


Same comment as I made elsewhere: I find it a bit odd that a verify method consumes self. It should be a read-only operation.

Maybe we can inline the code from verify and end up with something like

let digest = self.normal_broadcast.message_with_metadata.digest::<SP>()?; let signature = self.normal_broadcast.signature.deserialize::<SP>()?; verifier .verify_digest(digest, &signature) .map_err(MessageVerificationError::into_evidence_error)?; let deserialized = self .normal_broadcast .payload() .deserialize::<EchoRoundMessage<SP::Verifier>>(format) .map_err(|error| { EvidenceError::InvalidEvidence(format!("Failed to deserialize normal broadcast: {error:?}")) })?; self.error .verify_evidence::<SP>(self.normal_broadcast.metadata(), &deserialized)

(broken code, just to illustrate)

dvdplm · 2025-07-31T15:45:20Z

manul/src/protocol/round.rs

+        artifacts: BTreeMap<Id, Self::Artifact>,
+    ) -> Result<FinalizeOutcome<Id, Self::Protocol>, LocalError>;
+}
+


Round now has awesome docs, much appreciated.

fjarri mentioned this pull request Jun 7, 2025

Rename BoxedRound::new_dynamic to just new #112

Open

fjarri self-assigned this Jun 7, 2025

fjarri force-pushed the static-round branch 2 times, most recently from 8de19fb to 0ef0c98 Compare June 10, 2025 21:35

fjarri force-pushed the static-round branch from 0ef0c98 to d7b909e Compare June 10, 2025 21:38

fjarri force-pushed the static-round branch from 7226df2 to dc0371a Compare June 15, 2025 22:24

fjarri added 5 commits June 17, 2025 15:30

Relax Id bounds on traits

9edb13a

Add StaticRound to eliminate some boilerplate when writing protocols

e93bbfc

Add Protocol::round_info() routing method

e8d23b1

Add more methods

11620e5

Fix chain combinator

fa2028b

fjarri force-pushed the static-round branch from dc0371a to fa2028b Compare June 18, 2025 22:51

fjarri added 2 commits June 24, 2025 14:27

Replace Misbehave with Extend

c172784

Splitting error processing between rounds

959da23

fjarri force-pushed the static-round branch 4 times, most recently from 2b67199 to 49bb410 Compare June 29, 2025 23:25

fjarri force-pushed the static-round branch 4 times, most recently from 175e8e4 to 733fd6b Compare July 15, 2025 00:29

fjarri added 3 commits July 18, 2025 10:48

Fully switch to statically typed rounds

5ac9729

Separate round and group number types

a2ab642

Ungroup messages correctly for chained protocol errors

9ff447c

fjarri force-pushed the static-round branch from 733fd6b to 9ff447c Compare July 18, 2025 17:48

fjarri added 2 commits July 18, 2025 11:01

Merge dyn_protocol into protocol

32499ef

Rename Extension methods

8659991

fjarri mentioned this pull request Jul 18, 2025

Uniformize associated data and entry point inputs #89

Open

fjarri added 2 commits July 18, 2025 11:43

Add a TODO

7b742a4

Add a changelog

6952e51

fjarri marked this pull request as ready for review July 18, 2025 18:55

dvdplm reviewed Jul 21, 2025

View reviewed changes

fjarri mentioned this pull request Jul 21, 2025

Add various protocol writing helpers from synedrion #113

Open

dvdplm reviewed Jul 24, 2025

View reviewed changes

fjarri added 15 commits July 28, 2025 10:30

Add the round number to the evidence description

e8d2fca

Update obsolete docstrings

fb07279

These methods are now internal

Remove an implicit cast

5757b16

Fix an artifact type in dining_cryptographers

1a05741

Run dining_cryptographers in CI

fe8e2b8

Rename into_boxed() -> into_inner()

bc066c6

Remove unneeded (crate) specifiers

247ea03

Add some comments for ExtendedRound fields

af3d288

Fix extend docstring

e6856a5

Use a more idiomatic method

56fbe74

More descriptive error message in ungroup()

9df80d0

Update the term in chain docstring

6edb7b0

Reword Protocol::SharedData docstring

e2a2e3e

Rename ProtocolMessage -> DynProtocolMessage

d85e47b

Rename MessageParts -> ProtocolMessage

81e411c

fjarri force-pushed the static-round branch from c2db5dd to 81e411c Compare July 28, 2025 19:47

dvdplm reviewed Jul 31, 2025

View reviewed changes

	// Message contents would be checked here
	// Message content would be checked here

		// TODO (#4): we reuse `EchoBroadcast::none()` (that means `NoMessage` in the typed round)
		// to have a second meaning, the node not sending messages at all.

	/// An error that can be returned during deserialization error.
	/// An error that can be returned during deserialization.

Add StaticRound to eliminate some boilerplate when writing protocols #117

Are you sure you want to change the base?

Add StaticRound to eliminate some boilerplate when writing protocols #117

Uh oh!

Conversation

fjarri commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For reviewers

Uh oh!

dvdplm commented Jun 9, 2025

Uh oh!

fjarri commented Jun 9, 2025

Uh oh!

coveralls commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16578768783

Details

💛 - Coveralls

Uh oh!

fjarri commented Jun 11, 2025

Uh oh!

dvdplm left a comment

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Add `StaticRound` to eliminate some boilerplate when writing protocols #117

Add `StaticRound` to eliminate some boilerplate when writing protocols #117

fjarri commented Jun 7, 2025 •

edited

Loading

coveralls commented Jun 10, 2025 •

edited

Loading