Add follower-side soft check for below-floor sweep fees - #4194
Add follower-side soft check for below-floor sweep fees#4194piotr-roslaniec wants to merge 6 commits into
Conversation
Extract the 25% buffer + minimum floor + Bridge-max bound into a shared applyWalletTxFeeFloor helper and a shared minWalletTxSatPerVByteFee const, then apply it to redemptions, moving funds, and moved funds sweeps in addition to deposit sweeps. These are all non-RBF wallet transactions that jam the wallet if they get stuck at the relay floor, so the same protection applies (per lrsaturnino review on #4172). EstimateRedemptionFee now takes the redemption tx max total fee so the floor can be bounded by it; the caller fetches it from GetRedemptionParameters. deposit sweep fee estimation is refactored onto the shared helper with no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
📝 WalkthroughWalkthroughChangesThe PR adds a shared 5 sat/vByte wallet fee floor with 25% buffering and maximum-fee handling. Deposit, moved-funds, moving-funds, and redemption estimators use it, while follower deposit validation logs below-floor proposals. Wallet fee-floor enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Proposal
participant FeeEstimator
participant BridgeParameters
participant applyWalletTxFeeFloor
Proposal->>FeeEstimator: request transaction fee estimate
FeeEstimator->>BridgeParameters: read maximum total fee
BridgeParameters-->>FeeEstimator: txMaxTotalFee
FeeEstimator->>applyWalletTxFeeFloor: estimated fee, vsize, and cap
applyWalletTxFeeFloor-->>FeeEstimator: buffered fee or error
FeeEstimator-->>Proposal: adjusted transaction fee
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtcpg/redemptions.go (1)
516-521: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for the new raw-estimate-exceeds-cap branch.
This newly-added check has no corresponding test case in
redemptions_test.go'sTestEstimateRedemptionFeetable, unlike the analogous branch already covered for deposit sweeps ("raw estimate above the cap returns an error"indeposit_sweep_fee_test.go) and moved funds/moving funds sweeps (ErrSweepTxFeeTooHigh/ErrFeeTooHightests).Suggested test case addition
"raw estimate above the cap returns an error": { estimateSatPerVByte: 30, txMaxTotalFee: uint64(10 * vsize), expectErrorContains: "estimated fee exceeds the maximum fee", },🤖 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/redemptions.go` around lines 516 - 521, Add a table case named “raw estimate above the cap returns an error” to TestEstimateRedemptionFee, configuring estimateSatPerVByte and txMaxTotalFee so the raw totalFee exceeds the cap, and assert the error contains “estimated fee exceeds the maximum fee.”
🤖 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.
Nitpick comments:
In `@pkg/tbtcpg/redemptions.go`:
- Around line 516-521: Add a table case named “raw estimate above the cap
returns an error” to TestEstimateRedemptionFee, configuring estimateSatPerVByte
and txMaxTotalFee so the raw totalFee exceeds the cap, and assert the error
contains “estimated fee exceeds the maximum fee.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fa2afe2-6a98-40a5-94a1-2784619bd8f4
📒 Files selected for processing (12)
pkg/tbtc/deposit_sweep.gopkg/tbtc/sweep_fee_sync_test.gopkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/deposit_sweep_fee_test.gopkg/tbtcpg/fee.gopkg/tbtcpg/fee_test.gopkg/tbtcpg/moved_funds_sweep.gopkg/tbtcpg/moved_funds_sweep_test.gopkg/tbtcpg/moving_funds.gopkg/tbtcpg/moving_funds_test.gopkg/tbtcpg/redemptions.gopkg/tbtcpg/redemptions_test.go
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.
|
2 issues found, both fixed and pushed. 1. Zero test coverage for the follower-side soft-check (Major) — the follower-side sweep-fee soft check in 2. Fee-floor/buffer logic ignored the on-chain per-request redemption fee-share cap (Major) — No unresolved issues. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtc/deposit_sweep.go (1)
478-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWell-scoped, log-only soft check. Nil-check precedes the
Cmpcase so there's no nil-dereference risk,big.Int.Cmpis used correctly for the fee comparison, and the function never returns an error from this block, matching the "log-only, never reject" design goal. Format verbs also line up correctly with their args (and%dworks fine against*big.Int, which implementsfmt.Formatter).Given
ValidateDepositSweepProposalis already a long, multi-responsibility function, consider extracting this block into a small helper (e.g.warnIfSweepTxFeeUnsafe(logger, proposal)) to keep the "gather prerequisites → chain validation" flow readable and isolate this new sizing/warning concern.♻️ Illustrative extraction
- if sweepTxSize, sizeErr := bitcoin.NewTransactionSizeEstimator(). - AddPublicKeyHashInputs(1, true). - AddScriptHashInputs(len(proposal.DepositsKeys), DepositScriptByteSize, true). - AddPublicKeyHashOutputs(1, true). - VirtualSize(); sizeErr != nil { - validateProposalLogger.Warnf( - "cannot estimate sweep tx size for the fee sanity check: [%v]", - sizeErr, - ) - } else { - minSweepTxFee := big.NewInt(int64(MinSweepTxSatPerVByteFee) * sweepTxSize) - - switch { - case proposal.SweepTxFee == nil: - ... - case proposal.SweepTxFee.Cmp(minSweepTxFee) < 0: - ... - } - } + warnIfSweepTxFeeUnsafe(validateProposalLogger, proposal)🤖 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/tbtc/deposit_sweep.go` around lines 478 - 531, Extract the sweep-fee sizing and warning block from ValidateDepositSweepProposal into a focused helper, such as warnIfSweepTxFeeUnsafe, accepting the logger and proposal. Preserve the existing transaction-size calculation, nil handling, below-minimum warning, and log-only behavior, then invoke the helper at the same point in ValidateDepositSweepProposal.
🤖 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.
Nitpick comments:
In `@pkg/tbtc/deposit_sweep.go`:
- Around line 478-531: Extract the sweep-fee sizing and warning block from
ValidateDepositSweepProposal into a focused helper, such as
warnIfSweepTxFeeUnsafe, accepting the logger and proposal. Preserve the existing
transaction-size calculation, nil handling, below-minimum warning, and log-only
behavior, then invoke the helper at the same point in
ValidateDepositSweepProposal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9006fae7-8380-433c-88b2-e169db9ea7d0
📒 Files selected for processing (4)
pkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtcpg/redemptions.gopkg/tbtcpg/redemptions_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/tbtcpg/redemptions.go
- pkg/tbtcpg/redemptions_test.go
3 participants
Summary
Adds a follower-side soft (log-only) check that the leader's proposed deposit
sweep fee is not below the safe minimum. The on-chain
WalletProposalValidatoronly bounds the sweep fee from above, so a misbehaving or unpatched leader can
propose a fee at the ~1 sat/vByte relay floor that would otherwise be signed -
the same underpricing that jams the wallet (#4171).
Each node recomputes the safe minimum and logs a warning when the proposal is
below it.
The check is intentionally log-only, not a rejection: rejecting a
below-floor proposal would, during a mixed-version rollout, split signers
(patched nodes reject, unpatched nodes sign) and could stall signing. Hard
enforcement belongs on-chain in the
WalletProposalValidatoror behind acoordinated all-nodes upgrade.
Contents
This branch is rebased on current
mainand, in addition to the soft check,carries the safe-minimum sweep-fee floor applied to all wallet transactions
(the
feat/floor-all-wallet-tx-feeswork, also open as #4179 / #4192). Thefloor commit is bundled here so the branch builds and passes CI against
main;if the floor lands separately first, this branch should be rebased to drop it.
Review follow-ups addressed
Incorporates the multi-agent review of the soft check:
SweepTxFeeisnilinstead of silently skipping the check; amissing fee gets its own distinct log line.
big.Int.Cmpinstead ofInt64(), which isundefined above
MaxInt64.the (now exported)
pkg/tbtcmirrors against the canonicalpkg/tbtcpgconstants, so drift is caught regardless of which side changes. A
literal-based guard could be defeated by updating
tbtcpgand the literaltogether while forgetting the mirror; comparing the live values closes that
gap.
Supersedes
Replaces #4180, whose head lived on a fork branch that had drifted onto an old
mainand becomeCONFLICTING. This PR is the same work rebased clean ontocurrent
main.Summary by CodeRabbit