Abstract common patterns used within tECDSA signing #3386
Conversation
A common scenario occurring during the work with `net.Message` and `state .BaseAsyncState` is extracting message payloads of given type from the history, and ensuring their uniqueness using the sender as key. So far, this logic was defined in the tECDSA signing protocol code but, we noticed it should live on a higher abstraction level to avoid duplicating it across different protocols. That said, we decided to make it generic and move to the `pkg/net` to live next to the `net.Message` definition. This way, specific protocols only need to assemble the right call without defining the logic itself.
| // Payloads uses the TypedMessagesSupplier to extract TypedPayloads that | ||
| // represent a concrete type conforming the TypedPayload constraint. | ||
| func (tms TypedMessagesSupplier[T]) Payloads() TypedPayloads[T] { | ||
| var template T |
There was a problem hiding this comment.
It is quite elegant yet we require every protocol message to expose Type() function. They do expose them but it is a simplification to fulfil net.TaggedMarshaler requirements without introducing a separate marshaler type. Note that in the current design, net.BroadcastChannel accepts TaggedMarshaler in Send, so it is not a message but something that can serialize to bytes and exposes a type.
If we decide to move the logic to BaseAsyncState (see the other comment) I think we should use a type T for casting the payload (message.Payload().(T)) but instead of expecting T to expose Type(), I would pass the type as a string function parameter just like we do it for GetAllReceivedMessages(messageType string) and then filter on the net.Message level, just like we do here.
Then, I think we can get rid of TypedPayload, TypedPayloads and even TypedMessagesSupplier because GetAllReceivedMessage of BaseAsyncState is what is always called.
| // that describes its own type. Despite the Message interface extends TypedPayload, | ||
| // there is no guarantee that Message.Payload() does the same. The TypedPayload | ||
| // should be used in the context of Message.Payload() only in cases where the | ||
| // underlying Message.Payload() type actually implements TypedPayload. |
There was a problem hiding this comment.
I am staring at this PR for almost 3 hours now and I am quite convinced, pkg/net is not the right place for this code.
Just like the documentation for TypedPayload says, some payloads have Type() but this is not generally guaranteed. As noted in the other comment to this PR, the fact payloads have Type() is our simplification and not something net.BroadcastChannel interface expects.
Next, we deduplicate messages based on a condition that is out of the scope of pkg/net and comes from the protcol. This is not retransmission deduplication or floodsub deduplication the pkg/net should take care of. This creature does not come from pkg/net but from the protocol and I am afraid func (tp TypedPayloads[T]) Deduplicate(keyFn func(T) string) TypedPayloads[T] living in pkg/net would be a common point of confusion of what this deduplication is actually about.
Rephrasing what I think is a problem here: we are deduplicating some net.Messages (not all of them are "deduplicatable") based on a protocol-level requirements (abstracted out, but still coming from the protocol - this is not a network-level deduplication, it is a protocol-specific deduplication).
Naturally, the question arises: if not pkg/net then where and how?
My first thought was that this is something pkg/protocol should take care of. We are deduplicating received messages of the given type based on some protocol rules supplied. So this could be a common code in pkg/protocol/message, for example.
On the other hand, "deduplicating messages of the given type" is primarily a problem of an asynchronous state machine. And the async state machine is the reason we have this problem in the first place.
The given state of a synchronous state machine accepts only one type of message and can do the deduplication at the moment the message is received. In case of the async state machine, we are accepting all types of messages, also those from the future, and the deduplication can be done only when we finally reach the state understanding the given type of messages and knowing how to deduplicate them.
So this could also be a logic in BaseAsyncState.
I am personally leaning towards this option. It is the simplest one because I think we can avoid TypedPayload, TypedPayloads, and TypedMessagesSupplier. If we realize one day, sync state machine could also find this pattern useful, we can move it to pkg/protocol/message and play with typed abstractions.
What do you think?
This reverts commit f0718ce.
A common scenario occurring during the work with `net.Message` and `state .BaseAsyncState` is extracting message payloads of given type from the history, and ensuring their uniqueness using the sender as key. So far, this logic was defined in the tECDSA signing protocol code but, we noticed it should live on a higher abstraction level to avoid duplicating it across different protocols. That said, we decided to make it generic and move to the `pkg/state`. This way, specific protocols only need to assemble the right call without defining the logic itself.
2 participants
Refs: #3366
Depends on: #3385
Here we aim to abstract some common patterns used within tECDSA signing. The goal is to limit code duplication between protocols and have the generic things on the right abstraction level. Specific changes are described below.
Abstract
net.Messagepayload extraction and deduplicationA common scenario occurring during the work with
net.Messageandstate.BaseAsyncStateis extracting message payloads of a given type from the history, and ensuring their uniqueness using the sender as key. So far, this logic was defined in the tECDSA signing protocol code but, we noticed it should live on a higher abstraction level to avoid duplicating it across different protocols. That said, we decided to make it generic and move to thepkg/state. This way, specific protocols only need to assemble the right call without defining the logic itself.