fix: make MessagingClientConf.rateLimit reachable, warn on dead channel knobs

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) <noreply@anthropic.com>
This commit is contained in:
stubbsta 2026-07-21 09:50:20 +02:00
parent 13bf6a4b3d
commit 2acd2fd4b1
No known key found for this signature in database
5 changed files with 30 additions and 5 deletions

View File

@ -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.

View File

@ -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](),

View File

@ -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(

View File

@ -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)

View File

@ -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: