fix: harden client info pprof gating and SPV proof-header validation - #4339
Merged
Merged
Conversation
Importing net/http/pprof registers /debug/pprof/* on http.DefaultServeMux from that package's init, regardless of whether the import is blank or named. The client info server built an http.Server with a nil Handler, so it served DefaultServeMux and exposed the profiling endpoints on the client info port even when EnablePprof was false. Any caller able to reach that port got goroutine and runtime introspection plus the expensive profile and trace endpoints, and an operator who explicitly disabled profiling was silently overridden. Build the handler on a mux created per registry instead, registering the pprof handlers on it only when profiling is enabled. Constructing the mux per call also removes a latent panic: registering the registry's routes on the process-global mux made a second registry fail with a duplicate pattern conflict. EnableServer keeps its signature and never exposes profiling; Initialize routes through a private helper that takes the flag.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The flag is registered with cobra's UintVar, which accepts 0, and nothing validated the parsed value. The 144 default only covers the case where the flag is omitted, so --spv.maxProofHeaders 0 reached the proof assembly loop. getProofInfo compares the running header count against the bound at loop entry, with the count starting at zero, so a zero bound returns proofSkipExceededMaxHeaders on the first iteration for every transaction. That disables SPV proving entirely while logging each transaction as possibly permanently unprovable, which reads as a chain condition rather than a misconfiguration. Reject the value at startup rather than normalizing it to the default, so an explicit instruction is not silently replaced. Validation runs before any chain connection is attempted.
The release process described sub-PRs merging into dev and main before closing. If sub-PRs also landed on main, the aggregation PR's diff would be empty and there would be nothing left to release, contradicting the adjacent text describing that diff as what is queued for the next release. The maxWalletTxVsize comment called 10M vbytes roughly 2x a Bitcoin block weight. A block is capped at 4,000,000 weight units, which is 1,000,000 vbytes, so the bound is about 10x a maximum block's vsize; the original also conflated vbytes with weight units.
piotr-roslaniec
force-pushed
the
fix/pr-4256-review-findings
branch
from
September 23, 2026 08:44
8cb2644 to
be4371d
Compare
piotr-roslaniec
changed the title
- align validateMaintainerConfig with the library maintainer.Config.Validate conditional rule so a disabled-SPV setup is not rejected on an unrelated zero maxProofHeaders - extend spv.Config.Validate to reject zero/non-positive HistoryDepth, TransactionLimit, and backoff times: the flag defaults only cover omission, and each of these values silently degrades the maintainer at runtime the same way a zero MaxProofHeaders bound did - harden the clientinfo live-server tests: reserve an OS ephemeral port instead of the fixed 9799, following the cmd/maintainer_test.go pattern - add /profile and /trace to the enabled-path pprof endpoint assertions - warn when EnablePprof is set without a Port so the misconfiguration is not silently ignored - cover custom positive maxProofHeaders values through the flag -> config -> validation path - correct the stale DefaultServeMux comment in cmd/maintainer_test.go - document who merges sub-PRs into dev in docs/release-process.md
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes found while reviewing the cumulative
dev→maindiff in#4256. Each was verified against
devbefore being fixed; both behavioralchanges have a regression test that fails without the fix.
pprof exposed while disabled
EnablePprof: falsedid not disable profiling. Importingnet/http/pprofregisters
/debug/pprof/*onhttp.DefaultServeMuxfrom that package'sinit, whether the import is blank or named.EnableServerbuilt anhttp.Serverwith a nilHandler, so it servedDefaultServeMuxandexposed the profiling endpoints on the client info port regardless of the
setting.
Reproduced before fixing:
GET /debug/pprof/against the existingport-9799 test server returned
200while profiling was disabled.Impact is bounded by whether the client info port is reachable. Where it
is, a caller gets goroutine and runtime introspection plus the expensive
profileandtraceendpoints, and an operator who explicitly disabledprofiling is silently overridden. To be precise about scope: Go's heap
profile reports allocation metadata and stacks, not arbitrary process
memory, so this is not a key-material dump.
The handler is now built on a mux created per registry, with the pprof
handlers registered only when profiling is enabled. That also removes a
latent panic: registering the registry's routes on the process-global mux
made a second registry fail with a duplicate-pattern conflict.
EnableServer(port int)keeps its signature and never exposes profiling;Initializecalls a privateenableServer(port, cfg.EnablePprof). Noexported API change.
--spv.maxProofHeaders 0silently disables SPV provingThe flag uses cobra's
UintVar, which accepts0, and nothing validatedthe parsed value — the
144default only covers omission.getProofInfocompares the running header count against the bound at loop entry with the
count starting at zero, so a zero bound returns
proofSkipExceededMaxHeaderson the first iteration for everytransaction. SPV proving stops entirely while the logs report each
transaction as possibly permanently unprovable, which reads as a chain
condition rather than a misconfiguration.
Rejected at startup rather than normalized to the default, so an explicit
operator instruction is not silently replaced. Validation runs via
validateMaintainerConfig, the first statement inmaintainers(), beforeany chain connection is attempted.
Documentation corrections
docs/release-process.mddescribed sub-PRs merging intodevandmainbefore closing. If they also landed onmain, the aggregation PR'sdiff would be empty and there would be nothing to release, contradicting
the adjacent text describing that diff as what is queued for the next
release.
The
maxWalletTxVsizecomment called 10M vbytes "~2x Bitcoin blockweight". A block is capped at 4,000,000 weight units, which is 1,000,000
vbytes, so the bound is ~10x a maximum block's vsize; the original also
conflated vbytes with weight units. The bound itself is unchanged.
Verification
Run locally — not a full-repository suite, and CI has not been observed:
go build ./...go vetonpkg/clientinfo,pkg/maintainer/spv,pkg/tbtcpg,cmdgo testonpkg/clientinfo,cmd,pkg/maintainer/...,pkg/tbtcpg,pkg/tbtcTests added: pprof disabled (404 on the real server mux) and enabled (200
via
serverHandler(true));Config.Validaterejecting zero; and thecommand-level flag path asserting
--spv.maxProofHeaders 0parses intoconfig and is then rejected.
pkg/clientinfowas run serially — itsTestMainbinds a fixed port, soconcurrent runs can cross-talk.
Scope
These are the findings confirmed from a partial review of #4256; the
review did not cover the whole aggregation diff, so this PR is not
evidence that the rest of it is clean. Pre-existing issues unchanged by
#4256 (the
latesttag intbtc-v2-monitoring,thresholdnetworkreferences in
scripts/build.shand the docker sample) were deliberatelyleft alone.