feat(maintainer): record SPV proof-submission metrics - #4190
Open
piotr-roslaniec wants to merge 29 commits into
Open
feat(maintainer): record SPV proof-submission metrics#4190piotr-roslaniec wants to merge 29 commits into
piotr-roslaniec wants to merge 29 commits into
Conversation
…ments Extract registerAllMetrics into per-type helpers (counters, wallet actions, histograms, gauges) to isolate responsibilities, document the two-phase map-populate-then-observe concurrency invariant once per helper, remove field-group comments that restated field names, and correct the stale system-metrics ticker comment (60s).
…mments The coordinationFailed variable was only ever set true in branches that return immediately, so the success-metrics guard was always taken; remove the variable and simplify the guard. Also drop track-narration comments in coordination_window_metrics.go that restated the following line.
- add named DepositKey type replacing the anonymous struct used for DepositSweepProposal.DepositsKeys across tbtc, tbtcpg and ethereum - extract movingFundsSafetyMarginChain interface shared by ValidateMovingFundsSafetyMargin and isWalletPendingMovingFundsTarget - switch ParseWalletActionType on WalletActionType iota constants - collapse three identical frequency-window guards into a single guard
- EstimateDepositsSweepFee wraps the real error (was formatting the zero-valued sweepMaxSize) when GetDepositSweepMaxSize fails - sync_machine wraps the WaitForBlockHeight error with %w so callers can inspect the root cause - rename fnLogger to taskLogger to match the established logger naming - fix two Chain interface doc comments to start with the method name - correct the tools.go comment to describe indirect-dependency pinning
Wrap the finalSigningGroup error with %w so callers can inspect the underlying cause instead of only the outer message.
- add and register clientinfo deposit-sweep proof-submission metric constants, mirroring the redemption ones, and replace the raw metric name strings in the SPV maintainer with them - remove the getGlobalMetricsRecorder passthrough and call getMetricsRecorder directly - trim variable comments that restated the variable names in parseDepositSweepTransactionInputs, keeping the vault constraint note
Rename the minority marshalling.go files to the majority marshaling spelling for consistency across packages (git mv, no code changes).
The movingFundsSafetyMarginChain interface was inserted between the function's doc comment and its declaration, detaching the doc. Move the interface above the doc comment so it attaches again.
All five pinned modules are direct requires in go.mod, not indirect; describe them by what they actually are (build-time-only).
- declare loop index with var i int instead of var i = 0 in chain.Addresses.String
- use the any alias instead of interface{} for the requestWithRetry type parameter
Add non-integration unit tests for GetBlockNumberByTimestamp and closerBlock, which previously had coverage only under a skippable integration test. The tests use a lightweight in-memory client to exercise the backward/forward search loops and the closer-block tie-breaking.
The three-method metrics-recorder interface was declared inline in many places across the package. Introduce a named fullMetricsRecorder interface (MetricsRecorder plus SetGauge) and use it at those sites. The transport keeps its narrower two-method MetricsRecorder contract.
EstimateMovingFundsFee and EstimateMovedFundsSweepFee shared an identical virtual-size, fee-estimate, and cap-check block. Extract it into estimateCappedFee, parameterized by the size estimator, the cap, and the fee-too-high error to return.
…cs singleton - extract unprovenSearchStartBlock and collectUnprovenWalletTransactions, shared by the four getUnproven*Transactions functions - remove the package-level global metrics recorder and its setter/getter, which were never wired in production and always resolved to nil; the proof submission functions retain their metricsRecorder parameter as the DI seam
JoinDKGIfEligible and GenerateRelayEntry logged a SetFilter failure and then launched protocol goroutines on an unfiltered broadcast channel, accepting messages from operators outside the selected group. Abort on the failure instead, matching the fail-closed behavior already used by the tbtc node.
The receivedQualifiedSharesT (t_ji) map on the member struct was written and deleted on the production path but never read there; only tests consumed it. Remove it from the struct and keep only receivedQualifiedSharesS (s_ji), which is the actual reconstruction state. The share-count assertions now rely on the S map (populated identically), and the accusation tests obtain the t_ji shares from a value returned by the group-initialization helper.
The follower-routine coordination test slept a fixed second hoping the receiver had registered its broadcast channel handler before the sender started publishing. Wrap the follower channel so the sender waits for the actual Recv registration, removing the timing assumption.
cmd.start wires the performance metrics recorder into the provider via a structural type assertion against an anonymous interface. Naming that parameter fullMetricsRecorder made the assertion no longer match the provider (a defined type differs from an identical anonymous interface), so metrics were silently no longer wired. Restore the anonymous parameter and add a test that pins this wiring contract.
Directly exercise collectUnprovenWalletTransactions, pinning the stop-at-first-match branch in both directions and the chain and predicate error paths that were previously only reached indirectly.
Both JoinDKGIfEligible and GenerateRelayEntry installed the membership filter and aborted on failure with identical logic. Extract it into setBroadcastChannelFilter so the fail-closed contract lives in one place and can be exercised directly.
Assert setBroadcastChannelFilter surfaces the SetFilter error so callers abort instead of proceeding on an unfiltered channel that would accept messages from operators outside the group.
unprovenSearchStartBlock returned currentBlock - historyDepth without guarding the subtraction. On short chains where historyDepth exceeds the current tip the unsigned subtraction wraps to a near-maximum block number, silently changing the search range. Clamp to the genesis block instead. This preserves behavior on mainnet, where the tip always dwarfs the configured history depth.
The existing timestamp-search table uses a 12s block spacing, below the 13s averageBlockTime the algorithm assumes, so the initial backward jump always lands at or after the target and the forward-walk branch never runs. Add a case with 15s spacing so the backward jump overshoots below the target and the forward loop is exercised.
Replace the hand-rolled blockOutOfRangeError struct with a plain errors.New sentinel; it was only ever used as an opaque marker error.
The old comment block described the pre-refactor behavior (storing t_ji on the member) that no longer holds, named the wrong function, and carried a typo. Keep only the accurate block.
The SPV proof submitters accept an optional metrics recorder, but the maintainer command never built one, so proof-submission counters were never recorded in production (the recorder was always passed as nil). Wire a clientinfo registry and PerformanceMetrics into the maintainer boot path and thread the recorder through maintainer.Initialize into the SPV maintainer, replacing the hardcoded nil at the deposit sweep and redemption submitters. Add ClientInfo to the maintainer config categories so the metrics endpoint port is parsed. Moving funds and moved funds sweep submitters accept the recorder to satisfy the submitter type but are not yet instrumented, as those proof submissions have no metrics counters defined.
|
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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ilure Close review gaps on the SPV proof-submission metrics change: - assert the redemption success-path counters; the success test previously passed a nil recorder and asserted nothing, so a removed/misplaced success increment would go undetected. - cover the assemble-error failed-counter branch for both the deposit sweep and redemption provers (a realistic production failure mode); only the early zero-confirmations reject was asserted before. The on-chain-submit branch shares the same guard idiom but the local chain double cannot be forced to fail that call. - add a cmd test for initializeMaintainerMetrics: a 0 client-info port yields a nil recorder, guarding the sole production metrics on/off gate against an inverted condition.
…ntract - list the six SPV proof-submission counters in performance-metrics.adoc; this change is what makes them observable in production. - reframe the MetricsRecorder doc as an API contract (callers must treat nil as "metrics off" and guard every call) instead of the rot-prone "all call sites guard against it" status claim.
piotr-roslaniec
dismissed
lionakhnazarov’s stale review
August 7, 2026 19:30
The merge-base changed after approval.
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
2 participants
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.
What
Wires a
clientinfometrics registry +PerformanceMetricsinto themaintainercommand and threads a real metrics recorder throughmaintainer.Initializeinto the SPV maintainer, so deposit-sweep and redemption proof-submission counters are actually recorded in production.Why
The SPV proof submitters accept an optional metrics recorder, but the maintainer boot path never built one, so the recorder was always
niland the counters were dead in production.maintaineris the only command that runs the SPV maintainer, so these metrics were never observable.Changes
cmd/maintainer.go: build the client-info registry +PerformanceMetrics(newinitializeMaintainerMetricshelper), register baseline client-info + Ethereum sources, and pass the recorder tomaintainer.Initialize. The recorder is derived from the client-info registry: whenclientInfo.port=0,clientinfo.Initializereports not-configured and the recorder stays a genuinenilinterface (no typed-nil trap), disabling metrics recording.config/category.go: addClientInfotoMaintainerCategoriesso the client-info port is parsed for the maintainer command. BecauseclientInfo.portdefaults to9601(the same shared flag the nodestartcommand uses), the maintainer now serves the client-info metrics/diagnostics endpoint on:9601by default, consistent withstart. Operators who do not want the endpoint can setclientInfo.port=0. See the operator impact note below.pkg/maintainer/maintainer.go: threadspv.MetricsRecorderthroughInitializeintospv.Initialize.pkg/maintainer/spv: add an exportedMetricsRecorderinterface; store it onspvMaintainer; extend thetransactionProofSubmittertype; forward the recorder (instead of hardcodednil) at the deposit-sweep and redemption submitters. Moving-funds and moved-funds-sweep submitters accept the recorder to satisfy the type but are not yet instrumented (no counters defined for them).cmd/maintainercli.go: the two manual one-off proof-submit subcommands passnil(no registry in that path).docs/performance-metrics.adoc: document the six SPV proof-submission counters that this change makes observable in production.Release note / operator impact
The
maintainercommand now exposes the client-info metrics/diagnostics HTTP endpoint on port9601(all interfaces) by default, matching the behavior of the nodestartcommand. The endpoint serves system/process metrics, client version, Ethereum connectivity/latest-block info, and the SPV proof-submission counters. It does not expose the Ethereum operator key or peer/operator identity. Operators who do not want the maintainer to open this endpoint should setclientInfo.port=0in theClientInfoconfig section (or via--clientInfo.port=0).Tests
TestSubmitDepositSweepProofandTestSubmitRedemptionProofthread a real recorder and assert the attempt/success/failure counters fire on the success path.TestSubmitDepositSweepProofRecordsFailureMetrics/TestSubmitRedemptionProofRecordsFailureMetricscover the attempt+failure counters on the early zero-confirmations reject path.TestSubmitDepositSweepProofRecordsAssembleFailureMetrics/TestSubmitRedemptionProofRecordsAssembleFailureMetricscover the failed-counter increment on the SPV-proof-assembly error branch.TestInitializeMaintainerMetricsDisabledWhenPortUnsetguards the metrics on/off gate: a0client-info port yields a nil recorder.Base / stacking
Based on
chore/code-quality-followups(#4187), notmain: the param-based recorder seam this PR fills only exists after that PR removed the global metrics singleton. Once #4187 merges, this branch should be rebased--onto mainand becomes a standalone change.