fix: address validated multi-lens review findings (follow-up to #4338) - #4342
Merged
Merged
Conversation
P1: - add missing permissions block to contracts-random-beacon.yml (F-01) - document redemptions overflow-fallback semantic change (F-03) - declare EnableServer pprof-disabled breaking change in godoc (F-04) P2: - drop redundant Maintainer.Validate() call in maintainer.Initialize since spv.Initialize still validates at the use-site boundary (F-05) - correct Maintainer.Validate() godoc: it validates Spv, not 'all modules' (F-06) - document the dormant net/http/pprof DefaultServeMux side-effect in the clientinfo package doc and point registerPprofHandlers at it (F-07) - remove the redundant double error wrapper in maintainer.Initialize (F-09) P3: - reword DKG malformed-key comment to lead with current behavior rather than past-tense framing (F-10) - rename the redemptions overflow test case to reflect the branch actually exercised (fallback-to-txMaxTotalFee, not wrap-to-zero) (F-12) F-11 (replace sanitizeServerURL with (*url.URL).Redacted()) was reviewed and skipped: Redacted() keeps the username (only masks the password), which leaks more than the manual strip-everything implementation currently in place; the manual version is intentionally more conservative for Electrum server credentials. Kept as-is.
|
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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
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.
Follow-up to the merged PR #4338. This addresses all P1, P2, P3 findings raised by the multi-lens validated review of #4338.
Summary
Fixes all valid issues identified by the validated multi-agent review of the original #4338 PR, which covered a defensive security/reliability fix-up. This commit applies the remaining actionable findings that #4338 did not address.
P1
permissions: contents: readblock tocontracts-random-beacon.yml(the original fix: address validated review findings #4338 added it only tocontracts-ecdsa.yml, despite the beacon workflow using the samesecrets.CI_GITHUB_TOKENfornotify-workflow-completedand the sameyarn uppattern).txMaxFee * requestCountoverflows, the old code wrapped to 0 and rejected every estimate; the new code falls back totxMaxTotalFee. This semantic change is intentional (covered by the "huge per-request cap" test) but was not called out in fix: address validated review findings #4338's PR body.EnableServer(port int)backwards-incompatible breaking change in godoc. The previous implementation implicitly exposed pprof viahttp.DefaultServeMux; the new implementation callsenableServer(port, false)and unconditionally disables pprof. No in-repo callers; external consumers would silently lose pprof functionality.P2
Maintainer.Validate()call inmaintainer.Initializesincespv.Initializestill validates at the use-site boundary. Reduces 3 identical validation passes per command to 2.Maintainer.Validate()godoc — it currently validates only Spv, not "all modules" as the docstring claimed.net/http/pprofDefaultServeMux side-effect in the clientinfo package doc and pointregisterPprofHandlersat it. The current code builds a private ServeMux so the registration is dormant, but any future contributor adding a nil-handlerListenAndServecall would silently expose pprof.maintainer.Initialize("cannot validate spv maintainer config: cannot validate spv maintainer config: ...").P3
Skipped
sanitizeServerURLwith(*url.URL).Redacted()) was reviewed and skipped: Go 1.15+Redacted()keeps the username (only masks the password), which leaks more than the manual "strip everything" implementation currently in place. The manual version is intentionally more conservative for Electrum server credentials and was kept.Verification
gofmt -l .cleango vetcleango build ./...cleango test ./pkg/maintainer/...okgo test ./pkg/clientinfo/...okgo test ./pkg/bitcoin/electrum/...okgo test -run TestApplyWalletTxFeePolicy ./pkg/tbtc/...okgo test ./pkg/tbtcpg/...okgo test -run TestGenerateSymmetricKeys ./pkg/tecdsa/dkg/...okgo test ./config/...okRelated