chore: additional low-risk code-quality follow-ups#4187
Open
piotr-roslaniec wants to merge 17 commits into
Open
chore: additional low-risk code-quality follow-ups#4187piotr-roslaniec wants to merge 17 commits into
piotr-roslaniec wants to merge 17 commits into
Conversation
- 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.
|
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 |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.