diff --git a/cmd/maintainer.go b/cmd/maintainer.go index 921f02afd3..96457a0bcd 100644 --- a/cmd/maintainer.go +++ b/cmd/maintainer.go @@ -44,10 +44,30 @@ 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. 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.Validate(); err != nil { + return fmt.Errorf("invalid 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/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 new file mode 100644 index 0000000000..bac7af4f85 --- /dev/null +++ b/cmd/maxproofheaders_flag_test.go @@ -0,0 +1,108 @@ +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 skips every transaction, disabling SPV +// proving. A test exercising only the omitted-flag default would pass +// regardless and mask this. +// +// 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{} + 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, + ) + } +} + +// 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 78b3eb5da5..457aeda15c 100644 --- a/docs/release-process.md +++ b/docs/release-process.md @@ -16,18 +16,21 @@ 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." 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 c16be59126..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 } @@ -100,6 +108,15 @@ func registerPprofHandlers(mux *http.ServeMux) { mux.HandleFunc("/debug/pprof/trace", pprof.Trace) } +// newServeMux 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) newServeMux(enablePprof bool) *http.ServeMux { mux := http.NewServeMux() @@ -131,6 +148,8 @@ func (r *Registry) newServer(port int, enablePprof bool) *http.Server { } } +// 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 := r.newServer(port, enablePprof) diff --git a/pkg/clientinfo/pprof_exposure_test.go b/pkg/clientinfo/pprof_exposure_test.go new file mode 100644 index 0000000000..a8053646db --- /dev/null +++ b/pkg/clientinfo/pprof_exposure_test.go @@ -0,0 +1,135 @@ +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, + ) + } + }) + } +} + +// TestNewServeMux_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 TestNewServeMux_PprofEnabled(t *testing.T) { + handler := newRegistry(context.Background()).newServeMux(true) + + 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() + 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, + ) + } + }) + } +} + +// TestNewServeMux_PprofDisabledIsIsolatedFromDefaultServeMux asserts the +// mux 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 mux. +func TestNewServeMux_PprofDisabledIsIsolatedFromDefaultServeMux(t *testing.T) { + handler := newRegistry(context.Background()).newServeMux(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, + ) + } +} 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 0623ee0dbd..b966d9b317 100644 --- a/pkg/maintainer/spv/config.go +++ b/pkg/maintainer/spv/config.go @@ -83,10 +83,51 @@ type Config struct { MaxProofHeaders uint } -// Validate checks whether the SPV configuration is valid. +// Validate checks that the configuration is usable, returning an error +// describing the first problem found. +// +// 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("spv.maxProofHeaders must be greater than 0") + return fmt.Errorf( + "spv.maxProofHeaders must be greater than 0; " + + "a zero bound skips every transaction and disables SPV proving", + ) + } + + 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 df53dbfdf5..29dacd6928 100644 --- a/pkg/maintainer/spv/config_test.go +++ b/pkg/maintainer/spv/config_test.go @@ -5,29 +5,60 @@ import ( "testing" ) +// 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 { @@ -47,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/pkg/tbtcpg/fee.go b/pkg/tbtcpg/fee.go index ba980b76c7..9a3da0b96c 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. ) 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"