Skip to content

start basic reliable channel folder#3886

Merged
Ivansete-status merged 23 commits into
masterfrom
add-skeleton-for-reliable-channel
May 27, 2026
Merged

start basic reliable channel folder#3886
Ivansete-status merged 23 commits into
masterfrom
add-skeleton-for-reliable-channel

Conversation

@Ivansete-status

@Ivansete-status Ivansete-status commented May 18, 2026

Copy link
Copy Markdown
Collaborator

This PR:

  • Is inspired by pair session with @jazzz , 🙌 !

  • Adds a very basic folder structure of reliable channel and its components.
    The main suggested new folder is channels and inside, it contains segmentation, rate-limit-manager, sds, and encryption. This is just structural PR but no functionality is implemented yet.

In separate PRs:

  • Properly accommodate all the broker events and properly refactor the MessagingClient/MessagingAPI implementation.
  • Properly implement channels/scalable_data_sync/scalable_data_sync.nim and channels/scalable_data_sync/sds_persistence.nim modules.
  • Implement unit tests for reliable channel.

closes #3853

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3886

Built from 2ca66fd

Comment thread channels/reliable_channel.nim Outdated
##
## ... -> rate_limit_manager -> [encryption] -> dispatch
for m in msgs:
let wireBytes = self.encryption.encrypt(m.encode())

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We will need to revisit which fields need to be encrypted so that we allow proper routing without having for force-brute try to decrypt everything.

@NagyZoltanPeter NagyZoltanPeter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left some comments to think of ;-)

Comment thread channels/encryption/encryption.nim Outdated
type
EncryptionError* = object of CatchableError

EncryptFn* = proc(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be:

Suggested change
EncryptFn* = proc(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].}
RequestBroker:
type Encrypt = seq[byte]
proc signature*(payload: seq[byte]): Future[Result[Encrypt, string]] {.async.}

Comment thread channels/encryption/encryption.nim Outdated
EncryptionError* = object of CatchableError

EncryptFn* = proc(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].}
DecryptFn* = proc(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...and...

Suggested change
DecryptFn* = proc(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].}
RequestBroker:
type Decrypt = seq[byte]
proc signature*(payload: seq[byte]): Future[Result[Decrypt, string]] {.async.}

Comment thread channels/encryption/noop_encryption.nim Outdated
Comment on lines +7 to +14
proc noopEncrypt(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].} =
return ok(payload)

proc noopDecrypt(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].} =
return ok(payload)

proc init*(T: typedesc[EncryptionHook]): T =
return T(encrypt: noopEncrypt, decrypt: noopDecrypt)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...and follows...

Suggested change
proc noopEncrypt(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].} =
return ok(payload)
proc noopDecrypt(payload: seq[byte]): Result[seq[byte], string] {.gcsafe, raises: [].} =
return ok(payload)
proc init*(T: typedesc[EncryptionHook]): T =
return T(encrypt: noopEncrypt, decrypt: noopDecrypt)
proc setNoopEncryption() =
Encrypt.setProvider(proc(payload: seq[byte]): Future[Result[Encrypt, string]] {.async.} =
return ok(payload)
)
Decrypt.setProvider(proc(payload: seq[byte]): Future[Result[Decrypt, string]] {.async.} =
return ok(payload)
)

self.queue = @[]

ReadyToSendEvent.emit(
globalBrokerContext(), ReadyToSendEvent(channelId: self.channelId, msgs: ready)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead using globalBrokerContext here, the value of it can be captured and stored in RateLimitManager => during tests and multiple ffi context case the source of broker context truth shall come from initialization phase.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I think I will suggest, in upcoming commits, to make ReliableChannelManager the broker ctx owner for this layer.

Comment on lines +18 to +21
method markAcknowledged*(
self: SdsPersistence, messageId: SdsMessageID
) {.base.} =
discard

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a public facing part? I though such should come internally from SDS machinery.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But maybe this surface in incomplete in the sense of current SDS module.
@darshankabariya

Comment thread channels/events.nim Outdated
Comment on lines +9 to +27
type
MessageReceivedEvent* = object
channelId*: ChannelId
senderId*: SdsParticipantID
payload*: seq[byte]

MessageSentEvent* = object
requestId*: ReliableRequestId

MessageDeliveredEvent* = object
requestId*: ReliableRequestId

MessageSendErrorEvent* = object
requestId*: ReliableRequestId
reason*: string

MessageDeliveryErrorEvent* = object
requestId*: ReliableRequestId
reason*: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should brainstorm these namings though, might be misleading to overlap with Messaging API events.
Also it is a question if to emit those events when reliable channels in use..... This is a spec question though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry to barge in here but we are bailed out by scoping the type by e.g. import "as", e.g. channels.MessageSentEvent vs. messaging.MessageSentEvent (I think); nim-brokers dispatches on the actual type, not type strings/names AFAICT. Also solves human disambiguation by using the module import prefix. (not touching here on the consumer-producer relationship between events are different levels).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is exact! But Nim cannot enforce it (wish for namespace kind... )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is exact! But Nim cannot enforce it (wish for namespace kind... )

Right, I mean, in a module the compiler will complain if it cannot disambiguate, so you will have to do e.g. import xxx as yyy if you don't like the default dot-disambiguator it gives you (e.g. "import ...../channels" allows you to do channels.MyType in the code instead of MyType and it already works. Unless I'm missing something (possible, even likely).

Comment thread channels/reliable_channel.nim Outdated
Comment on lines +152 to +160
proc onMessageReceived*(self: ReliableChannel, wakuMsg: WakuMessage) =
## Ingress pipeline made visible:
##
## WakuMessage -> ReliablePayload -> decrypt -> sds -> reassemble -> emit
##
## Invoked from the waku `MessageReceivedEvent` listener after the
## inbound `WakuMessage` has been filtered to this channel's
## `contentTopic`. Each stage is a minimal stub for now.
let inWakuMsg: WakuMessage = wakuMsg

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I could suggest, to implement it as a listener to the Waku's (or rather MessagingAPI's) MessageReceivedEvent.
To decouple the two layer and not to let lower layer MessagingAPI exactly know about upper ReliableChannel layer.
WDYT?
cc: @fcecin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, so the natural way to think of it is that ReliableChannels is implemented entirely using the Messaging layer (which will contain a MessagingClient root type, etc. as specced by Igor, but that comes in my PR -- so you can use whatever is in the MAPI now instead to implement channels -- channels will be the first module merged with the correct layering, which is easier since it is new; and for now it uses whatever we have there, and I'll rewire it later).

Now in practice (if we want to take it up with Igor later) some "triangle-shape" interactions could make sense. That is, the ReliableChannels layer could have the Waku/Kernel/Core layer be a direct dependency -- that is, the channels layer (layer 3) implementation could call the API of the kernel layer (layer 1) since it IS an API, so that's fine, but the question then is whether we want that in general (not really) and whether it is the smart thing to do in some cases (possible). For example, you don't need to create a MessagingMessage type at the messaging layer (layer 2) to substitute for the WakuMessage API type which is a core type. And there may be other examples of actual functions that you may want to call directly from the core. I don't have a strong opinion either way, but as long as we're having implementations wired to APIs only, improving on this is easy -- you just expand the API at the layer directly beneath you to supply what you need so you can abandon making a layer 3 ---> layer 1 API direct dial.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm also a bit confused, this doesn't seem to be in specs 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think is better to properly decouple layers as Zoltán mentioned and that way I think it will be closer to specs. I'll revisit that asap.

Comment thread channels/reliable_channel_manager.nim Outdated
Comment on lines +115 to +120
proc send*(
manager: ReliableChannelManager,
channelId: ChannelId,
appPayload: seq[byte],
ephemeral: bool = false,
): Result[ReliableRequestId, string] =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks the public facing API of all.
I think we need to think of a mechanism to shadow/hide MessagingAPI level send when ReliableChannel is in use. WDYT?
Or... is it a valid scenario to send most of my messages through channels but some I may want to send as row low level payloads?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The logos-delivery library will give the ability to callers to operate at any layer they want. They should be able to instantiate a ReliableChannel and a MessagingClient and even a Waku if we end up exposing that via a logos delivery kernel.h. They can operate at all levels simultaneously -- they shouldn't, but they can, because we will have public methods to do that.

So when a ReliableChannel is created, it creates a MessagingClient for itself, internally, to get its job done. The MessagingClient in turn creates a Waku for itself. The only way the library user can access the MessagingClient that the ReliableChannel created directly is if we give a channels layer API that returns the MessagingClient associated with that ReliableChannel. Either that or the application can create their own MessagingClient and manage that separately. That's the API-spec decision.

That's what happens when we create a library that exposes multiple levels of service at once.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was actually seeing it like this (as I described in #3865):

# First create a MessagingClient
let client = MessagingClient.new(preset = TheWakuNetwork, mode = Edge)
await client.start()

# Create many channels, use same MessagingClient
let chat1 = client.createReliableChannel("channel-id-1", contentTopic="/myapp/1/chat1/proto")
let chat2 = client.createReliableChannel("channel-id-2", contentTopic="/myapp/1/chat2/proto")

So:

  • User is allowed to freely mix the usage of any layers. It shouldn't break.
  • Reliable Channels require aMessagingClient
  • Reliable Channels are created from a MessagingClient. And pointers to them are kept inside this MessagingClient instance.

Then such ReliableChannelManager would be a private thing held by MessagingClient. And it should not be the user-facing class. Only ReliableChannel itself should be public.

@fcecin fcecin May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that means the Reliable Channels feature goes into the Messaging layer. Now we have two layers (waku/kernel, and messaging) instead of three (waku/kernel, messaging, channels). My read of the diagrams was just wrong.

Channels is then an optional feature of the messaging client, so there's no "ReliableChannelsManager" root object (or something to that effect) (EDIT: with its own independent lifetime) in a separate layer that's analogous to MessagingClient and Waku.

That actually makes more sense.

cc: @darshankabariya

@igor-sirotin igor-sirotin May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@fcecin yeah when writing the ticket I was thinking if it's fair to call it a separate layer.
Ended up convinced that it is 😄

It's still an optional layer that can be ignored by the client. It introduces another object for the client to communicate with. And it contains quite some logic in it. But indeed, it depends on the MessagingClient.

But it doesn't matter much if we call it a layer really

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reliable Channels are created from a MessagingClient. And pointers to them are kept inside this MessagingClient instance.

I think I was wrong here. MessagingClient doesn't need to remember all channels. It only needs to subscribe it to own events, then return it to the client.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comments.
We decided to merge kernel (libwaku) and messaging (liblogosdelivery) libraries into one, ok.

Now that we are starting to introduce ReliableChannels, I think it should generate its own separate library. That way, we will have one single send on each library, and resolve Zoltán's concern.

Status will consume libwaku+liblogosdelivery.

LogosChat will consume libreliablechannels. ( or the names that you prefer .)

@NagyZoltanPeter , @fcecin , @igor-sirotin - wdyt about that lib separation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I start getting loss here.
If messaging has the interface to initialize a new reliable-channel-id (chat) than it sounds ReliableChannel is not usable standalone... thus it's against this concept of separate it into it's own library.
Separate library would assume the opposite ownership model. MessagingAPI -> ReliableChannelMgr vs. ReliableChannelMgr -> MessagingAPI.

My real question was, whether to allow use MessagingApi->send on content_topic used for an already initialized channel?

side note: As Igor stated, as being open source, we cannot prevent anybody to do so in a forked delivery lib.
So might be better to be prepared for such scenario of sending exploited payloads on channel-content-topics.

For me having yet another library, sounds, not solving problems, but generates more.

My proposal than is rather divide by naming:

~ Kernel_RelayPublish / Kernel_LightpushPublish <- after merge
~ Messaging_send() <- this is the current MAPI simple send interface
~ Channel_send() <- reliable channel API

Also events must be distinguishable, thus user can decide which to subscribe.
MessageSentEvent / MessageReceivedEvent vs. ChannelMessageSent / ChannelMessageReceived

We can debate on naming, just wanted to demonstrate the idea.

If we are talking about libraries, it also means ffi... for which we don't support hierarchical API (yet).

@fcecin fcecin May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Guys, in my ongoing refactor work (lift messaging, separate messaging and waku core) I am:

  • Assuming we are shipping one actual library (liblogosdelivery) which packs all code. There is no reason to ship two distinct libraries. The only reason libwaku.so is separate is because we have marked it for its own clean, discrete, independent deprecation and deletion cycle (which has to be negotiated, I assume).
  • This library packs a set of layers, which are high-level, formal components with boundaries, contracts, invariants, etc.
  • Not assuming where channels go (we can put them into its own separate layer, or implement it inside the messaging layer -- both will work; it's just a question of whether we decide they are separately mountable and startable as a standalone internal meta-component, that is, a layer, or not).
  • Not assuming what the C API is and not changing it unnecessarily. We control precisely what we expose in the C API at all times, irrespective of what Nim code we have behind it.
  • Not deleting libwaku.so yet, and not changing its behavior (unless you tell me to delete it, then it is gone).
  • Not changing config one bit -- that work will be resolvable once this lands (each layer can have its own config, or we can make it work with the "union", "global" config object we already have. Now that we have formally-modeled layers, we can actually make sense when discussing a layered config system.

About this new formal concept being introduced, of layers:

  • Aren't just a prefix/naming convention, although we do generally use name prefixes_ to distinguish in e.g. the otherwise flat list of functions that a C API is.
  • Can be mounted and unmounted individually in the library by controlling the existence of the zero-or-one-instance singleton Nim root object for the layer (e.g. Waku vs MessagingClient).
  • Have their own nim-brokers facade which the layer's implementation types cooperate to populate upon mounting the layer and starting the root object, and that is unplugged when stopping the root object and unmounting the layer.

In other words, we can guarantee that e.g. Waku, the most fundamental layer, does not depend on any other layer, and is individually mountable. So when you have e.g. a dedicated intrastructure node that is not doing any messaging, you can just mount that single layer and you will be guaranteed to not pull any messaging functionality. Each layer has its own independent lifetime and interface, and they cooperate to get things done, e.g. the Messaging layer calls the Waku (kernel, core) layer, not the other way around.

EDIT: We have decided to not go this route for now, and will instead do a simpler reorg.

Comment thread channels/reliable_channel.nim Outdated
Comment on lines +152 to +160
proc onMessageReceived*(self: ReliableChannel, wakuMsg: WakuMessage) =
## Ingress pipeline made visible:
##
## WakuMessage -> ReliablePayload -> decrypt -> sds -> reassemble -> emit
##
## Invoked from the waku `MessageReceivedEvent` listener after the
## inbound `WakuMessage` has been filtered to this channel's
## `contentTopic`. Each stage is a minimal stub for now.
let inWakuMsg: WakuMessage = wakuMsg

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm also a bit confused, this doesn't seem to be in specs 🤔

Comment thread channels/reliable_channel.nim Outdated
Comment thread channels/reliable_channel.nim Outdated
Comment thread channels/reliable_channel.nim Outdated
Comment thread channels/scalable_data_sync/scalable_data_sync.nim Outdated
Comment thread channels/rate_limit_manager/rate_limit_manager.nim Outdated
Comment thread channels/encryption/encryption.nim Outdated
Comment thread channels/types.nim Outdated
Comment thread channels/reliable_channel_manager.nim Outdated
Comment on lines +115 to +120
proc send*(
manager: ReliableChannelManager,
channelId: ChannelId,
appPayload: seq[byte],
ephemeral: bool = false,
): Result[ReliableRequestId, string] =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was actually seeing it like this (as I described in #3865):

# First create a MessagingClient
let client = MessagingClient.new(preset = TheWakuNetwork, mode = Edge)
await client.start()

# Create many channels, use same MessagingClient
let chat1 = client.createReliableChannel("channel-id-1", contentTopic="/myapp/1/chat1/proto")
let chat2 = client.createReliableChannel("channel-id-2", contentTopic="/myapp/1/chat2/proto")

So:

  • User is allowed to freely mix the usage of any layers. It shouldn't break.
  • Reliable Channels require aMessagingClient
  • Reliable Channels are created from a MessagingClient. And pointers to them are kept inside this MessagingClient instance.

Then such ReliableChannelManager would be a private thing held by MessagingClient. And it should not be the user-facing class. Only ReliableChannel itself should be public.

@jazzz

jazzz commented May 21, 2026

Copy link
Copy Markdown

Adding context: from the pairing session that @Ivansete-status and I had, I think it makes sense to do a quick lightning implementation to discover key gaps in the protocol and then make spec compliant after words.

There were some missing gaps that surfaced once we tried to integrate it into an actually use case, persuing this further would help uncover them all. Specifically reliable-channel-api spec defines the API but does not define key payload parsing/transformation requirements.

My suggestion would be:

  1. Lighting implementation: get something that works end-to-end.
  2. I'll gather what You all like and what needs to change into a ReliableChannel protocol specification.
  3. Update impl into final form.
  4. Integration into libchat:InboxV2.
  5. Profit

@Ivansete-status
Ivansete-status force-pushed the add-skeleton-for-reliable-channel branch from 22c4837 to f4b4807 Compare May 21, 2026 23:53

@igor-sirotin igor-sirotin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR is about initial objects definition and code structure — I'm happy with what I see, thank you! 👍 The comments I left inline are minor, and go beyond the goal of this PR.

Comment on lines +38 to +44
## TODO !! The proper ownership chain is:
## ReliableChannelManager -> DeliveryService (MessagingClient) -> Waku (Kernel/Protocols) -> WakuNode,
## and this will be implemented in the future. For now, `createNode`
## is called here to get a DeliveryService instance, and the WakuNode is immediately discarded.
## This is a temporary workaround to get the API

let waku = ?(await createNode(conf))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we just pass the DeliveryService in as arg then? 🙂

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's an option too. In fact, I was thinking about just passing a rel-channels conf. Something like:

proc new*(
    T: type ReliableChannelManager,
    conf: ReliableChannelManagerConf,
    brokerCtx: BrokerContext = globalBrokerContext(),
): Future[Result[T, string]] {.async.} =

Then, internally build the DeliveryService/MessagingClient.
wdyt?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Ivansete-status my point was that that we never build MessagingClient internally at all. Always use the provided one.

But your comment here states that it's a temporary solution, and I guess it's for compilation success, that's why I suggested to pass a DeliveryService right away — it would compile. If it's for testing, then fine to keep as is.

Comment thread channels/reliable_channel.nim Outdated
Comment on lines +37 to +40
const Lip173Meta* = "LIP173"
## Wire-level marker for the Reliable Channel layer. A `WakuMessage`
## whose `meta` field does not equal these bytes is not addressed to
## this layer and is silently dropped on ingress.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would like to see this discussed in spec first, before adding to implementation.

  1. What are the consequences of having this as unencrypted metadata?
  2. The goal of this field is to simplify the incoming pipeline, but:
    • This mechanism doesn't allow selecting 1 designated reliable channel. All instances of ReliableChannel will try to decrypt a message. Find the match by trying all — it's ok IMO, at least this is what I expected for the first implementation. But then Lip173Meta doesn't really help much with it.
    • An attacker can spam us with random data tagged with Lip173Meta, forcing us to try to decrypt it. Though it's the same case without Lip173Meta 😄

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Started the discussion in logos-co/logos-lips#348
Also, the slug attribute seems not very good. Instead, I suggest using a wire version string that can be adjusted manually within the spec itself.
Adjusted this PR in the following: e274189

Comment thread channels/reliable_channel_manager.nim Outdated
Comment on lines +72 to +73
if chn.getContentTopic() == evt.message.contentTopic:
await chn.onMessageReceived(evt.message.payload)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's only one possible match in all manager.channels. So we should break the loop as soon as match found.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Addressed in 4035cd0

rateLimit: RateLimitManager

requestIds: Table[RequestId, seq[RequestId]]
pendingRequests: seq[tuple[parent: RequestId, ephemeral: bool]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be simplified by storing extra information (like ephemeral flag) in table

Suggested change
pendingRequests: seq[tuple[parent: RequestId, ephemeral: bool]]
pendingRequests: Table[RequestId, bool]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The current proposal is indeed too poor. I will tackle that in a separate PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in the following: #3914

) {.async: (raises: []).} =
## Tail of the outgoing pipeline. Invoked from the `ReadyToSendEvent`
## listener once `rate_limit_manager` releases a batch of opaque
## blobs (already-encoded SDS messages):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This way onReadyToSend becomes a public method of ReliableChannel, which it shouldn't be.

I see 2 ways to avoid it:

  • Listen to the signal right in ReliableChannel (I like this)
  • Send from ReliableChannelManager

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 0b8cd51

Comment thread channels/reliable_channel.nim Outdated
## clear and encrypt only the application payload.
let encRes = await Encrypt.request(m)
let wireBytes =
if encRes.isOk():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not fail the operation if encryption failed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in e4caa0b

Comment thread channels/reliable_channel.nim Outdated

return ok(parentReqId)

proc onMessageReceived*(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, this method should be private

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 0b8cd51

Comment thread channels/reliable_channel.nim Outdated
## the `Decrypt` request broker.
let decRes = await Decrypt.request(payload)
let plaintext: seq[byte] =
if decRes.isOk():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If decryiption failed — halt the operation?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 8d95fcb

Comment thread channels/scalable_data_sync/scalable_data_sync.nim Outdated
Comment thread channels/reliable_channel.nim Outdated
if unwrapped.isErr():
return

let segment = SegmentMessageProto.decode(unwrapped.get().content)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SegmentMessageProto is a segmentation internal detail, we shouldn't operate with it directly in ReliableChannel.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 4648d15

@NagyZoltanPeter NagyZoltanPeter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Ivansete-status I think we lost some focus within here.
I don't really get why does ReliableChannelManager calls createNode?
It seems we promoted ReliableChannelManager to the lib root class.

Maybe I'm wrong but from those many discussion I distilled the following structure.

MessagingClient (library root) - creates node
- creates Kernel (Waku in old terms)
- creates DeliveryService (represents MessagingAPI)
- can create ReliableChannelManager (layer root of ReliableChannel - will orchestrate channels within)

So it is up to users decision which layer to use.
I think that should be one place where configuration is taken and instantiate the node itself (basically Kernel root object until we don't separate the impl from interface).
So the master orchestrator and first hand client facing object is MessagingClient.

It's up to us how we manifest it into FFI context.

appologize is my comment seems in-medias-res and I may have missed some part of the conversation. If so, please update me.
@fcecin @igor-sirotin @darshankabariya

@fcecin

fcecin commented May 27, 2026

Copy link
Copy Markdown
Contributor

@Ivansete-status I think we lost some focus within here. I don't really get why does ReliableChannelManager calls createNode? It seems we promoted ReliableChannelManager to the lib root class.

Sorry guys, I'm late with this; we have rerouted in the architecture aspect.

In my current (much shorter) reorg PR:

  • Root lib FFI context object: still always Waku (we can rename it later to e.g. LogosDelivery)
  • Component objects inside Waku:
    • WakuNode (and all other kernel objects)
    • MessagingClient (can be nil; if not nil, messaging is enabled)
    • ReliableChannelManager (can be nil; if not nil, reliable channels are enabled)

The ReliableChannelManager can also be put inside the MessagingClient if it becomes an implementation detail of the messaging layer, instead of its own layer.

Whatever code sits on top of this (FFI code or Nim application code) just does new Waku, then optionally mountMessagingClient / mountReliableChannelManager, then Waku.start() just knows what to do.

@Ivansete-status

Ivansete-status commented May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the comments @NagyZoltanPeter ! And you are absolutely right that it doesn't look 100% nice now. This PR is just a very initial step towards introducing ReliableChannelManager.

I think the following diagram explains very well the final structure that we want to have (extracted from lift API issue)

image

So, the "root library" concept fundamentally depends on the user, as you mentioned.

# User Root library object
1 Reliable App Dev, e.g., logoschat ReliableChannelManager
2 Plain App Dev, e.g., Status MessagingClient/DeliveryService
3 Node Operator, e.g., DST Kernel/Waku

With that, the layer #1 will internally instantiate #2 which in turn will instantiate #3.
The layer #2 only instantiates #3 and knows nothing about #1.

Each layer may have its own config object. The most internal the layer is, the less opinionated the config is, being the Kernel/Waku config layer (current WakuNodeConf) the one that brings full flexibility to the NodeOperator/DST.

@Ivansete-status

Copy link
Copy Markdown
Collaborator Author

Thanks @fcecin for the comments!
I think the best way of ensuring the diagram I linked above is by having a composition from top to down. In your proposal the Waku/Kernel has notion about ReliableChannelManager but this is not what we want. We want top-down ownership, i.e.,

ReliableChannelManager ➡︎ uses ➡︎ MessagingClient ➡︎ uses ➡︎ Waku/Kernel

@fcecin

fcecin commented May 27, 2026

Copy link
Copy Markdown
Contributor

Thanks @fcecin for the comments! I think the best way of ensuring the diagram I linked above is by having a composition from top to down. In your proposal the Waku/Kernel has notion about ReliableChannelManager but this is not what we want. We want top-down ownership, i.e.,

ReliableChannelManager ➡︎ uses ➡︎ MessagingClient ➡︎ uses ➡︎ Waku/Kernel

Yes, that's what I said. Waku is not the kernel. It's the entire system. It's just named "Waku" because in the past that's all that existed. We can rename it in my PR.

The kernel objects don't access the messaging objects, and the messaging objects don't access the reliable channel objects (if that's how we are going to structure it). So the arrows in the diagram (who knows who, who uses who) are respected.

So the requirement is literally fulfilled by that mechanic. Unless we want a different specific mechanic (EDIT: e.g. specific object composition hierarchy -- which type literally has which type as their field), which then I will need the explicit code structure that we want here spelled out. We can sort that out in reviews.

@NagyZoltanPeter

Copy link
Copy Markdown
Contributor

Thanks @fcecin for the comments! I think the best way of ensuring the diagram I linked above is by having a composition from top to down. In your proposal the Waku/Kernel has notion about ReliableChannelManager but this is not what we want. We want top-down ownership, i.e.,

ReliableChannelManager ➡︎ uses ➡︎ MessagingClient ➡︎ uses ➡︎ Waku/Kernel

Ok, here is my problem.
That diagram is a dependency diagram and not ownership diagram - at least how I understood it.

And I suggest to separate internal dependency from DevEx.

In order a developer should be clearly understand the roles of each layers we shall not expose such an ownership matrix.
Hence I vote for an orchestrator - library face - class (we can call it LogosDelivery (as @fcecin described called Waku), or even MessagingClient). But from DevEx point of view as a user of the library shall be able to clearly distinguish layers and how can I access them. So if I have an first hand library face, that returns me the proper layer accessor I can choose easily and naturally.

class LogosDelivery {
public:
   Result<void> init(config);
   Result<ReliableChannelManafeg> getReliableChannelManager(...);
   Result<DeliveryService> getDeliveryService(...);
   Result<DeliveryKernel> getDeliveryKernel(...);
}

This is just a showcasing example, namings are just arbitrary only for understanding the concept.

I think there are clean advantages to have a separate orchestrator class at the top that can not just give access to layers but can drive upon configuration (modes, roles, etc.) and can also manage lifetimes.
WDYT?

@Ivansete-status

Copy link
Copy Markdown
Collaborator Author

Sorry @fcecin , I misunderstood you.

I agree that we need to have such main-obj or orchestrator type, and I think we can follow @NagyZoltanPeter 's suggestion in that regard, i.e., keep a LogosDelivery type as the main library object that handles everything else. Let's see and coordinate for upcoming PRs.

Thanks both! 🥳

@fcecin

fcecin commented May 27, 2026

Copy link
Copy Markdown
Contributor

Sorry @fcecin , I misunderstood you.

I agree that we need to have such main-obj or orchestrator type, and I think we can follow @NagyZoltanPeter 's suggestion in that regard, i.e., keep a LogosDelivery type as the main library object that handles everything else. Let's see and coordinate for upcoming PRs.

Thanks both! 🥳

Right this somewhat my fault because "a reliable channel having a messaging client having a waku/kernel" was my idea also when I started the massive layering PR. But we won't do that massive refactor, so we should keep doing what we are doing: keeping a single FFIContext[SomeLibraryObject] at all times, and that object can "nil" certain objects -- controlled by whoever is using that object. Which is what we do internally in Waku today to decide what internal protocols are mounted (relay.isNil()? etc.); same idea, and the layering is implicit in the dependency chain between the objects, not necessarily in composition hierarchies. (We can change this later also).

@Ivansete-status
Ivansete-status merged commit 74057c6 into master May 27, 2026
41 of 48 checks passed
@Ivansete-status
Ivansete-status deleted the add-skeleton-for-reliable-channel branch May 27, 2026 21:05
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.

create basic reliable channel components

5 participants