chore: low-risk code-quality cleanup across tBTC/beacon node#4185
Open
piotr-roslaniec wants to merge 9 commits into
Open
chore: low-risk code-quality cleanup across tBTC/beacon node#4185piotr-roslaniec wants to merge 9 commits into
piotr-roslaniec wants to merge 9 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).
📝 WalkthroughWalkthroughChangesProtobuf serialization
Performance and proof metrics
tBTC coordination updates
Error and documentation cleanup
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Membership
participant ThresholdSigner
participant Protobuf
Caller->>Membership: Marshal membership
Membership->>ThresholdSigner: Marshal signer
ThresholdSigner->>Protobuf: Encode signer fields
Protobuf-->>Membership: Return serialized bytes
Caller->>Membership: Unmarshal membership
Membership->>Protobuf: Decode membership bytes
Membership->>ThresholdSigner: Unmarshal embedded signer
ThresholdSigner-->>Membership: Restore signer state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/beacon/dkg/marshaling.go`:
- Line 76: Validate protobuf member indices against group.MaxMemberIndex before
every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go
lines 76-76, reject pbThresholdSigner.MemberIndex before assigning
ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map
key before converting it. Preserve the existing error-handling flow for invalid
input.
In `@pkg/beacon/dkg/result/marshaling_test.go`:
- Around line 34-62: Assert the result of pbutils.RoundTrip in
TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it
fails; apply the same change to the claim-signature round-trip test in
pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing
fuzz setup and message construction.
In `@pkg/tbtcpg/deposit_sweep.go`:
- Around line 605-607: Update the error wrapping in the deposit sweep flow
around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the
underlying error for errors.Is and errors.As while keeping the existing context
message and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82ec20eb-1390-462e-9ffb-68d4184fb117
📒 Files selected for processing (28)
pkg/beacon/dkg/marshaling.gopkg/beacon/dkg/marshaling_test.gopkg/beacon/dkg/result/marshaling.gopkg/beacon/dkg/result/marshaling_test.gopkg/beacon/registry/marshaling.gopkg/beacon/registry/marshaling_test.gopkg/clientinfo/performance.gopkg/maintainer/spv/deposit_sweep.gopkg/maintainer/spv/deposit_sweep_test.gopkg/maintainer/spv/redemptions.gopkg/maintainer/spv/redemptions_test.gopkg/protocol/inactivity/marshaling.gopkg/protocol/inactivity/marshaling_test.gopkg/protocol/state/sync_machine.gopkg/tbtc/coordination.gopkg/tbtc/coordination_window_metrics.gopkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtc/dkg.gopkg/tbtc/marshaling.gopkg/tbtc/marshaling_test.gopkg/tbtc/moving_funds.gopkg/tbtc/wallet.gopkg/tbtcpg/chain.gopkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/internal/test/marshaling.gopkg/tbtcpg/redemptions.gotools.go
💤 Files with no reviewable changes (1)
- pkg/tbtc/coordination_window_metrics.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/beacon/dkg/marshaling.go`:
- Line 76: Validate protobuf member indices against group.MaxMemberIndex before
every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go
lines 76-76, reject pbThresholdSigner.MemberIndex before assigning
ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map
key before converting it. Preserve the existing error-handling flow for invalid
input.
In `@pkg/beacon/dkg/result/marshaling_test.go`:
- Around line 34-62: Assert the result of pbutils.RoundTrip in
TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it
fails; apply the same change to the claim-signature round-trip test in
pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing
fuzz setup and message construction.
In `@pkg/tbtcpg/deposit_sweep.go`:
- Around line 605-607: Update the error wrapping in the deposit sweep flow
around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the
underlying error for errors.Is and errors.As while keeping the existing context
message and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82ec20eb-1390-462e-9ffb-68d4184fb117
📒 Files selected for processing (28)
pkg/beacon/dkg/marshaling.gopkg/beacon/dkg/marshaling_test.gopkg/beacon/dkg/result/marshaling.gopkg/beacon/dkg/result/marshaling_test.gopkg/beacon/registry/marshaling.gopkg/beacon/registry/marshaling_test.gopkg/clientinfo/performance.gopkg/maintainer/spv/deposit_sweep.gopkg/maintainer/spv/deposit_sweep_test.gopkg/maintainer/spv/redemptions.gopkg/maintainer/spv/redemptions_test.gopkg/protocol/inactivity/marshaling.gopkg/protocol/inactivity/marshaling_test.gopkg/protocol/state/sync_machine.gopkg/tbtc/coordination.gopkg/tbtc/coordination_window_metrics.gopkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtc/dkg.gopkg/tbtc/marshaling.gopkg/tbtc/marshaling_test.gopkg/tbtc/moving_funds.gopkg/tbtc/wallet.gopkg/tbtcpg/chain.gopkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/internal/test/marshaling.gopkg/tbtcpg/redemptions.gotools.go
💤 Files with no reviewable changes (1)
- pkg/tbtc/coordination_window_metrics.go
🛑 Comments failed to post (3)
pkg/beacon/dkg/marshaling.go (1)
76-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate every protobuf member index before narrowing to
group.MemberIndex. Both fields cross from protobufuint32into theuint8domain, allowing wrapped or colliding member IDs.
pkg/beacon/dkg/marshaling.go#L76-L76: rejectpbThresholdSigner.MemberIndex > group.MaxMemberIndexbefore assigning it.pkg/beacon/dkg/marshaling.go#L90-L98: reject each oversizedGroupPublicKeySharesmap key before converting it.📍 Affects 1 file
pkg/beacon/dkg/marshaling.go#L76-L76(this comment)pkg/beacon/dkg/marshaling.go#L90-L98🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/beacon/dkg/marshaling.go` at line 76, Validate protobuf member indices against group.MaxMemberIndex before every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go lines 76-76, reject pbThresholdSigner.MemberIndex before assigning ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map key before converting it. Preserve the existing error-handling flow for invalid input.pkg/beacon/dkg/result/marshaling_test.go (1)
34-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert successful fuzz round-trips. Both tests discard the only failure signal from
pbutils.RoundTrip, reducing them to panic smoke tests.
pkg/beacon/dkg/result/marshaling_test.go#L34-L62: callt.Fatal(err)when the result-signature round-trip fails.pkg/protocol/inactivity/marshaling_test.go#L33-L61: callt.Fatal(err)when the claim-signature round-trip fails.📍 Affects 2 files
pkg/beacon/dkg/result/marshaling_test.go#L34-L62(this comment)pkg/protocol/inactivity/marshaling_test.go#L33-L61🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/beacon/dkg/result/marshaling_test.go` around lines 34 - 62, Assert the result of pbutils.RoundTrip in TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it fails; apply the same change to the claim-signature round-trip test in pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing fuzz setup and message construction.pkg/tbtcpg/deposit_sweep.go (1)
605-607: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant section with line numbers. sed -n '585,620p' pkg/tbtcpg/deposit_sweep.go # Find similar error-wrapping patterns in the file for consistency. rg -n 'fmt\.Errorf\(.*%[vw]' pkg/tbtcpg/deposit_sweep.goRepository: threshold-network/keep-core
Length of output: 1569
Wrap the sweep-size error with
%w. Returning%vdrops the underlying error, so callers can’t useerrors.Isorerrors.Ason this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtcpg/deposit_sweep.go` around lines 605 - 607, Update the error wrapping in the deposit sweep flow around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the underlying error for errors.Is and errors.As while keeping the existing context message and return behavior.
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 focused, low-risk code-quality pass across the tBTC/beacon node. Almost all
changes are behavior-preserving cleanups (dedup, named types, dead-code removal,
error wrapping, naming/doc/comment consistency). Two are small, beneficial
functional fixes, called out under Behavior changes below. Net -40 lines.
Each touched package builds, vets, and passes its test suite (including the full
pkg/tbtcsuite).Changes
registerAllMetricsinto per-type helpers(counters / wallet-actions / histograms / gauges), documenting the two-phase
map-populate-then-observe concurrency invariant once per helper; remove
field-group comments that restated field names; fix a stale ticker comment.
coordinationFailedflag (both set-true branchesreturn early, so the end guard was always taken); introduce a named
DepositKeytype replacing the anonymous struct shared across tbtc/tbtcpg/ethereum; extract a shared
movingFundsSafetyMarginChaininterface; switchParseWalletActionTypeon iota constants; collapse three identicalfrequency-window guards; wrap the final-signing-group resolution error.
EstimateDepositsSweepFeeand the sync machine (was dropping / formatting thewrong value); rename
fnLogger->taskLoggerto match convention; fix twointerface doc comments to start with the method name.
(mirroring redemptions) and replace raw metric-name strings; drop the
getGlobalMetricsRecorderpassthrough wrapper; trim restating variablecomments.
marshallingfilenames to the majoritymarshalingspelling (git mv, no code changes); correct thetools.gocomment to accurately describe the pinned modules as build-time-only
dependencies (they are direct
go.modrequires, not indirect).Behavior changes
Two changes alter runtime behavior; both are intentional fixes rather than pure
cleanups:
EstimateDepositsSweepFeenow reports the real error cause. The previousmessage formatted the zero-value
sweepMaxSize(cannot get sweep max size: [0]), dropping the actual error; it now wrapserr.deposit_sweep_proof_submissions_{total,success_total,failed_total}counterswere previously incremented but never registered as observers, so they were
never exported. Registering them in
registerCounterMetricsmakes themobservable for the first time.
Scope / follow-ups
This PR intentionally covers only safe-to-moderate cleanups. Deliberately left
out (larger blast radius, need dedicated review):
GetMovingFundsParameters),node.gomonoliths,
pkg/tbtcpackage layout, metrics DI),Testing
go build ./...cleango testgreen for every touched package (clientinfo, tbtc, tbtcpg,protocol, maintainer/spv, beacon/{registry,dkg,dkg/result}, protocol/inactivity)
Summary by CodeRabbit
New Features
Bug Fixes
Tests