Skip to content

Conversation

@tbagrel1
Copy link
Contributor

@tbagrel1 tbagrel1 commented Sep 12, 2025

This pull request introduces support for the ObjectDiffusion mini-protocol, required for Peras (for certificates and votes diffusion). It also plugs the PerasCertDiffusion (instance of ObjectDiffusion) mini-protocol in the networking layer.

This PR depends on an updated version of ouroboros-network, see IntersectMBO/ouroboros-network#5202 and https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2


Peras ObjectDiffusion Mini-protocol:

  • Added modules Ouroboros.Consensus.MiniProtocol.ObjectDiffusion{.Inbound,.Outbound} with implementations of the ObjectDiffusion protocol (quite similar/inspired from TX-submission, except that client = inbound, server = outbound)
  • Added module Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.API defining ObjectPool{Reader,Writer} interfaces, through which ObjectDiffusion accesses/stores the objects to send/that have been received.
  • Added modules Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.PerasCert and Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.PerasCert containing definitions specific to PerasCert diffusion through the ObjectDiffusion mini-protocol
  • Modifies Ouroboros.Consensus.Node.Serialisation to add CBOR serialisation (SerialiseNodeToNode) for Point blk, Tip blk, and PerasCert blk
  • Modifies Ouroboros.Consensus{.Node,.Node.Tracer,.Network.NodeToNode} to wire-in PerasCertDiffusion similarly to other mini-protocols (e.g. TX-submission)

Tests and benchmarks

  • Added module Test.Consensus.MiniProtocol.ObjectDiffusion.Smoke with smoke test for the general ObjectDiffusion mini-protocol (using mock objects)

  • Added module Test.Consensus.MiniProtocol.ObjectDiffusion.PerasCert.Smoke with smoke test specific to PerasCert diffusion through ObjectDiffusion

  • Updates Test.ThreadNet.Network in unstable-diffusion-testlib accordingly to the changes made in Ouroboros.Consensus.Network.NodeToNode


Network Protocol Version Updates:

@tbagrel1 tbagrel1 changed the base branch from main to main-pr/weighted-chain-selec September 12, 2025 13:09
@tbagrel1 tbagrel1 changed the title Add ObjectDiffusion and PerasCert diffusion (instace of ObjectDiffusion) Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion) Sep 12, 2025
@tbagrel1 tbagrel1 added the peras label Sep 12, 2025
@tbagrel1 tbagrel1 changed the title Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion) [Peras #4] Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion) Sep 12, 2025
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from 80d3c69 to f88b3ae Compare September 12, 2025 14:32
@tbagrel1 tbagrel1 force-pushed the main-pr/weighted-chain-selec branch 6 times, most recently from dca0315 to d7c3c64 Compare September 18, 2025 08:47
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch 3 times, most recently from 37a9f85 to 222f4be Compare September 18, 2025 09:25
@tbagrel1 tbagrel1 force-pushed the main-pr/weighted-chain-selec branch from d7c3c64 to f1158c0 Compare September 19, 2025 07:58
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from 222f4be to d47c331 Compare September 19, 2025 08:04
@tbagrel1 tbagrel1 changed the title [Peras #4] Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion) [Peras 4] Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion) Sep 25, 2025
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from d47c331 to 918307c Compare September 26, 2025 13:47
@amesgen amesgen force-pushed the main-pr/weighted-chain-selec branch from f1158c0 to 90131b2 Compare October 2, 2025 19:01
@amesgen amesgen force-pushed the main-pr/weighted-chain-selec branch 4 times, most recently from 8aba6fc to 8b987f6 Compare October 10, 2025 16:57
@amesgen amesgen force-pushed the main-pr/weighted-chain-selec branch from 328b50a to 1018f6e Compare October 20, 2025 15:25
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch 2 times, most recently from f8083cc to 8a88eb7 Compare October 20, 2025 17:24
Copy link
Contributor

@agustinmista agustinmista left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments only.

Co-authored-by: Agustin Mista <[email protected]>
Co-authored-by: Alexander Esgen <[email protected]>
Co-authored-by: Georgy Lukyanov <[email protected]>
Co-authored-by: Thomas BAGREL <[email protected]>
Co-authored-by: Nicolas BACQUEY <[email protected]>
Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from 2e7a690 to 09669d1 Compare November 7, 2025 18:06
@tbagrel1 tbagrel1 marked this pull request as ready for review November 10, 2025 09:11
@tbagrel1
Copy link
Contributor Author

Hello everyone,
This PR is now ready for review! Thanks in advance for your time :)

Copy link
Contributor Author

@tbagrel1 tbagrel1 left a comment

Choose a reason for hiding this comment

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

I've tried to address a few comments for which there was an easy solution in bf4767a, and I've tried to answer/explain other points raised in the review.

Thanks @dnadales for your thorough review, and see you tomorrow at the meeting!

@tbagrel1
Copy link
Contributor Author

During last Wednesday’s meeting, we agreed that it would be better to associate each object directly with its objectId type. This would let us avoid passing objectId object type parameters everywhere, and instead just pass object. It would also allow us to remove the oprObjectId and opwObjectId fields in ObjectPool{Reader,Writer}, which are essentially just getters for an object’s ID.

The idea would be to introduce a class:

class
  ( Ord (IdOf a)
  , Eq (IdOf a)
  , Show (IdOf a)
  , NoThunks (IdOf a)
  , Serialise (IdOf a)
  ) => HasId a where

  type IdOf a
  getId :: a -> IdOf a

and then provide instances such as HasId PerasCert, HasId PerasVote, etc.

NOTE: An alternative would be to use a multi-parameter type class with a functional dependency,
HasId object objectId | object -> objectId, but this would make it inconvenient to refer to the objectId type when defining a data type parameterised by object only.


The current concerns are:

  1. Various ObjectDiffusion types are already parameterised by objectId object, including some in ouroboros-network. This means the change would need to span both repositories. As a result, we’d need to define HasId in ouroboros-network, or even higher up the dependency chain (e.g. in cardano-base).

  2. We would need to enable FlexibleContexts and UndecidableInstances in some places. For example:

instance (Show objectId, Typeable object, Typeable objectId)
  => Exception (ObjectDiffusionInboundError objectId object)

would become:

instance (Show (IdOf object), Typeable object, Typeable (IdOf object))
  => Exception (ObjectDiffusionInboundError object)

and this seems to require those extensions.


Consequently, we’d like to get approval again from the IOG team before going down this route.
Whatever the final decision is, I think it would be best to continue with [Peras 4] as-is, and handle the objectId / HasId changes in a separate PR.

@tbagrel1
Copy link
Contributor Author

I changed the interface of ObjectPoolReader's method to oprObjectsAfter :: ticketNo -> Word64 -> STM m (Maybe (m (Map ticketNo object))) as we agreed last week, see b629e84 :)

@tbagrel1
Copy link
Contributor Author

Hi @IntersectMBO/ouroboros-consensus-maintainers , I've tried to addressed all required changes that we discussed last week, see commits bf4767a .. 5674b30. Let me know if anything still needs to be adjusted.

I'll squash these fixup commits once you've had time to look at them :)

Copy link
Contributor

@agustinmista agustinmista left a comment

Choose a reason for hiding this comment

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

I took a look at the fixups, and wrote some comments and questions, nothing too serious but some things we should address before merging.

@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from 5674b30 to e0cdbb5 Compare November 26, 2025 13:30
@tbagrel1 tbagrel1 force-pushed the main-pr/object-diffusion branch from e0cdbb5 to a02433c Compare November 26, 2025 14:07
@tbagrel1
Copy link
Contributor Author

Ok, now the last small issue on smoke tests has been fixed, so this is ready :)

@agustinmista agustinmista dismissed their stale review November 26, 2025 16:47

Fixed the concerns I had

-- given Word64. Only the IDs and ticketNos of the objects are directly
-- accessible; each actual object must be loaded through a monadic action.
, oprObjectsAfter :: ticketNo -> Word64 -> STM m (Maybe (m (Map ticketNo object)))
-- ^ Get the map of objects available in the pool with a ticketNo greater
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- ^ Get the map of objects available in the pool with a ticketNo greater
-- ^ Get the map of objects available in the pool with a 'ticketNo' greater

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

7 participants