fix: make rln rate limit spec compliant #2294
Conversation
|
You can find the image built from this PR at Built from 041d67e |
|
Note that the changes in this PR might be backwards incompatible, in some occasions, when having nodes sending > 1 msg/10 seconds:
This could fork the network, but only when having rln memberships sending messages at a rate greater than 1msg/10sec. |
There was a problem hiding this comment.
@rymnc This change triggered some edge cases? Mind checking my questions?
| @@ -445,7 +445,7 @@ procSuite "WakuNode - RLN relay": | |||
| proofAdded1 = node1.wakuRlnRelay.appendRLNProof(wm1, time) | |||
| # another message in the same epoch as wm1, it will break the messaging rate limit | |||
| wm2 = WakuMessage(payload: "message 2".toBytes(), contentTopic: contentTopic) | |||
| proofAdded2 = node1.wakuRlnRelay.appendRLNProof(wm2, time + EpochUnitSeconds) | |||
| proofAdded2 = node1.wakuRlnRelay.appendRLNProof(wm2, time) | |||
There was a problem hiding this comment.
@rymnc Unsure I understand why wm2 had time + EpochUnitSecods if its expected to "break the messaging rate limit". Isn't it better to set the same time?
There was a problem hiding this comment.
doesn't matter too much, just showing that 2 messages were generated within the same epoch window.
| # mount the relay handler for node2 | ||
| node2.subscribe((kind: PubsubSub, topic: DefaultPubsubTopic), some(relayHandler)) | ||
| await sleepAsync(2000.millis) | ||
|
|
||
| await node1.publish(some(DefaultPubsubTopic), wm1) | ||
| await sleepAsync(10.seconds) | ||
| await node1.publish(some(DefaultPubsubTopic), wm2) |
There was a problem hiding this comment.
@rymnc Mind clarifying why this sleep was needed? Epoch is hardcoded above, so perhaps not needed?
There was a problem hiding this comment.
it was for message propagation i believe, due to some race condition we had earlier
| res1 == true | ||
| res2 == false | ||
| res3 == true | ||
| node2.wakuRlnRelay.nullifierLog.len() == 2 |
There was a problem hiding this comment.
@rymnc The change from 10 to 1 in EpochUnitSeconds triggered this edge case (I belive). Mind clarifying what would be expected here?
There was a problem hiding this comment.
this is correct, thanks for the fix 😁
| (res1 and res2 and res3) == true # all 3 are valid | ||
| node2.wakuRlnRelay.nullifierLog.len() == 1 # after clearing, only 1 is stored | ||
| res1 == true | ||
| res2 == false |
There was a problem hiding this comment.
@rymnc Not sure why rest2 was expected to complete. Since as stated above the second message "it will break the messaging rate limit" and that implies that the handler wont be triggered? So res==false would be expected isn't it?
fix: make rln rate limit spec compliant
closes #2289