test: cover sweep-fee floor boundaries and add below-floor metric - #4197
Open
piotr-roslaniec wants to merge 3 commits into
Open
test: cover sweep-fee floor boundaries and add below-floor metric#4197piotr-roslaniec wants to merge 3 commits into
piotr-roslaniec wants to merge 3 commits into
Conversation
The follower-side soft check had no behavioral tests; only a constant drift guard existed, so a flipped comparison or dropped branch in the below-floor decision would pass CI silently. Extract the decision into a pure checkSweepFeeFloor helper so it can be tested directly instead of by scraping logger output, and cover the boundary (just below / exactly at / above the safe minimum, plus missing and zero fee). Emit a deposit_sweep_fee_below_floor_total counter on the follower path when a proposal is below the floor, so operators can alert on underpriced proposals during a mixed-version rollout instead of grepping node logs. The metric reuses the existing follower metricsRecorder and does not change ValidateDepositSweepProposal's signature; the check stays log-only and the node still signs the proposal.
EstimateRedemptionFee had no coverage for two boundary outcomes of the safe-minimum floor: - a buffered fee that exceeds the Bridge max is bounded down to the cap (not an error), and - a raw estimate already above the cap errors before the floor is applied, rather than broadcasting an underpriced transaction. The second case is intended behavior (redemptions.go documents it) but trades liveness for not broadcasting a fee the Bridge would reject; pinning it in a test makes that tradeoff explicit and regression-safe.
|
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 |
IncrementCounter/RecordDuration only ever write into PerformanceMetrics' internal map. A metric name is exposed on the /metrics endpoint only if it was also passed to registerAllMetrics() at startup, since that is what drives the underlying keep-common Registry.metrics map the HTTP handler actually reads; there is no fallback that dumps arbitrary counters/histograms that were never registered. deposit_sweep_fee_below_floor_total (the counter backing the follower-side below-floor sweep-fee alert, see #4171) and its five pre-existing siblings - deposit_sweep_executions_total, deposit_sweep_executions_success_total, deposit_sweep_executions_failed_total, deposit_sweep_execution_duration_seconds, and deposit_sweep_tx_signing_duration_seconds - were never added to that list, so none of them ever appeared in a scrape no matter how many deposit sweeps ran or how underpriced a proposal was. Add named clientinfo.Metric* constants for all six, matching the pattern already used by redemption.go and maintainer/spv/redemptions.go, register the four counters and two durations in registerAllMetrics(), and switch deposit_sweep.go from raw string literals to the new constants. Once registered, ObserveApplicationSource still prefixes the name with the application ('performance'), so the below-floor counter scrapes as performance_deposit_sweep_fee_below_floor_total, not the bare name - any alerting rule needs to account for that prefix. Add a registration test mirroring the existing join-failure-counters test: it checks that each counter/histogram is already present in the internal map before any increment, which is what proves it was pre-registered rather than lazily created on first use (lazy creation would still leave it unexposed on /metrics).
Member
|
1 issue found and fixed. Fixed
No unresolved findings. |
lrsaturnino
approved these changes
Jul 29, 2026
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
Follow-up to #4194, addressing the confirmed, actionable findings from a multi-agent review of that PR. Stacked on
feat/follower-sweep-fee-soft-check; rebase ontomainonce #4194 merges.Scope is deliberately narrow: test coverage for the untested soft check and fee-floor boundaries, plus one small observability add. No behavior of the leader-side floor changes.
Changes
Follower soft check: tests + metric (was untested — review's lead item)
ValidateDepositSweepProposalinto a purecheckSweepFeeFloorhelper, and unit-test it directly (just below / exactly at / above the safe minimum, plus missing and zero fee) instead of scraping logger output. The check previously had zero behavioral tests — only a constant-drift guard — so a flippedCmpor a dropped branch would pass CI silently.deposit_sweep_fee_below_floor_totalcounter on the follower path when a proposal is below the floor. The check was log-only; a metric lets operators alert on underpriced proposals during a mixed-version rollout instead of grepping node logs. Reuses the existing followermetricsRecorder;ValidateDepositSweepProposal's signature is unchanged and the node still signs the proposal.Redemption fee: cap boundary tests
EstimateRedemptionFeegains coverage for two boundary outcomes of the safe-minimum floor:redemptions.go), but it trades liveness (redemption not attempted this round) for not broadcasting a fee the Bridge would reject. The test pins that tradeoff so it is a visible, regression-safe decision.Reviewed but intentionally deferred (not fixed here)
5*vsizefloor, not the buffered estimate. Catching "above floor but underpaid" proposals would require a live fee-oracle query inside proposal validation — the wrong layer, and it introduces non-determinism into validation. Hard enforcement belongs on-chain in theWalletProposalValidator.isWitness=true) — already documented as a code caveat infee.go.applyWalletTxFeeFloor— a no-op for current callers (EstimateFeereturns exact multiples); safe to defer.SweepTxFeebranch — defensive; a nil fee fails at ABI packing before the soft check runs, so it is not reachable in production.tbtc<->tbtcpg) — already guarded by the drift test added in Add follower-side soft check for below-floor sweep fees #4194.An overflow concern raised during review was rejected as logically unreachable: the
int64(maxTotalFee)cast runs only insideuint64(totalFee) > maxTotalFee, which for anint64 totalFeeimpliesmaxTotalFee < MaxInt64, so the cast cannot wrap.Testing
go test ./pkg/tbtcpg/— passgo test ./pkg/tbtc/ -run 'TestDepositSweepAction|TestCheckSweepFeeFloor|TestSweepFeeConstantsMirrorTbtcpg'— passgo vet/gofmtclean on both packages