feat(via_btc_sender): harden spendability, fee floors, and anti-dust policy#343
feat(via_btc_sender): harden spendability, fee floors, and anti-dust policy#343cryptocake wants to merge 6 commits intovianetwork:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces robust production guardrails to the Bitcoin sender path, significantly enhancing its resilience and stability. The changes aim to prevent common issues such as transaction stalls caused by pending UTXO chains, the creation of economically unviable tiny outputs, and problematic low-fee chaining behavior. By implementing configurable policies for output sizes, fee rates, and operational limits, the system will now gracefully degrade throughput under challenging network conditions rather than encountering hard stalls, leading to more predictable and reliable operation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR introduces configurable “sender policy” guardrails for BTC inscription sending to avoid pipeline stalls from deep pending tx chains, dust-adjacent outputs, and low-fee chaining.
Changes:
- Added new
ViaBtcSenderConfigpolicy knobs (output floors, fee floors/cap, chain depth and spendable-balance guards) and wired defaults into base env config. - Introduced
InscriberPolicyinvia_btc_clientand wired it through core/verifier node layers into theInscriber. - Added runtime “pause send” guards (chain depth + spendable balance) plus a
chain_guard_blocksmetric to observe policy-blocked attempts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| via_verifier/node/via_btc_sender/src/metrics.rs | Adds chain_guard_blocks counter for verifier sender guard blocks. |
| via_verifier/node/via_btc_sender/src/btc_inscription_manager.rs | Adds pending-chain-depth + spendable-balance guards before broadcasting. |
| etc/env/base/via_btc_sender.toml | Adds base defaults for new sender policy fields. |
| core/node/via_btc_sender/src/metrics.rs | Adds chain_guard_blocks counter for core sender guard blocks. |
| core/node/via_btc_sender/src/btc_inscription_manager.rs | Adds pending-chain-depth + spendable-balance guards before broadcasting. |
| core/node/node_framework/src/implementations/layers/via_btc_sender/vote_manager.rs | Wires InscriberPolicy into verifier vote-manager inscriber initialization. |
| core/node/node_framework/src/implementations/layers/via_btc_sender/manager.rs | Wires InscriberPolicy into core manager inscriber initialization. |
| core/node/node_framework/src/implementations/layers/via_btc_sender/aggregator.rs | Wires InscriberPolicy into core aggregator inscriber initialization. |
| core/lib/via_btc_client/src/inscriber/mod.rs | Implements InscriberPolicy and enforces min outputs + fee floors/cap. |
| core/lib/config/src/configs/via_btc_sender.rs | Adds new config fields + getters with defaults + for_tests() updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn get_fee_rate(&self) -> Result<u64> { | ||
| debug!("Getting fee rate"); | ||
| let res = self.client.get_fee_rate(FEE_RATE_CONF_TARGET).await?; | ||
| debug!("Fee rate obtained: {}", res); | ||
| Ok(std::cmp::max(res, 1)) | ||
| let min_floor = if self.context.fifo_queue.is_empty() { | ||
| self.policy.min_feerate_sat_vb | ||
| } else { | ||
| self.policy.min_chained_feerate_sat_vb | ||
| }; | ||
|
|
||
| let mut effective = std::cmp::max(res, min_floor); | ||
| effective = std::cmp::min(effective, self.policy.max_feerate_sat_vb); | ||
|
|
||
| debug!("Fee rate obtained: {}, effective: {}", res, effective); | ||
| Ok(std::cmp::max(effective, 1)) |
There was a problem hiding this comment.
New policy behavior is introduced here (fee floor/cap selection), but the unit tests in this module still only cover the happy-path inscribe() flow. Since this file already has a test harness, consider adding targeted tests asserting get_fee_rate() applies the configured floors/caps for empty vs non-empty context (and respects the cap).
| if commit_tx_change_output_value < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount: {:?}, Spendable Amount: {:?}. change output {:?} is below minimum {:?}", | ||
| required_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlocked_value, | ||
| commit_tx_change_output_value, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Minimum change-output enforcement for commit/reveal is new behavior that can block sending under low balances. Add unit tests that exercise the below-minimum paths (both commit and reveal) to ensure the failure mode and error details stay stable as fee estimation logic evolves.
| let spendable_balance = self.inscriber.get_balance().await?; | ||
| if spendable_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new inscription broadcast due to low spendable balance guard. spendable={} min={}", | ||
| spendable_balance, | ||
| self.config.min_spendable_balance_sats() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
send_new_inscription_txs() calls self.inscriber.get_balance() even though the same loop iteration already calls get_balance() in update_inscription_status() for the balance gauge. This adds an extra Bitcoin RPC + tx deserialization pass every poll interval and can become a noticeable overhead (and additional failure surface) under load. Consider computing the spendable balance once per loop iteration (e.g., in loop_iteration), passing it into both methods, or caching it on the manager for the current tick.
| let spendable_balance = self.inscriber.get_balance().await?; | ||
| if spendable_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new verifier inscription broadcast due to low spendable balance guard. spendable={} min={}", | ||
| spendable_balance, | ||
| self.config.min_spendable_balance_sats() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
send_new_inscription_txs() calls self.inscriber.get_balance() even though the same loop iteration already calls get_balance() in update_inscription_status_or_resend() for the balance gauge. This adds an extra Bitcoin RPC + tx deserialization pass every poll interval and can become a noticeable overhead (and additional failure surface) under load. Consider computing the spendable balance once per loop iteration and reusing it for both metrics reporting and guard checks.
|
|
||
| let mut effective = std::cmp::max(res, min_floor); | ||
| effective = std::cmp::min(effective, self.policy.max_feerate_sat_vb); |
There was a problem hiding this comment.
get_fee_rate() clamps effective by applying min_floor first and then max_feerate_sat_vb. If max_feerate_sat_vb is configured lower than min_feerate_sat_vb / min_chained_feerate_sat_vb, this logic will silently violate the intended minimum fee floor (and still return a value below the floor). Consider validating the policy invariants (e.g., ensure max_feerate_sat_vb >= min_*) when setting the policy, or adjust the clamping logic to preserve the floor and surface a clear error when the cap is inconsistent.
| let mut effective = std::cmp::max(res, min_floor); | |
| effective = std::cmp::min(effective, self.policy.max_feerate_sat_vb); | |
| let max_cap = self.policy.max_feerate_sat_vb; | |
| let effective = if max_cap < min_floor { | |
| // Misconfigured policy: max fee rate is below the minimum floor. | |
| // Preserve the minimum floor and log the inconsistency instead of | |
| // silently returning a value below the floor. | |
| warn!( | |
| "Inconsistent fee policy: max_feerate_sat_vb ({}) < min_floor ({}); \ | |
| ignoring max cap", | |
| max_cap, | |
| min_floor | |
| ); | |
| std::cmp::max(res, min_floor) | |
| } else { | |
| // Clamp to the configured [min_floor, max_cap] range. | |
| std::cmp::min(std::cmp::max(res, min_floor), max_cap) | |
| }; |
| anyhow::bail!( | ||
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | ||
| fee_amount + recipient_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlock_value, | ||
| reveal_change_amount, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); |
There was a problem hiding this comment.
The new reveal change-output minimum enforcement introduces an error message with inconsistent formatting (missing spaces after Amount: / between clauses), which makes logs harder to scan during incidents. Consider normalizing this message format (e.g., consistent spacing/punctuation and casing) with the other sender errors.
| let inscriber = Inscriber::new(client, &self.wallet.private_key, None) | ||
| .await | ||
| .unwrap(); | ||
| .unwrap() | ||
| .with_policy(inscriber_policy); |
There was a problem hiding this comment.
This wiring layer still uses unwrap() on Inscriber::new(...). If inscriber initialization fails (e.g., invalid key/network config), the node will panic during wiring rather than returning a structured WiringError with context. Prefer propagating the error (?) and adding context (similar to the other via_btc_sender layers) so startup failures are diagnosable and don't crash the process abruptly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new configuration options and an InscriberPolicy to manage various aspects of BTC inscription sending, including minimum output values, dynamic fee rate adjustments, and pausing new sends based on pending chain depth or spendable balance. The Inscriber component now utilizes this policy for its operations. Feedback includes a suggestion to fix a minor typo in an error message and to refactor duplicated InscriberPolicy creation logic into a helper method within ViaBtcSenderConfig to improve maintainability.
|
|
||
| if reveal_change_amount < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
There was a problem hiding this comment.
There's a minor typo in the error message format string. A space is missing after Required Amount:, which could affect log parsing and readability.
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | |
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
| let inscriber_policy = InscriberPolicy { | ||
| min_inscription_output_sats: self.config.min_inscription_output_sats(), | ||
| min_change_output_sats: self.config.min_change_output_sats(), | ||
| min_feerate_sat_vb: self.config.min_feerate_sat_vb(), | ||
| min_chained_feerate_sat_vb: self.config.min_chained_feerate_sat_vb(), | ||
| max_feerate_sat_vb: self.config.max_feerate_sat_vb(), | ||
| }; |
There was a problem hiding this comment.
This block of code for creating InscriberPolicy from ViaBtcSenderConfig is duplicated across three files:
core/node/node_framework/src/implementations/layers/via_btc_sender/aggregator.rscore/node/node_framework/src/implementations/layers/via_btc_sender/manager.rscore/node/node_framework/src/implementations/layers/via_btc_sender/vote_manager.rs
To improve maintainability and reduce duplication, consider adding a helper method to ViaBtcSenderConfig to create the InscriberPolicy.
For example, in core/lib/config/src/configs/via_btc_sender.rs:
use via_btc_client::inscriber::InscriberPolicy;
// ... inside impl ViaBtcSenderConfig
pub fn to_inscriber_policy(&self) -> InscriberPolicy {
InscriberPolicy {
min_inscription_output_sats: self.min_inscription_output_sats(),
min_change_output_sats: self.min_change_output_sats(),
min_feerate_sat_vb: self.min_feerate_sat_vb(),
min_chained_feerate_sat_vb: self.min_chained_feerate_sat_vb(),
max_feerate_sat_vb: self.max_feerate_sat_vb(),
}
}Then you can simplify the wiring layers to:
let inscriber_policy = self.config.to_inscriber_policy();
|
/codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new InscriberPolicy to centralize and configure various parameters for Bitcoin inscription transactions, including minimum output amounts, minimum/maximum fee rates, maximum pending chain depth, and minimum spendable balance. These policy settings are now configurable via ViaBtcSenderConfig and are applied within the Inscriber and ViaBtcInscriptionManager components to control transaction creation, fee calculation, and sending logic. The changes also include new configuration fields, corresponding getter methods with defaults, a new InscriberPolicy struct, and logic to apply these policies in transaction construction and sending guards, along with new metrics and test cases. The review suggests centralizing default constant definitions to improve maintainability, using the clamp method for more idiomatic fee rate calculations, and fixing a minor typo in an error message.
| const DEFAULT_MIN_INSCRIPTION_OUTPUT_SATS: u64 = 600; | ||
| const DEFAULT_MIN_CHANGE_OUTPUT_SATS: u64 = 1_000; | ||
| const DEFAULT_MIN_FEERATE_SAT_VB: u64 = 8; | ||
| const DEFAULT_MIN_CHAINED_FEERATE_SAT_VB: u64 = 20; | ||
| const DEFAULT_MAX_FEERATE_SAT_VB: u64 = 80; |
There was a problem hiding this comment.
These default values are also hardcoded in core/lib/config/src/configs/via_btc_sender.rs. Duplicating these constants can lead to inconsistencies if they are updated in one place but not the other.
Consider defining these constants in a single location, for example in the zksync_config crate, and reusing them in both ViaBtcSenderConfig and InscriberPolicy::default(). This would improve maintainability.
| ); | ||
| std::cmp::max(res, min_floor) | ||
| } else { | ||
| std::cmp::min(std::cmp::max(res, min_floor), max_cap) |
There was a problem hiding this comment.
The expression std::cmp::min(std::cmp::max(res, min_floor), max_cap) can be simplified by using the clamp method, which is more idiomatic and readable.
| std::cmp::min(std::cmp::max(res, min_floor), max_cap) | |
| res.clamp(min_floor, max_cap) |
|
|
||
| if reveal_change_amount < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
There was a problem hiding this comment.
There's a minor typo in the error message string. A space is missing after Amount:. For consistency with other error messages in this file, it should be Required Amount: {:?}.
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | |
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if spendable_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); |
There was a problem hiding this comment.
spendable_balance comes from Inscriber::get_balance(), which includes pending context outputs (unconfirmed change) and therefore may not reflect spendable UTXOs. Using it for min_spendable_balance_sats can fail to block sends when UTXO selection would still produce Spendable Amount: 0. Consider computing a spendable balance from the available UTXO set or renaming the guard/config to match the semantics.
| async fn test_get_fee_rate_applies_floor_when_context_empty() { | ||
| let mut inscriber = get_mock_inscriber_and_conditions(); | ||
| inscriber.set_policy(InscriberPolicy { | ||
| min_inscription_output_sats: 600, | ||
| min_change_output_sats: 1_000, | ||
| min_feerate_sat_vb: 12, | ||
| min_chained_feerate_sat_vb: 20, | ||
| max_feerate_sat_vb: 50, | ||
| }); |
There was a problem hiding this comment.
These new get_fee_rate tests call get_mock_inscriber_and_conditions(), but that helper sets strict mockall expectations for methods like get_network, sign_ecdsa, and sign_schnorr. Since get_fee_rate() doesn't invoke those, the test will likely fail due to unmet expectations when the mock is dropped. Consider relaxing/removing those times(...) constraints in the shared helper or creating a minimal helper specifically for fee-rate tests.
| anyhow::bail!( | ||
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | ||
| fee_amount + recipient_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlock_value, | ||
| reveal_change_amount, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); |
There was a problem hiding this comment.
Typo/formatting in this error message: Required Amount:{:?} is missing a space after the colon, and the required/spendable fields are formatted differently than the commit-tx errors. Please standardize the message formatting for consistency and readability.
| if pending_chain_depth >= self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); |
There was a problem hiding this comment.
PR description says the pending-chain guard should trigger when depth exceeds the configured max, but the code blocks when pending_chain_depth >= max_pending_chain_depth(). Either update the comparison to > or adjust the config docs/description to clarify that reaching the max is considered blocked.
| if pending_chain_depth >= self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); |
There was a problem hiding this comment.
The PR description says to pause when pending chain depth exceeds the configured max, but this guard blocks when pending_chain_depth >= max_pending_chain_depth(). Please align the comparison or update the stated semantics so operators understand whether max_pending_chain_depth=3 allows depth 3 or blocks at depth 3.
| async fn test_get_fee_rate_handles_inconsistent_cap_when_context_non_empty() { | ||
| let mut inscriber = get_mock_inscriber_and_conditions(); | ||
| inscriber.context.fifo_queue.push_back(InscriptionRequest { | ||
| message: InscriptionMessage::L1BatchDAReference(L1BatchDAReferenceInput { | ||
| l1_batch_hash: zksync_basic_types::H256([0; 32]), | ||
| l1_batch_index: zksync_basic_types::L1BatchNumber(0_u32), |
There was a problem hiding this comment.
Same issue as above: this test also uses get_mock_inscriber_and_conditions(), which currently configures strict expectations for methods unrelated to get_fee_rate(). Unless those expectations are made optional (e.g., allowing 0 calls), the test will fail even if the fee-rate logic is correct.
| @@ -373,17 +421,29 @@ impl Inscriber { | |||
| let fee_amount_before_decrease = fee_amount; | |||
| fee_amount -= (fee_amount * FEE_RATE_DECREASE_COMMIT_TX) / 100; | |||
|
|
|||
| let required_amount = fee_amount + min_inscription_output; | |||
|
|
|||
| let commit_tx_change_output_value = tx_input_data | |||
| .unlocked_value | |||
| .checked_sub(fee_amount + P2TR_DUST_LIMIT) | |||
| .checked_sub(required_amount) | |||
| .ok_or_else(|| { | |||
| anyhow::anyhow!( | |||
| "Required Amount: {:?}, Spendable Amount: {:?} ", | |||
| fee_amount + P2TR_DUST_LIMIT, | |||
| required_amount, | |||
| tx_input_data.unlocked_value | |||
| ) | |||
| })?; | |||
|
|
|||
| if commit_tx_change_output_value < Amount::from_sat(self.policy.min_change_output_sats) { | |||
| anyhow::bail!( | |||
| "Required Amount: {:?}, Spendable Amount: {:?}. change output {:?} is below minimum {:?}", | |||
| required_amount + Amount::from_sat(self.policy.min_change_output_sats), | |||
| tx_input_data.unlocked_value, | |||
| commit_tx_change_output_value, | |||
| Amount::from_sat(self.policy.min_change_output_sats) | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
This change introduces new policy behavior (inscription output floor + minimum change output enforcement) but there are no unit tests covering the new boundary conditions (e.g., configured min below dust, insufficient funds for min_change_output_sats, etc.). Since this module already has tests, please add coverage for both the happy-path and the failure cases introduced by these checks.
| if reveal_change_amount < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount:{:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | ||
| fee_amount + recipient_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlock_value, | ||
| reveal_change_amount, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); | ||
| } |
There was a problem hiding this comment.
min_change_output_sats enforcement for the reveal tx is new behavior and should have a dedicated unit test to ensure it fails fast when the computed change is below the configured floor (and that it does not regress when fee calculation changes).
| if spendable_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); |
There was a problem hiding this comment.
spendable_balance is sourced from Inscriber::get_balance(), which explicitly adds outputs from the in-memory pending context (i.e., unconfirmed / not-yet-spendable change). Using this value for the min_spendable_balance_sats guard can let the sender proceed even when the actually spendable UTXO set is empty (the failure mode this guard is meant to prevent). Consider either computing a true spendable balance from the filtered UTXO set (similar to prepare_commit_tx_input), or rename the config/variable to reflect that it is not strictly spendable.
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if commit_tx_change_output_value < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount: {:?}, Spendable Amount: {:?}. change output {:?} is below minimum {:?}", | ||
| required_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlocked_value, | ||
| commit_tx_change_output_value, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new commit-tx change-output minimum (min_change_output_sats) introduces a new failure path, but there is no unit test asserting that prepare_commit_tx_output fails when the computed change output is below the policy floor. Adding a targeted test would prevent regressions in the anti-dust guardrail logic.
| if reveal_change_amount < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | ||
| fee_amount + recipient_amount + Amount::from_sat(self.policy.min_change_output_sats), | ||
| tx_input_data.unlock_value, | ||
| reveal_change_amount, | ||
| Amount::from_sat(self.policy.min_change_output_sats) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new reveal-tx change-output minimum (min_change_output_sats) introduces a new failure path, but there is no unit test asserting that prepare_reveal_tx_output fails when the computed change output is below the policy floor. Adding a targeted test would prevent regressions in the anti-dust guardrail logic.
| let via_btc_inscription_manager = | ||
| ViaBtcInscriptionManager::new(inscriber, master_pool, self.config) |
There was a problem hiding this comment.
ViaBtcInscriptionManager::new(...) is awaited and then unwrapped a couple of lines below, which can panic the node during wiring (e.g., if config/env issues make initialization fail). Since this wire() returns Result<_, WiringError>, propagate the error instead of unwrapping so startup failures are reported gracefully.
| let via_btc_inscription_manager = | ||
| ViaBtcInscriptionManager::new(inscriber, master_pool, self.config) |
There was a problem hiding this comment.
ViaBtcInscriptionManager::new(...) is awaited and then unwrapped a couple of lines below, which can panic the node during wiring (e.g., if config/env issues make initialization fail). Since this wire() returns Result<_, WiringError>, propagate the error instead of unwrapping so startup failures are reported gracefully.
| pub struct Inscriber { | ||
| client: Arc<dyn BitcoinOps>, | ||
| signer: Arc<dyn BitcoinSigner>, | ||
| context: InscriberContext, | ||
| policy: InscriberPolicy, |
There was a problem hiding this comment.
Adding the policy field to Inscriber requires updating all Inscriber { ... } struct literals. core/lib/via_btc_client/src/inscriber/test_utils.rs::get_mock_inscriber_and_conditions currently initializes Inscriber without policy, which will not compile. Initialize policy there (e.g., InscriberPolicy::default()), or refactor helpers to construct via Inscriber::new() / with_policy() to avoid future breakage when fields change.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive policy framework for the Bitcoin inscriber, enhancing control over inscription sending behavior. New configuration parameters have been added to ViaBtcSenderConfig to define minimum inscription and change output amounts, minimum and maximum fee rates (including a specific minimum for chained transactions), maximum pending chain depth, and a minimum spendable balance. These parameters are now encapsulated within an InscriberPolicy struct and integrated into the Inscriber's transaction creation and fee rate calculation logic. The ViaBtcInscriptionManager has been updated to leverage these policies, preventing new inscriptions if the pending chain depth or trusted balance falls below configured thresholds, with new metrics to track these policy-driven blocks. New test cases for fee rate application have also been added. Feedback from the review suggests addressing a minor inconsistency in error message formatting for Required Amount and Spendable Amount in prepare_reveal_tx_output for better uniformity, and improving test readability by extracting the InscriptionRequest object creation into a helper function for the test_get_fee_rate_handles_inconsistent_cap_when_context_non_empty test.
|
|
||
| if reveal_change_amount < Amount::from_sat(self.policy.min_change_output_sats) { | ||
| anyhow::bail!( | ||
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
There was a problem hiding this comment.
There's a minor inconsistency in the error message format. This message is missing a comma between 'Required Amount' and 'Spendable Amount', while a similar error message in prepare_commit_tx_output (line 445) includes it. For consistency in logging and error reporting, it would be good to make them uniform.
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | |
| "Required Amount: {:?}, Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
| inscriber.context.fifo_queue.push_back(InscriptionRequest { | ||
| message: InscriptionMessage::L1BatchDAReference(L1BatchDAReferenceInput { | ||
| l1_batch_hash: zksync_basic_types::H256([0; 32]), | ||
| l1_batch_index: zksync_basic_types::L1BatchNumber(0_u32), | ||
| da_identifier: "da_identifier_celestia".to_string(), | ||
| blob_id: "batch_temp_blob_id".to_string(), | ||
| prev_l1_batch_hash: zksync_basic_types::H256([0; 32]), | ||
| }), | ||
| inscriber_output: InscriberOutput { | ||
| commit_txid: Txid::all_zeros(), | ||
| commit_raw_tx: String::new(), | ||
| commit_tx_fee_rate: 0, | ||
| reveal_txid: Txid::all_zeros(), | ||
| reveal_raw_tx: String::new(), | ||
| reveal_tx_fee_rate: 0, | ||
| is_broadcasted: false, | ||
| }, | ||
| fee_payer_ctx: FeePayerCtx { | ||
| fee_payer_utxo_txid: Txid::all_zeros(), | ||
| fee_payer_utxo_vout: 0, | ||
| fee_payer_utxo_value: Amount::from_sat(0), | ||
| }, | ||
| commit_tx_input: CommitTxInput { | ||
| spent_utxo: vec![TxIn::default()], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The setup for this test involves creating a large, detailed InscriptionRequest object inline. This makes the test harder to read and maintain. To improve readability and reusability, consider extracting the creation of this test object into a helper function, for example fn dummy_inscription_request() -> InscriptionRequest. You could then call this function to get a test object, making the test logic clearer.
|
Addressed latest active review comments in commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | ||
| if pending_chain_depth > self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new verifier inscription broadcast due to pending chain depth guard. depth={} max={}.", | ||
| pending_chain_depth, | ||
| self.config.max_pending_chain_depth() |
There was a problem hiding this comment.
pending_chain_depth() returns usize, but it’s cast to u32 before comparison. Using usize for both sides (or an explicit checked conversion) avoids silent truncation and keeps the types consistent.
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | |
| if pending_chain_depth > self.config.max_pending_chain_depth() { | |
| METRICS.chain_guard_blocks.inc(); | |
| tracing::warn!( | |
| "Skipping new verifier inscription broadcast due to pending chain depth guard. depth={} max={}.", | |
| pending_chain_depth, | |
| self.config.max_pending_chain_depth() | |
| let pending_chain_depth = self.inscriber.pending_chain_depth(); | |
| let max_pending_chain_depth = match usize::try_from(self.config.max_pending_chain_depth()) { | |
| Ok(value) => value, | |
| Err(_) => { | |
| METRICS.chain_guard_blocks.inc(); | |
| tracing::warn!( | |
| "Skipping new verifier inscription broadcast because configured max pending chain depth does not fit into usize. max={}.", | |
| self.config.max_pending_chain_depth() | |
| ); | |
| return Ok(()); | |
| } | |
| }; | |
| if pending_chain_depth > max_pending_chain_depth { | |
| METRICS.chain_guard_blocks.inc(); | |
| tracing::warn!( | |
| "Skipping new verifier inscription broadcast due to pending chain depth guard. depth={} max={}.", | |
| pending_chain_depth, | |
| max_pending_chain_depth |
| anyhow::anyhow!( | ||
| "Required Amount:{:?} Spendable Amount: {:?} ", | ||
| "Required Amount: {:?} Spendable Amount: {:?} ", | ||
| fee_amount + recipient_amount, | ||
| tx_input_data.unlock_value | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
The new error string has inconsistent punctuation/spacing (missing comma between fields and trailing space), which makes log/error parsing harder. Please standardize the formatting of the "Required Amount" / "Spendable Amount" messages (e.g., consistent separators and no trailing whitespace).
| self.update_inscription_status(storage).await?; | ||
| self.send_new_inscription_txs(storage).await?; | ||
| let balance_with_pending_context = self.update_inscription_status(storage).await?; | ||
| let trusted_balance = self.inscriber.get_trusted_balance().await?; |
There was a problem hiding this comment.
update_inscription_status() already calls inscriber.get_balance() (which does an RPC balance call) and the loop iteration now adds a second RPC call via get_trusted_balance(). Consider adding an Inscriber API that returns both the confirmed balance and the context-adjusted balance in one call (confirmed = RPC result; adjusted = confirmed + context outputs), so the manager doesn’t need two balance RPCs per poll tick.
| let trusted_balance = self.inscriber.get_trusted_balance().await?; | |
| let trusted_balance = balance_with_pending_context; |
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | ||
| if pending_chain_depth > self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new inscription broadcast due to pending chain depth guard. depth={} max={}.", | ||
| pending_chain_depth, | ||
| self.config.max_pending_chain_depth() |
There was a problem hiding this comment.
pending_chain_depth() returns usize, but it’s cast to u32 before comparison. Even though the queue is expected to stay small, using usize for both sides (or try_into() with an explicit error) avoids a silent truncation on 64-bit platforms.
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | |
| if pending_chain_depth > self.config.max_pending_chain_depth() { | |
| METRICS.chain_guard_blocks.inc(); | |
| tracing::warn!( | |
| "Skipping new inscription broadcast due to pending chain depth guard. depth={} max={}.", | |
| pending_chain_depth, | |
| self.config.max_pending_chain_depth() | |
| let pending_chain_depth = self.inscriber.pending_chain_depth(); | |
| let max_pending_chain_depth = self.config.max_pending_chain_depth() as usize; | |
| if pending_chain_depth > max_pending_chain_depth { | |
| METRICS.chain_guard_blocks.inc(); | |
| tracing::warn!( | |
| "Skipping new inscription broadcast due to pending chain depth guard. depth={} max={}.", | |
| pending_chain_depth, | |
| max_pending_chain_depth |
| let balance_with_pending_context = self.update_inscription_status_or_resend(storage).await?; | ||
| let trusted_balance = self.inscriber.get_trusted_balance().await?; | ||
| self.send_new_inscription_txs(storage, balance_with_pending_context, trusted_balance) | ||
| .await?; |
There was a problem hiding this comment.
update_inscription_status_or_resend() already calls inscriber.get_balance() (RPC + context adjustment) and the loop iteration now adds a second RPC call via get_trusted_balance(). Consider an Inscriber API that returns both confirmed and context-adjusted balances using a single RPC call, to avoid doubling balance RPC traffic in the verifier sender loop.
| if trusted_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new verifier inscription broadcast due to low trusted balance guard. trusted={} (with_pending={}) min={}", | ||
| trusted_balance, | ||
| balance_with_pending_context, | ||
| self.config.min_spendable_balance_sats() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The new low-trusted-balance guard isn’t covered by tests. Please add a verifier sender test that sets a low mock confirmed balance and a higher min_spendable_balance_sats, and asserts the manager skips broadcasting and increments chain_guard_blocks.
| pub fn min_inscription_output_sats(&self) -> u64 { | ||
| self.min_inscription_output_sats.unwrap_or(600) | ||
| } | ||
|
|
||
| pub fn min_change_output_sats(&self) -> u64 { | ||
| self.min_change_output_sats.unwrap_or(1_000) | ||
| } | ||
|
|
||
| pub fn min_feerate_sat_vb(&self) -> u64 { | ||
| self.min_feerate_sat_vb.unwrap_or(8) | ||
| } | ||
|
|
||
| pub fn min_chained_feerate_sat_vb(&self) -> u64 { | ||
| self.min_chained_feerate_sat_vb.unwrap_or(20) | ||
| } | ||
|
|
||
| pub fn max_feerate_sat_vb(&self) -> u64 { | ||
| self.max_feerate_sat_vb.unwrap_or(80) | ||
| } | ||
|
|
||
| pub fn max_pending_chain_depth(&self) -> u32 { | ||
| self.max_pending_chain_depth.unwrap_or(3) | ||
| } | ||
|
|
||
| pub fn min_spendable_balance_sats(&self) -> u64 { | ||
| self.min_spendable_balance_sats.unwrap_or(2_000) | ||
| } |
There was a problem hiding this comment.
These default policy values are now duplicated across config getters, env defaults, and InscriberPolicy::default(). To reduce drift risk, consider defining the defaults in a single place (e.g., shared constants) and referencing them from both the config getters and the inscriber policy defaults.
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | ||
| if pending_chain_depth > self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new inscription broadcast due to pending chain depth guard. depth={} max={}.", | ||
| pending_chain_depth, | ||
| self.config.max_pending_chain_depth() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The new pending-chain-depth guard is behavior-changing but currently isn’t covered by the existing manager tests. Please add a test case that ensures no new inscriptions are broadcast when pending_chain_depth exceeds max_pending_chain_depth (and that chain_guard_blocks increments). This likely requires enhancing the mock inscriber helper to start with a non-empty persisted context.
| if trusted_balance < self.config.min_spendable_balance_sats() as u128 { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new inscription broadcast due to low trusted balance guard. trusted={} (with_pending={}) min={}", | ||
| trusted_balance, | ||
| balance_with_pending_context, | ||
| self.config.min_spendable_balance_sats() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The new low-trusted-balance guard is behavior-changing but currently isn’t covered by the existing manager tests. Please add a test that sets a low mock confirmed balance and a higher min_spendable_balance_sats and asserts the manager skips broadcasting and increments chain_guard_blocks.
| let pending_chain_depth = self.inscriber.pending_chain_depth() as u32; | ||
| if pending_chain_depth > self.config.max_pending_chain_depth() { | ||
| METRICS.chain_guard_blocks.inc(); | ||
| tracing::warn!( | ||
| "Skipping new verifier inscription broadcast due to pending chain depth guard. depth={} max={}.", | ||
| pending_chain_depth, | ||
| self.config.max_pending_chain_depth() | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The new pending-chain-depth guard isn’t covered by tests. Please add a verifier sender test that ensures no new inscriptions are broadcast when pending_chain_depth exceeds max_pending_chain_depth (and that chain_guard_blocks increments).
|
Pushed another follow-up commit |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive policy controls for the BTC inscriber, allowing configuration of minimum inscription and change output sizes, fee rate floors and caps (including a higher floor for chained transactions), maximum pending chain depth, and a minimum spendable balance threshold. These policies are integrated into the Inscriber and ViaBtcInscriptionManager to manage transaction creation and prevent undesirable outputs or excessive chain depth. New metrics for tracking policy guard activations and updated configuration options are also included. A high-severity issue was identified in the get_balances function, where the balance_with_pending_context calculation is incorrect, leading to an inflated balance by double-counting change outputs from pending transactions. This should be corrected by reusing the logic from prepare_commit_tx_input to accurately sum spendable UTXOs.
| let mut balance_with_pending_context = trusted_balance; | ||
|
|
||
| // Include the transactions in mempool when calculate the balance | ||
| for inscription in &self.context.fifo_queue { | ||
| let tx: Transaction = deserialize_hex(&inscription.inscriber_output.reveal_raw_tx)?; | ||
|
|
||
| tx.output.iter().for_each(|output| { | ||
| if output.script_pubkey == address_ref.script_pubkey() { | ||
| balance += output.value.to_sat() as u128; | ||
| balance_with_pending_context += output.value.to_sat() as u128; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current logic for calculating balance_with_pending_context is incorrect and leads to a significantly inflated balance. It sums the trusted_balance with the change outputs from all pending transactions in the queue. In a transaction chain, only the final output is unspent, so this method double-counts (and more) the value.
The prepare_commit_tx_input function already contains the complex logic to determine the set of spendable UTXOs, both confirmed and unconfirmed. The sum of the values of these UTXOs (unlocked_value) represents the true total balance.
I suggest reusing this logic to get an accurate balance_with_pending_context.
| let mut balance_with_pending_context = trusted_balance; | |
| // Include the transactions in mempool when calculate the balance | |
| for inscription in &self.context.fifo_queue { | |
| let tx: Transaction = deserialize_hex(&inscription.inscriber_output.reveal_raw_tx)?; | |
| tx.output.iter().for_each(|output| { | |
| if output.script_pubkey == address_ref.script_pubkey() { | |
| balance += output.value.to_sat() as u128; | |
| balance_with_pending_context += output.value.to_sat() as u128; | |
| } | |
| }); | |
| } | |
| let commit_tx_input_info = self.prepare_commit_tx_input().await?; | |
| let balance_with_pending_context = commit_tx_input_info.unlocked_value.to_sat() as u128; |
|
Addressed latest Gemini high-priority comment in commit e65e12a:\n\n- fixed get_balances() to avoid overcounting pending-chain outputs\n- now computes �alance_with_pending_context by reusing prepare_commit_tx_input() and taking unlocked_value\n- keeps rusted_balance from RPC as-is\n\nThis aligns balance accounting with actual spendable UTXO selection logic used for commit preparation. |
|
/copilot review |
|
Thank you for addressing the high-priority comment regarding balance accounting. The changes in commit |
|
closing in favor of 333 (all the good stuff has been taken. Thanksss) |
Summary
This PR adds production guardrails to the BTC sender path to prevent stalls caused by pending UTXO chains, borderline tiny outputs, and low-fee chaining behavior.
Motivation
Observed runtime behavior:
Spendable Amount: 0 SATmax_txs_in_flight = 1This change is intended to make slow-confirmation conditions degrade throughput instead of hard-stalling the pipeline.
What changed
1) New configurable policy fields (
ViaBtcSenderConfig)min_inscription_output_satsmin_change_output_satsmin_feerate_sat_vbmin_chained_feerate_sat_vbmax_feerate_sat_vbmax_pending_chain_depthmin_spendable_balance_satsDefaults and getters were added, plus base env config in:
etc/env/base/via_btc_sender.toml2) Inscriber policy wiring
InscriberPolicyinvia_btc_client.3) Anti-dust / output floor enforcement
max(dust_limit, configured_min)).min_change_output_sats.4) Fee hardening
5) Runtime send guardrails (core + verifier)
Before sending new inscriptions:
6) Observability
chain_guard_blockscounter metric (core + verifier) for policy-blocked send attempts.Expected impact
Out of scope / follow-up
Not included in this PR:
These can be follow-up PRs.
Rollout notes
Suggested initial config (testnet canary):
min_inscription_output_sats = 600min_change_output_sats = 1000min_feerate_sat_vb = 8min_chained_feerate_sat_vb = 20max_feerate_sat_vb = 80max_pending_chain_depth = 3min_spendable_balance_sats = 2000Monitor:
chain_guard_blocksNotes
This PR is complementary to #333: