fix(rln-relay): make nullifier log abide by epoch ordering (#2508)

* fix(rln-relay): nullifier log abide by epoch ordering

* fix: cleaner hasKey method, test

* chore: idiomatic usage of results, error handling

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

---------

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

tests/waku_rln_relay/test_waku_rln_relay.nim

@@ -632,30 +632,25 @@ suite "Waku rln relay":
 
     # check whether hasDuplicate correctly finds records with the same nullifiers but different secret shares
     # no duplicate for proof1 should be found, since the log is empty
-    let result1 = wakurlnrelay.hasDuplicate(proof1.extractMetadata().tryGet())
+    let result1 = wakurlnrelay.hasDuplicate(epoch, proof1.extractMetadata().tryGet())
-    require:
+    assert result1.isOk(), $result1.error
-      result1.isOk()
+    assert result1.value == false, "no duplicate should be found"
-      # no duplicate is found
-      result1.value == false
     #  add it to the log
-    discard wakurlnrelay.updateLog(proof1.extractMetadata().tryGet())
+    discard wakurlnrelay.updateLog(epoch, proof1.extractMetadata().tryGet())
 
-    # # no duplicate for proof2 should be found, its nullifier differs from proof1
+    # no duplicate for proof2 should be found, its nullifier differs from proof1
-    let result2 = wakurlnrelay.hasDuplicate(proof2.extractMetadata().tryGet())
+    let result2 = wakurlnrelay.hasDuplicate(epoch, proof2.extractMetadata().tryGet())
-    require:
+    assert result2.isOk(), $result2.error
-      result2.isOk()
+    # no duplicate is found
-      # no duplicate is found
+    assert result2.value == false, "no duplicate should be found"
-      result2.value == false
     #  add it to the log
-    discard wakurlnrelay.updateLog(proof2.extractMetadata().tryGet())
+    discard wakurlnrelay.updateLog(epoch, proof2.extractMetadata().tryGet())
 
     #  proof3 has the same nullifier as proof1 but different secret shares, it should be detected as duplicate
-    let result3 = wakurlnrelay.hasDuplicate(proof3.extractMetadata().tryGet())
+    let result3 = wakurlnrelay.hasDuplicate(epoch, proof3.extractMetadata().tryGet())
-    require:
+    assert result3.isOk(), $result3.error
-      result3.isOk()
+    # it is a duplicate
-    check:
+    assert result3.value, "duplicate should be found"
-      # it is a duplicate
-      result3.value == true
 
   asyncTest "validateMessageAndUpdateLog test":
     let index = MembershipIndex(5)
@@ -710,6 +705,52 @@ suite "Waku rln relay":
       msgValidate3 == MessageValidationResult.Valid
       msgValidate4 == MessageValidationResult.Invalid
 
+  asyncTest "validateMessageAndUpdateLog: multiple senders with same external nullifier":
+    let index1 = MembershipIndex(5)
+    let index2 = MembershipIndex(6)
+
+    let rlnConf1 = WakuRlnConfig(rlnRelayDynamic: false,
+                                 rlnRelayCredIndex: some(index1),
+                                 rlnEpochSizeSec: 1,
+                                 rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_3"))
+    let wakuRlnRelay1 = (await WakuRlnRelay.new(rlnConf1)).valueOr:
+      raiseAssert "failed to create waku rln relay: " & $error
+
+    let rlnConf2 = WakuRlnConfig(rlnRelayDynamic: false,
+                                 rlnRelayCredIndex: some(index2),
+                                 rlnEpochSizeSec: 1,
+                                 rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_4"))
+    let wakuRlnRelay2 = (await WakuRlnRelay.new(rlnConf2)).valueOr:
+      raiseAssert "failed to create waku rln relay: " & $error
+    # get the current epoch time
+    let time = epochTime()
+
+    #  create messages from different peers and append rln proofs to them
+    var
+      wm1 = WakuMessage(payload: "Valid message from sender 1".toBytes())
+      # another message in the same epoch as wm1, it will break the messaging rate limit
+      wm2 = WakuMessage(payload: "Valid message from sender 2".toBytes())
+
+
+    let
+      proofAdded1 = wakuRlnRelay1.appendRLNProof(wm1, time)
+      proofAdded2 = wakuRlnRelay2.appendRLNProof(wm2, time)
+
+    # ensure proofs are added
+    assert proofAdded1.isOk(), "failed to append rln proof: " & $proofAdded1.error
+    assert proofAdded2.isOk(), "failed to append rln proof: " & $proofAdded2.error
+
+    # validate messages
+    # validateMessage proc checks the validity of the message fields and adds it to the log (if valid)
+    let
+      msgValidate1 = wakuRlnRelay1.validateMessageAndUpdateLog(wm1, some(time))
+      # since this message is from a different sender, it should be validated successfully
+      msgValidate2 = wakuRlnRelay1.validateMessageAndUpdateLog(wm2, some(time))
+
+    check:
+      msgValidate1 == MessageValidationResult.Valid
+      msgValidate2 == MessageValidationResult.Valid
+
   test "toIDCommitment and toUInt256":
     # create an instance of rln
     let rlnInstance = createRLNInstanceWrapper()

waku/waku_rln_relay/rln_relay.nim

@@ -79,7 +79,7 @@ proc createMembershipList*(rln: ptr RLN, n: int): RlnRelayResult[(
 
 type WakuRLNRelay* = ref object of RootObj
   # the log of nullifiers and Shamir shares of the past messages grouped per epoch
-  nullifierLog*: OrderedTable[Epoch, seq[ProofMetadata]]
+  nullifierLog*: OrderedTable[Epoch, Table[Nullifier, ProofMetadata]]
   lastEpoch*: Epoch # the epoch of the last published rln message
   rlnEpochSizeSec*: uint64
   rlnMaxEpochGap*: uint64
@@ -103,58 +103,49 @@ proc stop*(rlnPeer: WakuRLNRelay) {.async: (raises: [Exception]).} =
   await rlnPeer.groupManager.stop()
 
 proc hasDuplicate*(rlnPeer: WakuRLNRelay,
+                   epoch: Epoch,
                    proofMetadata: ProofMetadata): RlnRelayResult[bool] =
   ## returns true if there is another message in the  `nullifierLog` of the `rlnPeer` with the same
   ## epoch and nullifier as `proofMetadata`'s epoch and nullifier
   ## otherwise, returns false
   ## Returns an error if it cannot check for duplicates
 
-  let externalNullifier = proofMetadata.externalNullifier
   # check if the epoch exists
-  if not rlnPeer.nullifierLog.hasKey(externalNullifier):
+  let nullifier = proofMetadata.nullifier
+  if not rlnPeer.nullifierLog.hasKey(epoch):
     return ok(false)
   try:
-    if rlnPeer.nullifierLog[externalNullifier].contains(proofMetadata):
+    if rlnPeer.nullifierLog[epoch].hasKey(nullifier):
       # there is an identical record, mark it as spam
       return ok(true)
 
-    # check for a message with the same nullifier but different secret shares
-    let matched = rlnPeer.nullifierLog[externalNullifier].filterIt((
-        it.nullifier == proofMetadata.nullifier) and ((it.shareX != proofMetadata.shareX) or
-        (it.shareY != proofMetadata.shareY)))
-
-    if matched.len != 0:
-      # there is a duplicate
-      return ok(true)
-
     # there is no duplicate
     return ok(false)
 
-  except KeyError as e:
+  except KeyError:
-    return err("the epoch was not found")
+    return err("the epoch was not found: " & getCurrentExceptionMsg())
 
 proc updateLog*(rlnPeer: WakuRLNRelay,
+                epoch: Epoch,
                 proofMetadata: ProofMetadata): RlnRelayResult[void] =
   ## saves supplied proofMetadata `proofMetadata`
   ## in the `nullifierLog` of the `rlnPeer`
   ## Returns an error if it cannot update the log
 
-  let externalNullifier = proofMetadata.externalNullifier
+  # check if the epoch exists
-  # check if the externalNullifier exists
+  if not rlnPeer.nullifierLog.hasKeyOrPut(epoch, { proofMetadata.nullifier: proofMetadata }.toTable()):
-  if not rlnPeer.nullifierLog.hasKey(externalNullifier):
-    rlnPeer.nullifierLog[externalNullifier] = @[proofMetadata]
     return ok()
 
   try:
     # check if an identical record exists
-    if rlnPeer.nullifierLog[externalNullifier].contains(proofMetadata):
+    if rlnPeer.nullifierLog[epoch].hasKeyOrPut(proofMetadata.nullifier, proofMetadata):
+      # the above condition could be `discarded` but it is kept for clarity, that slashing will 
+      # be implemented here
       # TODO: slashing logic
       return ok()
-    # add proofMetadata to the log
-    rlnPeer.nullifierLog[externalNullifier].add(proofMetadata)
     return ok()
-  except KeyError as e:
+  except KeyError:
-    return err("the external nullifier was not found") # should never happen
+    return err("the epoch was not found: " & getCurrentExceptionMsg()) # should never happen
 
 proc getCurrentEpoch*(rlnPeer: WakuRLNRelay): Epoch =
   ## gets the current rln Epoch time
@@ -249,7 +240,7 @@ proc validateMessage*(rlnPeer: WakuRLNRelay,
   if proofMetadataRes.isErr():
     waku_rln_errors_total.inc(labelValues=["proof_metadata_extraction"])
     return MessageValidationResult.Invalid
-  let hasDup = rlnPeer.hasDuplicate(proofMetadataRes.get())
+  let hasDup = rlnPeer.hasDuplicate(msgEpoch, proofMetadataRes.get())
   if hasDup.isErr():
     waku_rln_errors_total.inc(labelValues=["duplicate_check"])
   elif hasDup.value == true:
@@ -282,7 +273,7 @@ proc validateMessageAndUpdateLog*(
     return MessageValidationResult.Invalid
 
   # insert the message to the log (never errors)
-  discard rlnPeer.updateLog(proofMetadataRes.get())
+  discard rlnPeer.updateLog(msgProof.epoch, proofMetadataRes.get())
 
   return result