test: cover sweep-fee floor boundaries and add below-floor metric - #4197
Open
piotr-roslaniec wants to merge 9 commits into
Open
test: cover sweep-fee floor boundaries and add below-floor metric#4197piotr-roslaniec wants to merge 9 commits into
piotr-roslaniec wants to merge 9 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 |
Member
|
1 issue found and fixed. Fixed
No unresolved findings. |
lrsaturnino
previously approved these changes
Jul 29, 2026
The on-chain WalletProposalValidator bounds the sweep fee only from above, so a misbehaving or unpatched coordination leader can propose a sweep at the ~1 sat/vByte relay floor that patched followers would still sign - the same underpricing that jams the wallet (#4171). ValidateDepositSweepProposal now recomputes the safe minimum and warns if the proposed fee is below it. The check is intentionally log-only, not a rejection: rejecting a below-floor proposal during a mixed-version rollout would split signers and could stall signing. Hard enforcement belongs on-chain in the WalletProposalValidator or behind a coordinated all-nodes upgrade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The follower-side soft check in pkg/tbtc hand-copies the safe minimum sweep-fee rate and worst-case deposit script size from pkg/tbtcpg, because pkg/tbtcpg imports pkg/tbtc and the canonical constants cannot be imported back without a dependency cycle. Only sync comments kept them aligned, so silent drift would make the check compute a wrong floor. Export the canonical constants (MinWalletTxSatPerVByteFee, DepositScriptByteSize) and add a guard test in an external tbtc_test package - which can import pkg/tbtcpg without a cycle - that fails if the canonical values drift from the pkg/tbtc mirrors.
Address review on the follower-side below-floor sweep-fee check: - Warn when SweepTxFee is nil instead of silently skipping the check; a missing fee gets its own distinct log line. - Compare the proposed fee with big.Int.Cmp instead of Int64(), which is undefined above MaxInt64. - Replace the literal-pinned drift guard with a direct comparison of the exported pkg/tbtc mirrors against the canonical pkg/tbtcpg constants, so drift is caught regardless of which side changes. The constants are exported for this cross-package comparison.
Add TestValidateDepositSweepProposal_SweepFeeSoftCheck exercising the log-only warning in ValidateDepositSweepProposal for a proposal's SweepTxFee: below the safe minimum, at/above it, and unset (nil). The nil case is only reachable through a test/mock chain implementation. On the real production path, a nil fee is already ABI-packed for the on-chain WalletProposalValidator call a few lines above the soft check and panics there first, and wire deserialization always constructs a non-nil fee. Document that inline next to the nil check so a future reader does not mistake it for a reachable production guard. Adds a capturingLogger test double, mirroring the existing pattern in pkg/net/retransmission, plus a minimal stub satisfying the chain interface ValidateDepositSweepProposal expects, so the soft check can be exercised in isolation from on-chain validation and deposit-lookup concerns it does not depend on.
The redemption fee floor/buffer logic bounded the estimated fee only by the Bridge's flat total-fee cap (redemptionTxMaxTotalFee). The Bridge separately enforces a per-request fee-share cap (redemptionTxMaxFee) once the total fee is split evenly across the redemption requests, and that check is independent of the total cap. For small request counts the per-request cap, aggregated over the requests, can be tighter than the total cap. Once the 25% safe-minimum buffer is applied on top, the buffered fee could exceed the aggregated per-request cap while still passing the (looser) total-fee check, causing the proposal to fail the on-chain/self-check validation that runs right after estimation for what would otherwise be a routable redemption. Fetch the per-request cap alongside the total cap (already fetched in the same call) and bound the estimate by the tighter of the two, mirroring the on-chain per-request division. Also correct the docstrings that previously implied bounding by the total cap alone was sufficient to avoid a Bridge rejection.
The added per-request cap (previous commit) clamps the estimated fee to an exact multiple of the request count whenever it is the binding constraint, which always produces a zero remainder and can never trigger the per-request-share warning. The existing test's numbers happened to hit exactly that clamped case, so the warning no longer fired. Rework the warning test case so txMaxTotalFee (not the aggregate per-request ceiling) is the binding, non-multiple-of-count constraint, reproducing a genuine remainder-driven violation that survives the aggregate cap fix.
piotr-roslaniec
dismissed
lrsaturnino’s stale review
August 7, 2026 16:22
The merge-base changed after approval.
piotr-roslaniec
force-pushed
the
feat/follower-sweep-fee-soft-check
branch
from
August 7, 2026 16:22
cca60b9 to
044fbbd
Compare
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.
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).
piotr-roslaniec
force-pushed
the
chore/sweep-fee-review-followups
branch
from
August 7, 2026 19:37
f5a284f to
a0fd5bd
Compare
piotr-roslaniec
force-pushed
the
feat/follower-sweep-fee-soft-check
branch
from
August 18, 2026 09:04
044fbbd to
581783e
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
3 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