From 571ed17c0f62de5053daed531bee8fa0eb100274 Mon Sep 17 00:00:00 2001 From: stubbsta Date: Fri, 10 Jul 2026 12:46:01 +0200 Subject: [PATCH] Drop nonce reuse on RLN retry; consume a fresh nonce instead The retry path threaded a rejected attempt's message id back so NonceManager.rollbackNonce could CAS the counter and reclaim the slot. This bought at most one nonce per retry (a rare event) at the cost of an Option[Nonce] param, a tuple return, and a concurrent-draw race the CAS had to defend against. Simpler to optimistically waste the nonce: checkAndGenerateRLNProof returns Result[WakuMessage, string] and callers drop the tuple destructuring. NonceManager.rollbackNonce and its tests, now dead, are removed. Co-Authored-By: Claude Opus 4.7 --- .../waku/node/waku_node/lightpush.nim | 22 +++++-------- logos_delivery/waku/rln/nonce_manager.nim | 10 ------ logos_delivery/waku/rln/proof.nim | 28 +++++------------ tests/node/test_wakunode_legacy_lightpush.nim | 7 +++-- tests/node/test_wakunode_lightpush.nim | 22 ++++++------- .../waku_rln_relay/test_rln_nonce_manager.nim | 31 ------------------- 6 files changed, 30 insertions(+), 90 deletions(-) diff --git a/logos_delivery/waku/node/waku_node/lightpush.nim b/logos_delivery/waku/node/waku_node/lightpush.nim index 1e2159c62..00e626e35 100644 --- a/logos_delivery/waku/node/waku_node/lightpush.nim +++ b/logos_delivery/waku/node/waku_node/lightpush.nim @@ -30,8 +30,7 @@ import ../../waku_lightpush as lightpush_protocol, ../peer_manager, ../../common/rate_limit/setting, - ../../rln, - ../../rln/nonce_manager + ../../rln logScope: topics = "waku node lightpush api" @@ -109,7 +108,6 @@ proc runRlnRefreshRetry( node: WakuNode, rln: Option[Rln], msgWithProof: WakuMessage, - drawnMessageId: Option[Nonce], pubsubForPublish: PubsubTopic, peer: RemotePeerInfo, fallback: legacy_lightpush_protocol.WakuLightPushResult[string], @@ -124,10 +122,8 @@ proc runRlnRefreshRetry( proc runRetry(): Future[legacy_lightpush_protocol.WakuLightPushResult[string]] {. async, gcsafe .} = - let (retryMsg, _) = ( - await checkAndGenerateRLNProof( - rln, msgWithProof, regenerate = true, reuseMessageId = drawnMessageId - ) + let retryMsg = ( + await checkAndGenerateRLNProof(rln, msgWithProof, regenerate = true) ).valueOr: return err("failed call checkAndGenerateRLNProof from lightpush retry: " & error) return await internalLegacyLightpushPublish(node, pubsubForPublish, retryMsg, peer) @@ -162,7 +158,7 @@ proc legacyLightpushPublish*( none(Rln) else: some(node.rln) - let (msgWithProof, drawnMessageId) = (await checkAndGenerateRLNProof(rln, message)).valueOr: + let msgWithProof = (await checkAndGenerateRLNProof(rln, message)).valueOr: return err("failed call checkAndGenerateRLNProof from lightpush: " & error) try: @@ -182,7 +178,7 @@ proc legacyLightpushPublish*( return firstResult return await runRlnRefreshRetry( - node, rln, msgWithProof, drawnMessageId, pubsubForPublish, peer, firstResult + node, rln, msgWithProof, pubsubForPublish, peer, firstResult ) except CatchableError: return err(getCurrentExceptionMsg()) @@ -345,7 +341,7 @@ proc lightpushPublish*( none(Rln) else: some(node.rln) - let (msgWithProof, drawnMessageId) = (await checkAndGenerateRLNProof(rln, message)).valueOr: + let msgWithProof = (await checkAndGenerateRLNProof(rln, message)).valueOr: return lighpushErrorResult(LightPushErrorCode.OUT_OF_RLN_PROOF, error) let firstResult = @@ -372,10 +368,8 @@ proc lightpushPublish*( rln.get().groupManager.invalidateMerkleProofCache() proc runRetry(): Future[lightpush_protocol.WakuLightPushResult] {.async, gcsafe.} = - let (retryMsg, _) = ( - await checkAndGenerateRLNProof( - rln, msgWithProof, regenerate = true, reuseMessageId = drawnMessageId - ) + let retryMsg = ( + await checkAndGenerateRLNProof(rln, msgWithProof, regenerate = true) ).valueOr: return lighpushErrorResult(LightPushErrorCode.OUT_OF_RLN_PROOF, error) return diff --git a/logos_delivery/waku/rln/nonce_manager.nim b/logos_delivery/waku/rln/nonce_manager.nim index 889abedcd..257d9692c 100644 --- a/logos_delivery/waku/rln/nonce_manager.nim +++ b/logos_delivery/waku/rln/nonce_manager.nim @@ -57,13 +57,3 @@ proc getNonce*(n: NonceManager): NonceManagerResult[Nonce] = ) return ok(retNonce) - -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 diff --git a/logos_delivery/waku/rln/proof.nim b/logos_delivery/waku/rln/proof.nim index 8a79a1d2d..2f956e043 100644 --- a/logos_delivery/waku/rln/proof.nim +++ b/logos_delivery/waku/rln/proof.nim @@ -74,7 +74,7 @@ proc generateRLNProofWithRootRefresh*( ## sees an empty cache and refetches from chain. Returns the proof bytes ## the caller can attach to the message. let proofBytes = (await rln.generateRLNProof(input, senderEpochTime)).valueOr: - return err(error) + return err("failed to generate RLN proof: " & $error) let rlnProof = RateLimitProof.init(proofBytes).valueOr: return err("could not decode proof for root check: " & $error) @@ -87,35 +87,21 @@ proc generateRLNProofWithRootRefresh*( return await rln.generateRLNProof(input, senderEpochTime) proc checkAndGenerateRLNProof*( - rln: Option[Rln], - message: WakuMessage, - regenerate: 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. + rln: Option[Rln], message: WakuMessage, regenerate: bool = false +): Future[Result[WakuMessage, string]] {.async.} = + ## Returns the message with an attached RLN proof. ## Set `regenerate = true` to bypass the "already has proof" short-circuit ## and always generate a fresh proof — used by the retry path after a stale ## proof was rejected; the caller should first `invalidateMerkleProofCache` ## so the fresh proof is generated against a refetched merkle path. if message.proof.len > 0 and not regenerate: - return ok((msg: message, messageId: none(Nonce))) + return ok(message) if rln.isNone(): notice "Publishing message without RLN proof" - return ok((msg: message, messageId: none(Nonce))) + return ok(message) 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) @@ -128,4 +114,4 @@ proc checkAndGenerateRLNProof*( ).valueOr: return err("error in checkAndGenerateRLNProof: " & $error) msgWithProof.proof = proof.encode().buffer - return ok((msg: msgWithProof, messageId: some(nonce))) + return ok(msgWithProof) diff --git a/tests/node/test_wakunode_legacy_lightpush.nim b/tests/node/test_wakunode_legacy_lightpush.nim index f09f38716..aec37e18c 100644 --- a/tests/node/test_wakunode_legacy_lightpush.nim +++ b/tests/node/test_wakunode_legacy_lightpush.nim @@ -121,7 +121,10 @@ suite "RLN Proofs as a Lightpush Service": # mount rln-relay # match prod epoch window to reduce test flake wakuRlnConfig = getWakuRlnConfig( - manager = manager, index = MembershipIndex(1), epochSizeSec = 600 + manager = manager, + userMessageLimit = 20, + index = MembershipIndex(1), + epochSizeSec = 600, ) await allFutures(server.start(), client.start()) @@ -166,7 +169,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 diff --git a/tests/node/test_wakunode_lightpush.nim b/tests/node/test_wakunode_lightpush.nim index 57228f9fa..6766d8eca 100644 --- a/tests/node/test_wakunode_lightpush.nim +++ b/tests/node/test_wakunode_lightpush.nim @@ -117,7 +117,10 @@ suite "RLN Proofs as a Lightpush Service": # mount rln-relay # match prod epoch window to reduce test flake wakuRlnConfig = getWakuRlnConfig( - manager = manager, index = MembershipIndex(1), epochSizeSec = 600 + manager = manager, + userMessageLimit = 20, + index = MembershipIndex(1), + epochSizeSec = 600, ) await allFutures(server.start(), client.start()) @@ -162,7 +165,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,8 +186,7 @@ suite "RLN Proofs as a Lightpush Service": # invalidateMerkleProofCache empties the cached path so the next # proof-gen refetches from chain, and `regenerate = true` on # checkAndGenerateRLNProof bypasses the "already has proof" short-circuit. - let (firstMsg, drawnNonce) = - (await checkAndGenerateRLNProof(some(server.rln), message)).get() + let firstMsg = (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 @@ -195,20 +197,16 @@ suite "RLN Proofs as a Lightpush Service": check manager.merkleProofCache != goodCache # Retry path: invalidate the cache so the next proof-gen refetches from - # chain, then regenerate the proof. Pass drawnNonce so the CAS rollback - # reclaims it — mirroring the production retry path. + # chain, then regenerate the proof. manager.invalidateMerkleProofCache() - let (secondMsg, _) = ( - await checkAndGenerateRLNProof( - some(server.rln), firstMsg, regenerate = true, reuseMessageId = drawnNonce - ) + let secondMsg = ( + await checkAndGenerateRLNProof(some(server.rln), firstMsg, regenerate = true) ).get() check: secondMsg.proof.len > 0 # 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. + # blinding, so a fresh call produces different bytes. secondMsg.proof != firstMsg.proof # Cache was refetched from chain, overwriting the corruption. manager.merkleProofCache == goodCache diff --git a/tests/waku_rln_relay/test_rln_nonce_manager.nim b/tests/waku_rln_relay/test_rln_nonce_manager.nim index ffaf691d5..f9e541e1a 100644 --- a/tests/waku_rln_relay/test_rln_nonce_manager.nim +++ b/tests/waku_rln_relay/test_rln_nonce_manager.nim @@ -41,34 +41,3 @@ suite "Nonce manager": check: nonce == 0.uint nonce2 == 0.uint - - 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 - check nm.rollbackNonce(first) - let reissued = nm.getNonce().valueOr: - raiseAssert $error - - check: - first == 0.uint - reissued == first - nm.nextNonce == 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 - 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) - check not nm.rollbackNonce(Nonce(0)) - - check: - nm.nextNonce == 0.uint