feat(cbindings): first commit - waku relay (#1632) #1714

Ivansete-status · 2023-05-04T15:05:52Z

Description

This allows the creation of a libcwakuv2.a library which exposes the next Waku functions:

waku_version()
waku_set_event_callback(..)
waku_content_topic(..)
waku_pubsub_topic(..)
waku_default_pubsub_topic(..)
waku_relay_publish(..)
waku_start()
waku_stop()
waku_relay_subscribe(..)
waku_relay_unsubscribe(..)
waku_connect(..)
waku_poll()

Changes

Have a static library (libwaku.a) for unix systems.
Have a C program that links the Nwaku library. That program should mount the Relay protocol, allowing:
- subscribe/unsubscribe from a PubSub topic.
- send a message to a PubSub topic.

How to test

Build the cwaku_example with:
make cwaku_example -j$(cat /proc/cpuinfo | grep processor | wc -l)
note: on completion, the cwaku_example binary and the libwaku.a library will be in the build folder.
Open a terminal and run:
./build/cwaku_example --peers=/ip4/127.0.0.1/tcp/60001/p2p/16Uiu2HAmVFXtAfSj4EiR7mL2KvL4EE2wztuQgUSBoj2Jx2KeXFLN --port=60000 --key=364d111d729a6eb6d2e6113e163f017b5ef03a6f94c9b5b7bb1bb36fa5cb07a9
Open a terminal and run:
./build/cwaku_example --peers=/ip4/127.0.0.1/tcp/60000/p2p/16Uiu2HAm2eqzqp6xn32fzgGi8K4BuF88W4Xy6yxsmDcW8h1gj6ie --port=60001 --key=0d714a1fada214dead6dc9c7274585eca0ff292451866e7d6d677dc818e8ccd2
Subscribe both nodes to the same pubsubtopic and from one node, publish a message. For that, follow the instructions that are shown in the naïve menu.
i.e.

Please, select an option:
	1.) Subscribe to topic
	2.) Connect to other node
	3.) Publish a message

Issue

#1632

Pending tasks

Generate dynamic library
Pending to know how to retrieve the message_ID when publishing a new message
Pending to implement the publish_enc_symmetric and publish_enc_asymmetric, which is already done in go-waku.
Pending to implement waku_relay_enough_peers(char* pubsubTopic). This feature is not currently implemented in
libp2p-1.0.0/libp2p/protocols/pubsub/gossipsub.nim.
@richard-ramos - It might be interesting to offer a show_version function in the rfc. That would be helpful for users to know the library's version. This function is given in this PR.

richard-ramos

Very cool, i like this PR.
I just left some comments with minor observations and items for discussion

richard-ramos · 2023-05-04T20:23:25Z

apps/wakunode2/app.nim

+  app.node.wakuRelay.unsubscribeAll(PubsubTopic($pubSubTopic))
+
+proc publishMessage*(app: App, pubSubTopic: cstring, message: WakuMessage): Future[int] {.gcsafe, async.} =
+  # Returns the number of peers connected to the given pubSubTopic.


Suggested change

# Returns the number of peers connected to the given pubSubTopic.

richard-ramos · 2023-05-04T21:43:50Z

library/cwakuv2.nim

+  else:
+    jsonResp = errResp("Timeout expired")
+
+proc init(config_file: cstring) {.dynlib, exportc.} =


Could these be renamed to waku_new and waku_start? that way we follow the waku_* convention. The same goes for show_version

richard-ramos · 2023-05-04T21:53:38Z

apps/wakunode2/wakunode2.nim


-  const versionString = "version / git commit hash: " & app.git_version
+{.pop.} # @TODO confutils.nim(775, 17) Error: can raise an unlisted exception: ref IOError
+proc init*(configFilePath = "") =


I see init here uses a path vs the rfc which suggests a json string. We should decide which approach to use to unify behaviors between implementations. cc: @danielSanchezQ for his thoughts as he has worked with the rust+go bindings before

I don't think we should init an app here, but operate on the WakuNode object as defined in waku_node.nim (see comments elsewhere). wakunode2 is simply an application written around the Nim API. As stated elsewhere, we want to provide C bindings wrapping the Nim API, not the wakunode2 application

I see init here uses a path vs the rfc which suggests a json string. We should decide which approach to use to unify behaviors between implementations. cc: @danielSanchezQ for his thoughts as he has worked with the rust+go bindings before

IMO using a configuration is better. A config file should be used for a binary, a configuration object for a library.

@jm-clius Where can I find the nim api documentation? Don't know if Im understanding correctly, but, does it mean that the wakunode2 is an external application running and the bindings just communicate with it. Or do this works in a similar way to go-waku in where we do embedded the node within the application?

The application simply provides a way to configure and instantiate the node, so I guess more similar to "embedded within the application". The Nim API documentation is sparse and outdated (since it's only used internally), but in theory lives here: https://github.com/waku-org/nwaku/blob/master/docs/api/v2/node.md

Oh, it's very outdated, in fact. 😟 @Ivansete-status perhaps a good idea that we clean the Nim API doc up at some point during the development of the C bindings, since we're likely to expand/change it in any case.

richard-ramos · 2023-05-04T21:56:08Z

examples/v2/cbindings/cwakuv2example.c

+        exit(-1);
+    }
+
+    NimMain(); // initialize the Nim runtime


We should probably specify in the RFC or in the .h docs that NimMain() must be executed when using the nim c-bindings

richard-ramos · 2023-05-04T22:02:06Z

library/cwakuv2.nim

+proc show_version(): cstring {.dynlib, exportc.} =
+  return wakuNode2VersionString


I think this can be waku_version(). waku_node already provides git_version, so I don't think this needs to be exported elsewhere. If we need to construct something specific, that should be done on the Nim API in waku_node.nim.

richard-ramos · 2023-05-04T22:08:30Z

library/cwakuv2.nim

+  for i in 0 .. timeoutMs:
+    if pubMsgFut.finished():
+      break
+    sleep(1)


chronos has a withTimeout function that i think could be used here instead of sleep:
https://github.com/status-im/nim-chronos/blob/master/chronos/asyncloop.nim#L1116

Super interesting @richard-ramos. Thanks for that!
However, we cannot invoke await, and therefore, return a Future[..] in this proc because it is exported to C and this call needs to be synchronous.

richard-ramos · 2023-05-04T22:12:45Z

library/cwakuv2.nim

+    wakuMessage = WakuMessage(
+        # Visit https://rfc.vac.dev/spec/14/ for further details
+        payload: jsonContent["payload"].getStr().toSeq().mapIt(byte (it)),
+        contentTopic: $jsonContent["content_topic"].getStr(),
+        version: version,
+        timestamp: getTime().toUnix(),
+        ephemeral: false
+    )


It should be possible to specify these values instead of having them hardcoded. The meta field is also missing https://github.com/waku-org/nwaku/blob/master/waku/v2/waku_core/message/message.nim#L28 (this field should probably be added to the bindings rfc)

richard-ramos · 2023-05-04T22:15:18Z

library/events/json_message_event.nim

+        payload: payload,
+        contentTopic: msg.contentTopic,
+        version: msg.version,
+        timestamp: int64(msg.timestamp)


Some fields from WakuMessage are missing. Maybe it also makes sense to include the proof if RLN is enabled?

What do you think if instead of creating a JsonMessage with the WakuMessage content, to instead serialize it with https://github.com/status-im/nim-json-serialization? I imagine it should be possible to deserialize it too in waku_relay_publish. The advantage of doing that is that if in the future new fields are added to WakuMessage protobuffer, they'll be serialized/unserialized automatically in the c-bindings API functions

Yes, I absolutely agree. Nevertheless, I added the fields indicated in the next:
https://rfc.vac.dev/spec/36/#jsonmessage-type

richard-ramos · 2023-05-04T22:22:44Z

library/events/json_message_event.nim

+  # https://rfc.vac.dev/spec/36/#jsonmessageevent-type
+
+  var payload = newString(len(msg.payload))
+  copyMem(addr payload[0], unsafeAddr msg.payload[0], len(msg.payload))


I'm curious about this line. Why is it necessary to copy the payload like this instead of doing it directly?

Good point! I will try to directly use seq[byte] instead

I think I will leave it as is because if I change it to use seq[byte] we don't get a readable base64 string. In this case, it appears sth like "payload":[97,71,57,115,89,81,61,61]

jm-clius

Great to see progress here and thanks for including an example! Most important directional change I suggest is to keep the interface between the WakuNode and the wakunode2 application very clean. Our library is the WakuNode (defined in waku_node.nim) and it already exposes a Nim API (as well as JSON, REST, etc.). We want to provide C bindings for this library (more or less just wrapping the Nim API) not the wakunode2 application.

jm-clius · 2023-05-05T07:28:27Z

Makefile

+################
+.PHONY: cbindings cwakuv2example
+
+libcwakuv2.a: | build deps


I think we should move away from always having to specify v2 for Waku. Waku v1 is deprecated, in a sense, and not used anywhere. We even want to remove Waku v1 code from this repo, so it becomes simpler if we start referring to Waku v2 simply as the Waku. In other words, I think this can be libwaku (just replacing the name occupied by previous unused target).

jm-clius · 2023-05-05T07:55:01Z

apps/wakunode2/wakunode2.nim

@@ -92,20 +94,19 @@ when isMainModule:
    error "4/7 Mounting protocols failed", error=res5.error
    quit(QuitFailure)

+proc startNode*() =


Quite confused about this. Does this split the application's setup into an init and start phase?

Exaclty, that is a simple split. However that might not be needed, and I will likely revert it back, when I wrap the waku_node.nim library instead of the wakunode2 app.

jm-clius · 2023-05-05T08:17:44Z

apps/wakunode2/wakunode2.nim

+proc subscribeCallbackToTopic*(pubSubTopic: cstring,
+                               callback: proc(pubsubTopic: string, data: seq[byte]): Future[void] {.gcsafe, raises: [Defect].}) =
+  wakunode2.subscribeCallbackToTopic(pubSubTopic, callback)
+
+proc unsubscribeCallbackFromTopic*(pubSubTopic: cstring,
+                                   callback: proc(pubsubTopic: string, data: seq[byte]): Future[void] {.gcsafe, raises: [Defect].}) =
+  wakunode2.unsubscribeCallbackFromTopic(pubSubTopic, callback)
+
+proc unsubscribeAllCallbacksFromTopic*(pubSubTopic: cstring) =
+  wakunode2.unsubscribeAllCallbackFromTopic(pubSubTopic)
+
+proc publishMessage*(pubSubTopic: cstring, message: WakuMessage): Future[int] {.gcsafe, async.} =
+  return await wakunode2.publishMessage(pubSubTopic, message)


The way I see it, these do not belong here. The C bindings should be a wrapper around the Nim API defined in waku_node.nim. As such the app modules should remain unchanged and preferably unimported elsewhere. Any additional API functionality should go to waku_node.nim. Of course, since we need a configured WakuNode object for those bindings, we may have to provide App-like functionality when initiating the node. For now, though I think the configuration will be much, much simpler than is needed for a wakunode2 application and providing the basic configuration in a JsonConfig via waku_new() will be enough.

jm-clius · 2023-05-05T08:23:26Z

library/cwakuv2.nim

+proc show_version(): cstring {.dynlib, exportc.} =
+  return wakuNode2VersionString


I think this can be waku_version(). waku_node already provides git_version, so I don't think this needs to be exported elsewhere. If we need to construct something specific, that should be done on the Nim API in waku_node.nim.

jm-clius · 2023-05-05T08:26:45Z

apps/wakunode2/wakunode2.nim


-  const versionString = "version / git commit hash: " & app.git_version
+{.pop.} # @TODO confutils.nim(775, 17) Error: can raise an unlisted exception: ref IOError
+proc init*(configFilePath = "") =


I don't think we should init an app here, but operate on the WakuNode object as defined in waku_node.nim (see comments elsewhere). wakunode2 is simply an application written around the Nim API. As stated elsewhere, we want to provide C bindings wrapping the Nim API, not the wakunode2 application

…waku_*)

Ivansete-status · 2023-05-11T17:47:09Z

Thanks for the comments ! It is now ready to be reviewed again :)

Btw, very happy for the hint that you shared with me @LNSD. Thanks for that! The approach that is applied in this PR is inspired by https://github.com/KomodoPlatform/nimbus/blob/master/wrappers/wrapper_example.c

jm-clius

Really great work with this. Well done! I've added a couple of non-blocking comments. Feel free to merge once those are addressed.

I would suggest thinking of ways in which we can split the logic into separate modules in future (the main library module is going to grow very large!) I'd suggest using the REST API as inspiration and separate relay, store and other protocol bindings as well as types, serdes, etc. For now this is OK as a starting point.

jm-clius · 2023-05-12T08:12:49Z

library/waku.nim

+  chronos,
+  libp2p/crypto/secp,
+  stew/shims/net,
+  eth/net/nat as todo_delete_this_module


Leftover? :)

I will clean the setupNat, once this is merged, in a separate PR :)

jm-clius · 2023-05-12T08:14:32Z

library/waku.nim

+  ../../waku/v2/waku_core/message/message,
+  ../../waku/v2/waku_core/topics/pubsub_topic,
+  ../../waku/v2/node/peer_manager/peer_manager,
+  ../../waku/v2/node/waku_node as waku_node_module,


Nitpick: is this alias necessary?

True, is not needed. Thanks!

jm-clius · 2023-05-12T08:15:13Z

library/waku.nim

@@ -0,0 +1,399 @@
+


I would consider calling this module something like libwaku.

jm-clius · 2023-05-12T08:22:33Z

library/waku.nim

+proc waku_pubsub_topic(topicName: cstring, encoding: cstring, outPubsubTopic: var string) {.dynlib, exportc.} =
+  # https://rfc.vac.dev/spec/36/#extern-char-waku_pubsub_topicchar-name-char-encoding
+  outPubsubTopic = fmt"/waku/2/{topicName}/{encoding}"


Note that for pubsub topics we've deprecated the {encoding} suffix (https://rfc.vac.dev/spec/23/#pubsub-topic-format).

jm-clius · 2023-05-12T08:34:21Z

library/waku.nim

+  let connectFut = node.connectToNodes(peers, source="static")
+  while not connectFut.finished():
+    poll()


Wait, where does timeoutMs come in here?

Indeed, this is something we need to fix from lower levels. I'll add a TODO statement here.

jm-clius · 2023-05-12T08:37:31Z

library/waku.nim

+  # }
+  return $(%* { "error": message })
+
+proc setupNat(natConf, clientId: string, tcpPort, udpPort: Port):


I must be missing some details here, but why wouldn't setupNat in the common utility work?

The setupNat in the common is deprecated with the next comment: deprecated: "Unsafe: this proc quits the app if something is not ok".

Once this PR is fixed I want to raise a separate PR to fix the setupNat proc from common and only use it. Also, clean the eth/net/nat as todo_delete_this_module :)

jm-clius · 2023-05-12T08:41:05Z

waku/v2/node/waku_node.nim

@@ -71,6 +70,7 @@ const clientId* = "Nimbus Waku v2 node"
 # Default Waku Filter Timeout
 const WakuFilterTimeout: Duration = 1.days

+const wakuNode2VersionString* = "version / git commit hash: " & git_version


It's not followed everywhere, but we want constants to be in PascalCase (the exception are for those overridden by compiler definitions, such as git_version above).
This would just be WakuNodeVersionString (not WakuNode2)

jm-clius

Really great work with this. Well done! I've added a couple of non-blocking comments. Feel free to merge once those are addressed.

I would suggest thinking of ways in which we can split the logic into separate modules in future (the main library module is going to grow very large!) I'd suggest using the REST API as inspiration and separate relay, store and other protocol bindings as well as types, serdes, etc. For now this is OK as a starting point.

richard-ramos

LGTM!

richard-ramos · 2023-05-12T12:24:18Z

library/libwaku.nim

+proc waku_poll() {.dynlib, exportc.} =
+  poll()


What is this function used for?

The poll() function is part of the asynchronous chronos library. It is needed to make the Nim runtime to take steps further.
For each call to poll() checks whether there are OS signals, or network events. This is therefore needed to make the Nwaku active so that it can attend and reply requests.

https://github.com/status-im/nim-chronos/blob/ab5a8c2e0f6941fe3debd61dff0293790079d1b0/chronos/asyncloop.nim#LL822C16-L822C26

For example, in the Nwaku app, there is a runForever() call that is an infinite loop around poll().
https://github.com/waku-org/nwaku/blob/2ec9809cf31b202ee705d337cf3384fe0f7374f4/apps/wakunode2/wakunode2.nim#LL151C4-L151C4

richard-ramos · 2023-05-12T12:27:56Z

library/libwaku.nim

+  if node.wakuRelay == nil:
+    jsonResp = errResp("""Cannot unsubscribe without a callback.
+Kindly set it with the 'waku_set_event_callback' function""")
+    return false
+
+  if node.wakuRelay.isNil():


for learning purposes, what's the difference between wakuRelay == nil and wakuRelay.isNil()?

Good point my friend! Actually you discovered a bug hehe.

It seems isNil() is more efficient, so I'd rather use it from now on.

proc isNil*[T](x: ptr T): bool {.noSideEffect, magic: "IsNil".}
proc isNil*(x: pointer): bool {.noSideEffect, magic: "IsNil".}
proc isNil*(x: cstring): bool {.noSideEffect, magic: "IsNil".}
proc isNil*[T: proc](x: T): bool {.noSideEffect, magic: "IsNil".}
Fast check whether x is nil. This is sometimes more efficient than ## == nil.

feat(cbindings): first commit - waku relay (#1632)

55c7b38

Ivansete-status requested review from jm-clius, alrevuelta, rymnc, vpavlin and richard-ramos May 4, 2023 15:14

test_app.nim: fix compilation issue. App.init(..) -> App.new(..)

8ef3002

richard-ramos reviewed May 4, 2023

View reviewed changes

Ivansete-status self-assigned this May 5, 2023

jm-clius requested changes May 5, 2023

View reviewed changes

Ivansete-status added 5 commits May 5, 2023 17:02

Simplifying library name (libwaku) and standardizing function names (…

84ebb32

…waku_*)

Proper wrapper of the waku_node API and creation of the libwaku.a

b21b52d

Rolling back changes that are not needed

43557e0

Rolling back changes that are out of the scope of this task

c89b6c4

wakunode.nim: Removing unnecessary import

c2760a4

Ivansete-status requested review from jm-clius and richard-ramos May 11, 2023 17:47

jm-clius approved these changes May 12, 2023

View reviewed changes

Ivansete-status added 2 commits May 12, 2023 13:52

Aplying PR suggestions

2c23e65

Renaming 'waku.h' -> 'libwaku.h'

b9eae6e

richard-ramos approved these changes May 12, 2023

View reviewed changes

Ivansete-status added 2 commits May 12, 2023 14:43

Use of 'isNil' instead of '== nil'

13b1368

libwaku.nim: explicitly setting waku_poll() as gcsafe

9eab90e

Ivansete-status merged commit 2defbd2 into master May 12, 2023

Ivansete-status deleted the feat-1632-cbindings-relay branch May 12, 2023 16:08

vpavlin mentioned this pull request May 16, 2023

feat: add release-notes target #1734

Merged

1 task

Ivansete-status mentioned this pull request May 16, 2023

docs(changelog): release notes for v0.17.0 #1735

Merged

		proc show_version(): cstring {.dynlib, exportc.} =
		return wakuNode2VersionString

feat(cbindings): first commit - waku relay (#1632) #1714

feat(cbindings): first commit - waku relay (#1632) #1714

Conversation

Ivansete-status commented May 4, 2023 • edited Loading

Description

Changes

How to test

Issue

Pending tasks

richard-ramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status May 11, 2023 • edited Loading

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status commented May 11, 2023 • edited Loading

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

richard-ramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status commented May 4, 2023 •

edited

Loading

Ivansete-status May 11, 2023 •

edited

Loading

Ivansete-status commented May 11, 2023 •

edited

Loading