From e7afdce63e19674cd21da546a077851140cf9dd5 Mon Sep 17 00:00:00 2001 From: stubbsta Date: Mon, 6 Jul 2026 15:01:16 +0200 Subject: [PATCH] Fix nonce rollback race: make rollbackNonce CAS-conditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../waku/node/waku_node/lightpush.nim | 24 ++++++---- logos_delivery/waku/rln/nonce_manager.nim | 18 +++---- logos_delivery/waku/rln/proof.nim | 47 ++++++++++++------- tests/node/test_wakunode_legacy_lightpush.nim | 15 ++++-- tests/node/test_wakunode_lightpush.nim | 15 ++++-- .../waku_rln_relay/test_rln_nonce_manager.nim | 25 +++++----- 6 files changed, 84 insertions(+), 60 deletions(-) diff --git a/logos_delivery/waku/node/waku_node/lightpush.nim b/logos_delivery/waku/node/waku_node/lightpush.nim index 6631da808..7b444fed5 100644 --- a/logos_delivery/waku/node/waku_node/lightpush.nim +++ b/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) diff --git a/logos_delivery/waku/rln/nonce_manager.nim b/logos_delivery/waku/rln/nonce_manager.nim index c27c4e84d..889abedcd 100644 --- a/logos_delivery/waku/rln/nonce_manager.nim +++ b/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 diff --git a/logos_delivery/waku/rln/proof.nim b/logos_delivery/waku/rln/proof.nim index 277de6b2a..c1bcb70e9 100644 --- a/logos_delivery/waku/rln/proof.nim +++ b/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))) diff --git a/tests/node/test_wakunode_legacy_lightpush.nim b/tests/node/test_wakunode_legacy_lightpush.nim index 6a94f95a8..052c09d11 100644 --- a/tests/node/test_wakunode_legacy_lightpush.nim +++ b/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() diff --git a/tests/node/test_wakunode_lightpush.nim b/tests/node/test_wakunode_lightpush.nim index 0811dac2c..de771348c 100644 --- a/tests/node/test_wakunode_lightpush.nim +++ b/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() diff --git a/tests/waku_rln_relay/test_rln_nonce_manager.nim b/tests/waku_rln_relay/test_rln_nonce_manager.nim index 32adfaea6..ffaf691d5 100644 --- a/tests/waku_rln_relay/test_rln_nonce_manager.nim +++ b/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