feat: running validators in REST API #2373

gabrielmer · 2024-01-26T14:44:45Z

Description

We currently add message validators to libp2p's gossipsub, which are run before sending a message. In this PR, we're running the validators in POST /relay/v1/messages/{pubsubTopic} REST handler in order to catch earlier any wrong message and be able to return the user an error in case it's invalid and the message is not published.

In addition to that, a check for messages bigger than the maximum configured size was added to the handler. That's also a check currently made at the libp2p layer before relaying the message, and we want to catch these messages earlier in the flow.

A big change of this PR is that we got rid of topic-specific validators. Now the same validators will run for every pubsub topic.

After completing this PR, a new PR will be opened implementing the same validation logic for other autosharding and lightpush's REST handlers.

Changes

removed topic-specific validators and converted them to general validators
running validators in POST /relay/v1/messages/{pubsubTopic} REST handler
checking for max message size in the above REST handler
implemented test to check that the max size validator is run at the handler

Issue

Part of #2284

github-actions · 2024-01-26T14:49:26Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2373

Built from 2f820b7

SionoiS

👍 This approach seams sensible. No strong opinion either way. Maybe WakuMessages could check their own size?

tests/waku_relay/test_protocol.nim

gabrielmer · 2024-01-26T17:36:00Z

waku/waku_api/rest/relay/handlers.nim

+      (await node.wakuRelay.validateMessage(pubsubTopic, message)).isOkOr:
+        return RestApiResponse.badRequest("Failed to publish: " & error)


Notice that for RLN's case for example, the validator's inside logic is used directly in the handler and therefore we had more possible error messages to return the user.

Once we use our validators directly, which only return if validation passed or not, we can only have one possible error message per validator.

The specific details still get logged, but one con of this approach vs hardcoding the checks in the handlers is that we can only have one unique error message per validator

gabrielmer · 2024-01-29T16:49:24Z

👍 This approach seams sensible. No strong opinion either way. Maybe WakuMessages could check their own size?

Makes sense! Thought about it and found 2 issues to this.

First one, is that we don't use a WakuMessage.init() proc or something similar that runs logic when creating a WakuMessage, we just assign values to the struct as they are. We could create one, but not sure if refactoring it in the whole code only for this use case is worth it.

Apart from that, the easiest way to calculate the size in bytes of a WakuMessage is to create the struct and then check its size. We could calculate the size of all its separate fields and add them together, but that might be prone to bugs in case we add a field or make any change in the type.

LMK what you all think :))

Ivansete-status

Thanks for the PR!
I've added a few comments. If possible, I'd try to simplify the approach and just have one kind of validator (avoiding the use of the generic/default validators list) but ofc I might be missing something :)

tests/waku_relay/test_protocol.nim

tests/waku_relay/test_wakunode_relay.nim

waku/node/waku_node.nim

waku/waku_relay/protocol.nim

SionoiS · 2024-01-29T20:44:53Z

Apart from that, the easiest way to calculate the size in bytes of a WakuMessage is to create the struct and then check its size. We could calculate the size of all its separate fields and add them together, but that might be prone to bugs in case we add a field or make any change in the type.

What I was thinking is something like

if not myMessage.isWakuValid():
   return err("Max size 150KB! This message is too large per Waku RFC 64.")

if not myMessage.isGossipSubValid():
   return err("Max size 1MiB! This message is too large for GossipSub.")

Plus some tests.

gabrielmer · 2024-01-30T09:43:46Z

Apart from that, the easiest way to calculate the size in bytes of a WakuMessage is to create the struct and then check its size. We could calculate the size of all its separate fields and add them together, but that might be prone to bugs in case we add a field or make any change in the type.

What I was thinking is something like
if not myMessage.isWakuValid():
   return err("Max size 150KB! This message is too large per Waku RFC 64.")

if not myMessage.isGossipSubValid():
   return err("Max size 1MiB! This message is too large for GossipSub.")
Plus some tests.

Oooh got it! So basically having the check inside a proc. That's cool, 100% can be useful and nicer :)

NagyZoltanPeter

Overall thank you for this great stuff. I agree with the comments added before me, but I can approve it from my side, bet you will answer the rest. ;-)

waku/waku_relay/protocol.nim

tests/waku_relay/test_protocol.nim

waku/node/waku_node.nim

waku/waku_api/rest/relay/handlers.nim

SionoiS · 2024-01-30T13:16:44Z

@gabrielmer you are aware of #1700 right?

gabrielmer · 2024-01-30T13:49:55Z

@gabrielmer you are aware of #1700 right?

Ooooh, was definitely not aware of that GitHub issue! I was aware of the overall idea, that's why I took this approach here with validators instead of hardcoding the checks.

Thanks!

gabrielmer · 2024-01-30T16:17:16Z

Changing PR to draft as there's still many changes to be done

gabrielmer · 2024-01-31T16:31:59Z

waku/waku_relay/protocol.nim

    return ValidationResult.Accept
  return wrappedValidator

+proc isValidSize(message: WakuMessage): Future[Result[void, string]] {.async.} =


I'll also add to this proc a check that the message size is smaller than GossipSub's max.

Currently the max size is defined in nim-libp2p here: https://github.com/status-im/nim-libp2p/blob/e3c967ad1939fb33b8e13759037d193734acd202/libp2p/protocols/pubsub/pubsub.nim#L556

I don't want to have it hardcoded, so I'll first open a PR in nim-libp2p defining it as a global const and will then add an if condition in this proc

gabrielmer · 2024-01-31T16:46:46Z

I think I implemented all the feedback received, please let me know if I missed something or if there's anything else we can improve :)

So this PR doesn't get way too big, I'll implement the same validation logic in a separate PR for lightpush's and autosharding's REST handlers.

SionoiS

LGTM thanks!

Ivansete-status

Looks great indeed! However, I can't approve yet as per the doubt I have re rest/relay/handlers.nim :)

apps/wakunode2/wakunode2_validator_signed.nim

Ivansete-status · 2024-02-01T10:15:17Z

waku/waku_api/rest/relay/handlers.nim

@@ -135,16 +135,8 @@ proc installRelayApiHandlers*(router: var RestRouter, node: WakuNode, cache: Mes
      if not success:
        return RestApiResponse.internalServerError("Failed to publish: error appending RLN proof to message")

-      # validate the message before sending it
-      let result = node.wakuRlnRelay.validateMessageAndUpdateLog(message)


Why are we removing this wakuRlnRelay validation? This is quite important :) Or are we making this validation in another place?

Yes!

So now we register the validator here:

nwaku/waku/node/waku_node.nim

Line 990 in 6171f31

node.wakuRelay.addValidator(validator, "RLN validation failed")

And this runs in the REST handler here:

nwaku/waku/waku_api/rest/relay/handlers.nim

Line 138 in 6171f31

(await node.wakuRelay.validateMessage(pubsubTopic, message)).isOkOr:

So instead of hardcoding again the RLN validation in the handler, this gets run when going over all the registered validators

Beautiful!, I see that the validation is performed in the following place:

nwaku/waku/waku_rln_relay/rln_relay.nim

Line 331 in 3e65cc1

let validationRes = wakuRlnRelay.validateMessageAndUpdateLog(message)

Ivansete-status

LGTM! Thanks for the master-piece PR and the patience! 🥳

Ivansete-status · 2024-02-01T10:45:06Z

waku/waku_api/rest/relay/handlers.nim

@@ -135,16 +135,8 @@ proc installRelayApiHandlers*(router: var RestRouter, node: WakuNode, cache: Mes
      if not success:
        return RestApiResponse.internalServerError("Failed to publish: error appending RLN proof to message")

-      # validate the message before sending it
-      let result = node.wakuRlnRelay.validateMessageAndUpdateLog(message)


Beautiful!, I see that the validation is performed in the following place:

nwaku/waku/waku_rln_relay/rln_relay.nim

Line 331 in 3e65cc1

let validationRes = wakuRlnRelay.validateMessageAndUpdateLog(message)

gabrielmer marked this pull request as draft January 26, 2024 14:45

gabrielmer self-assigned this Jan 26, 2024

gabrielmer force-pushed the chore-add-message-size-check-in-rest-3 branch from c71585e to 8cf79cd Compare January 26, 2024 17:07

gabrielmer marked this pull request as ready for review January 26, 2024 17:36

gabrielmer requested review from Ivansete-status, SionoiS and NagyZoltanPeter January 26, 2024 17:36

SionoiS reviewed Jan 26, 2024

View reviewed changes

gabrielmer commented Jan 29, 2024

View reviewed changes

Ivansete-status reviewed Jan 29, 2024

View reviewed changes

NagyZoltanPeter approved these changes Jan 30, 2024

View reviewed changes

waku/waku_relay/protocol.nim Outdated Show resolved Hide resolved

waku/waku_relay/protocol.nim Outdated Show resolved Hide resolved

tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved

waku/node/waku_node.nim Outdated Show resolved Hide resolved

alrevuelta reviewed Jan 30, 2024

View reviewed changes

waku/waku_api/rest/relay/handlers.nim Outdated Show resolved Hide resolved

gabrielmer force-pushed the chore-add-message-size-check-in-rest-3 branch 2 times, most recently from d82b19c to 298fe4c Compare January 30, 2024 12:14

chair28980 mentioned this pull request Jan 30, 2024

bug: multiple messages published in the same second via RLN RELAY are not dropped #2371

Closed

gabrielmer marked this pull request as draft January 30, 2024 16:16

gabrielmer force-pushed the chore-add-message-size-check-in-rest-3 branch from b919235 to df60564 Compare January 31, 2024 10:39

gabrielmer commented Jan 31, 2024

View reviewed changes

gabrielmer marked this pull request as ready for review January 31, 2024 16:36

gabrielmer requested review from Ivansete-status, alrevuelta and SionoiS January 31, 2024 16:47

SionoiS approved these changes Jan 31, 2024

View reviewed changes

gabrielmer changed the title ~~feat: running validators in REST API (in progress)~~ feat: running validators in REST API Jan 31, 2024

Ivansete-status reviewed Feb 1, 2024

View reviewed changes

Ivansete-status approved these changes Feb 1, 2024

View reviewed changes

gabrielmer added 20 commits February 1, 2024 17:19

adding default validators and including RLN validator

a2d454a

deleting empty spaces

0aa92d8

Initial validation implementation

5ed496a

Adding custom error messages for validators

c6bb9db

cleaning test for max size validation

f257955

Deleting empty spaces and comments

0ccb18c

fixing comments

2962e61

refactoring setting error messages

6a6e68d

improving error messages

11c5846

fixing varargs argument passing

41c81aa

Initial attempt (not complete) to remove pubsub specific validators

81e3b29

updating test suite

2e1d8ef

fix ProtectedTopic instantiation

22a20bb

re-adding validatorInserted map

9d894f0

cleaning

b849143

Changing log level of message validator error log

db780aa

Adding validator errors as a tuple with the validators

a912fa9

Changing message size validation

09d58f1

small fixes

c8b704c

Aesthetic changes

f08420f

gabrielmer force-pushed the chore-add-message-size-check-in-rest-3 branch from 6171f31 to f08420f Compare February 1, 2024 16:45

gabrielmer merged commit 59d8b62 into master Feb 1, 2024
9 of 10 checks passed

gabrielmer deleted the chore-add-message-size-check-in-rest-3 branch February 1, 2024 17:16

gabrielmer mentioned this pull request Feb 2, 2024

feat: running validators in /relay/v1/auto/messages/{topic} #2394

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: running validators in REST API #2373

feat: running validators in REST API #2373

gabrielmer commented Jan 26, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024 •

edited

Loading

SionoiS left a comment •

edited

Loading

gabrielmer Jan 26, 2024

gabrielmer commented Jan 29, 2024

Ivansete-status left a comment

SionoiS commented Jan 29, 2024

gabrielmer commented Jan 30, 2024

NagyZoltanPeter left a comment

SionoiS commented Jan 30, 2024

gabrielmer commented Jan 30, 2024

gabrielmer commented Jan 30, 2024

gabrielmer Jan 31, 2024

gabrielmer commented Jan 31, 2024 •

edited

Loading

SionoiS left a comment

Ivansete-status left a comment

Ivansete-status Feb 1, 2024

gabrielmer Feb 1, 2024

Ivansete-status Feb 1, 2024

Ivansete-status left a comment

Ivansete-status Feb 1, 2024

		(await node.wakuRelay.validateMessage(pubsubTopic, message)).isOkOr:
		return RestApiResponse.badRequest("Failed to publish: " & error)

feat: running validators in REST API #2373

feat: running validators in REST API #2373

Conversation

gabrielmer commented Jan 26, 2024 • edited Loading

Description

Changes

Issue

github-actions bot commented Jan 26, 2024 • edited Loading

SionoiS left a comment • edited Loading

Choose a reason for hiding this comment

gabrielmer Jan 26, 2024

Choose a reason for hiding this comment

gabrielmer commented Jan 29, 2024

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS commented Jan 29, 2024

gabrielmer commented Jan 30, 2024

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

SionoiS commented Jan 30, 2024

gabrielmer commented Jan 30, 2024

gabrielmer commented Jan 30, 2024

gabrielmer Jan 31, 2024

Choose a reason for hiding this comment

gabrielmer commented Jan 31, 2024 • edited Loading

SionoiS left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Ivansete-status Feb 1, 2024

Choose a reason for hiding this comment

gabrielmer Feb 1, 2024

Choose a reason for hiding this comment

Ivansete-status Feb 1, 2024

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Ivansete-status Feb 1, 2024

Choose a reason for hiding this comment

gabrielmer commented Jan 26, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024 •

edited

Loading

SionoiS left a comment •

edited

Loading

gabrielmer commented Jan 31, 2024 •

edited

Loading