From 9b04d9efe5ea7c0669c8e62060aba8ebe1ff25aa Mon Sep 17 00:00:00 2001 From: rymnc <43716372+rymnc@users.noreply.github.com> Date: Wed, 6 Mar 2024 00:42:35 +0530 Subject: [PATCH 1/3] fix(rln-relay): nullifier log abide by epoch ordering --- tests/waku_rln_relay/test_waku_rln_relay.nim | 33 +++++++-------- waku/waku_rln_relay/rln_relay.nim | 44 +++++++++----------- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/tests/waku_rln_relay/test_waku_rln_relay.nim b/tests/waku_rln_relay/test_waku_rln_relay.nim index 832d0242e1..0b91acac7b 100644 --- a/tests/waku_rln_relay/test_waku_rln_relay.nim +++ b/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()) - require: - result1.isOk() - # no duplicate is found - result1.value == false + let result1 = wakurlnrelay.hasDuplicate(epoch, proof1.extractMetadata().tryGet()) + assert result1.isOk(), $result1.error + assert result1.value == false, "no duplicate should be found" # 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 - let result2 = wakurlnrelay.hasDuplicate(proof2.extractMetadata().tryGet()) - require: - result2.isOk() - # no duplicate is found - result2.value == false + # no duplicate for proof2 should be found, its nullifier differs from proof1 + let result2 = wakurlnrelay.hasDuplicate(epoch, proof2.extractMetadata().tryGet()) + assert result2.isOk(), $result2.error + # no duplicate is found + assert result2.value == false, "no duplicate should be found" # 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()) - require: - result3.isOk() - check: - # it is a duplicate - result3.value == true + let result3 = wakurlnrelay.hasDuplicate(epoch, proof3.extractMetadata().tryGet()) + assert result3.isOk(), $result3.error + # it is a duplicate + assert result3.value, "duplicate should be found" asyncTest "validateMessageAndUpdateLog test": let index = MembershipIndex(5) diff --git a/waku/waku_rln_relay/rln_relay.nim b/waku/waku_rln_relay/rln_relay.nim index 9599553fcf..c6eb4b1b4f 100644 --- a/waku/waku_rln_relay/rln_relay.nim +++ b/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,54 @@ 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") 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 externalNullifier exists - if not rlnPeer.nullifierLog.hasKey(externalNullifier): - rlnPeer.nullifierLog[externalNullifier] = @[proofMetadata] + # check if the epoch exists + if not rlnPeer.nullifierLog.hasKey(epoch): + rlnPeer.nullifierLog[epoch] = initTable[Nullifier, ProofMetadata]() + try: + rlnPeer.nullifierLog[epoch][proofMetadata.nullifier] = proofMetadata + except KeyError: + return err("the epoch was not found") # should never happen return ok() try: # check if an identical record exists - if rlnPeer.nullifierLog[externalNullifier].contains(proofMetadata): + if rlnPeer.nullifierLog[epoch].hasKey(proofMetadata.nullifier): # TODO: slashing logic return ok() # add proofMetadata to the log - rlnPeer.nullifierLog[externalNullifier].add(proofMetadata) + rlnPeer.nullifierLog[epoch][proofMetadata.nullifier] = proofMetadata return ok() - except KeyError as e: - return err("the external nullifier was not found") # should never happen + except KeyError: + return err("the epoch was not found") # should never happen proc getCurrentEpoch*(rlnPeer: WakuRLNRelay): Epoch = ## gets the current rln Epoch time @@ -249,7 +245,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 +278,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 From 6d20aef7d3d6d423e133d1471e3a7eebddcf3739 Mon Sep 17 00:00:00 2001 From: rymnc <43716372+rymnc@users.noreply.github.com> Date: Wed, 6 Mar 2024 15:46:51 +0530 Subject: [PATCH 2/3] fix: cleaner hasKey method, test --- tests/waku_rln_relay/test_waku_rln_relay.nim | 49 ++++++++++++++++++++ waku/waku_rln_relay/rln_relay.nim | 13 ++---- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/tests/waku_rln_relay/test_waku_rln_relay.nim b/tests/waku_rln_relay/test_waku_rln_relay.nim index 0b91acac7b..9886f39667 100644 --- a/tests/waku_rln_relay/test_waku_rln_relay.nim +++ b/tests/waku_rln_relay/test_waku_rln_relay.nim @@ -705,6 +705,55 @@ 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 wakuRlnRelay1Res = await WakuRlnRelay.new(rlnConf1) + if wakuRlnRelay1Res.isErr(): assert false, "failed to create waku rln relay: " & $wakuRlnRelay1Res.error + let wakuRlnRelay1 = wakuRlnRelay1Res.get() + + let rlnConf2 = WakuRlnConfig(rlnRelayDynamic: false, + rlnRelayCredIndex: some(index2), + rlnEpochSizeSec: 1, + rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_4")) + let wakuRlnRelay2Res = await WakuRlnRelay.new(rlnConf2) + if wakuRlnRelay2Res.isErr(): + assert false, "failed to create waku rln relay: " & $wakuRlnRelay2Res.error + let wakuRlnRelay2 = wakuRlnRelay2Res.get() + # 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() diff --git a/waku/waku_rln_relay/rln_relay.nim b/waku/waku_rln_relay/rln_relay.nim index c6eb4b1b4f..42c0fed0fc 100644 --- a/waku/waku_rln_relay/rln_relay.nim +++ b/waku/waku_rln_relay/rln_relay.nim @@ -133,21 +133,16 @@ proc updateLog*(rlnPeer: WakuRLNRelay, ## Returns an error if it cannot update the log # check if the epoch exists - if not rlnPeer.nullifierLog.hasKey(epoch): - rlnPeer.nullifierLog[epoch] = initTable[Nullifier, ProofMetadata]() - try: - rlnPeer.nullifierLog[epoch][proofMetadata.nullifier] = proofMetadata - except KeyError: - return err("the epoch was not found") # should never happen + if not rlnPeer.nullifierLog.hasKeyOrPut(epoch, { proofMetadata.nullifier: proofMetadata }.toTable()): return ok() try: # check if an identical record exists - if rlnPeer.nullifierLog[epoch].hasKey(proofMetadata.nullifier): + 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[epoch][proofMetadata.nullifier] = proofMetadata return ok() except KeyError: return err("the epoch was not found") # should never happen From c8bc4a694103cb9005e4506d6dd18298d5f30f9e Mon Sep 17 00:00:00 2001 From: Aaryamann Challani <43716372+rymnc@users.noreply.github.com> Date: Wed, 6 Mar 2024 20:54:54 +0530 Subject: [PATCH 3/3] chore: idiomatic usage of results, error handling Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com> --- tests/waku_rln_relay/test_waku_rln_relay.nim | 11 ++++------- waku/waku_rln_relay/rln_relay.nim | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/waku_rln_relay/test_waku_rln_relay.nim b/tests/waku_rln_relay/test_waku_rln_relay.nim index 9886f39667..7969976a06 100644 --- a/tests/waku_rln_relay/test_waku_rln_relay.nim +++ b/tests/waku_rln_relay/test_waku_rln_relay.nim @@ -713,18 +713,15 @@ suite "Waku rln relay": rlnRelayCredIndex: some(index1), rlnEpochSizeSec: 1, rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_3")) - let wakuRlnRelay1Res = await WakuRlnRelay.new(rlnConf1) - if wakuRlnRelay1Res.isErr(): assert false, "failed to create waku rln relay: " & $wakuRlnRelay1Res.error - let wakuRlnRelay1 = wakuRlnRelay1Res.get() + 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 wakuRlnRelay2Res = await WakuRlnRelay.new(rlnConf2) - if wakuRlnRelay2Res.isErr(): - assert false, "failed to create waku rln relay: " & $wakuRlnRelay2Res.error - let wakuRlnRelay2 = wakuRlnRelay2Res.get() + let wakuRlnRelay2 = (await WakuRlnRelay.new(rlnConf2)).valueOr: + raiseAssert "failed to create waku rln relay: " & $error # get the current epoch time let time = epochTime() diff --git a/waku/waku_rln_relay/rln_relay.nim b/waku/waku_rln_relay/rln_relay.nim index 42c0fed0fc..d0a4583da2 100644 --- a/waku/waku_rln_relay/rln_relay.nim +++ b/waku/waku_rln_relay/rln_relay.nim @@ -123,7 +123,7 @@ proc hasDuplicate*(rlnPeer: WakuRLNRelay, return ok(false) except KeyError: - return err("the epoch was not found") + return err("the epoch was not found: " & getCurrentExceptionMsg()) proc updateLog*(rlnPeer: WakuRLNRelay, epoch: Epoch, @@ -145,7 +145,7 @@ proc updateLog*(rlnPeer: WakuRLNRelay, return ok() return ok() except KeyError: - return err("the epoch 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