From 2acd2fd4b1193f8bd52b998b7d2386af8599b35a Mon Sep 17 00:00:00 2001 From: stubbsta Date: Tue, 21 Jul 2026 09:50:20 +0200 Subject: [PATCH] fix: make MessagingClientConf.rateLimit reachable, warn on dead channel knobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two review findings on the move PR: - `rateLimit` was a plain `RateLimitConfig`, but `merge` only copies `Opt` fields (`when oField is Opt`), so every override path (`LogosDeliveryConf.init`, JSON `messagingOverrides`) silently dropped it and the field was always taken from `base` — unsettable by any caller. Make it `Opt[RateLimitConfig]` like every other field; `MessagingClient.new` falls back to `DefaultRateLimitConfig` (new const, rate limiting disabled) when unset. Adds a merge test. - The channel-level knobs (`rateLimitEnabled` / `rateLimitEpochPeriodSec` / `rateLimitMessagesPerEpoch`) are still parsed but unread since rate limiting moved to the messaging client, so setting them was silently ignored. `ReliableChannelManager.new` now logs a deprecation warning when any is set. Full removal remains a follow-up API decision. Co-Authored-By: Claude Opus 4.8 (1M context) --- logos_delivery/api/conf/messaging_conf.nim | 7 ++++--- .../channels/reliable_channel_manager.nim | 5 +++++ logos_delivery/messaging/messaging_client.nim | 5 +++-- .../rate_limit_manager/rate_limit_manager.nim | 4 ++++ tests/api/test_conf.nim | 14 ++++++++++++++ 5 files changed, 30 insertions(+), 5 deletions(-) diff --git a/logos_delivery/api/conf/messaging_conf.nim b/logos_delivery/api/conf/messaging_conf.nim index 3ec35481a..62abea75e 100644 --- a/logos_delivery/api/conf/messaging_conf.nim +++ b/logos_delivery/api/conf/messaging_conf.nim @@ -51,9 +51,10 @@ type MessagingClientConf* = object ## Process log format (TEXT or JSON); applied by the kernel on node creation. nodeKey* {.name: "nodekey".}: Opt[crypto.PrivateKey] ## P2P node private key (64-char hex): stable identity / peerId across restarts. - rateLimit*: RateLimitConfig = RateLimitConfig( - epochPeriodSec: DefaultEpochPeriodSec, messagesPerEpoch: DefaultMessagesPerEpoch - ) ## RLN-epoch transmission budget enforced by the send service. + rateLimit*: Opt[RateLimitConfig] + ## Per-epoch message rate limit enforced by the send service. `Opt` like + ## every other field so `merge` propagates a caller's override; unset falls + ## back to `DefaultRateLimitConfig` (rate limiting disabled). proc applyMode*(conf: var WakuNodeConf, mode: LogosDeliveryMode): ConfResult[void] = ## Sets the protocol flags implied by the mode. diff --git a/logos_delivery/channels/reliable_channel_manager.nim b/logos_delivery/channels/reliable_channel_manager.nim index 37a53aaf1..13f5d5937 100644 --- a/logos_delivery/channels/reliable_channel_manager.nim +++ b/logos_delivery/channels/reliable_channel_manager.nim @@ -32,6 +32,11 @@ proc new*( conf: ReliableChannelManagerConf, brokerCtx: BrokerContext = globalBrokerContext(), ): Result[T, string] = + if conf.rateLimitEnabled.isSome() or conf.rateLimitEpochPeriodSec.isSome() or + conf.rateLimitMessagesPerEpoch.isSome(): + warn "channel-level rate-limit config is deprecated and ignored; " & + "rate limiting moved to the messaging client (MessagingClientConf.rateLimit)" + return ok( T( channels: initTable[ChannelId, ReliableChannel](), diff --git a/logos_delivery/messaging/messaging_client.nim b/logos_delivery/messaging/messaging_client.nim index 494ba5ad1..c33b3ff6a 100644 --- a/logos_delivery/messaging/messaging_client.nim +++ b/logos_delivery/messaging/messaging_client.nim @@ -25,8 +25,9 @@ proc new*( ## The messaging layer chains onto Waku: it drives the underlying Waku kernel ## for transport while exposing its own send/recv API. let reliability = conf.reliabilityEnabled.get(DefaultP2pReliability) - let sendService = - ?SendService.new(reliability, waku, RateLimitManager.new(conf.rateLimit)) + let sendService = ?SendService.new( + reliability, waku, RateLimitManager.new(conf.rateLimit.get(DefaultRateLimitConfig)) + ) let recvService = RecvService.new(waku) return ok( T( diff --git a/logos_delivery/messaging/rate_limit_manager/rate_limit_manager.nim b/logos_delivery/messaging/rate_limit_manager/rate_limit_manager.nim index 7ee197d8a..ba4944dca 100644 --- a/logos_delivery/messaging/rate_limit_manager/rate_limit_manager.nim +++ b/logos_delivery/messaging/rate_limit_manager/rate_limit_manager.nim @@ -32,6 +32,10 @@ const DefaultEpochPeriodSec* = 600 DefaultMessagesPerEpoch* = 1 + DefaultRateLimitConfig* = RateLimitConfig( + epochPeriodSec: DefaultEpochPeriodSec, messagesPerEpoch: DefaultMessagesPerEpoch + ) ## Used when no rate-limit config is supplied; `enabled` defaults false. + proc new*(T: type RateLimitManager, config: RateLimitConfig): T = return T(config: config, queue: @[], currentEpochStart: getTime(), sentInCurrentEpoch: 0) diff --git a/tests/api/test_conf.nim b/tests/api/test_conf.nim index 7f4f23315..ba1bf6f91 100644 --- a/tests/api/test_conf.nim +++ b/tests/api/test_conf.nim @@ -112,6 +112,20 @@ suite "MessagingClientConf - merge (override wins)": mc.clusterId == Opt.some(2'u16) # override wins mc.maxMessageSize == Opt.some("1MB") # base preserved + test "a rateLimit override propagates through merge": + ## Regression: rateLimit must be `Opt` so `merge` copies it; as a plain + ## object it was always taken from base, leaving the field unreachable. + let overrides = MessagingClientConf( + rateLimit: Opt.some( + RateLimitConfig(enabled: true, epochPeriodSec: 30, messagesPerEpoch: 5) + ) + ) + let mc = merge(MessagingClientConf(), overrides) + check: + mc.rateLimit == overrides.rateLimit + mc.rateLimit.get().enabled + mc.rateLimit.get().messagesPerEpoch == 5 + suite "parseLogosDeliveryConf - JSON parsing": test "empty object resolves to a full Core node conf": let lc = parseLogosDeliveryConf("{}").valueOr: