diff --git a/logos_delivery/waku/node/waku_node/lightpush.nim b/logos_delivery/waku/node/waku_node/lightpush.nim index 1b26aa79d..6631da808 100644 --- a/logos_delivery/waku/node/waku_node/lightpush.nim +++ b/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]: diff --git a/logos_delivery/waku/node/waku_node/relay.nim b/logos_delivery/waku/node/waku_node/relay.nim index 18a3f0f59..77143fabb 100644 --- a/logos_delivery/waku/node/waku_node/relay.nim +++ b/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) diff --git a/logos_delivery/waku/rln/constants.nim b/logos_delivery/waku/rln/constants.nim index 961bbcce7..6ffe51d2b 100644 --- a/logos_delivery/waku/rln/constants.nim +++ b/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 diff --git a/logos_delivery/waku/rln/nonce_manager.nim b/logos_delivery/waku/rln/nonce_manager.nim index 257d9692c..c27c4e84d 100644 --- a/logos_delivery/waku/rln/nonce_manager.nim +++ b/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 diff --git a/logos_delivery/waku/rln/proof.nim b/logos_delivery/waku/rln/proof.nim index c61a55a07..277de6b2a 100644 --- a/logos_delivery/waku/rln/proof.nim +++ b/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 = ( diff --git a/tests/node/test_wakunode_legacy_lightpush.nim b/tests/node/test_wakunode_legacy_lightpush.nim index a829dc272..6a94f95a8 100644 --- a/tests/node/test_wakunode_legacy_lightpush.nim +++ b/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 diff --git a/tests/node/test_wakunode_lightpush.nim b/tests/node/test_wakunode_lightpush.nim index 53ff719c8..0811dac2c 100644 --- a/tests/node/test_wakunode_lightpush.nim +++ b/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 diff --git a/tests/waku_rln_relay/test_rln_nonce_manager.nim b/tests/waku_rln_relay/test_rln_nonce_manager.nim index f9e541e1a..32adfaea6 100644 --- a/tests/waku_rln_relay/test_rln_nonce_manager.nim +++ b/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