-
Notifications
You must be signed in to change notification settings - Fork 3
feat(via_btc_sender): harden spendability, fee floors, and anti-dust policy #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
30770d1
355b15f
814fa31
01e3cf7
0eec8bd
e65e12a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -64,12 +64,39 @@ const BROADCAST_RETRY_COUNT: u32 = 3; | |||||
| // https://bitcoin.stackexchange.com/questions/10986/what-is-meant-by-bitcoin-dust | ||||||
| // https://bitcointalk.org/index.php?topic=5453107.msg62262343#msg62262343 | ||||||
| const P2TR_DUST_LIMIT: Amount = Amount::from_sat(330); | ||||||
| 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; | ||||||
|
Comment on lines
+67
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These default values are also hardcoded in Consider defining these constants in a single location, for example in the |
||||||
|
|
||||||
| #[derive(Debug, Clone)] | ||||||
| pub struct InscriberPolicy { | ||||||
| pub min_inscription_output_sats: u64, | ||||||
| pub min_change_output_sats: u64, | ||||||
| pub min_feerate_sat_vb: u64, | ||||||
| pub min_chained_feerate_sat_vb: u64, | ||||||
| pub max_feerate_sat_vb: u64, | ||||||
| } | ||||||
|
|
||||||
| impl Default for InscriberPolicy { | ||||||
| fn default() -> Self { | ||||||
| Self { | ||||||
| min_inscription_output_sats: DEFAULT_MIN_INSCRIPTION_OUTPUT_SATS, | ||||||
| min_change_output_sats: DEFAULT_MIN_CHANGE_OUTPUT_SATS, | ||||||
| min_feerate_sat_vb: DEFAULT_MIN_FEERATE_SAT_VB, | ||||||
| min_chained_feerate_sat_vb: DEFAULT_MIN_CHAINED_FEERATE_SAT_VB, | ||||||
| max_feerate_sat_vb: DEFAULT_MAX_FEERATE_SAT_VB, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug)] | ||||||
| pub struct Inscriber { | ||||||
| client: Arc<dyn BitcoinOps>, | ||||||
| signer: Arc<dyn BitcoinSigner>, | ||||||
| context: InscriberContext, | ||||||
| policy: InscriberPolicy, | ||||||
|
Comment on lines
95
to
+99
|
||||||
| } | ||||||
|
|
||||||
| impl Inscriber { | ||||||
|
|
@@ -90,6 +117,7 @@ impl Inscriber { | |||||
| client, | ||||||
| signer, | ||||||
| context, | ||||||
| policy: InscriberPolicy::default(), | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -114,6 +142,25 @@ impl Inscriber { | |||||
| Ok(balance) | ||||||
| } | ||||||
|
|
||||||
| #[instrument(skip(self), target = "bitcoin_inscriber")] | ||||||
| pub async fn get_trusted_balance(&self) -> Result<u128> { | ||||||
| let address_ref = &self.signer.get_p2wpkh_address()?; | ||||||
| Ok(self.client.get_balance(address_ref).await?) | ||||||
| } | ||||||
|
|
||||||
| pub fn with_policy(mut self, policy: InscriberPolicy) -> Self { | ||||||
| self.policy = policy; | ||||||
| self | ||||||
| } | ||||||
|
|
||||||
| pub fn set_policy(&mut self, policy: InscriberPolicy) { | ||||||
| self.policy = policy; | ||||||
| } | ||||||
|
|
||||||
| pub fn pending_chain_depth(&self) -> usize { | ||||||
| self.context.fifo_queue.len() | ||||||
| } | ||||||
|
|
||||||
| #[instrument(skip(self, input), target = "bitcoin_inscriber")] | ||||||
| pub async fn prepare_inscribe( | ||||||
| &mut self, | ||||||
|
|
@@ -355,8 +402,15 @@ impl Inscriber { | |||||
| inscription_pubkey: ScriptBuf, | ||||||
| ) -> Result<CommitTxOutputRes> { | ||||||
| debug!("Preparing commit transaction output"); | ||||||
| let min_inscription_output = Amount::from_sat( | ||||||
| std::cmp::max( | ||||||
| self.policy.min_inscription_output_sats, | ||||||
| P2TR_DUST_LIMIT.to_sat(), | ||||||
| ), | ||||||
| ); | ||||||
|
|
||||||
| let inscription_commitment_output = TxOut { | ||||||
| value: P2TR_DUST_LIMIT, | ||||||
| value: min_inscription_output, | ||||||
| script_pubkey: inscription_pubkey, | ||||||
| }; | ||||||
|
|
||||||
|
|
@@ -373,17 +427,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) | ||||||
| ); | ||||||
| } | ||||||
|
Comment on lines
+442
to
+450
|
||||||
|
|
||||||
| let commit_tx_change_output = TxOut { | ||||||
| value: commit_tx_change_output_value, | ||||||
| script_pubkey: self.signer.get_p2wpkh_script_pubkey().clone(), | ||||||
|
|
@@ -405,8 +471,26 @@ impl Inscriber { | |||||
| 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 max_cap = self.policy.max_feerate_sat_vb; | ||||||
| let effective = if max_cap < min_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 { | ||||||
| std::cmp::min(std::cmp::max(res, min_floor), max_cap) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| }; | ||||||
|
|
||||||
| debug!("Fee rate obtained: {}, effective: {}", res, effective); | ||||||
| Ok(std::cmp::max(effective, 1)) | ||||||
|
Comment on lines
470
to
+492
|
||||||
| } | ||||||
|
|
||||||
| #[instrument(skip(self, input, output), target = "bitcoin_inscriber")] | ||||||
|
|
@@ -609,12 +693,22 @@ impl Inscriber { | |||||
| .checked_sub(fee_amount + recipient_amount) | ||||||
| .ok_or_else(|| { | ||||||
| anyhow::anyhow!( | ||||||
| "Required Amount:{:?} Spendable Amount: {:?} ", | ||||||
| "Required Amount: {:?} Spendable Amount: {:?} ", | ||||||
| fee_amount + recipient_amount, | ||||||
| tx_input_data.unlock_value | ||||||
| ) | ||||||
| })?; | ||||||
|
Comment on lines
694
to
699
|
||||||
|
|
||||||
| 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 {:?}", | ||||||
|
||||||
| "Required Amount: {:?} Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", | |
| "Required Amount: {:?}, Spendable Amount: {:?}. reveal change output {:?} is below minimum {:?}", |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,15 @@ | ||
| use anyhow::Context; | ||
| use via_btc_client::inscriber::Inscriber; | ||
| use via_btc_sender::btc_inscription_aggregator::ViaBtcInscriptionAggregator; | ||
| use zksync_config::{configs::via_wallets::ViaWallet, ViaBtcSenderConfig}; | ||
|
|
||
| use crate::{ | ||
| implementations::resources::{ | ||
| pools::{MasterPool, PoolResource}, | ||
| via_btc_client::BtcClientResource, | ||
| implementations::{ | ||
| layers::via_btc_sender::policy::build_inscriber_policy, | ||
| resources::{ | ||
| pools::{MasterPool, PoolResource}, | ||
| via_btc_client::BtcClientResource, | ||
| }, | ||
| }, | ||
| service::StopReceiver, | ||
| task::{Task, TaskId}, | ||
|
|
@@ -66,9 +70,12 @@ impl WiringLayer for ViaBtcInscriptionAggregatorLayer { | |
| let master_pool = input.master_pool.get().await.unwrap(); | ||
| let client = input.btc_client_resource.btc_sender.unwrap(); | ||
|
|
||
| let inscriber_policy = build_inscriber_policy(&self.config); | ||
|
|
||
| let inscriber = Inscriber::new(client, &self.wallet.private_key, None) | ||
| .await | ||
| .unwrap(); | ||
| .with_context(|| "Error init inscriber")? | ||
| .with_policy(inscriber_policy); | ||
|
Comment on lines
75
to
+78
|
||
|
|
||
| let via_btc_inscription_aggregator = | ||
| ViaBtcInscriptionAggregator::new(inscriber, master_pool, self.config).await?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.