Add nonce rollback for lightpush publish retry

logos_delivery/waku/node/waku_node/lightpush.nim

@@ -142,17 +142,11 @@ proc legacyLightpushPublish*(
 
     let firstResult = await internalPublish(node, pubsubForPublish, msgWithProof, peer)
 
-    # Legacy protocol has no status code taxonomy: the server collapses every
-    # failure into isSuccess=false + a free-text info string. The only stable
-    # substring we can safely branch on is "RLN validation failed" — the
-    # errorMessage registered for the RLN validator in waku_node/relay.nim.
-    # Match it and treat it as the legacy equivalent of a v3 420/504: force
-    # refresh the cached merkle path, regenerate the RLN proof, and retry the
-    # send exactly once. All other failure modes (decode error, rate limit,
-    # no peers) are surfaced to the caller unchanged because a fresh proof
-    # would not change their outcome.
+    # A publish error mentioning RLN can indicate a stale merkle proof path;
+    # refresh it and retry the publish once. Legacy lightpush has no status
+    # codes, so we string-match the RLN error for backward compatibility.
     if firstResult.isOk() or rln.isNone() or
-        not firstResult.error.contains("RLN validation failed"):
+        not firstResult.error.contains(RlnValidatorErrorMsg):
       return firstResult
 
     info "legacy lightpush send rejected as RLN-invalid; " &
@@ -330,10 +324,9 @@ proc lightpushPublish*(
   let firstResult =
     await lightpushPublishHandler(node, pubsubForPublish, msgWithProof, toPeer, mixify)
 
-  # If message is rejected with error code 420 (INVALID_MESSAGE) or 504 
-  # (OUT_OF_RLN_PROOF) then cached merkle path is likely stale relative
-  # to the current on-chain group. Force-refresh it, regenerate the proof,
-  # and retry the send exactly once.
+  # A publish error with status 420 (INVALID_MESSAGE) or 504 (OUT_OF_RLN_PROOF)
+  # can indicate a stale merkle proof path; refresh it and retry the publish
+  # once.
   if firstResult.isOk() or rln.isNone() or
       firstResult.error.code notin
       [LightPushErrorCode.INVALID_MESSAGE, LightPushErrorCode.OUT_OF_RLN_PROOF]:

logos_delivery/waku/node/waku_node/relay.nim

@@ -261,4 +261,4 @@ proc setRlnValidator*(
 
   # register rln validator as default validator
   info "Registering RLN validator"
-  node.wakuRelay.addValidator(validator, "RLN validation failed")
+  node.wakuRelay.addValidator(validator, RlnValidatorErrorMsg)

logos_delivery/waku/rln/constants.nim

@@ -17,6 +17,9 @@ const RootsRefreshMinInterval* = 2.seconds
 # RLN membership key and index files path
 const RlnCredentialsFilename* = "rlnCredentials.txt"
 
+# RLN Validator message rejection Error string, is used to trigger proof refresh and publish retry in the lightpush client
+const RlnValidatorErrorMsg* = "RLN validation failed"
+
 # inputs of the membership contract constructor
 # TODO may be able to make these constants private and put them inside the waku_rln_utils
 const

logos_delivery/waku/rln/nonce_manager.nim

@@ -57,3 +57,13 @@ proc getNonce*(n: NonceManager): NonceManagerResult[Nonce] =
     )
 
   return ok(retNonce)
+
+proc rollbackNonce*(n: NonceManager) =
+  # Undo the last `getNonce` so the message id can be reissued. Used when a
+  # proof was generated but never left the node (e.g. a lightpush retry
+  # after a 420/504 rejection): the rejected attempt never reached the
+  # network, so letting the retry reuse the same id keeps one delivered
+  # message consuming one id out of the per-epoch userMessageLimit instead
+  # of two.
+  if n.nextNonce > 0:
+    n.nextNonce = n.nextNonce - 1

logos_delivery/waku/rln/proof.nim

@@ -61,6 +61,13 @@ proc generateRLNProof*(
     forceMerkleProofRefresh: bool = false,
 ): Future[RlnResult[seq[byte]]] {.async.} =
   let epoch = rln.calcEpoch(senderEpochTime)
+  # A forced refresh is a retry after the prior attempt was rejected before
+  # it reached the network. That attempt already consumed a message id, so
+  # give it back before drawing a new one — otherwise one delivered message
+  # eats two ids of the per-epoch userMessageLimit, moving the sender
+  # closer to the on-chain slashing threshold for no reason.
+  if forceMerkleProofRefresh:
+    rln.nonceManager.rollbackNonce()
   let nonce = rln.nonceManager.getNonce().valueOr:
     return err("could not get new message id to generate an rln proof: " & $error)
   let proof = (

tests/node/test_wakunode_legacy_lightpush.nim

@@ -1,7 +1,7 @@
 {.used.}
 
 import
-  std/[options, tempfiles, net, osproc],
+  std/[options, tempfiles, net, osproc, strutils],
   testutils/unittests,
   chronos,
   std/strformat,
@@ -16,6 +16,7 @@ import
     waku_lightpush_legacy/common,
     waku_lightpush_legacy/protocol_metrics,
     rln,
+    rln/constants,
   ],
   ../testlib/[wakucore, wakunode, testasync, futures, testutils],
   ../resources/payloads,
@@ -205,12 +206,102 @@ suite "RLN Proofs as a Lightpush Service":
 
       check:
         secondMsg.proof.len > 0
-        # Regenerated, not passed through — nonce manager consumes a new id
-        # per call, so the two proofs cannot be byte-equal.
+        # Regenerated, not passed through — Groth16 proofs carry random
+        # blinding, so a fresh call produces different bytes even though the
+        # rejected attempt's message id is intentionally reused on retry.
         secondMsg.proof != firstMsg.proof
         # Cache was refetched from chain, overwriting the corruption.
         manager.merkleProofCache == goodCache
 
+    # The tests below drive `server.legacyLightpushPublish(...)` against the
+    # server node. Because `server.wakuLegacyLightPush` is mounted (and no
+    # legacy client is), the call takes the self-request path — it still runs
+    # the full client-side flow (proof gen, retry on RlnValidatorErrorMsg
+    # substring, one-retry cap), but the request lands in the local
+    # pushHandler. Swapping in a stub pushHandler lets each test control what
+    # attempt N sees.
+
+    asyncTest "retry fires on RlnValidatorErrorMsg substring and second attempt succeeds":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult[void]] {.async.} =
+        inc callCount
+        if callCount == 1:
+          return err(RlnValidatorErrorMsg & ": simulated stale merkle path")
+        return ok()
+      server.wakuLegacyLightPush.pushHandler = stub
+
+      let response = await server.legacyLightpushPublish(
+        some(pubsubTopic), message, server.peerInfo.toRemotePeerInfo()
+      )
+
+      check:
+        callCount == 2
+        response.isOk()
+
+    asyncTest "no retry when error does not contain RlnValidatorErrorMsg":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult[void]] {.async.} =
+        inc callCount
+        return err("unrelated failure")
+      server.wakuLegacyLightPush.pushHandler = stub
+
+      let response = await server.legacyLightpushPublish(
+        some(pubsubTopic), message, server.peerInfo.toRemotePeerInfo()
+      )
+
+      check:
+        callCount == 1
+        response.isErr()
+        response.error == "unrelated failure"
+
+    asyncTest "retry cap: two consecutive RLN errors surface the second":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult[void]] {.async.} =
+        inc callCount
+        return err(RlnValidatorErrorMsg & ": still stale")
+      server.wakuLegacyLightPush.pushHandler = stub
+
+      let response = await server.legacyLightpushPublish(
+        some(pubsubTopic), message, server.peerInfo.toRemotePeerInfo()
+      )
+
+      check:
+        callCount == 2
+        response.isErr()
+        response.error.contains(RlnValidatorErrorMsg)
+
+    asyncTest "no retry when node.rln is nil":
+      # Detach RLN so the retry branch short-circuits on rln.isNone() even
+      # when the error string carries RlnValidatorErrorMsg. Restore before
+      # teardown so server.stop() sees the same object graph it was
+      # constructed with.
+      let savedRln = server.rln
+      server.rln = nil
+
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult[void]] {.async.} =
+        inc callCount
+        return err(RlnValidatorErrorMsg & ": simulated")
+      server.wakuLegacyLightPush.pushHandler = stub
+
+      let response = await server.legacyLightpushPublish(
+        some(pubsubTopic), message, server.peerInfo.toRemotePeerInfo()
+      )
+
+      server.rln = savedRln
+
+      check:
+        callCount == 1
+        response.isErr()
+
 suite "Waku Legacy Lightpush message delivery":
   asyncTest "Legacy lightpush message flow succeed":
     ## Setup

tests/node/test_wakunode_lightpush.nim

@@ -203,12 +203,121 @@ suite "RLN Proofs as a Lightpush Service":
 
       check:
         secondMsg.proof.len > 0
-        # Regenerated, not passed through — nonce manager consumes a new id
-        # per call, so the two proofs cannot be byte-equal.
+        # Regenerated, not passed through — Groth16 proofs carry random
+        # blinding, so a fresh call produces different bytes even though the
+        # rejected attempt's message id is intentionally reused on retry.
         secondMsg.proof != firstMsg.proof
         # Cache was refetched from chain, overwriting the corruption.
         manager.merkleProofCache == goodCache
 
+    # The tests below drive `server.lightpushPublish(...)` directly against the
+    # server node. Because `server.wakuLightPush` is mounted (and no lightpush
+    # client is), lightpushPublish takes the self-request path — it still runs
+    # the full client-side flow (proof gen, retry-on-420/504, one-retry cap),
+    # but the request lands in the local pushHandler instead of going over the
+    # wire. Swapping in a stub pushHandler lets each test control exactly what
+    # attempt N sees.
+
+    asyncTest "retry fires on 420 (INVALID_MESSAGE) and second attempt succeeds":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult] {.async.} =
+        inc callCount
+        if callCount == 1:
+          return lighpushErrorResult(
+            LightPushErrorCode.INVALID_MESSAGE, "simulated stale merkle path"
+          )
+        return lightpushSuccessResult(1)
+      server.wakuLightPush.pushHandler = stub
+
+      let response = await server.lightpushPublish(some(pubsubTopic), message)
+
+      check:
+        callCount == 2
+        response.isOk()
+        response.get() == 1
+
+    asyncTest "retry fires on 504 (OUT_OF_RLN_PROOF) and second attempt succeeds":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult] {.async.} =
+        inc callCount
+        if callCount == 1:
+          return lighpushErrorResult(
+            LightPushErrorCode.OUT_OF_RLN_PROOF, "simulated out-of-proof"
+          )
+        return lightpushSuccessResult(1)
+      server.wakuLightPush.pushHandler = stub
+
+      let response = await server.lightpushPublish(some(pubsubTopic), message)
+
+      check:
+        callCount == 2
+        response.isOk()
+        response.get() == 1
+
+    asyncTest "no retry on error codes outside 420/504":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult] {.async.} =
+        inc callCount
+        return lighpushErrorResult(
+          LightPushErrorCode.INTERNAL_SERVER_ERROR, "unrelated failure"
+        )
+      server.wakuLightPush.pushHandler = stub
+
+      let response = await server.lightpushPublish(some(pubsubTopic), message)
+
+      check:
+        callCount == 1
+        response.isErr()
+        response.error.code == LightPushErrorCode.INTERNAL_SERVER_ERROR
+
+    asyncTest "retry cap: two consecutive 420s surface the second error":
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult] {.async.} =
+        inc callCount
+        return lighpushErrorResult(LightPushErrorCode.INVALID_MESSAGE, "still stale")
+      server.wakuLightPush.pushHandler = stub
+
+      let response = await server.lightpushPublish(some(pubsubTopic), message)
+
+      check:
+        callCount == 2
+        response.isErr()
+        response.error.code == LightPushErrorCode.INVALID_MESSAGE
+
+    asyncTest "no retry when node.rln is nil":
+      # Detach RLN so the retry branch short-circuits on rln.isNone() even
+      # for a 420. Restore before teardown so server.stop() sees the same
+      # object graph it was constructed with.
+      let savedRln = server.rln
+      server.rln = nil
+
+      var callCount = 0
+      let stub: PushMessageHandler = proc(
+          pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[WakuLightPushResult] {.async.} =
+        inc callCount
+        return lighpushErrorResult(
+          LightPushErrorCode.INVALID_MESSAGE, "simulated stale merkle path"
+        )
+      server.wakuLightPush.pushHandler = stub
+
+      let response = await server.lightpushPublish(some(pubsubTopic), message)
+
+      server.rln = savedRln
+
+      check:
+        callCount == 1
+        response.isErr()
+        response.error.code == LightPushErrorCode.INVALID_MESSAGE
+
 suite "Waku Lightpush message delivery":
   asyncTest "lightpush message flow succeed":
     ## Setup

tests/waku_rln_relay/test_rln_nonce_manager.nim

@@ -41,3 +41,37 @@ suite "Nonce manager":
     check:
       nonce == 0.uint
       nonce2 == 0.uint
+
+  test "rollbackNonce lets the next getNonce reissue the same id":
+    let nm = NonceManager.init(nonceLimit = 100.uint)
+    let first = nm.getNonce().valueOr:
+      raiseAssert $error
+    nm.rollbackNonce()
+    let reissued = nm.getNonce().valueOr:
+      raiseAssert $error
+
+    check:
+      first == 0.uint
+      reissued == first
+      nm.nextNonce == 1.uint
+
+  test "rollbackNonce frees the message id so nonceLimit is not exceeded":
+    let nm = NonceManager.init(nonceLimit = 1.uint)
+    discard nm.getNonce().valueOr:
+      raiseAssert $error
+    # Without rollback a second call would fail (nonceLimit = 1, i.e. one
+    # message id per epoch). Rollback returns the id and the retry succeeds
+    # using the same id — the on-chain slashing threshold is not crossed.
+    nm.rollbackNonce()
+    let retry = nm.getNonce()
+
+    check:
+      retry.isOk()
+      retry.get() == 0.uint
+
+  test "rollbackNonce is a no-op when no nonce has been issued":
+    let nm = NonceManager.init(nonceLimit = 100.uint)
+    nm.rollbackNonce()
+
+    check:
+      nm.nextNonce == 0.uint