chore(api): validate rln message before sending (rest + rpc) #1968

alrevuelta · 2023-08-30T11:46:51Z

Description:

Adds validation to rest+rpc api so that an rln message proof is verified before sending the message. If the proof is wrong or the message is considered spam, the message won't be sent and it will error out so that the user is aware that the message didn't made it.
Adds validation to rest+rpc api so that it errors out when trying to publish to a topic that the node is not subcribed, since it risks that there are no connected peers "listening" in that topic.
Update tests so that they work with rln enabled/disabled.

How to test it:

./build/wakunode2 \
--rln-relay=true \
--rpc-address=0.0.0.0 \
--rpc-admin=true \
--rest=true \
--rest-admin=true \
--rest-private=true \
--rest-address=0.0.0.0 \
--rln-relay-dynamic=true \
--rln-relay-cred-password=password \
--rln-relay-cred-path=rlnKeystore_420.json \
--rln-relay-membership-index=420 \
--rln-relay-tree-path=rln_tree_1.db \
--rln-relay-eth-contract-address=0x8e1F3742B987d8BA376c0CBbD7357fE1F003ED71  \
--rln-relay-eth-client-address=wss://you_endpoint \
--log-level=debug

Rest API

curl -X 'POST'   'localhost:8645/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto'   -H 'accept: */*'   -H 'Content-Type: application/json'   -d '{
  "payload": "string",
  "contentTopic": "string",
  "version": 0,
  "timestamp": 0
}'

RPC API

curl http://localhost:8545 -d '{"jsonrpc":"2.0","method":"post_waku_v2_relay_v1_message","params":["/waku/2/default-waku/proto", {"payload": "SGkgdGhlcmUgZXRoY2M=", "timestamp": 1, "contentTopic": "some-topic", "ephemeral": false}],"id":1}' -H 'Content-Type: application/json'

alrevuelta · 2023-08-30T11:50:58Z

waku/node/jsonrpc/relay/handlers.nim

+          raise newException(ValueError, "Failed to publish: error appending RLN proof to message")
+
+        # validate the message before sending it
+        let result = node.wakuRlnRelay.validateMessage(message)


unsure if its ok to use validateMessage here, since inside it updates a bunch of metrics and call updateLog. Any opinion @rymnc

yeah I don't think we should use validateMessage, because if you try publishing the same message it would get stuck since the proofMetadata would exist in the nullifierLog afaik. should probably split the function into the check, and then the state update

Something like this 74d38f5 ?

github-actions · 2023-08-30T11:51:41Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1968

vpavlin

lgtm

alrevuelta · 2023-08-31T16:36:21Z

waku/waku_rln_relay/rln_relay.nim

  let rootIndex = rlnPeer.groupManager.indexOfRoot(proof.merkleRoot)
  waku_rln_valid_messages_total.observe(rootIndex.toFloat())
  return MessageValidationResult.Valid

+proc validateMessageAndUpdateLog*(


@rymnc created this function, which is like the old validateMessage but now we have:

validateMessage

validateMessageAndUpdateLog

Ivansete-status

LGTM! Thanks for it! I just added a question to learn sth :)

Ivansete-status · 2023-09-01T06:28:54Z

waku/waku_relay/protocol.nim

+proc subscribedTopics*(w: WakuRelay): seq[PubsubTopic] =
+  return toSeq(GossipSub(w).topics.keys())


Morning! Is this directly related to the purpose of the PR?

nope, but imho there is no need to use generators for this. the amount of topics is not enough to justify it. also using poorly documented features such as lent lent is a bad idea, unless strictly needed.

rymnc

LGTM, minor comment

rymnc · 2023-09-01T07:35:10Z

waku/node/jsonrpc/relay/handlers.nim

+        if result == MessageValidationResult.Invalid:
+          raise newException(ValueError, "Failed to publish: invalid RLN proof")
+        elif result == MessageValidationResult.Spam:
+          raise newException(ValueError, "Failed to publish: limit exceeded, try again later")
+        elif result == MessageValidationResult.Valid:
+          debug "Publishing message WITH RLN proof", pubSubTopic=pubSubTopic
+          let publishFut = node.publish(pubsubTopic, message)
+          if not await publishFut.withTimeout(futTimeout):
+            raise newException(ValueError, "Failed to publish: timed out")
+        else:
+          raise newException(ValueError, "Failed to publish: unknown RLN proof validation result")
+      else:
+        raise newException(ValueError, "Failed to publish: RLN enabled but not mounted")
+
+    # if RLN is not mounted, publish the message as is
+    else:
+      debug "Publishing message WITHOUT RLN proof", pubSubTopic=pubSubTopic
+      let publishFut = node.publish(pubsubTopic, message)
+
+      if not await publishFut.withTimeout(futTimeout):
+        raise newException(ValueError, "Failed to publish: timed out")


I think these ValueError's can be CatchableError's

also, maybe we can operate on the same message in both cases? i.e rln is enabled, rln is disabled?
then we can save some loc by having just one

let publishFut = node.publish(pubsubTopic, message) if not await publishFut.withTimeout(futTimeout): raise newException(ValueError, "Failed to publish: timed out")

ow indeed, that was a bit spageti.
c964916

alrevuelta mentioned this pull request Aug 30, 2023

chore(rln-relay): Requirements to consider RLN ready (non experimental) #1906

Closed

56 tasks

alrevuelta commented Aug 30, 2023

View reviewed changes

alrevuelta requested review from rymnc and NagyZoltanPeter August 30, 2023 11:51

alrevuelta force-pushed the simulate-tx-before-sending branch from 74d38f5 to f5ba118 Compare August 31, 2023 10:44

alrevuelta marked this pull request as ready for review August 31, 2023 10:45

alrevuelta requested a review from vpavlin August 31, 2023 10:45

vpavlin approved these changes Aug 31, 2023

View reviewed changes

alrevuelta force-pushed the simulate-tx-before-sending branch from 6b76a8c to c130158 Compare August 31, 2023 16:34

alrevuelta commented Aug 31, 2023

View reviewed changes

Ivansete-status approved these changes Sep 1, 2023

View reviewed changes

alrevuelta added the E:2023-rln label Sep 1, 2023

rymnc approved these changes Sep 1, 2023

View reviewed changes

alrevuelta added 5 commits September 1, 2023 13:36

chore(api): validate rln message before sending (rest + rpc)

96a55cb

take updateLog out

6a4b28b

fix tests

aa3c96a

create validateMessageAndUpdateLog function

0bcc43e

Remove redundant code

c964916

alrevuelta force-pushed the simulate-tx-before-sending branch from c130158 to c964916 Compare September 1, 2023 11:37

alrevuelta merged commit 05c9886 into master Sep 1, 2023
13 checks passed

alrevuelta deleted the simulate-tx-before-sending branch September 1, 2023 13:04

rymnc mentioned this pull request Sep 11, 2023

RLN key mechanism and proofs for all publishers waku-org/pm#54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(api): validate rln message before sending (rest + rpc) #1968

chore(api): validate rln message before sending (rest + rpc) #1968

alrevuelta commented Aug 30, 2023 •

edited

alrevuelta Aug 30, 2023

rymnc Aug 30, 2023 •

edited

alrevuelta Aug 30, 2023

rymnc Aug 30, 2023

github-actions bot commented Aug 30, 2023

vpavlin left a comment

alrevuelta Aug 31, 2023

Ivansete-status left a comment

Ivansete-status Sep 1, 2023

alrevuelta Sep 1, 2023

rymnc left a comment

rymnc Sep 1, 2023

rymnc Sep 1, 2023

alrevuelta Sep 1, 2023

		proc subscribedTopics*(w: WakuRelay): seq[PubsubTopic] =
		return toSeq(GossipSub(w).topics.keys())

chore(api): validate rln message before sending (rest + rpc) #1968

chore(api): validate rln message before sending (rest + rpc) #1968

Conversation

alrevuelta commented Aug 30, 2023 • edited

Choose a reason for hiding this comment

rymnc Aug 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 30, 2023

vpavlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc 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

alrevuelta commented Aug 30, 2023 •

edited

rymnc Aug 30, 2023 •

edited