1581 add upper cap limits to chosen websocket endpoints to increase resilliency#1582
Conversation
pragmaxim
left a comment
There was a problem hiding this comment.
Automated review of the cap-limit changes. The core logic is sound — the ERC-1155 TransferBatch bounds/overflow hardening in particular holds up well (candidate concerns about countIds misuse and erc1155BatchOffsetHex rejecting valid <0x40 offsets were checked and refuted). Findings below are behavior-change + cleanup, no crashes.
Inline comments cover the actionable items. One extra very-minor nit not inline (the function isn't in the diff): getFiatRatesResult (api/fiat_rates_api.go:81 and :104) still calls strings.ToLower on each selector, but the slice it receives is already lowercased by normalizeFiatCurrencies — redundant, bounded work. (Note line 72 is not redundant; it iterates raw ticker-map keys.)
The MaxFiatRatesCurrencies check fired only when a unique normalized code was appended, so an all-duplicate list bypassed the cap while still costing a full trim/lowercase/dedup pass per element. Reject over-long lists up front so the normalization work itself is bounded by the cap; this also makes the in-loop unique-count check unreachable, so drop it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setFiatRateToBalanceHistories re-normalized a slice that both of its callers (GetBalanceHistory, GetXpubBalanceHistory) had already passed through normalizeFiatCurrencies, and getFiatRatesResult re-lowercased selectors already lowercased by its callers. Drop the redundant passes and document the pre-normalized precondition instead. Normalization was the only error path of setFiatRateToBalanceHistories, so it no longer returns an error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The endValues <= len(data) check already bounds countValues to len(data)/evmWordHex, far below maxInt, so the truncation guard on the int conversion could never fire (the load-bearing overflow check lives in erc1155BatchArrayEnd). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The slot is deliberately held from before the handler computes the response until it is written to the websocket (or drained on close): the cap exists to bound peak memory in computed-but-unwritten filter responses, so it covers compute, queueing, and write together rather than compute concurrency alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
32df92c to
66bee88
Compare
pragmaxim
left a comment
There was a problem hiding this comment.
Two nit-level comments below. The rest of the review found no correctness or security issues — the websocket slot lifecycle and the ERC-1155 bounds math both hold up, all tests pass with -tags unittest, and go vet is clean.
…rite Adds TestWebsocketOutputLoopReleasesMempoolFilterSlotAfterWrite, which drives a release-bearing WsRes through outputLoop over a live websocket connection and asserts the mempool-filters slot is freed after WriteJSON. This pins the primary release path (outputLoop's c.finalize(m)) that the existing semaphore and drain-on-close tests do not exercise; without it, removing that call would leak a slot on every successful response and go undetected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Adds upper-cap / bound-checking limits to several server endpoints to improve resilience against unbounded memory growth and abusive request patterns (closes #1581). Each cap is covered by unit tests.
Changes
WebSocket — cap in-flight
getMempoolFiltersresponsesPer-connection semaphore (
maxWebsocketMempoolFiltersResponses = 4) bounds each request from acceptance until its response is written (or drained on close), capping the peak memory held in large filter responses; excess requests are rejected with amempool_filters_limiterror. Adds arelease/finalizehook onWsResso the slot is freed exactly once across write, drain-on-close, and overflow paths.Fiat rates — bound and normalize currency selectors
Replaces
removeEmptywithnormalizeFiatCurrencies(trim, lowercase, dedupe, bound) applied once at the entry points (GetCurrentFiatRates,GetFiatRatesForTimestamps,GetBalanceHistory,GetXpubBalanceHistory) before any per-result maps are allocated; downstream helpers rely on the pre-normalized slice. New limitsMaxFiatRatesCurrencies = 128(applied to the raw selector list length) andMaxFiatRatesCurrencyCodeLength = 16return a client-facing error when exceeded.EVM — harden ERC-1155
TransferBatchlog parsingRewrites offset/length handling with explicit bounds and overflow checks to avoid possible OOM / out-of-range slicing from malformed log data, validating total data length before allocating the result slice.
Behavior changes
APIError(HTTP 400 on REST). The REST tickers endpoint never splits?currency=on commas — it passes the whole value as a single selector — so a comma-joined value is only rejected once it exceeds 16 chars.?currency=usd,eur,gbp,jpy(15 chars) therefore still returns200with{"usd,eur,gbp,jpy": -1}exactly as before; only a longer value such as?currency=usd,eur,gbp,cad,jpy(19 chars) now returns400 "currency code too long, max 16".getCurrentFiatRates,getFiatRatesForTimestamps) and the balance-history paths; the single-param REST tickers endpoint always passes 0 or 1 selector, so it can never trip the count cap.mempool_filters_limiterror.Tests
server/websocket_test.go— mempool filters slot capping, including release after a successful write (viaoutputLoopover a live connection) and drain-on-closeapi/fiat_rates_api_test.go,api/fiat_balance_history_test.go— currency normalization/limitsbchain/coins/eth/contract_test.go— malformed ERC-1155 batch logs