From 864d65817904a8d97781f19ef92339245bc096d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Wed, 23 Sep 2026 08:43:12 +0000 Subject: [PATCH 1/4] fix(clientinfo): serve a private mux so EnablePprof is authoritative 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. --- pkg/clientinfo/clientinfo.go | 82 ++++++++++------ pkg/clientinfo/pprof_exposure_test.go | 133 ++++++++++++++++++++++++++ 2 files changed, 184 insertions(+), 31 deletions(-) create mode 100644 pkg/clientinfo/pprof_exposure_test.go diff --git a/pkg/clientinfo/clientinfo.go b/pkg/clientinfo/clientinfo.go index b0f915331f..d531dbd057 100644 --- a/pkg/clientinfo/clientinfo.go +++ b/pkg/clientinfo/clientinfo.go @@ -75,53 +75,73 @@ func Initialize( registry := newRegistry(ctx) - if cfg.EnablePprof { - // Register the pprof handlers on http.DefaultServeMux, which is the - // mux that EnableServer hands to the http.Server. Registering them - // explicitly here avoids the side-effecting blank import of - // net/http/pprof, which would otherwise register /debug/pprof/* - // unconditionally on DefaultServeMux regardless of this flag. - registerPprofHandlers() - logger.Infof("pprof profiling endpoints enabled at /debug/pprof/") - } - - registry.EnableServer(cfg.Port) + registry.enableServer(cfg.Port, cfg.EnablePprof) return registry, true } -// registerPprofHandlers registers the standard net/http/pprof handlers on -// http.DefaultServeMux. It is invoked explicitly from Initialize when -// EnablePprof is true, in place of the blank import of net/http/pprof that -// would otherwise register the endpoints at init time. -func registerPprofHandlers() { - http.HandleFunc("/debug/pprof/", pprof.Index) - http.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) - http.HandleFunc("/debug/pprof/profile", pprof.Profile) - http.HandleFunc("/debug/pprof/symbol", pprof.Symbol) - http.HandleFunc("/debug/pprof/trace", pprof.Trace) +// registerPprofHandlers registers the standard net/http/pprof handlers on the +// supplied mux. Registering them on a caller-owned mux, rather than relying on +// the handlers net/http/pprof installs on http.DefaultServeMux at init time, +// is what makes Config.EnablePprof authoritative: the server below never +// serves DefaultServeMux, so the init-time registrations are unreachable. +func registerPprofHandlers(mux *http.ServeMux) { + mux.HandleFunc("/debug/pprof/", pprof.Index) + mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) + mux.HandleFunc("/debug/pprof/profile", pprof.Profile) + mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) + mux.HandleFunc("/debug/pprof/trace", pprof.Trace) } -// EnableServer enables the client info server on the given port. Data will -// be exposed on `/metrics` and `/diagnostics` paths. -func (r *Registry) EnableServer(port int) { - server := &http.Server{ - Addr: ":" + strconv.Itoa(port), - ReadHeaderTimeout: readHeaderTimeout, - } - - http.HandleFunc("/metrics", func(response http.ResponseWriter, _ *http.Request) { +// serverHandler builds the HTTP handler served on the client info port. +// +// The mux is created per call and never shared with http.DefaultServeMux. +// That isolation is load-bearing for two reasons: importing net/http/pprof +// registers /debug/pprof/* on DefaultServeMux from that package's init +// regardless of how it is imported, so serving DefaultServeMux would expose +// profiling endpoints even when disabled; and registering this registry's own +// routes on a process-global mux makes a second registry panic on duplicate +// patterns. +func (r *Registry) serverHandler(enablePprof bool) http.Handler { + mux := http.NewServeMux() + + mux.HandleFunc("/metrics", func(response http.ResponseWriter, _ *http.Request) { if _, err := io.WriteString(response, r.exposeMetrics()); err != nil { logger.Errorf("could not write response: [%v]", err) } }) - http.HandleFunc("/diagnostics", func(response http.ResponseWriter, _ *http.Request) { + mux.HandleFunc("/diagnostics", func(response http.ResponseWriter, _ *http.Request) { if _, err := io.WriteString(response, r.exposeDiagnostics()); err != nil { logger.Errorf("could not write response: [%v]", err) } }) + if enablePprof { + registerPprofHandlers(mux) + logger.Infof("pprof profiling endpoints enabled at /debug/pprof/") + } + + return mux +} + +// EnableServer enables the client info server on the given port. Data will +// be exposed on `/metrics` and `/diagnostics` paths. Profiling endpoints are +// never exposed through this entry point; use Config.EnablePprof with +// Initialize to opt into them. +func (r *Registry) EnableServer(port int) { + r.enableServer(port, false) +} + +// enableServer starts the client info HTTP server, exposing the profiling +// endpoints only when enablePprof is true. +func (r *Registry) enableServer(port int, enablePprof bool) { + server := &http.Server{ + Addr: ":" + strconv.Itoa(port), + Handler: r.serverHandler(enablePprof), + ReadHeaderTimeout: readHeaderTimeout, + } + go func() { if err := server.ListenAndServe(); err != http.ErrServerClosed { logger.Errorf("client info server error: [%v]", err) diff --git a/pkg/clientinfo/pprof_exposure_test.go b/pkg/clientinfo/pprof_exposure_test.go new file mode 100644 index 0000000000..8adf749b3c --- /dev/null +++ b/pkg/clientinfo/pprof_exposure_test.go @@ -0,0 +1,133 @@ +package clientinfo + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +// TestClientInfoServerDoesNotExposePprofWhenDisabled asserts that the Go +// profiling endpoints are NOT reachable on the client info port when profiling +// was not enabled. +// +// This is a security regression guard, not a coverage exercise. Importing +// net/http/pprof registers /debug/pprof/* on http.DefaultServeMux from that +// package's init function, whether the import is blank or named. If the client +// info server serves DefaultServeMux, those endpoints are exposed on the client +// info port regardless of Config.EnablePprof, silently defeating an operator +// who explicitly disabled profiling and handing any caller that can reach the +// port goroutine/runtime introspection plus the expensive profile and trace +// endpoints. +// +// The server under test is started by TestMain with profiling disabled. +func TestClientInfoServerDoesNotExposePprofWhenDisabled(t *testing.T) { + for _, path := range []string{ + "/debug/pprof/", + "/debug/pprof/cmdline", + "/debug/pprof/profile?seconds=1", + "/debug/pprof/symbol", + "/debug/pprof/trace?seconds=1", + } { + t.Run(path, func(t *testing.T) { + response, err := http.Get( + fmt.Sprintf("http://localhost:%d%s", port, path), + ) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + + if response.StatusCode != http.StatusNotFound { + t.Errorf( + "profiling endpoint [%s] is reachable while profiling is disabled\n"+ + "expected status: %d\nactual status: %d", + path, + http.StatusNotFound, + response.StatusCode, + ) + } + }) + } +} + +// TestClientInfoServerServesMetricsWhenPprofDisabled asserts the endpoints the +// server exists to serve stay reachable, so the pprof fix cannot regress them. +func TestClientInfoServerServesMetricsWhenPprofDisabled(t *testing.T) { + for _, path := range []string{"/metrics", "/diagnostics"} { + t.Run(path, func(t *testing.T) { + response, err := http.Get( + fmt.Sprintf("http://localhost:%d%s", port, path), + ) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + + if response.StatusCode != http.StatusOK { + t.Errorf( + "expected [%s] to be reachable\nexpected status: %d\nactual status: %d", + path, + http.StatusOK, + response.StatusCode, + ) + } + }) + } +} + +// TestServerHandler_PprofEnabled asserts the flag remains functional in the +// other direction: opting into profiling must actually serve the endpoints. +// Without this, the disabled-path test above could be satisfied by never +// registering the handlers at all. +func TestServerHandler_PprofEnabled(t *testing.T) { + handler := newRegistry(context.Background()).serverHandler(true) + + for _, path := range []string{ + "/debug/pprof/", + "/debug/pprof/cmdline", + "/debug/pprof/symbol", + } { + t.Run(path, func(t *testing.T) { + recorder := httptest.NewRecorder() + handler.ServeHTTP( + recorder, + httptest.NewRequest(http.MethodGet, path, nil), + ) + + if recorder.Code != http.StatusOK { + t.Errorf( + "expected [%s] to be served when profiling is enabled\n"+ + "expected status: %d\nactual status: %d", + path, + http.StatusOK, + recorder.Code, + ) + } + }) + } +} + +// TestServerHandler_PprofDisabledIsIsolatedFromDefaultServeMux asserts the +// handler is isolated from http.DefaultServeMux, which is where importing +// net/http/pprof installs its handlers. This is the root cause the live-server +// test above observes, asserted directly against the constructed handler. +func TestServerHandler_PprofDisabledIsIsolatedFromDefaultServeMux(t *testing.T) { + handler := newRegistry(context.Background()).serverHandler(false) + + recorder := httptest.NewRecorder() + handler.ServeHTTP( + recorder, + httptest.NewRequest(http.MethodGet, "/debug/pprof/", nil), + ) + + if recorder.Code != http.StatusNotFound { + t.Errorf( + "handler is serving http.DefaultServeMux, exposing profiling "+ + "endpoints while disabled\nexpected status: %d\nactual status: %d", + http.StatusNotFound, + recorder.Code, + ) + } +} From 79fd90f80078a228a91894208431c66c897fee69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Wed, 23 Sep 2026 08:43:13 +0000 Subject: [PATCH 2/4] fix(spv): reject a zero maxProofHeaders at startup 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. --- cmd/maintainer.go | 15 +++++++ cmd/maxproofheaders_flag_test.go | 74 +++++++++++++++++++++++++++++++ pkg/maintainer/spv/config.go | 22 +++++++++ pkg/maintainer/spv/config_test.go | 57 ++++++++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 cmd/maxproofheaders_flag_test.go create mode 100644 pkg/maintainer/spv/config_test.go diff --git a/cmd/maintainer.go b/cmd/maintainer.go index 48ce663a01..1c727cb10f 100644 --- a/cmd/maintainer.go +++ b/cmd/maintainer.go @@ -44,11 +44,26 @@ func init() { ) } +// validateMaintainerConfig checks the maintainer configuration before any +// chain connection is attempted, so a misconfiguration fails fast and loudly +// at startup instead of degrading into silent runtime behavior. +func validateMaintainerConfig(cfg *config.Config) error { + if err := cfg.Maintainer.Spv.Validate(); err != nil { + return fmt.Errorf("invalid SPV maintainer configuration: [%v]", err) + } + + return nil +} + // maintainers initializes maintainer tasks specified by flags passed to the // maintainer command. func maintainers(cmd *cobra.Command, args []string) error { ctx := context.Background() + if err := validateMaintainerConfig(clientConfig); err != nil { + return err + } + btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum) if err != nil { return fmt.Errorf("could not connect to Electrum chain: [%v]", err) diff --git a/cmd/maxproofheaders_flag_test.go b/cmd/maxproofheaders_flag_test.go new file mode 100644 index 0000000000..a549373a42 --- /dev/null +++ b/cmd/maxproofheaders_flag_test.go @@ -0,0 +1,74 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/keep-network/keep-core/config" +) + +// TestMaintainerConfig_RejectsExplicitZeroMaxProofHeaders asserts that an +// explicit `--spv.maxProofHeaders 0` is rejected by the same validation the +// maintainer command runs at startup. +// +// The flag is registered with cobra's UintVar, which accepts 0 as a valid +// unsigned value, so the 144 default only protects the case where the flag is +// omitted entirely. Without validation a zero bound reaches the proof assembly +// loop, where getProofInfo compares the header count against it at loop entry +// and skips every transaction, disabling SPV proving. A test exercising only +// the omitted-flag default would pass regardless and mask this. +// +// The test drives validateMaintainerConfig, the helper maintainers() calls +// before connecting to any chain. It covers the validation logic, not the +// call site: removing the call from maintainers() would not fail this test. +func TestMaintainerConfig_RejectsExplicitZeroMaxProofHeaders(t *testing.T) { + command := &cobra.Command{Use: "maintainer-test"} + cfg := &config.Config{} + initMaintainerFlags(command, cfg) + + if err := command.Flags().Parse( + []string{"--spv.maxProofHeaders", "0"}, + ); err != nil { + t.Fatalf("failed to parse flags: [%v]", err) + } + + // Establishes that zero is genuinely reachable through the CLI rather than + // being rejected by flag parsing itself. + if got := cfg.Maintainer.Spv.MaxProofHeaders; got != 0 { + t.Fatalf("expected the flag to parse zero into config, got [%d]", got) + } + + err := validateMaintainerConfig(cfg) + if err == nil { + t.Fatal( + "expected startup validation to reject --spv.maxProofHeaders 0, got no error", + ) + } + if !strings.Contains(err.Error(), "maxProofHeaders") { + t.Errorf( + "expected the error to name the offending setting, got: [%v]", + err, + ) + } +} + +// TestMaintainerConfig_AcceptsDefaultMaxProofHeaders asserts the validation +// added above does not reject a normally-configured maintainer. +func TestMaintainerConfig_AcceptsDefaultMaxProofHeaders(t *testing.T) { + command := &cobra.Command{Use: "maintainer-test"} + cfg := &config.Config{} + initMaintainerFlags(command, cfg) + + if err := command.Flags().Parse([]string{}); err != nil { + t.Fatalf("failed to parse flags: [%v]", err) + } + + if err := validateMaintainerConfig(cfg); err != nil { + t.Errorf( + "expected the default configuration to be accepted, got error: [%v]", + err, + ) + } +} diff --git a/pkg/maintainer/spv/config.go b/pkg/maintainer/spv/config.go index d9f3dfcf7f..1787bb2633 100644 --- a/pkg/maintainer/spv/config.go +++ b/pkg/maintainer/spv/config.go @@ -1,6 +1,7 @@ package spv import ( + "fmt" "time" ) @@ -81,3 +82,24 @@ type Config struct { // insufficient. MaxProofHeaders uint } + +// Validate checks that the configuration is usable, returning an error +// describing the first problem found. +// +// A zero MaxProofHeaders is rejected rather than normalized to the default. +// getProofInfo compares the running header count against this bound at loop +// entry, so zero causes every transaction to be skipped as +// proofSkipExceededMaxHeaders on the first iteration, disabling SPV proving +// entirely while the logs report each transaction as possibly permanently +// unprovable. Silently substituting the default would hide an operator's +// explicit, if mistaken, instruction; failing at startup surfaces it. +func (c *Config) Validate() error { + if c.MaxProofHeaders == 0 { + return fmt.Errorf( + "maxProofHeaders must be greater than zero; " + + "a zero bound skips every transaction and disables SPV proving", + ) + } + + return nil +} diff --git a/pkg/maintainer/spv/config_test.go b/pkg/maintainer/spv/config_test.go new file mode 100644 index 0000000000..de5fb5942a --- /dev/null +++ b/pkg/maintainer/spv/config_test.go @@ -0,0 +1,57 @@ +package spv + +import ( + "testing" +) + +// TestConfigValidate_MaxProofHeaders guards against a zero proof-header bound +// reaching the proof assembly loop. +// +// getProofInfo evaluates `headerCount >= maxProofHeaders` at loop entry with +// headerCount starting at zero, so a zero bound returns +// proofSkipExceededMaxHeaders on the first iteration for every transaction. +// That silently disables all SPV proving while logging each transaction as +// "may be permanently unprovable", which reads as a chain condition rather +// than a misconfiguration. The flag default only covers omission, so the zero +// value has to be rejected explicitly. +func TestConfigValidate_MaxProofHeaders(t *testing.T) { + tests := map[string]struct { + maxProofHeaders uint + expectError bool + }{ + "zero is rejected": { + maxProofHeaders: 0, + expectError: true, + }, + "default is accepted": { + maxProofHeaders: DefaultMaxProofHeaders, + expectError: false, + }, + "one is accepted": { + maxProofHeaders: 1, + expectError: false, + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + config := Config{MaxProofHeaders: test.maxProofHeaders} + + err := config.Validate() + + if test.expectError && err == nil { + t.Errorf( + "expected validation to reject maxProofHeaders [%d], got no error", + test.maxProofHeaders, + ) + } + if !test.expectError && err != nil { + t.Errorf( + "expected maxProofHeaders [%d] to be accepted, got error: [%v]", + test.maxProofHeaders, + err, + ) + } + }) + } +} From be4371d83615ae1de0c674b02aa4a5d7381c34fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Wed, 23 Sep 2026 08:43:13 +0000 Subject: [PATCH 3/4] docs: correct aggregation-PR branch flow and vsize bound rationale 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. --- docs/release-process.md | 6 ++++-- pkg/tbtcpg/fee.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/release-process.md b/docs/release-process.md index 78b3eb5da5..716c78f0e1 100644 --- a/docs/release-process.md +++ b/docs/release-process.md @@ -16,8 +16,10 @@ changes, the project uses a long-lived aggregation PR instead of landing everything via normal `feature → main` PRs: - **Base:** `main` -- **Head:** a moving `dev` branch that tracks `main` by merging each - sub-PR into `dev` (and `main`) before the sub-PR closes +- **Head:** a moving `dev` branch that accumulates the cycle's work by + merging each sub-PR into `dev` before the sub-PR closes. Sub-PRs are + not merged into `main` — `main` advances only when the aggregation + PR itself merges at the end of the cycle. - **State:** the PR stays open across the whole cycle. Its diff against `main` is the live view of "what is still queued for the next release." diff --git a/pkg/tbtcpg/fee.go b/pkg/tbtcpg/fee.go index 4f7e7af9ef..1021d943fc 100644 --- a/pkg/tbtcpg/fee.go +++ b/pkg/tbtcpg/fee.go @@ -27,7 +27,7 @@ var ErrMaxFeeTooLow = errors.New( // bounds is guaranteed (modulo the explicit checks) to keep the internal // int64 multiplications in range. const ( - maxWalletTxVsize int64 = 10_000_000 // 10M vbytes; ~2x Bitcoin block weight. + maxWalletTxVsize int64 = 10_000_000 // 10M vbytes; ~10x the 1M vbyte maximum Bitcoin block size. maxWalletTxEstimatedFee int64 = 1_000_000_000 // 1e9 satoshis = 10 BTC. ) From 7c3251a8f38af2992ab76fdf122ed6f7d3302e29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Fri, 25 Sep 2026 16:49:38 +0000 Subject: [PATCH 4/4] fix: address validated multi-agent review findings on PR #4339 - 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 --- cmd/maintainer.go | 10 ++- cmd/maintainer_test.go | 4 +- cmd/maxproofheaders_flag_test.go | 48 +++++++++++-- config/config_test.go | 54 ++++++++++++++- docs/release-process.md | 11 +-- pkg/clientinfo/clientinfo.go | 8 +++ pkg/clientinfo/pprof_exposure_test.go | 2 + pkg/clientinfo/registry_test.go | 17 ++++- pkg/maintainer/config_test.go | 98 +++++++++++++++++++++++++-- pkg/maintainer/spv/config.go | 43 ++++++++++-- pkg/maintainer/spv/config_test.go | 81 ++++++++++++++++------ test/config_flags.toml | 4 ++ test/config_mixed_contracts.toml | 4 ++ test/config_no_contracts.toml | 4 ++ test/config_no_password.toml | 4 ++ 15 files changed, 337 insertions(+), 55 deletions(-) diff --git a/cmd/maintainer.go b/cmd/maintainer.go index 12805e22e6..96457a0bcd 100644 --- a/cmd/maintainer.go +++ b/cmd/maintainer.go @@ -46,10 +46,14 @@ func init() { // validateMaintainerConfig checks the maintainer configuration before any // chain connection is attempted, so a misconfiguration fails fast and loudly -// at startup instead of degrading into silent runtime behavior. +// at startup instead of degrading into silent runtime behavior. It delegates +// to maintainer.Config.Validate so the command-level check and the +// config-load check (config.ReadConfig) share one rule: SPV settings are +// validated only when the SPV maintainer will actually run (explicitly +// enabled, or neither maintainer enabled). func validateMaintainerConfig(cfg *config.Config) error { - if err := cfg.Maintainer.Spv.Validate(); err != nil { - return fmt.Errorf("invalid SPV maintainer configuration: [%v]", err) + if err := cfg.Maintainer.Validate(); err != nil { + return fmt.Errorf("invalid maintainer configuration: [%v]", err) } return nil diff --git a/cmd/maintainer_test.go b/cmd/maintainer_test.go index 76863c2289..1edee53850 100644 --- a/cmd/maintainer_test.go +++ b/cmd/maintainer_test.go @@ -167,8 +167,8 @@ func TestInitializeMaintainerMetricsEnabledWhenPortSet(t *testing.T) { defer func() { clientConfig.ClientInfo.Port = originalPort }() // Reserve a genuinely free ephemeral port from the OS and release it - // immediately. clientinfo.Initialize binds this port on - // http.DefaultServeMux via an unowned ListenAndServe goroutine with no + // immediately. clientinfo.Initialize binds this port on a private + // per-registry ServeMux via an unowned ListenAndServe goroutine with no // shutdown handle, so a hardcoded port risks colliding with another // process or a parallel test run. listener, err := net.Listen("tcp", "127.0.0.1:0") diff --git a/cmd/maxproofheaders_flag_test.go b/cmd/maxproofheaders_flag_test.go index a549373a42..bac7af4f85 100644 --- a/cmd/maxproofheaders_flag_test.go +++ b/cmd/maxproofheaders_flag_test.go @@ -15,14 +15,16 @@ import ( // // The flag is registered with cobra's UintVar, which accepts 0 as a valid // unsigned value, so the 144 default only protects the case where the flag is -// omitted entirely. Without validation a zero bound reaches the proof assembly -// loop, where getProofInfo compares the header count against it at loop entry -// and skips every transaction, disabling SPV proving. A test exercising only -// the omitted-flag default would pass regardless and mask this. +// omitted entirely. Without validation a zero bound reaches the proof +// assembly loop, where getProofInfo skips every transaction, disabling SPV +// proving. A test exercising only the omitted-flag default would pass +// regardless and mask this. // -// The test drives validateMaintainerConfig, the helper maintainers() calls -// before connecting to any chain. It covers the validation logic, not the -// call site: removing the call from maintainers() would not fail this test. +// The test drives validateMaintainerConfig, which delegates to +// maintainer.Config.Validate - the same rule the config-load pass applies. +// A fresh flag-initialized config has neither maintainer enabled, so the +// launch-all branch validates the SPV settings and reports the offending +// field. func TestMaintainerConfig_RejectsExplicitZeroMaxProofHeaders(t *testing.T) { command := &cobra.Command{Use: "maintainer-test"} cfg := &config.Config{} @@ -72,3 +74,35 @@ func TestMaintainerConfig_AcceptsDefaultMaxProofHeaders(t *testing.T) { ) } } + +// TestMaintainerConfig_AcceptsCustomMaxProofHeaders asserts that explicitly +// provided positive values flow through the flag -> config -> validation path, +// not just the omitted-flag default. Without these cases a regression that +// breaks non-default values (e.g. a type conversion or off-by-one in +// validation) would only surface when an operator actually deviates from the +// 144 default. +func TestMaintainerConfig_AcceptsCustomMaxProofHeaders(t *testing.T) { + values := []string{"1", "288"} + + for _, value := range values { + t.Run(value, func(t *testing.T) { + command := &cobra.Command{Use: "maintainer-test"} + cfg := &config.Config{} + initMaintainerFlags(command, cfg) + + if err := command.Flags().Parse( + []string{"--spv.maxProofHeaders", value}, + ); err != nil { + t.Fatalf("failed to parse flags: [%v]", err) + } + + if err := validateMaintainerConfig(cfg); err != nil { + t.Errorf( + "expected --spv.maxProofHeaders %s to be accepted, got error: [%v]", + value, + err, + ) + } + }) + } +} diff --git a/config/config_test.go b/config/config_test.go index 36b2de5864..cd270468da 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -33,7 +33,11 @@ func TestValidateConfig_TransactionMonitor(t *testing.T) { }}, Maintainer: maintainer.Config{ Spv: spv.Config{ - MaxProofHeaders: spv.DefaultMaxProofHeaders, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, }, }, } @@ -81,7 +85,14 @@ func TestValidateConfig_Maintainer(t *testing.T) { config: &Config{ Maintainer: maintainer.Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: false, MaxProofHeaders: spv.DefaultMaxProofHeaders}, + Spv: spv.Config{ + Enabled: false, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, }, }, expectErr: false, @@ -90,11 +101,48 @@ func TestValidateConfig_Maintainer(t *testing.T) { config: &Config{ Maintainer: maintainer.Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: true, MaxProofHeaders: spv.DefaultMaxProofHeaders}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, }, }, expectErr: false, }, + "enabled SPV with zero historyDepth fails": { + config: &Config{ + Maintainer: maintainer.Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + }, + expectErr: true, + }, + "enabled SPV with zero transactionLimit fails": { + config: &Config{ + Maintainer: maintainer.Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + }, + expectErr: true, + }, } for testName, test := range tests { diff --git a/docs/release-process.md b/docs/release-process.md index 716c78f0e1..457aeda15c 100644 --- a/docs/release-process.md +++ b/docs/release-process.md @@ -25,11 +25,12 @@ landing everything via normal `feature → main` PRs: next release." Sub-PRs are still reviewed and CI'd independently — the aggregation -PR is just the place to watch the cumulative state. When the cycle is -ready to ship, fast-forward `dev` to the latest `main`, resolve any -final conflicts, and merge the aggregation PR into `main` as a single -merge commit. The version tag is then cut from `main` per "Creating -a Release" below. +PR is just the place to watch the cumulative state. Sub-PRs target `dev` +as their base branch; maintainers merge them into `dev` after review. +When the cycle is ready to ship, fast-forward `dev` to the latest `main`, +resolve any final conflicts, and merge the aggregation PR into `main` +as a single merge commit. The version tag is then cut from `main` per +"Creating a Release" below. ## Creating a Release diff --git a/pkg/clientinfo/clientinfo.go b/pkg/clientinfo/clientinfo.go index 8276c2c40a..bf29882b62 100644 --- a/pkg/clientinfo/clientinfo.go +++ b/pkg/clientinfo/clientinfo.go @@ -79,6 +79,14 @@ func Initialize( cfg Config, ) (*Registry, bool) { if cfg.Port == 0 { + if cfg.EnablePprof { + // Enabling pprof without a port would be silently ignored because + // no server is started; surface the misconfiguration instead. + logger.Warnf( + "EnablePprof is set but Port is 0; no server is started and " + + "profiling endpoints will not be exposed", + ) + } return nil, false } diff --git a/pkg/clientinfo/pprof_exposure_test.go b/pkg/clientinfo/pprof_exposure_test.go index 5f7134bc35..a8053646db 100644 --- a/pkg/clientinfo/pprof_exposure_test.go +++ b/pkg/clientinfo/pprof_exposure_test.go @@ -87,7 +87,9 @@ func TestNewServeMux_PprofEnabled(t *testing.T) { for _, path := range []string{ "/debug/pprof/", "/debug/pprof/cmdline", + "/debug/pprof/profile?seconds=1", "/debug/pprof/symbol", + "/debug/pprof/trace?seconds=1", } { t.Run(path, func(t *testing.T) { recorder := httptest.NewRecorder() diff --git a/pkg/clientinfo/registry_test.go b/pkg/clientinfo/registry_test.go index b8575b1a0d..f4d6be33c9 100644 --- a/pkg/clientinfo/registry_test.go +++ b/pkg/clientinfo/registry_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "net/http/httptest" "os" @@ -15,9 +16,23 @@ import ( var registry *Registry -const port = 9799 +// port is an OS-reserved ephemeral port the package's live server listens on. +// Reserving it in TestMain avoids the flakiness of a hardcoded port: another +// process or a parallel test run could bind 9799 while this suite runs. The +// brief gap between closing the listener and the server's ListenAndServe is +// acceptable for a test server (same pattern as cmd/maintainer_test.go). +var port int func TestMain(m *testing.M) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + panic(fmt.Sprintf("could not reserve a test port: [%v]", err)) + } + port = listener.Addr().(*net.TCPAddr).Port + if err := listener.Close(); err != nil { + panic(fmt.Sprintf("could not release the reserved test port: [%v]", err)) + } + registry = newRegistry(context.Background()) registry.EnableServer(port) diff --git a/pkg/maintainer/config_test.go b/pkg/maintainer/config_test.go index 04e23819df..d56b795014 100644 --- a/pkg/maintainer/config_test.go +++ b/pkg/maintainer/config_test.go @@ -8,6 +8,14 @@ import ( ) func TestConfig_Validate(t *testing.T) { + validSpv := spv.Config{ + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + } + tests := map[string]struct { config Config expectErr bool @@ -15,38 +23,118 @@ func TestConfig_Validate(t *testing.T) { "launch-all with zero SPV maxProofHeaders fails": { config: Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: false, MaxProofHeaders: 0}, + Spv: spv.Config{ + Enabled: false, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: 0, + }, }, expectErr: true, }, "enabled SPV with zero maxProofHeaders fails": { config: Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: true, MaxProofHeaders: 0}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: 0, + }, }, expectErr: true, }, "difficulty-only with zero SPV maxProofHeaders passes": { config: Config{ BitcoinDifficulty: btcdiff.Config{Enabled: true}, - Spv: spv.Config{Enabled: false, MaxProofHeaders: 0}, + Spv: spv.Config{ + Enabled: false, + MaxProofHeaders: 0, + }, }, expectErr: false, }, "launch-all with positive SPV maxProofHeaders passes": { config: Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: false, MaxProofHeaders: spv.DefaultMaxProofHeaders}, + Spv: validSpv, }, expectErr: false, }, "enabled SPV with positive maxProofHeaders passes": { config: Config{ BitcoinDifficulty: btcdiff.Config{Enabled: false}, - Spv: spv.Config{Enabled: true, MaxProofHeaders: spv.DefaultMaxProofHeaders}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, }, expectErr: false, }, + "enabled SPV with zero historyDepth fails": { + config: Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: 0, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + expectErr: true, + }, + "enabled SPV with non-positive transactionLimit fails": { + config: Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: 0, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + expectErr: true, + }, + "enabled SPV with zero restartBackoffTime fails": { + config: Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: 0, + IdleBackoffTime: spv.DefaultIdleBackOffTime, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + expectErr: true, + }, + "enabled SPV with zero idleBackoffTime fails": { + config: Config{ + BitcoinDifficulty: btcdiff.Config{Enabled: false}, + Spv: spv.Config{ + Enabled: true, + HistoryDepth: spv.DefaultHistoryDepth, + TransactionLimit: spv.DefaultTransactionLimit, + RestartBackoffTime: spv.DefaultRestartBackoffTime, + IdleBackoffTime: 0, + MaxProofHeaders: spv.DefaultMaxProofHeaders, + }, + }, + expectErr: true, + }, } for testName, test := range tests { diff --git a/pkg/maintainer/spv/config.go b/pkg/maintainer/spv/config.go index 0164a87437..b966d9b317 100644 --- a/pkg/maintainer/spv/config.go +++ b/pkg/maintainer/spv/config.go @@ -86,13 +86,13 @@ type Config struct { // Validate checks that the configuration is usable, returning an error // describing the first problem found. // -// A zero MaxProofHeaders is rejected rather than normalized to the default. -// getProofInfo compares the running header count against this bound at loop -// entry, so zero causes every transaction to be skipped as -// proofSkipExceededMaxHeaders on the first iteration, disabling SPV proving -// entirely while the logs report each transaction as possibly permanently -// unprovable. Silently substituting the default would hide an operator's -// explicit, if mistaken, instruction; failing at startup surfaces it. +// A zero or negative value is rejected rather than normalized to the +// default: the default only protects against omission, and each of these +// values silently degrades the maintainer at runtime when set to zero - +// for example a zero MaxProofHeaders makes getProofInfo skip every +// transaction on the first iteration. Silently substituting the default +// would hide an operator's explicit, if mistaken, instruction; failing at +// startup surfaces it. func (c Config) Validate() error { if c.MaxProofHeaders == 0 { return fmt.Errorf( @@ -101,5 +101,34 @@ func (c Config) Validate() error { ) } + if c.HistoryDepth == 0 { + return fmt.Errorf( + "spv.historyDepth must be greater than 0; " + + "a zero depth makes the event search start at the " + + "current tip, so no past transactions are ever found", + ) + } + + if c.TransactionLimit <= 0 { + return fmt.Errorf( + "spv.transactionLimit must be greater than 0; " + + "the maintainer would find no candidate transactions", + ) + } + + if c.RestartBackoffTime <= 0 { + return fmt.Errorf( + "spv.restartBackoffTime must be greater than 0; " + + "a non-positive backoff tight-loops the restart of the maintainer", + ) + } + + if c.IdleBackoffTime <= 0 { + return fmt.Errorf( + "spv.idleBackoffTime must be greater than 0; " + + "a non-positive backoff tight-loops the proof task rounds", + ) + } + return nil } diff --git a/pkg/maintainer/spv/config_test.go b/pkg/maintainer/spv/config_test.go index 1adc83fc33..29dacd6928 100644 --- a/pkg/maintainer/spv/config_test.go +++ b/pkg/maintainer/spv/config_test.go @@ -5,39 +5,60 @@ import ( "testing" ) -// TestConfig_Validate guards against a zero proof-header bound reaching the -// proof assembly loop. -// -// getProofInfo evaluates `headerCount >= maxProofHeaders` at loop entry with -// headerCount starting at zero, so a zero bound returns -// proofSkipExceededMaxHeaders on the first iteration for every transaction. -// That silently disables all SPV proving while logging each transaction as -// "may be permanently unprovable", which reads as a chain condition rather -// than a misconfiguration. The flag default only covers omission, so the -// zero value has to be rejected explicitly. +// TestConfig_Validate guards against silently-degrading zero settings +// reaching the maintainer's runtime. Each field is checked for the +// misconfiguration that looks like a valid value but renders the maintainer +// inert: a zero MaxProofHeaders makes getProofInfo skip every transaction on +// the first loop iteration; a zero HistoryDepth anchors the event search at +// the current chain tip; a non-positive TransactionLimit returns no +// transactions to prove; zero backoff times remove the pauses between +// control-loop iterations. The flag/file defaults only cover omission, so an +// operator's explicit zero has to be rejected at startup. func TestConfig_Validate(t *testing.T) { + validConfig := Config{ + HistoryDepth: DefaultHistoryDepth, + TransactionLimit: DefaultTransactionLimit, + RestartBackoffTime: DefaultRestartBackoffTime, + IdleBackoffTime: DefaultIdleBackOffTime, + MaxProofHeaders: DefaultMaxProofHeaders, + } + tests := map[string]struct { config Config expectErr bool }{ + "valid config is accepted": { + config: validConfig, + expectErr: false, + }, "zero maxProofHeaders is rejected": { - config: Config{ - MaxProofHeaders: 0, - }, + config: withZeroField(validConfig, func(c *Config) { c.MaxProofHeaders = 0 }), expectErr: true, }, - "default positive maxProofHeaders is accepted": { - config: Config{ - MaxProofHeaders: DefaultMaxProofHeaders, - }, - expectErr: false, - }, "custom positive maxProofHeaders is accepted": { - config: Config{ - MaxProofHeaders: 288, - }, + config: withCustomMaxProofHeaders(validConfig, 288), expectErr: false, }, + "zero historyDepth is rejected": { + config: withZeroField(validConfig, func(c *Config) { c.HistoryDepth = 0 }), + expectErr: true, + }, + "zero transactionLimit is rejected": { + config: withZeroField(validConfig, func(c *Config) { c.TransactionLimit = 0 }), + expectErr: true, + }, + "negative transactionLimit is rejected": { + config: withZeroField(validConfig, func(c *Config) { c.TransactionLimit = -1 }), + expectErr: true, + }, + "zero restartBackoffTime is rejected": { + config: withZeroField(validConfig, func(c *Config) { c.RestartBackoffTime = 0 }), + expectErr: true, + }, + "zero idleBackoffTime is rejected": { + config: withZeroField(validConfig, func(c *Config) { c.IdleBackoffTime = 0 }), + expectErr: true, + }, } for testName, test := range tests { @@ -57,6 +78,22 @@ func TestConfig_Validate(t *testing.T) { } } +// withZeroField returns a copy of base with one field mutated to its invalid +// value; used to build single-field-invalid configs for the table test. +func withZeroField(base Config, mutate func(*Config)) Config { + c := base + mutate(&c) + return c +} + +func withCustomMaxProofHeaders(base Config, value uint) Config { + c := base + c.MaxProofHeaders = value + return c +} + +// TestInitialize_InvalidConfig guards the public entry point: Initialize must +// reject a misconfigured config before touching any chain interface. func TestInitialize_InvalidConfig(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() diff --git a/test/config_flags.toml b/test/config_flags.toml index fdc0f31ef6..9e4efc474e 100644 --- a/test/config_flags.toml +++ b/test/config_flags.toml @@ -30,3 +30,7 @@ MaxTrackingAge = "12h" [maintainer.spv] MaxProofHeaders = 144 +HistoryDepth = 25000 +TransactionLimit = 80 +RestartBackoffTime = "2h" +IdleBackoffTime = "15m" diff --git a/test/config_mixed_contracts.toml b/test/config_mixed_contracts.toml index 1985c36844..8628a78bbe 100644 --- a/test/config_mixed_contracts.toml +++ b/test/config_mixed_contracts.toml @@ -18,3 +18,7 @@ WalletRegistryAddress = "0x143ba24e66fce8bca22f7d739f9a932c519b1c76" [Maintainer.Spv] MaxProofHeaders = 144 +HistoryDepth = 25000 +TransactionLimit = 80 +RestartBackoffTime = "2h" +IdleBackoffTime = "15m" diff --git a/test/config_no_contracts.toml b/test/config_no_contracts.toml index 39c630d43e..fd2d989383 100644 --- a/test/config_no_contracts.toml +++ b/test/config_no_contracts.toml @@ -15,3 +15,7 @@ Dir = "/my/secure/location" [Maintainer.Spv] MaxProofHeaders = 144 +HistoryDepth = 25000 +TransactionLimit = 80 +RestartBackoffTime = "2h" +IdleBackoffTime = "15m" diff --git a/test/config_no_password.toml b/test/config_no_password.toml index e65d62a4b7..0d4f7ffe65 100644 --- a/test/config_no_password.toml +++ b/test/config_no_password.toml @@ -15,3 +15,7 @@ Dir = "/my/secure/location" [Maintainer.Spv] MaxProofHeaders = 144 +HistoryDepth = 25000 +TransactionLimit = 80 +RestartBackoffTime = "2h" +IdleBackoffTime = "15m"