feat(bridge-exec): introduce FeeSource trait with BitcoinCore + MempoolExplorer policies (STR-3438) - #563
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## azz/fireblocks-general-wallet #563 +/- ##
=================================================================
+ Coverage 87.45% 87.52% +0.07%
=================================================================
Files 264 265 +1
Lines 35727 35994 +267
=================================================================
+ Hits 31244 31504 +260
- Misses 4483 4490 +7
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The only major comment here is in regards to how this API will be exercised by the tx-driver during RBF in case a transaction does not confirm within the supplied deadline. The rest are mostly about improving doc comments.
Also, love how the unit tests are written!
| pub const fn new(client: Arc<R>, conf_target: u16) -> Self { | ||
| Self { | ||
| client, | ||
| conf_target, |
There was a problem hiding this comment.
To make this API easier to use, conf_target should be an argument to estimate, not part of the struct. For example, we should be able to call tx_driver::drive(...) with a deadline parameter and it should estimate fees based on that, the closer the deadline is the more aggressive should the fee rate be (without having to reconstruct the source).
Even semantically, any FeeSource must only include the source itself (the client), not how aggressive the estimate should be.
There was a problem hiding this comment.
For mempool explorer, you can map the fee policy to the confirmation target (Fastest if deadline is in the next couple of blocks, HalfHour if we have several blocks to go until the deadline, etc.). Feel free to use reasonable boundaries here.
In this case, it should. But it's also fine to leave it as is. Once we have CPFP, we'll remove the |
a797cac to
55adcc4
Compare
|
reworked to layer ontop of the cpfp work and FB |
83f7e80 to
5c863c2
Compare
|
I see that the base for this PR has changed. What other PR does this depend on? |
5c863c2 to
9211909
Compare
Rajil1213
left a comment
Rajil1213
left a comment
There was a problem hiding this comment.
Only some nits remain in the new changes. However, there are still some unresolved comments from the previous round. And the following have been flagged by Codex:
fee::FEE_RATEis still being used to refill the claim-funding UTXOs. This should also use thecfg.fee_source.current()estimate (clamped as necessary).unstaking_burnexecutor also still uses bitcoin core directly instead of the new fee source abstraction.
408d189 to
7faea59
Compare
7faea59 to
41d88b8
Compare
|
Went through this PR again. And some of the reviews from the last rounds have still not been addressed. Here is the list:
I still don't fully understand why the base PR for this one is the fireblocks branch. I'd have assumed that the fee rate policy is a standalone change. |
41d88b8 to
47b318b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b318b5a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // then floor it at `MIN_WALLET_TX_FEE_RATE` so the withdrawal-fulfillment v3 transaction stays | ||
| // relayable. The underlying source already clamps to the ≥1 sat/vB truncation guard; this is | ||
| // the higher bridge-policy minimum. | ||
| let fee_rate = cfg.fee_source.current().max(MIN_WALLET_TX_FEE_RATE); |
There was a problem hiding this comment.
When background refreshes fail after a low initial estimate, current() retains that value indefinitely, so this time-sensitive path can proceed even after the market rate has risen above the configured maximum. The previous live estimate_smart_fee call aborted on an estimation failure, whereas this path never consults seconds_since_last_refresh; because the same stale value also drives CPFP, a fulfillment can remain underpriced until reassignment and expose the operator to the fund-loss scenario described below. Reject an excessively stale cached estimate or refresh synchronously before funding the withdrawal.
Useful? React with 👍 / 👎.
| FeeRate::from_sat_per_vb(raw_sat_per_vb) | ||
| .unwrap_or(MIN_SOURCE_FEE_RATE) | ||
| .max(MIN_SOURCE_FEE_RATE) |
There was a problem hiding this comment.
If a configured explorer returns a malformed or compromised recommendation above FeeRate's representable sat/vB range, from_sat_per_vb returns None and this code converts the extreme quote into the 1 sat/vB minimum. The cache then reports a low rate, and wallet callers floor it to 2 sat/vB without triggering maximum_fee_rate, reversing the intended safety behavior. Treat conversion failure as a source error so the Bitcoin Core fallback is used, or preserve it as a high rate that the maximum check rejects.
Useful? React with 👍 / 👎.
47b318b to
f3c5bff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3c5bff8a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| policy, | ||
| fallback_conf_target, | ||
| } => { | ||
| let fallback = BitcoindFeeSource::new(bitcoind, fallback_conf_target); |
There was a problem hiding this comment.
When MempoolExplorer is configured with fallback_conf_target = 0 or a value above 1008, this branch accepts it as long as the explorer succeeds during startup. Bitcoin Core's estimatesmartfee contract only accepts confirmation targets from 1 through 1008, so once that explorer later fails every fallback RPC also fails and CachedFeeSource retains the previous explorer quote indefinitely instead of providing the promised Core fallback. Validate this range in build so the invalid configuration fails at startup.
Useful? React with 👍 / 👎.
f3c5bff to
3e0b008
Compare
5a6646f to
e4ca801
Compare
e4ca801 to
a0ed52a
Compare
a0ed52a to
137cf73
Compare
137cf73 to
d739e4a
Compare
…nding leaks (STR-3439) - Add `CpfpWallet::release`. Each terminal bump path hands the last child's funding leases back: lost shared anchor, confirmed competitor, torn-down fallback, entry removed or dormant. - Do not release while our own child holds the shared anchor. A release there lets a concurrent build evict the live child. `AnchorSpendState::SpentInMempool` names the spender for this check. - Keep the `ParentTxCombined` payout outpoint in the lease record. It is a wallet UTXO, and only the terminal releases free it. - Keep a mined parent's entry, marked dormant, until burial. A reorg puts the parent back in play and the bumps resume. Release leases at mined depth; a bump that raced the mined event releases its own. - Insert the CPFP entry before the eager new-job bump, and run that bump only while no batch or eviction bump is in flight. Concurrent bumps of one entry lost lease records. The fallback removes only an entry that it inserted. - Resubmit the bare parent when an eviction finds the bump slot busy. Before, the eviction was skipped with no effect. - Re-admit `ReplacedChild.inputs` in funding selection as foreign inputs after a sync marks them spent. Before, each rebuild consumed a fresh UTXO, and a one-UTXO wallet stopped permanently. - Floor the replacement fee at `ReplacedChild.fee` plus 1 sat/vB over the child's vbytes (BIP-125 rule 4), so a small target rise makes a valid replacement. - Replace `CpfpKind::InferAnchor` with `CpfpKind::AnchorAt`. Callers name the anchor vout from the tx type's constant. The publish helper validates script and value; the value check is a floor because the counterproof ACK anchor carries two times dust. The old exact-value scan published every ACK without a bump path. - Retry the startup wallet sync every 30 s until one succeeds. Drop the write lock before each retry sleep. - Fund withdrawal fulfillment with confirmed inputs only. TRUC rejects a v3 tx with an unconfirmed ancestor outside 1P1C. - Round the package fee up, and disable the per-transaction fee-rate guard in `submitpackage`. That guard rejected well-formed high-ratio packages.
… (STR-3437) Adds a second GeneralWallet backend that holds funds in a Fireblocks vault instead of a local BDK wallet. The backend, the trait additions, and every implementor and call site move together in one commit, because the trait grows two methods and an exhaustive split does not compile. The backend builds and signs funding transactions and CPFP children through the Fireblocks RAW signing API. It threads bip44 address indices into every signing request, validates untrusted API responses, retries transient poll errors, and uses checked fee arithmetic. The implementation follows the Fireblocks API docs and OpenAPI spec, because we do not hold a test key. The orchestrator selects native or Fireblocks from configuration and builds an erased AnyOperatorWallet handle, so no call site names a concrete backend. Payout destinations are backend-aware. Each backend reports the script that it can actually spend. The native wallet reports its own receive script, and Fireblocks reports the vault address. The native payout descriptor previously wrapped the raw general key, and BOSD treats that payload as an already-tweaked output key. Payouts to that script were invisible to the wallet and unspendable by every production signer. At startup the orchestrator compares the covenant payout descriptor in the params file against the payout script of the wallet, and warns on a mismatch. The presigned graph pays the params descriptor. On a mismatch, those payouts land on a script that this wallet cannot spend, and every CPFP bump of them fails. The check warns rather than aborts, because graphs presigned under older params can differ legitimately. The payout e2e builds its payout outputs from the production descriptor, so a descriptor that the production signers cannot spend fails real bitcoind script verification. An invariant test pins the native descriptor to the wallet receive script. The Fireblocks child builder handles both anchor shapes and predicts the child size from the real anchor witness. Also bumps jsonwebtoken 9.3.1 to 10.4.0 (GHSA-9hjw-9xqj-r4r5).
…re modes visible and recoverable (STR-3437) - Record the outcome and time of each general-backend sync. The wallet health probe reports a failed sync as unhealthy and a stale success as degraded with its age. Before, the probe read only the reserved wallet's chain tip, which advances while the general backend fails, so a dead Fireblocks backend reported healthy without limit. - Validate each stake funding reservation prevout against the current general wallet. Discard a reservation that fails validation, release its rehydrated leases, and fund afresh. A reservation funded under one backend and signed under another failed on every attempt, and the durable row blocked stake publication permanently. - Compare payout-descriptor types before the startup mismatch warning. A same-type mismatch keeps the rotation warning. A cross-type mismatch states the real condition: the backend class cannot receive presigned-graph payouts. The recovery claim in the message is checked against the secret-service general key, not assumed. Before, the warning fired on every Fireblocks start and read as rotation drift. - Select confirmed UTXOs only for CPFP funding. The child has one unconfirmed parent already, and a second unconfirmed input breaks the TRUC one-parent-one-child shape. - Stop CPFP funding selection at the TRUC 1000 vB child limit with a clean error. A child past the limit cannot enter any mempool. - Bound one RAW-signing wait at 180 s wall clock. The attempt cap alone allowed ~17 minutes under the wallet write lock when each poll ran to the HTTP timeout. - Keep the hard asset-id checks only where they are load-bearing: "BTC" is required on mainnet and rejected off mainnet. Other test network ids warn, and the first API call validates them. Before, a signet operator with a non-"BTC_TEST" id was unable to start. - Warn when a deposit retry reuses a payout descriptor that differs from the current backend. Reuse itself is unchanged: peers nonced against that descriptor, and a substitution invalidates the session.
…olExplorer policies (STR-3438) One FeeSource trait (AFIT, defined in btc-tracker) with three implementations: Bitcoin Core estimatesmartfee, a mempool.space compatible explorer with a bitcoind fallback, and a fixed rate for tests and manual override. The orchestrator builds the configured source at startup and wraps it in one shared CachedFeeSource. The executors and the CPFP bump loop read the same cache, so no caller pays a network round trip per estimate. A source that reports no estimate falls back to the 1 sat/vB floor and warns loudly. Without the warning, a misconfigured node looks identical to a calm mempool.
Claim-funding refills funded at the presigned-graph constant of 2 sat/vB. A refill issued into a busy mempool sat unconfirmed and starved graph generation of funding UTXOs. Wallet-funded transactions (claim-funding refills, stake funding) now fund at the live cached rate, with a 2 sat/vB relay floor and a rejection above the operator maximum. Both sites share one helper, so the policy cannot drift between them. Presigned bridge transactions keep the protocol constant by construction. The CPFP bump loop raises their rate after the fact.
…ee quotes (STR-3438) - Validate `estimatesmartfee` confirmation targets (1 to 1008) for both fee-source variants at build time, and report the exact config key with the rejected value. The `bitcoin_core` variant failed at boot with a cryptic RPC error. The `mempool_explorer` variant was worse: a working explorer masked a broken `fallback_conf_target`, and the bad value surfaced on the day the fallback was first necessary. - Bound fee-cache staleness at ten refresh intervals. Duty pricing reads the cache through `try_current` and aborts with `FeeSourceStale` when the bound is exceeded; the duty retries after the source recovers. Before, a dead source froze the cache and each duty priced at whatever the market was when the source died, without any signal. - Keep the bump path on the infallible read: a bump at a stale rate is better than no bump, because the parent already sits at the protocol floor. A stale read on that path logs a warning.
…next-block targets (STR-3438)
CPFP children must confirm in the next block per the 2026-08-26 product fee
decision. Wallet-funded transactions are not time-critical. One estimate
served both consumers, so the wallet transaction paid for the next-block
tier without needing it.
- Add FeeTarget { Standard, NextBlock } and TargetRates to the FeeSource
trait. estimate() takes a target; estimate_all() returns both rates in
one upstream round trip
- CachedFeeSource stores one slot per target under one staleness
timestamp. The background refresh fills both slots per tick
- The CPFP bump ladder reads NextBlock. Wallet-funded paths (claim-funding
refill, stake funding, unstaking burn, withdrawal fulfillment) read
Standard
- Bitcoind: NextBlock is estimatesmartfee(1); Standard is the configured
conf target. Deduplicate the round trip when the two coincide
- Mempool explorer: NextBlock is fastestFee; Standard is the configured
policy tier. One HTTP call feeds both slots
- Fixed: both targets return the configured rate verbatim
Signed-off-by: azz <azz@alpenlabs.io>
…ckage floor to CPFP bumps (STR-3438) - Add `fee_premium_percent` (default 5) and `min_package_fee_rate` (default 10 sat/vB) to `CpfpContext`. - The bump ladder applies the premium to the next-block estimate, then the floor, then the existing `max_fee_rate` clamp. - The premium is a percentage of the estimate, computed in sat/kwu and rounded up. The premium raises the target and never lowers it. - The floor is a lower bound on the package fee rate target. The child pays the package shortfall, so the package always lands at or above the floor. - Add `cpfp_fee_premium_percent` and `cpfp_min_package_fee_rate` TOML knobs to the bridge config with the PRD defaults. - Add a startup check that rejects a package floor above the `max_fee_rate` cap. - The knobs are CPFP consumer policy. Wallet-funded transactions price from `wallet_tx_fee_rate` and never pass through either knob. - Tests: premium applied, floor lifts a low estimate, premium-then-floor order pinned, cap wins over both, rounding at the sat/kwu boundary. Existing ladder tests keep neutral knobs (premium 0, floor 0) and pin the raw-estimate-to-cap behavior.
d739e4a to
cbaee9b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbaee9b592
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let prior_child_fee = handle | ||
| .last_child_fee | ||
| .filter(|_| handle.last_child_txid.is_some()); |
There was a problem hiding this comment.
When a child is already live and the fee estimate remains unchanged, Tick and NewBlock bypass the same-rate skip, and passing its fee here forces the otherwise deterministic rebuild to pay the incumbent fee plus another child-vsize increment. The rebuilt child therefore gets a new txid and is accepted as an RBF replacement instead of deduplicating; at the default 30-second tick this continually ratchets fees above the configured package target and eventually above max_fee_rate until the parent confirms. Check that the recorded child is still present before rebuilding, or avoid adding the replacement floor for unchanged periodic targets.
Useful? React with 👍 / 👎.
| // the mempool and pays less than the recorded fee. The floor is then conservative, | ||
| // which is safe: the next child pays slightly more than the rule requires. | ||
| handle.last_child_inputs = funded.spent.clone(); | ||
| handle.last_child_fee = funded.psbt.fee().ok(); |
There was a problem hiding this comment.
When a replacement build succeeds but signing or submitpackage subsequently fails, this assignment records the unsubmitted candidate's higher fee while last_child_txid still identifies the older mempool child. Every retry then treats that nonexistent fee as the BIP-125 baseline and adds another incremental fee, so a prolonged signer/RPC failure can ratchet the next successful replacement far above the configured target and cap. Keep the incumbent child fee unchanged until submission succeeds, while recording only the newly leased inputs on the failure path.
Useful? React with 👍 / 👎.
2 participants
Closes STR-3438.
Summary
Replaces the two direct `estimate_smart_fee(1)` calls in `deposit.rs` and `stake/staking.rs` with a configurable fee-source abstraction. Mirrors strata's btcio shape (per Slack discussion with @Rajil1213 and @bewakes), kept bridge-local per Rajil's call against a shared crate.
Design
`FeeSource` trait + 3 impls in `crates/bridge-exec/src/fees.rs`:
Operator-side config selects via `FeeSourceConfig` (snake_case tagged enum), built into `Arc` at orchestrator boot. `#[serde(default)]` → `BitcoinCore { conf_target: 1 }`, preserving pre-PR behaviour for existing operator config.toml files.
Deliberate divergences from strata's impl
Hardening from local audit
Scope notes
The ticket cited three call sites but the third (`graph/common.rs:166`) is a hardcoded `fee::FEE_RATE` constant for the claim-funding refill self-spend, not a live `estimate_smart_fee` call. Left as-is — that's a different conversation about whether self-spend refills should use live estimates.
Responsibility split (for context)
The `FeeSource` trait is stateless. Callers fetch once per tx-build. Live fee tracking — periodically re-checking to drive RBF bumps — is the tx-driver's job once CPFP wiring lands in STR-3439. The trait deliberately does not poll, cache, or push.
Test plan