feat(relay): ordered validator execution #1966

rymnc · 2023-08-30T05:23:22Z

Description

Important

This PR modifies waku_relay code and must be reviewed strictly :)
Maybe we should run it in waku-simulator?

nim-libp2p does not order the execution of validators
https://github.com/status-im/nim-libp2p/blob/66f9dc9167f5ba8e5b6867be54f3e63052b93797/libp2p/protocols/pubsub/pubsub.nim#L520-L526

but for our validators (signedTopicValidator, Rln), we need to guarantee order of execution. This PR sets the foundation
for ordering the validators (they should all be appended sequentially)

Changes

Removed bandwidth constraint validator from rln relay
left the config option in though, since it is still relevant (--rln-relay-bandwidth-threshold)
Modified WakuRelay.addValidator to accept validators that accept WakuMessage's instead of nim-libp2p Message's (thereby saving the extra decoding)
Updated SignedTopicValidator and RlnRelay to reflect the same
Tests updated

Issue

closes #1967

waku/waku_rln_relay/rln_relay.nim

github-actions · 2023-08-30T05:32:44Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1966

rymnc · 2023-08-30T07:48:23Z

hmm looks like the tests fail where it shouldn't - valid rln messages are not hitting the relayHandlers -

nwaku/tests/waku_rln_relay/test_wakunode_rln_relay.nim

Lines 230 to 234 in 0ac8a7f

    
           var completionFut = newFuture[bool]() 
        
           proc relayHandler(topic: PubsubTopic, msg: WakuMessage): Future[void] {.async, gcsafe.} = 
        
             debug "The received topic:", topic 
        
             if topic == DefaultPubsubTopic: 
        
               completionFut.complete(true)

alrevuelta

left some comments, perhaps one is related to your problem?

waku/waku_relay/protocol.nim

Ivansete-status

LGTM! I've added a few comments/questions :)

Btw, I believe we should move the "validator" logic to a more appropriate place.
Given that the validation is part of the Relay protocol, I think we need to make the next refactoring:

1. Create a validators folder in "nwaku/waku/waku_relay/"
1. Move the logic of apps/wakunode2/wakunode2_validator_signed.nim into nwaku/waku/waku_relay/validators/signed_topic_validator.nim
1. Move the RLN validation logic to nwaku/waku/waku_relay/validators/rln_validator.nim

WDYT @jm-clius , @alrevuelta , @rymnc ?

Ivansete-status · 2023-08-31T15:38:34Z

waku/node/waku_node.nim

@@ -749,7 +749,7 @@ when defined(rln):
    # register rln validator for all subscribed relay pubsub topics
    for pubsubTopic in node.wakuRelay.subscribedTopics:
      debug "Registering RLN validator for topic", pubsubTopic=pubsubTopic
-      procCall GossipSub(node.wakuRelay).addValidator(pubsubTopic, validator)
+      node.wakuRelay.addValidator(pubsubTopic, validator)


Is this change directly related to the purpose of reordering the validators?

yep, before we were calling gossipsubs addValidator. with this now we call wakuRelay addValidator.

In other words, we override addValidator with a new implementation.

yes, this is. as @jm-clius described below, we will create a new waku_validators directory where we initialize them in the order we wish for them to execute

Ivansete-status · 2023-08-31T15:59:14Z

waku/waku_relay/protocol.nim

+  # add the ordered validator to the topic
+  if not w.validatorInserted.hasKey(pubSubTopic):
+    procCall GossipSub(w).addValidator(pubSubTopic, w.generateOrderedValidator())
+    w.validatorInserted[pubSubTopic] = true


Should we update this Table upon an unsubscribe event?

this seems important right?

Thanks, addressed in 893ddf5

waku/waku_relay/protocol.nim

Ivansete-status · 2023-08-31T16:06:48Z

waku/waku_relay/protocol.nim

 proc isSubscribed*(w: WakuRelay, topic: PubsubTopic): bool =
  GossipSub(w).topics.hasKey(topic)

 iterator subscribedTopics*(w: WakuRelay): lent PubsubTopic =
  for topic in GossipSub(w).topics.keys():
    yield topic

+proc generateOrderedValidator*(w: WakuRelay): auto {.gcsafe.} =


The 'Ordered' in this case refers to first validating the basic Waku Message encoding, and then, the rest?

Sort of, we describe here 1 single validator that gossipsub calls back to - and we control the order of

Ivansete-status · 2023-08-31T16:16:21Z

waku/waku_relay/protocol.nim

@@ -122,7 +121,10 @@ const GossipsubParameters = GossipSubParams(
 type
  WakuRelayResult*[T] = Result[T, string]
  WakuRelayHandler* = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[void] {.gcsafe, raises: [Defect].}
+  WakuValidatorHandler* = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[ValidationResult] {.gcsafe, raises: [Defect].}


Suggested change

WakuValidatorHandler* = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[ValidationResult] {.gcsafe, raises: [Defect].}

WakuValidatorHandler = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[ValidationResult] {.gcsafe, raises: [Defect].}

I believe this should be public so other protocols may use this to better describe the handlers

Ivansete-status · 2023-08-31T16:19:47Z

tests/waku_rln_relay/test_waku_rln_relay.nim

@@ -710,61 +710,6 @@ suite "Waku rln relay":
      msgValidate3 == MessageValidationResult.Valid
      msgValidate4 == MessageValidationResult.Invalid

-  asyncTest "should validate invalid proofs if bandwidth is available":


Ooh what a pitty. Why we are removing that? I am happy with it although not fully understanding the test logic ;P

yep, any reason why removing it? having some coverage for validateMessage is good.

this pr also removes the bandwidth constraint validator, which should be a separate validator!

alrevuelta · 2023-08-31T16:44:11Z

LGTM! I've added a few comments/questions :)

Btw, I believe we should move the "validator" logic to a more appropriate place. Given that the validation is part of the Relay protocol, I think we need to make the next refactoring:
* 1. Create a `validators` folder in "nwaku/waku/waku_relay/"

* 2. Move the logic of `apps/wakunode2/wakunode2_validator_signed.nim` into `nwaku/waku/waku_relay/validators/signed_topic_validator.nim`

* 3. Move the RLN validation logic to `nwaku/waku/waku_relay/validators/rln_validator.nim`
WDYT @jm-clius , @alrevuelta , @rymnc ?

Kind of agree but its a bit more complex than that. While rln is very coupled with relay, they are still kind of separate things. I would be happy to "merge" both since I don't conceive relay without rln, but not sure everyone agrees. Until we agree on this, 1. would be blocked. Same applies to 3.

This validator is a bit different, since its not part of waku protocol. Is some kind of special validator app level validator, thats why its in the app folder. If you ask me, I would completely remove this validator. But since some people may be using it, not sure until when, guess we can't.

Ivansete-status · 2023-09-01T07:19:37Z

LGTM! I've added a few comments/questions :)
Btw, I believe we should move the "validator" logic to a more appropriate place. Given that the validation is part of the Relay protocol, I think we need to make the next refactoring:
* 1. Create a `validators` folder in "nwaku/waku/waku_relay/"

* 2. Move the logic of `apps/wakunode2/wakunode2_validator_signed.nim` into `nwaku/waku/waku_relay/validators/signed_topic_validator.nim`

* 3. Move the RLN validation logic to `nwaku/waku/waku_relay/validators/rln_validator.nim`
WDYT @jm-clius , @alrevuelta , @rymnc ?
Kind of agree but its a bit more complex than that. While rln is very coupled with relay, they are still kind of separate things. I would be happy to "merge" both since I don't conceive relay without rln, but not sure everyone agrees. Until we agree on this, 1. would be blocked. Same applies to 3.

This validator is a bit different, since its not part of waku protocol. Is some kind of special validator app level validator, thats why its in the app folder. If you ask me, I would completely remove this validator. But since some people may be using it, not sure until when, guess we can't.

Thanks for the answers @alrevuelta !
I personally vote for having both validators added to Relay. For sure, the RLN one. And re the "signed-pubsubtopics" validator I'd add it to Relay and, if we decide in the future not to have it, then we remove it. I believe that the libwaku should have also the possibility to benefit from those validators.

alrevuelta · 2023-09-01T09:53:17Z

trying to upstream this vacp2p/nim-libp2p#945 so perhaps we can simplify this PR. Our case would be the Sequential one.

jm-clius · 2023-09-01T10:45:14Z

Even without the upstream change, I quite like explicitly decoupling "Waku Validators" from the internal relay validators. This is exactly because validators rely so much on application-specific configuration, decisions and ordering.
This PR is probably fine for now, but in future I would suggest something like:

move all validators to the node level (can be in a subfolder) and convert them to WakuValidator (as has been done here)
let the app (wakunode2) do some setupValidators logic that add validators according to the external configuration, etc. and combine them into a single validator
this single validator are then added as relay validator on WakuNode creation

Given that the validation is part of the Relay protocol

Don't quite agree. The validation internals is not part of relay protocol. It exists exactly because the application (in this case the Waku Node) has knowledge about message validity that is not available (and should not be available) to the relay protocol. In other words, validators are a mechanism for the application to pass down validation knowledge (which belongs to them) to the relay protocol. See for example this discussion on message processing in the gossipsub spec:

Payload processing will validate the message according to application-defined rules...

rymnc · 2023-09-01T11:24:23Z

Even without the upstream change, I quite like explicitly decoupling "Waku Validators" from the internal relay validators. This is exactly because validators rely so much on application-specific configuration, decisions and ordering.
This PR is probably fine for now, but in future I would suggest something like:

move all validators to the node level (can be in a subfolder) and convert them to WakuValidator (as has been done here)

let the app (wakunode2) do some setupValidators logic that add validators according to the external configuration, etc. and combine them into a single validator

this single validator are then added as relay validator on WakuNode creation

Given that the validation is part of the Relay protocol

Don't quite agree. The validation internals is not part of relay protocol. It exists exactly because the application (in this case the Waku Node) has knowledge about message validity that is not available (and should not be available) to the relay protocol. In other words, validators are a mechanism for the application to pass down validation knowledge (which belongs to them) to the relay protocol. See for example this discussion on message processing in the gossipsub spec:

Payload processing will validate the message according to application-defined rules...

I like this idea - this way the application (wakunode2) defines the validators for waku relay, in the order it wants. reduces dependency issues too.

so I guess we have to define the "minimum validation" for waku_relay, which could be the waku message decoding.

jm-clius · 2023-09-01T11:48:25Z

so I guess we have to define the "minimum validation" for waku_relay, which could be the waku message decoding.

Indeed, but even that could in theory be on the "node" layer - relay shouldn't necessarily have to know it's passing around WakuMessage. From it's perspective it's a routing protocol for "payload" with some security properties. It's the waku node layer that requires this payload to be valid decodable waku messages. But, of course, it's not always possible to refactor architecture in one leap, so this is just an idea.

alrevuelta · 2023-09-04T12:13:26Z

trying to upstream this vacp2p/nim-libp2p#945 so perhaps we can simplify this PR. Our case would be the Sequential one.

Lets try to wrap this up this PR. Not sure if we can upstream the feature of sequential validators see so don't want to block this PR :)

Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>

alrevuelta

minor comment, looking almost ready.

@rymnc As far as I understand we must ensure that addValidator is called always before subscribe. Otherwise the wrapped validator created with generateOrderedValidator won't contain the ones added later.

This behaviour is ok, but can we ensure that addValidator is not called after subscribe() is called? So that we can avoid people adding a validator that is not really applied.

alrevuelta · 2023-09-04T12:24:58Z

waku/waku_relay/protocol.nim

+  # add the ordered validator to the topic
+  if not w.validatorInserted.hasKey(pubSubTopic):
+    procCall GossipSub(w).addValidator(pubSubTopic, w.generateOrderedValidator())
+    w.validatorInserted[pubSubTopic] = true


this seems important right?

rymnc · 2023-09-04T12:42:08Z

minor comment, looking almost ready.

@rymnc As far as I understand we must ensure that addValidator is called always before subscribe. Otherwise the wrapped validator created with generateOrderedValidator won't contain the ones added later.

This behaviour is ok, but can we ensure that addValidator is not called after subscribe() is called? So that we can avoid people adding a validator that is not really applied.

not sure I understand "ensure that addValidator is called always before subscribe."

nwaku/waku/waku_relay/protocol.nim

Lines 217 to 241 in 893ddf5

    
           proc subscribe*(w: WakuRelay, pubsubTopic: PubsubTopic, handler: WakuRelayHandler) = 
        
             debug "subscribe", pubsubTopic=pubsubTopic 
        
             # we need to wrap the handler since gossipsub doesnt understand WakuMessage 
        
             let wrappedHandler = proc(pubsubTopic: string, data: seq[byte]): Future[void] {.gcsafe, raises: [].} = 
        
               let decMsg = WakuMessage.decode(data) 
        
               if decMsg.isErr(): 
        
                 # fine if triggerSelf enabled, since validators are bypassed 
        
                 error "failed to decode WakuMessage, validator passed a wrong message", error = decMsg.error 
        
                 let fut = newFuture[void]() 
        
                 fut.complete() 
        
                 return fut 
        
               else: 
        
                 return handler(pubsubTopic, decMsg.get()) 
        
             # add the ordered validator to the topic 
        
             if not w.validatorInserted.hasKey(pubSubTopic): 
        
               procCall GossipSub(w).addValidator(pubSubTopic, w.generateOrderedValidator()) 
        
               w.validatorInserted[pubSubTopic] = true 
        
             # set this topic parameters for scoring 
        
             w.topicParams[pubsubTopic] = TopicParameters 
        
             # subscribe to the topic with our wrapped handler 
        
             procCall GossipSub(w).subscribe(pubsubTopic, wrappedHandler)

it is called before subscribe 🤔

github-actions · 2023-09-04T12:59:58Z

You can find the experimental image built from this PR at

quay.io/wakuorg/nwaku-pr:1966-experimental

alrevuelta · 2023-09-04T14:37:37Z

not sure I understand "ensure that addValidator is called always before subscribe."

@rymnc mm the addValidator you are referring to is the gossipsub one. I'm refering to wakuRelay addValidator. Unless I'm missing something, what would happen if:

node.subscribe(topic)
node.addValidator(validator)

That validator wont be executed right? So its added but it never "reaches" gossipsub

rymnc · 2023-09-04T14:47:17Z

I think it will still be executed since the seq of validators is not copied, and it will always access them from WakuNode.wakuValidators

alrevuelta

lgtm!

rymnc added E:2023-rln release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions labels Aug 30, 2023

rymnc self-assigned this Aug 30, 2023

rymnc force-pushed the ordered-validator-execution branch from 65876ac to 4972ac7 Compare August 30, 2023 05:27

rymnc commented Aug 30, 2023

View reviewed changes

waku/waku_rln_relay/rln_relay.nim Show resolved Hide resolved

rymnc linked an issue Aug 30, 2023 that may be closed by this pull request

chore(relay): ordered validator execution #1967

Closed

2 tasks

feat(relay): ordered validator execution

c733752

rymnc force-pushed the ordered-validator-execution branch from 4972ac7 to c733752 Compare August 30, 2023 08:47

alrevuelta requested changes Aug 30, 2023

View reviewed changes

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

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

fix: make more readable

449e96f

alrevuelta reviewed Aug 30, 2023

View reviewed changes

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

rymnc added 2 commits August 30, 2023 19:59

test: ignore accepts only

bf5bfb9

fix: idempotent .subscribe

3b45109

rymnc requested a review from alrevuelta August 30, 2023 18:12

rymnc marked this pull request as ready for review August 30, 2023 18:17

Ivansete-status approved these changes Aug 31, 2023

View reviewed changes

rymnc mentioned this pull request Sep 1, 2023

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

Closed

56 tasks

alrevuelta mentioned this pull request Sep 1, 2023

Add validationStrategy Paralel/Seq + ordered validators vacp2p/nim-libp2p#945

Closed

fix(rln-relay): make validators private

c6a4487

Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>

alrevuelta reviewed Sep 4, 2023

View reviewed changes

fix: include comments, unsubscribe behaviour

893ddf5

rymnc added 2 commits September 4, 2023 18:18

Merge branch 'master' into ordered-validator-execution

7e18c66

fix: compilation

e97a38a

rymnc force-pushed the ordered-validator-execution branch from 00c36ee to e97a38a Compare September 4, 2023 12:54

rymnc requested a review from alrevuelta September 4, 2023 14:27

alrevuelta approved these changes Sep 5, 2023

View reviewed changes

Merge branch 'master' into ordered-validator-execution

93b32db

rymnc merged commit debc5f1 into master Sep 5, 2023
14 checks passed

rymnc deleted the ordered-validator-execution branch September 5, 2023 09:05

rymnc mentioned this pull request Sep 5, 2023

feat(relay): bandwidth constrained validator #1992

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(relay): ordered validator execution #1966

feat(relay): ordered validator execution #1966

rymnc commented Aug 30, 2023 •

edited

github-actions bot commented Aug 30, 2023 •

edited

rymnc commented Aug 30, 2023

alrevuelta left a comment

Ivansete-status left a comment

Ivansete-status Aug 31, 2023

alrevuelta Sep 1, 2023

rymnc Sep 4, 2023

Ivansete-status Aug 31, 2023

alrevuelta Sep 4, 2023

rymnc Sep 4, 2023

Ivansete-status Aug 31, 2023

rymnc Sep 4, 2023

Ivansete-status Aug 31, 2023

rymnc Sep 4, 2023

Ivansete-status Aug 31, 2023

alrevuelta Sep 1, 2023

rymnc Sep 4, 2023

alrevuelta commented Aug 31, 2023

Ivansete-status commented Sep 1, 2023

alrevuelta commented Sep 1, 2023

jm-clius commented Sep 1, 2023 •

edited

rymnc commented Sep 1, 2023

jm-clius commented Sep 1, 2023

alrevuelta commented Sep 4, 2023

alrevuelta left a comment

alrevuelta Sep 4, 2023

rymnc commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

alrevuelta commented Sep 4, 2023

rymnc commented Sep 4, 2023 •

edited

alrevuelta left a comment

	WakuValidatorHandler* = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[ValidationResult] {.gcsafe, raises: [Defect].}
	WakuValidatorHandler = proc(pubsubTopic: PubsubTopic, message: WakuMessage): Future[ValidationResult] {.gcsafe, raises: [Defect].}

feat(relay): ordered validator execution #1966

feat(relay): ordered validator execution #1966

Conversation

rymnc commented Aug 30, 2023 • edited

Description

Changes

Issue

github-actions bot commented Aug 30, 2023 • edited

rymnc commented Aug 30, 2023

alrevuelta left a 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

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

alrevuelta commented Aug 31, 2023

Ivansete-status commented Sep 1, 2023

alrevuelta commented Sep 1, 2023

jm-clius commented Sep 1, 2023 • edited

rymnc commented Sep 1, 2023

jm-clius commented Sep 1, 2023

alrevuelta commented Sep 4, 2023

alrevuelta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

alrevuelta commented Sep 4, 2023

rymnc commented Sep 4, 2023 • edited

alrevuelta left a comment

Choose a reason for hiding this comment

rymnc commented Aug 30, 2023 •

edited

github-actions bot commented Aug 30, 2023 •

edited

jm-clius commented Sep 1, 2023 •

edited

rymnc commented Sep 4, 2023 •

edited