chore: additional low-risk code-quality follow-ups - #4187
Open
piotr-roslaniec wants to merge 29 commits into
Open
chore: additional low-risk code-quality follow-ups#4187piotr-roslaniec wants to merge 29 commits into
piotr-roslaniec wants to merge 29 commits into
Conversation
|
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 |
…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).
DepositKey replaced the anonymous struct previously inlined as the element type of DepositSweepProposal.DepositsKeys. Go does not allow assigning an anonymous-struct-typed slice literal to a named-struct-typed slice field, so any code outside this module that constructs a DepositSweepProposal from the old anonymous struct shape fails to compile against the new type, even though every in-repo consumer was already updated. Document this on the DepositKey type so downstream consumers of this package are not surprised by a silent compile break on upgrade.
Neither of this PR's stated behavior fixes had direct test coverage: every EstimateDepositsSweepFee table case used depositsCount > 0, so the branch calling GetDepositSweepMaxSize (and its corrected error-wrapping) was never reached, and the panicking LocalChain double would have crashed the suite had it ever been exercised. - add LocalChain.SetDepositSweepMaxSizeError to let tests configure that failure without a real chain implementation - add a depositsCount: 0 case asserting the wrapped error keeps the real underlying cause instead of the old zero-value formatting - add TestDepositSweepProofSubmissionCountersRegistered, mirroring TestJoinFailureAndOnChainCountersRegistered, asserting the three new deposit-sweep proof-submission counters are pre-registered and exported
piotr-roslaniec
force-pushed
the
chore/code-quality-cleanup
branch
from
August 7, 2026 15:59
29bbc17 to
21232fe
Compare
- 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
- 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.
piotr-roslaniec
force-pushed
the
chore/code-quality-followups
branch
from
August 7, 2026 19:30
5465e7d to
8e628e3
Compare
piotr-roslaniec
force-pushed
the
chore/code-quality-cleanup
branch
from
August 18, 2026 09:03
ee28988 to
b6945c3
Compare
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.
Summary
A second, focused batch of low-risk code-quality improvements across the node,
continuing the cleanup in #4185. The changes reduce duplication, remove dead
code, add missing test coverage, and fix two latent correctness issues found
while cleaning up. No feature or protocol behavior changes beyond the two fixes
noted below.
Fixes (behavior)
JoinDKGIfEligibleandGenerateRelayEntrypreviously logged and thencontinued when
SetFilterfailed, running the protocol on an unfilteredbroadcast channel that would accept messages from operators outside the
group. Both now fail closed and abort before launching the protocol
goroutines.
cmd.startwires the performance metrics recorder into the libp2p providervia a structural assertion against an anonymous interface. Naming that
interface would silently break the assertion at runtime (metrics stop being
recorded) with no build or test failure. A guard test now pins the contract.
Refactors (dedup / clarity)
singleton — the four
getUnproven*Transactionsfunctions shared identicalsearch scaffolding, now extracted into
unprovenSearchStartBlockandcollectUnprovenWalletTransactions. Also removes an unwired global metricssingleton.
moved-funds-sweep fee estimation shared the same size/fee/cap logic, now in
estimateCappedFee.a repeated inline interface literal with a named type at the sites that can
safely use it (see the fix above for the one that must stay anonymous).
set-filter-and-abort logic (see the beacon fix) lived in two places; now in
setBroadcastChannelFilter.field that was only ever read by tests; coverage preserved by returning the
value through the existing test helper.
Tests
test(ethereum): cover timestamp-based block search — new unit tests for
closerBlock/GetBlockNumberByTimestamp, previously only exercised viaintegration.
test(spv): cover shared unproven-transaction search helper — pins the
stop-at-first-match branch (both directions) and the error paths of the
extracted helper.
test(beacon): cover broadcast-channel filter abort path — asserts the
fail-closed contract from the beacon fix.
test(tbtc): synchronize follower routine instead of sleeping — replaces a
time.Sleep(1s)in a coordination test with deterministic synchronization.style: use idiomatic zero-value and any declarations — minor, mechanical.
Verification
go build ./...andgo vet ./...clean.pkg/beacon/...(incl. theDKG integration test),
pkg/maintainer/spv,pkg/tbtc,pkg/tbtcpg,pkg/net/libp2p,pkg/chain/ethereum,pkg/bitcoin/electrum.