Fix nonce rollback race: make rollbackNonce CAS-conditional

Two bugs in the previous blind-decrement rollbackNonce():

1. Concurrent draw: another proof generated between attempt 1 and the retry
   advances nextNonce, so decrementing steals a nonce already claimed by
   another live message — nullifier collision, on-chain slashable.

2. Pre-attached proof: if the incoming message already carries a proof,
   checkAndGenerateRLNProof short-circuits and draws no nonce, yet the retry
   still called rollback, stomping on whatever nonce the preceding message
   legitimately drew.

Fix: rollbackNonce now takes the specific nonce as a parameter and only
decrements when nextNonce == nonce + 1. checkAndGenerateRLNProof returns
(msg, messageId) so callers can thread the drawn id through to the retry
as reuseMessageId; generateRLNProof no longer performs an implicit rollback.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

logos_delivery/waku/node/waku_node/lightpush.nim

@@ -92,7 +92,7 @@ proc legacyLightpushPublish*(
       none(Rln)
     else:
       some(node.rln)
-  var msgWithProof = (await checkAndGenerateRLNProof(rln, message)).valueOr:
+  let (msgWithProof, drawnMessageId) = (await checkAndGenerateRLNProof(rln, message)).valueOr:
     return err("failed call checkAndGenerateRLNProof from lightpush: " & error)
 
   let internalPublish = proc(
@@ -151,12 +151,17 @@ proc legacyLightpushPublish*(
 
     info "legacy lightpush send rejected as RLN-invalid; " &
       "refreshing merkle proof and retrying once"
-    msgWithProof = (
-      await checkAndGenerateRLNProof(rln, msgWithProof, forceMerkleProofRefresh = true)
+    let (retryMsg, _) = (
+      await checkAndGenerateRLNProof(
+        rln,
+        msgWithProof,
+        forceMerkleProofRefresh = true,
+        reuseMessageId = drawnMessageId,
+      )
     ).valueOr:
       return err("failed call checkAndGenerateRLNProof from lightpush retry: " & error)
 
-    return await internalPublish(node, pubsubForPublish, msgWithProof, peer)
+    return await internalPublish(node, pubsubForPublish, retryMsg, peer)
   except CatchableError:
     return err(getCurrentExceptionMsg())
 
@@ -318,7 +323,7 @@ proc lightpushPublish*(
       none(Rln)
     else:
       some(node.rln)
-  var msgWithProof = (await checkAndGenerateRLNProof(rln, message)).valueOr:
+  let (msgWithProof, drawnMessageId) = (await checkAndGenerateRLNProof(rln, message)).valueOr:
     return lighpushErrorResult(LightPushErrorCode.OUT_OF_RLN_PROOF, error)
 
   let firstResult =
@@ -334,10 +339,11 @@ proc lightpushPublish*(
 
   info "lightpush send rejected; refreshing merkle proof and retrying once",
     statusCode = $firstResult.error.code
-  msgWithProof = (
-    await checkAndGenerateRLNProof(rln, msgWithProof, forceMerkleProofRefresh = true)
+  let (retryMsg, _) = (
+    await checkAndGenerateRLNProof(
+      rln, msgWithProof, forceMerkleProofRefresh = true, reuseMessageId = drawnMessageId
+    )
   ).valueOr:
     return lighpushErrorResult(LightPushErrorCode.OUT_OF_RLN_PROOF, error)
 
-  return
-    await lightpushPublishHandler(node, pubsubForPublish, msgWithProof, toPeer, mixify)
+  return await lightpushPublishHandler(node, pubsubForPublish, retryMsg, toPeer, mixify)

logos_delivery/waku/rln/nonce_manager.nim

@@ -58,12 +58,12 @@ 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
+proc rollbackNonce*(n: NonceManager, nonce: Nonce): bool =
+  # CAS: only give back `nonce` when it is still the last id issued.
+  # If another draw advanced the counter between this attempt and the retry,
+  # leave the counter alone — the retry will draw a fresh id rather than
+  # risk a nullifier collision with the intervening message.
+  if n.nextNonce == nonce + 1:
+    n.nextNonce = nonce
+    return true
+  false

logos_delivery/waku/rln/proof.nim

@@ -61,13 +61,6 @@ 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 = (
@@ -79,28 +72,46 @@ proc generateRLNProof*(
   return ok(proof.encode().buffer)
 
 proc checkAndGenerateRLNProof*(
-    rln: Option[Rln], message: WakuMessage, forceMerkleProofRefresh: bool = false
-): Future[Result[WakuMessage, string]] {.async.} =
-  # When forcing a refresh (e.g. retry after a lightpush 420/504 rejection) we
-  # deliberately regenerate the proof even if one is already attached, since
-  # the existing proof is presumed to have been rejected.
+    rln: Option[Rln],
+    message: WakuMessage,
+    forceMerkleProofRefresh: bool = false,
+    reuseMessageId: Option[Nonce] = none(Nonce),
+): Future[Result[tuple[msg: WakuMessage, messageId: Option[Nonce]], string]] {.async.} =
+  ## Returns the message with an attached RLN proof and the message id drawn
+  ## from the nonce manager (`messageId = none` when no id was consumed —
+  ## message already had a valid proof, or RLN is not configured).
+  ## Pass `messageId` back as `reuseMessageId` on a retry so the CAS rollback
+  ## can reclaim it when no concurrent draw has advanced the counter.
   if message.proof.len > 0 and not forceMerkleProofRefresh:
-    return ok(message)
+    return ok((msg: message, messageId: none(Nonce)))
 
   if rln.isNone():
     notice "Publishing message without RLN proof"
-    return ok(message)
+    return ok((msg: message, messageId: none(Nonce)))
+
+  let r = rln.get()
+  if reuseMessageId.isSome():
+    if not r.nonceManager.rollbackNonce(reuseMessageId.get()):
+      # A concurrent draw advanced the counter past the rejected attempt's id;
+      # the retry will consume a fresh message id instead of reusing the old one.
+      debug "rln retry: concurrent draw prevented nonce reuse; consuming a fresh message id",
+        attempted = reuseMessageId.get(), nextNonce = r.nonceManager.nextNonce
 
   let
     time = getTime().toUnix()
     senderEpochTime = float64(time)
+    epoch = r.calcEpoch(senderEpochTime)
+  let nonce = r.nonceManager.getNonce().valueOr:
+    return err("could not get new message id to generate an rln proof: " & $error)
   var msgWithProof = message
-  msgWithProof.proof = (
-    await rln.get().generateRLNProof(
+  let proof = (
+    await r.groupManager.generateProof(
       msgWithProof.toRLNSignal(),
-      senderEpochTime,
+      epoch,
+      nonce,
       forceMerkleProofRefresh = forceMerkleProofRefresh,
     )
   ).valueOr:
     return err("error in checkAndGenerateRLNProof: " & $error)
-  return ok(msgWithProof)
+  msgWithProof.proof = proof.encode().buffer
+  return ok((msg: msgWithProof, messageId: some(nonce)))

tests/node/test_wakunode_legacy_lightpush.nim

@@ -166,7 +166,7 @@ suite "RLN Proofs as a Lightpush Service":
       # Attach the RLN proof. In production the client mounts RLN and generates the
       # proof in legacyLightpushPublish; here we generate it using the server's RLN
       # instance since both ends share group state via the in-memory manager.
-      let msgWithProof =
+      let (msgWithProof, _) =
         (await checkAndGenerateRLNProof(some(server.rln), message)).get()
 
       # When the client publishes a message
@@ -186,7 +186,8 @@ suite "RLN Proofs as a Lightpush Service":
       # "RLN validation failed" rejection: an already attached proof must NOT
       # short-circuit checkAndGenerateRLNProof when forceMerkleProofRefresh=true,
       # and the cache must be refetched from chain instead of trusted.
-      let firstMsg = (await checkAndGenerateRLNProof(some(server.rln), message)).get()
+      let (firstMsg, drawnNonce) =
+        (await checkAndGenerateRLNProof(some(server.rln), message)).get()
       check firstMsg.proof.len > 0
 
       # Corrupt the cache to model a stale/invalid witness — the same state a
@@ -197,10 +198,14 @@ suite "RLN Proofs as a Lightpush Service":
       check manager.merkleProofCache != goodCache
 
       # Force-regenerate. The existing proof must be discarded, the cache
-      # refetched from chain, and a fresh proof produced.
-      let secondMsg = (
+      # refetched from chain, and a fresh proof produced. Pass drawnNonce so
+      # the CAS rollback reclaims it — mirroring the production retry path.
+      let (secondMsg, _) = (
         await checkAndGenerateRLNProof(
-          some(server.rln), firstMsg, forceMerkleProofRefresh = true
+          some(server.rln),
+          firstMsg,
+          forceMerkleProofRefresh = true,
+          reuseMessageId = drawnNonce,
         )
       ).get()
 

tests/node/test_wakunode_lightpush.nim

@@ -162,7 +162,7 @@ suite "RLN Proofs as a Lightpush Service":
       # Attach the RLN proof. In production the client mounts RLN and generates the
       # proof in lightpushPublish; here we generate it using the server's RLN instance
       # since both ends share group state via the in-memory manager.
-      let msgWithProof =
+      let (msgWithProof, _) =
         (await checkAndGenerateRLNProof(some(server.rln), message)).get()
 
       # When the client publishes a message
@@ -183,7 +183,8 @@ suite "RLN Proofs as a Lightpush Service":
       # attached proof must NOT short-circuit checkAndGenerateRLNProof when
       # forceMerkleProofRefresh=true, and the cache must be refetched from
       # chain instead of trusted.
-      let firstMsg = (await checkAndGenerateRLNProof(some(server.rln), message)).get()
+      let (firstMsg, drawnNonce) =
+        (await checkAndGenerateRLNProof(some(server.rln), message)).get()
       check firstMsg.proof.len > 0
 
       # Corrupt the cache to model a stale/invalid witness — the same state a
@@ -194,10 +195,14 @@ suite "RLN Proofs as a Lightpush Service":
       check manager.merkleProofCache != goodCache
 
       # Force-regenerate. The existing proof must be discarded, the cache
-      # refetched from chain, and a fresh proof produced.
-      let secondMsg = (
+      # refetched from chain, and a fresh proof produced. Pass drawnNonce so
+      # the CAS rollback reclaims it — mirroring the production retry path.
+      let (secondMsg, _) = (
         await checkAndGenerateRLNProof(
-          some(server.rln), firstMsg, forceMerkleProofRefresh = true
+          some(server.rln),
+          firstMsg,
+          forceMerkleProofRefresh = true,
+          reuseMessageId = drawnNonce,
         )
       ).get()
 

tests/waku_rln_relay/test_rln_nonce_manager.nim

@@ -42,11 +42,11 @@ suite "Nonce manager":
       nonce == 0.uint
       nonce2 == 0.uint
 
-  test "rollbackNonce lets the next getNonce reissue the same id":
+  test "rollbackNonce reclaims the last-drawn id when counter has not advanced":
     let nm = NonceManager.init(nonceLimit = 100.uint)
     let first = nm.getNonce().valueOr:
       raiseAssert $error
-    nm.rollbackNonce()
+    check nm.rollbackNonce(first)
     let reissued = nm.getNonce().valueOr:
       raiseAssert $error
 
@@ -55,23 +55,20 @@ suite "Nonce manager":
       reissued == first
       nm.nextNonce == 1.uint
 
-  test "rollbackNonce frees the message id so nonceLimit is not exceeded":
-    let nm = NonceManager.init(nonceLimit = 1.uint)
+  test "rollbackNonce is a no-op when another draw has advanced the counter":
+    let nm = NonceManager.init(nonceLimit = 100.uint)
+    let first = nm.getNonce().valueOr:
+      raiseAssert $error
+    # A second draw slips in — simulates concurrent publish between attempt 1
+    # and the retry. The counter now points past first+1.
     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
+    check not nm.rollbackNonce(first)
+    check nm.nextNonce == 2.uint
 
   test "rollbackNonce is a no-op when no nonce has been issued":
     let nm = NonceManager.init(nonceLimit = 100.uint)
-    nm.rollbackNonce()
+    check not nm.rollbackNonce(Nonce(0))
 
     check:
       nm.nextNonce == 0.uint