Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft Cardano Decentralized Message Queue CIP #1

Closed
wants to merge 1 commit into from

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Aug 2, 2024

Important

This is a buffer PR used for final reviews/comments before submitting to the https://github.com/cardano-foundation/CIPs repository.

This proposal specifies a decentralized message queue leveraging the Cardano network layer. This protocol allows a topic based mechanism to diffuse messages from publishers to consumers in a decentralized way.

(rendered proposal)

Copy link

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I could have done this review already on the upstream repo. That we would also get some guidance on structuring the document (i.e. my appendix comment)

CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
In this section, we propose an architecture for `Cardano` and `Mithril`. Note,
in this section, `Mithril` is used as a placeholder for a possible `Mithril`
application: we've seen many ideas about how `Mithril` can be used in `Cardano`
and all of them can follow this design.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird section and does not fit as a top-level section the CIP template.

I think it's great that we can give more details how mithril and cardano processes would interact, but ultimately the CIP is about the network protocol.

Maybe it would fit better as an Appendix titled "Cardano/Mithril node architecture" or so?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree I think this should be clear from the beginning and it is not. In the abstract, motivation and protocol implementation sections, there's only mention of cardano-node and its network and it leads the reader to think this is going to be an addition to them. Sure mithril is about the protocol, but as we already have discussed there's no strong reason why this protocol should be added to the protocol stack of cardano-node. The viable option that we settled on was what this "architechure" section describes. This is the optimal route, even for Peras and Leios.

CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
CIP-0XXX/README.md Outdated Show resolved Hide resolved
In this section, we propose an architecture for `Cardano` and `Mithril`. Note,
in this section, `Mithril` is used as a placeholder for a possible `Mithril`
application: we've seen many ideas about how `Mithril` can be used in `Cardano`
and all of them can follow this design.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree I think this should be clear from the beginning and it is not. In the abstract, motivation and protocol implementation sections, there's only mention of cardano-node and its network and it leads the reader to think this is going to be an addition to them. Sure mithril is about the protocol, but as we already have discussed there's no strong reason why this protocol should be added to the protocol stack of cardano-node. The viable option that we settled on was what this "architechure" section describes. This is the optimal route, even for Peras and Leios.


## Acceptance Criteria

1. A Cardano node implementing the previously described mini-protocols is released for production.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the wording is correct here. It should not be "a cardano node" but rather "a mithril node", right?

Comment on lines +508 to +507
1. A message producer node (e.g. a Mithril signer) publishing messages to the local Cardano node through mini-protocols is released.
1. A message subscriber node (e.g. a Mithril aggregator) receiving messages from the local Cardano node is released.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right so I think it should be made even more clearer (in the earlier sections or even in the architecture section) that the final solution implies 3 pieces of software:

  • the mithril node
  • the signer
  • the aggregator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like the Mithril node will be considered part of the Cardano node as they will be bundled together.

CIP-0XXX/README.md Outdated Show resolved Hide resolved

We propose to create a decentralized message diffusion protocol leveraging the Cardano network layer. This protocol allows to follow a topic based publish-subscribe pattern to diffuse messages from publisher nodes to subscriber nodes in a decentralized way.

The messages can be sent and received by nodes running in a non intrusive way side by side to the Cardano node in order to enable inter-nodes communications.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non intrusive way side by side to the Cardano node

This is a weird way to hint at the architecture section. I would just say it instead of being mysterious about it.

Suggested change
The messages can be sent and received by nodes running in a non intrusive way side by side to the Cardano node in order to enable inter-nodes communications.
The messages will be sent and received by mithril nodes. Mithril nodes are implemented using the cardano node network stack and will have to run side by side with full cardano nodes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to stay generic in the formulation as Mithril is only one of the use cases

Comment on lines 192 to 195
> [!WARNING]
> We also probably need to make sure that the KES key used to sign is from the latest rotation:
> - either the last seen opcert number in the block headers of the chain.
> - or the last seen opcert number from a previous message diffused.
>
> If the opcert number received is strictly lower than the previous one which has been seen, it should be considered as a protocol violation.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add a note saying that there might be needed an extra step of handshake to negotiate a mithril magic to forward the connection to the mithril node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a bit too much of an implementation detail?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A standalone Mithril node will use its own handshake; it can introduce its own protocol parameters, but quite likely it will start with NodeToNodeVersionData:

data NodeToNodeVersionData = NodeToNodeVersionData
  { networkMagic  :: !NetworkMagic
  , diffusionMode :: !DiffusionMode
  , peerSharing   :: !PeerSharing
  , query         :: !Bool
  }
  deriving (Show, Typeable, Eq)
  • networkMagic - I think that's useful for debugging, and making sure the Mithril node runs on the right network; but it should be different than the existing networkMagics
  • diffusionMode - this is useful if there are initiator-only nodes, e.g. a Mithril node running next to an edge node (a wallet)
  • peerSharing - this will be useful, since we want peer sharing in the Mithril network
  • query - that's useful for tools like cardano-cli ping, so I'd keep it.


### Description

The local message submission mini-protocol is used by local clients to submit message to a local node. This mini-protocol is **not** used to diffuse messages from a core node to another.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a core node to another

I don't like this wording as it hints that the protocol will run on cardano-node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our discussions, the cardano node would be called directly by the n2c mini-protocols

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but it won't be cardano-node running the protocols right? You'll just be querying its chain/state

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entry point for the n2c will be the cardano-node, and the n2n would be done in the mithril-node (for the use case of Mithril)


### Description

The local message notification mini-protocol is used by local clients to be notified about new message received by the core node.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer for this n2c mini-protocol

@jpraynaud
Copy link
Member Author

@ch1bo, @bolt12 I have taken into consideration your comments and updated the PR accordingly.

I feel that the PR is now in a draft version that is ready to be pushed to the CIPs repository upstream.
This is the place where many of the comments should belong and where we should follow the discussions with the community 🙂

@jpraynaud jpraynaud force-pushed the decentralized-message-queue branch 2 times, most recently from 7e41918 to c317daa Compare August 6, 2024 15:19
@bolt12
Copy link

bolt12 commented Aug 6, 2024

Thank you! Yes I agree 👍

@ch1bo
Copy link

ch1bo commented Aug 7, 2024

Available upstream now with cardano-foundation#876

@ch1bo ch1bo closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants