feat(via_btc_client): implement Largest-First UTXO selection#333
feat(via_btc_client): implement Largest-First UTXO selection#333romanornr wants to merge 20 commits intovianetwork:mainfrom
Conversation
Summary of ChangesHello @romanornr, 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 a significant optimization to the Bitcoin client's UTXO selection process by implementing a "Largest-First" algorithm. This new strategy aims to enhance transaction efficiency by selecting fewer, larger unspent transaction outputs, thereby potentially reducing transaction costs. Crucially, it also incorporates mechanisms to ensure that there is always a sufficient change output to fund subsequent dependent transactions, such as the Reveal Transaction, addressing a potential funding challenge. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a 'Largest-First' UTXO selection strategy, which is a good optimization over the previous greedy approach. The new logic sorts UTXOs by value and selects the largest ones until the target amount is reached, which should lead to more efficient transaction construction. The changes are well-structured and include relevant unit tests. I've identified a couple of minor areas for improvement in the new select_utxos function to enhance performance and simplify the logic.
| let mut total_value = Amount::ZERO; | ||
|
|
||
| for (outpoint, txout) in utxos { | ||
| selected.push((outpoint, txout.clone())); |
There was a problem hiding this comment.
The for loop consumes utxos and provides ownership of each (outpoint, txout) tuple. This means txout is an owned TxOut instance, and cloning it via txout.clone() before pushing to the selected vector is unnecessary and introduces a small performance overhead. You can move txout directly.
| selected.push((outpoint, txout.clone())); | |
| selected.push((outpoint, txout)); |
| if total_value >= final_target { | ||
| return Ok((selected, total_value)); | ||
| } |
There was a problem hiding this comment.
This if condition is redundant. The code at this point is only reached if the loop completes without meeting the target amount. The final_target is calculated using the same logic as the target in the last iteration of the loop. Therefore, total_value >= final_target will always evaluate to false. You can simplify the code by removing this check and allowing the function to proceed directly to returning the Insufficient funds error.
romanornr
force-pushed
the
largest-first
branch
from
c5c7f89 to
8ee08d5
Compare
romanornr
force-pushed
the
largest-first
branch
from
8ee08d5 to
e2a788c
Compare
0xatomFusion
changed the title
- Remove redundant `if total_value >= final_target` check after loop (condition is always false since loop already checks same condition) - Remove unnecessary clone by extracting value before moving txout
|
@0xatomFusion Gemini issues resolved in the second commit. 1 was not as straight forward as Gemini pointed out Test are passing |
There was a problem hiding this comment.
Pull request overview
Implements a Largest-First UTXO selection strategy in via_btc_client to reduce the number of inputs used for the Commit TX, aiming to improve efficiency over the prior “add everything” approach while preserving sufficient change for the Reveal TX.
Changes:
- Added Largest-First UTXO selection with a dynamic target calculation based on estimated Commit TX fees + dust + change buffer.
- Updated
prepare_commit_tx_inputto select a subset of UTXOs rather than consuming all available ones. - Added unit tests for UTXO selection behavior and target calculation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Limit UTXOs to consider for performance | ||
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); | ||
|
|
There was a problem hiding this comment.
utxos.truncate(MAX_UTXOS_TO_CONSIDER) can produce a false "Insufficient funds" error when the wallet has >100 UTXOs: if the sum of the top 100 UTXOs is below the target but the overall balance is sufficient, selection will fail even though a valid solution exists. Consider removing the hard cap, or implementing a fallback that expands the candidate set when the target isn't met (e.g., progressively increase the limit / scan all UTXOs in worst case).
| // Limit UTXOs to consider for performance | |
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); |
| /// Minimum buffer for change output to ensure Reveal TX can be funded. | ||
| /// This accounts for Reveal TX fees plus safety margin. | ||
| const MIN_CHANGE_BUFFER: Amount = Amount::from_sat(10_000); | ||
|
|
||
| /// Maximum number of UTXOs to consider for selection (performance reasoning) | ||
| const MAX_UTXOS_TO_CONSIDER: usize = 100; | ||
|
|
||
| /// Calculates the minimum target amount needed for UTXO selection. | ||
| /// This includes: Commit TX fee (estimated), P2TR dust output, and minimum change buffer. |
There was a problem hiding this comment.
The comment on MIN_CHANGE_BUFFER / calculate_selection_target says it “accounts for Reveal TX fees”, but the buffer is a fixed 10_000 sats and does not scale with fee rate, inscription script size, pending-tx incentive, or optional recipient output (all of which directly affect prepare_reveal_tx_output fee). This can still lead to Reveal TX funding failures at higher fee rates / larger inscriptions. Either (a) compute a buffer from an estimated Reveal TX fee (likely requires passing inscription_data.script_size and recipient info into selection), or (b) adjust the comment to avoid implying the Reveal TX fees are actually covered by this constant.
| /// Minimum buffer for change output to ensure Reveal TX can be funded. | |
| /// This accounts for Reveal TX fees plus safety margin. | |
| const MIN_CHANGE_BUFFER: Amount = Amount::from_sat(10_000); | |
| /// Maximum number of UTXOs to consider for selection (performance reasoning) | |
| const MAX_UTXOS_TO_CONSIDER: usize = 100; | |
| /// Calculates the minimum target amount needed for UTXO selection. | |
| /// This includes: Commit TX fee (estimated), P2TR dust output, and minimum change buffer. | |
| /// Minimum fixed buffer for change output used as a safety margin when funding | |
| /// subsequent transactions (e.g. Reveal TX). Note: this does **not** dynamically | |
| /// scale with fee rate or inscription size and therefore does not guarantee | |
| /// Reveal TX fees are fully covered under all conditions. | |
| const MIN_CHANGE_BUFFER: Amount = Amount::from_sat(10_000); | |
| /// Maximum number of UTXOs to consider for selection (performance reasoning) | |
| const MAX_UTXOS_TO_CONSIDER: usize = 100; | |
| /// Calculates the minimum target amount needed for UTXO selection. | |
| /// This includes: Commit TX fee (estimated), P2TR dust output, and a static | |
| /// safety buffer (`MIN_CHANGE_BUFFER`). It does **not** explicitly model | |
| /// Reveal TX fees; those must still be affordable from the remaining funds. |
| // Get fee rate for selection calculation | ||
| let fee_rate = self.get_fee_rate().await?; | ||
|
|
||
| // Select optimal UTXOs using the Largest-First selection algorithm | ||
| let (selected_utxos, unlocked_value) = select_utxos(utxos, fee_rate)?; |
There was a problem hiding this comment.
prepare_commit_tx_input selects UTXOs using a fee rate fetched here, but prepare_commit_tx_output fetches the fee rate again later. If the fee rate increases between these calls, selection may succeed but output construction can still fail (or produce much smaller change than intended) because the actual commit fee used later is higher than the fee assumed during selection. Consider fetching the fee rate once and threading it through (e.g., store it in CommitTxInputRes or pass it into prepare_commit_tx_output) to keep selection and fee calculation consistent.
|
I pushed a follow-up series on top of based on what we learned from the Via main testnet incident. New commits added What these changes add
Why this matters From the real main testnet incident:
So my current view is:
What still remains as follow-up work
But this PR is now much closer to the actual failure mode we observed in production-like conditions. Current note about local validation
One small note |
|
Follow-up update: I pushed additional changes on top of New commits now on the branch:
What the latest commit adds
Why I think this matters
So I think the right shape is:
Local validation
What I still think remains for follow-up
|
|
One more suggested follow-up after the latest sender-guardrail changes: I still think the next highest-value improvement is adaptive capped fee escalation with an RBF-first policy. Reasoning from the incident:
What I would add next
I think that would complement this PR well:
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Calculates the minimum target amount needed for UTXO selection. | ||
| /// This includes: Commit TX fee (estimated), a safe inscription output amount, and a | ||
| /// minimum change budget that stays reusable for follow-up transactions. | ||
| fn calculate_selection_target(input_count: u32, fee_rate: u64) -> Result<Amount> { | ||
| let commit_fee = InscriberFeeCalculator::estimate_fee( | ||
| input_count, | ||
| COMMIT_TX_P2TR_INPUT_COUNT, | ||
| COMMIT_TX_P2WPKH_OUTPUT_COUNT, | ||
| COMMIT_TX_P2TR_OUTPUT_COUNT, | ||
| vec![], | ||
| fee_rate, | ||
| )?; | ||
|
|
||
| let minimum_change_budget = std::cmp::max(MIN_CHANGE_BUFFER, MIN_CHANGE_OUTPUT); | ||
| let target = commit_fee | ||
| .checked_add(MIN_INSCRIPTION_OUTPUT) | ||
| .and_then(|v| v.checked_add(minimum_change_budget)) | ||
| .ok_or_else(|| anyhow::anyhow!("Target amount overflow"))?; | ||
| Ok(target) |
There was a problem hiding this comment.
calculate_selection_target() hard-codes MIN_INSCRIPTION_OUTPUT / MIN_CHANGE_OUTPUT constants, but commit/reveal output construction uses self.policy.min_inscription_output / self.policy.min_change_output. If the policy/config increases these mins, UTXO selection can under-estimate the required amount and later fail during output construction. Consider passing the policy into calculate_selection_target() / select_utxos() (or making them Inscriber methods) and basing the target on the configured policy values (plus the change buffer).
| utxos.push((reveal_change_output, reveal_txout)); | ||
| } | ||
| } else if context_queue_len > 0 { | ||
| warn!( |
There was a problem hiding this comment.
The warn!("Skipping reuse of unconfirmed context change output...") branch will execute whenever there is pending context depth and allow_unconfirmed_change_reuse is disabled (the default). This can generate continuous warning-level noise in normal operation. Consider lowering this to debug/trace or rate-limiting so warnings are reserved for truly unexpected conditions.
| warn!( | |
| debug!( |
|
|
||
| // Verify largest first ordering - first selected should be 100k sats | ||
| assert_eq!(selected[0].1.value, Amount::from_sat(100_000)); | ||
| assert!(total >= Amount::from_sat(100_000)); |
There was a problem hiding this comment.
test_select_utxos_largest_first() only asserts that the first selected UTXO is the largest and that total >= 100_000, but it doesn't verify that selection stops as soon as the target is met (i.e., that it doesn't unnecessarily select extra inputs). Consider asserting selected.len() == 1 for this input set, or comparing total against calculate_selection_target(1, fee_rate) and asserting that adding the next-largest UTXO would be unnecessary.
| assert!(total >= Amount::from_sat(100_000)); | |
| // Ensure selection stops as soon as the target is met (no unnecessary inputs) | |
| assert_eq!(selected.len(), 1); | |
| assert_eq!(total, Amount::from_sat(100_000)); |
| "Stuck inscription {} for request {} (type {}) exceeded escalation interval {}s; attempting replacement/rebroadcast", | ||
| last_inscription_history.reveal_tx_id, | ||
| inscription_id, | ||
| request.request_type, | ||
| self.config.escalation_interval_sec() | ||
| ); | ||
| METRICS.rbf_retries.inc(); |
There was a problem hiding this comment.
The log message/metric imply an RBF fee-bump ("replacement/rebroadcast", rbf_retries), but the code path calls send_inscription_tx(), which creates and broadcasts a fresh commit+reveal pair rather than re-broadcasting the previously signed txs or deliberately constructing an RBF replacement spending the same inputs with a higher fee. This is potentially misleading operationally; consider either (a) renaming the metric/message to reflect a generic retry, or (b) implementing actual rebroadcast/RBF using the last stored signed txs / original inputs and a higher fee rate.
| "Stuck inscription {} for request {} (type {}) exceeded escalation interval {}s; attempting replacement/rebroadcast", | |
| last_inscription_history.reveal_tx_id, | |
| inscription_id, | |
| request.request_type, | |
| self.config.escalation_interval_sec() | |
| ); | |
| METRICS.rbf_retries.inc(); | |
| "Stuck inscription {} for request {} (type {}) exceeded escalation interval {}s; retrying by sending a new commit+reveal transaction", | |
| last_inscription_history.reveal_tx_id, | |
| inscription_id, | |
| request.request_type, | |
| self.config.escalation_interval_sec() | |
| ); | |
| // Note: this path currently sends a fresh commit+reveal transaction | |
| // rather than performing an on-chain RBF replacement of the prior tx. |
| /// Error when broadcast a transaction. | ||
| pub l1_transient_errors: Counter, | ||
|
|
||
| /// Number of replacement / fee-bump attempts triggered for stuck inscriptions. |
There was a problem hiding this comment.
The rbf_retries metric is documented as "replacement / fee-bump attempts", but the current retry path in ViaBtcInscriptionManager creates a new inscription rather than performing an RBF replacement or re-broadcasting the same signed tx. Consider renaming the metric (or updating the retry implementation) so the metric semantics match what is actually happening.
| /// Number of replacement / fee-bump attempts triggered for stuck inscriptions. | |
| /// Number of retry attempts triggered for stuck inscriptions (may create new inscriptions | |
| /// rather than using RBF/fee-bump). |
| let policy = InscriberPolicy { | ||
| min_inscription_output: bitcoin::Amount::from_sat(self.config.min_inscription_output_sats()), | ||
| min_change_output: bitcoin::Amount::from_sat(self.config.min_change_output_sats()), | ||
| allow_unconfirmed_change_reuse: self.config.allow_unconfirmed_change_reuse(), |
There was a problem hiding this comment.
This wiring code constructs InscriberPolicy using bitcoin::Amount::from_sat(...), but zksync_node_framework doesn't currently declare a direct dependency on the bitcoin crate (transitive deps can't be used directly). This will fail to compile unless bitcoin is added to this crate's Cargo.toml or the conversion is moved into via_btc_client (e.g., a helper that accepts sats as u64).
| // Limit UTXOs to consider for performance | ||
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); | ||
|
|
There was a problem hiding this comment.
Truncating the candidate UTXO list to MAX_UTXOS_TO_CONSIDER can produce a false "Insufficient funds" error when the wallet has >100 UTXOs and the (discarded) smaller UTXOs are needed to reach the target. If you need a performance guard, consider only applying a limit after confirming the remaining UTXOs can still meet the target, or iterating all UTXOs but short-circuiting once the target is met.
| // Limit UTXOs to consider for performance | |
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); |
| // Get fee rate for selection calculation | ||
| let fee_rate = self.get_fee_rate(self.context.fifo_queue.len()).await?; | ||
|
|
||
| // Select optimal UTXOs using the Largest-First selection algorithm | ||
| let (selected_utxos, unlocked_value) = select_utxos(utxos, fee_rate)?; | ||
|
|
There was a problem hiding this comment.
prepare_commit_tx_input() now fetches a fee rate for UTXO selection, but prepare_commit_tx_output() and prepare_reveal_tx_output() fetch again later. This adds extra RPC calls and can make selection inconsistent with fee estimation if the network fee changes between calls (selection may pick too few inputs and output construction can then fail). Consider computing the effective fee rate once per inscription flow and threading it through (e.g., store it in CommitTxInputRes / InscriberInfo) so selection + fee calculations use the same value.
| let escalated = floor.saturating_add(self.policy.escalation_step_sat_vb.saturating_mul(pending_chain_depth as u64)); | ||
| let effective = std::cmp::min( | ||
| self.policy.max_feerate_sat_vb, | ||
| std::cmp::max(std::cmp::max(network_rate, floor), escalated), | ||
| ); | ||
| debug!("Fee rate obtained: network={}, pending_depth={}, effective={}", network_rate, pending_chain_depth, effective); |
There was a problem hiding this comment.
get_fee_rate() clamps using min(self.policy.max_feerate_sat_vb, max(network_rate, floor, escalated)). If max_feerate_sat_vb is configured below the chosen floor, the returned fee rate will end up below the intended minimum. Consider validating policy/config invariants (e.g., max >= min floors) at startup, or clamping with effective = max(floor, min(max, ...)) / treating max<floor as a config error.
| let escalated = floor.saturating_add(self.policy.escalation_step_sat_vb.saturating_mul(pending_chain_depth as u64)); | |
| let effective = std::cmp::min( | |
| self.policy.max_feerate_sat_vb, | |
| std::cmp::max(std::cmp::max(network_rate, floor), escalated), | |
| ); | |
| debug!("Fee rate obtained: network={}, pending_depth={}, effective={}", network_rate, pending_chain_depth, effective); | |
| // Ensure configuration invariants: the maximum fee rate must not be below the chosen floor. | |
| if self.policy.max_feerate_sat_vb < floor { | |
| anyhow::bail!( | |
| "Invalid fee policy: max_feerate_sat_vb ({}) is lower than the required floor ({})", | |
| self.policy.max_feerate_sat_vb, | |
| floor | |
| ); | |
| } | |
| let escalated = floor.saturating_add( | |
| self.policy | |
| .escalation_step_sat_vb | |
| .saturating_mul(pending_chain_depth as u64), | |
| ); | |
| let candidate = std::cmp::max(std::cmp::max(network_rate, floor), escalated); | |
| let effective = std::cmp::max( | |
| floor, | |
| std::cmp::min(self.policy.max_feerate_sat_vb, candidate), | |
| ); | |
| debug!( | |
| "Fee rate obtained: network={}, pending_depth={}, floor={}, max_feerate={}, effective={}", | |
| network_rate, | |
| pending_chain_depth, | |
| floor, | |
| self.policy.max_feerate_sat_vb, | |
| effective | |
| ); |
| let policy = InscriberPolicy { | ||
| min_inscription_output: bitcoin::Amount::from_sat(self.config.min_inscription_output_sats()), | ||
| min_change_output: bitcoin::Amount::from_sat(self.config.min_change_output_sats()), | ||
| allow_unconfirmed_change_reuse: self.config.allow_unconfirmed_change_reuse(), |
There was a problem hiding this comment.
This wiring code constructs InscriberPolicy using bitcoin::Amount::from_sat(...), but zksync_node_framework doesn't currently declare a direct dependency on the bitcoin crate (transitive deps can't be used directly). This will fail to compile unless bitcoin is added to this crate's Cargo.toml or the conversion is moved into via_btc_client (e.g., a helper that accepts sats as u64).
There was a problem hiding this comment.
Code Review
This pull request introduces a new InscriberPolicy to configure Bitcoin inscription transaction parameters, including minimum output values, feerates (normal, chained, and escalated), and the option to reuse unconfirmed change outputs. The UTXO selection logic has been refactored to use a largest-first strategy, dynamically calculating fees and ensuring sufficient funds. Additionally, a mechanism for detecting and re-broadcasting stuck inscriptions based on a configurable escalation interval has been implemented, along with a new metric for RBF retries. The review suggests improving error handling by propagating errors instead of unwrapping during Inscriber initialization and making a test assertion more precise for calculate_selection_target.
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Using .unwrap() here can cause the service to panic if Inscriber::new_with_policy returns an error (e.g., due to an invalid private key in the configuration). It's safer to propagate the error using the ? operator, which will be handled by the WiringLayer's wire method. This will lead to a cleaner shutdown and more informative logs on failure.
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) | |
| .await | |
| .unwrap(); | |
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) | |
| .await?; |
| assert!(target.to_sat() > 10_600); | ||
| assert!(target.to_sat() < 1_000_000); |
There was a problem hiding this comment.
The assertions in this test are a bit loose. For better test precision and to guard against future regressions if the fee calculation logic changes, consider using a more specific assertion. Based on the current fee estimation logic, the target value should be exactly 12,170 sats for the given inputs.
// Target = commit fee (1570) + safe inscription output (600) + reusable change budget (10_000).
let expected_target_sats = 12_170;
assert_eq!(target.to_sat(), expected_target_sats);There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
core/lib/via_btc_client/src/inscriber/mod.rs:839
get_fee_rate()now escalates the sat/vB based onpending_chain_depth, butprepare_reveal_tx_output()still applies an additional pending-depth multiplier (FEE_RATE_INCREASE_PER_PENDING_TX * pending_tx_in_context) on top of that. This compounds fee escalation twice for the same signal (queue depth) and may lead to significant overpay. If the new policy is intended to replace the old pending-depth multiplier, consider removing or gating thetxs_stuck_factorincrease, or documenting why both are required.
let pending_tx_in_context = self.context.fifo_queue.len();
let fee_rate = tx_input_data.fee_rate;
let mut reveal_tx_p2wpkh_output_count = REVEAL_TX_P2WPKH_OUTPUT_COUNT;
let mut reveal_tx_p2tr_output_count = REVEAL_TX_P2TR_OUTPUT_COUNT;
if let Some(r) = &recipient {
if r.address.script_pubkey().is_p2tr() {
reveal_tx_p2tr_output_count += 1;
} else {
reveal_tx_p2wpkh_output_count += 1;
};
}
let mut fee_amount = InscriberFeeCalculator::estimate_fee(
REVEAL_TX_P2WPKH_INPUT_COUNT,
REVEAL_TX_P2TR_INPUT_COUNT,
reveal_tx_p2wpkh_output_count,
reveal_tx_p2tr_output_count,
vec![inscription_data.script_size],
fee_rate,
)?;
let txs_stuck_factor = FEE_RATE_INCREASE_PER_PENDING_TX * pending_tx_in_context as u64;
let increase_factor = txs_stuck_factor + FEE_RATE_INCENTIVE;
fee_amount += (fee_amount * increase_factor) / 100;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Calculates the minimum target amount needed for UTXO selection. | ||
| /// This includes: Commit TX fee (estimated), a safe inscription output amount, and a | ||
| /// minimum change budget that stays reusable for follow-up transactions. | ||
| fn calculate_selection_target( | ||
| input_count: u32, | ||
| fee_rate: u64, | ||
| policy: &InscriberPolicy, | ||
| ) -> Result<Amount> { | ||
| let commit_fee = InscriberFeeCalculator::estimate_fee( | ||
| input_count, | ||
| COMMIT_TX_P2TR_INPUT_COUNT, | ||
| COMMIT_TX_P2WPKH_OUTPUT_COUNT, | ||
| COMMIT_TX_P2TR_OUTPUT_COUNT, | ||
| vec![], | ||
| fee_rate, | ||
| )?; | ||
|
|
||
| let minimum_change_budget = std::cmp::max(MIN_CHANGE_BUFFER, policy.min_change_output); | ||
| let target = commit_fee | ||
| .checked_add(policy.min_inscription_output) | ||
| .and_then(|v| v.checked_add(minimum_change_budget)) | ||
| .ok_or_else(|| anyhow::anyhow!("Target amount overflow"))?; | ||
| Ok(target) |
There was a problem hiding this comment.
calculate_selection_target() only budgets for the commit fee + inscription output + a fixed MIN_CHANGE_BUFFER (10k sats). The reveal fee depends on fee_rate, script_size, and the pending-chain multiplier, so under higher feerates / larger scripts this selection can stop early and later fail in prepare_reveal_tx_output() even though more UTXOs are available. Consider including an estimate of the reveal fee (and recipient amount if applicable) in the selection target, or retry selection with more inputs when downstream steps detect insufficient funds.
| .fold(Amount::ZERO, |acc, (_, txout)| acc + txout.value); | ||
| let full_total = full_candidates | ||
| .iter() | ||
| .fold(Amount::ZERO, |acc, (_, txout)| acc + txout.value); |
There was a problem hiding this comment.
In the truncated-selection fallback totals, fold(Amount::ZERO, |acc, ...| acc + txout.value) uses unchecked addition, while the rest of selection uses checked_add to return a proper error on overflow. For consistency and to avoid potential panics/overflow behavior in extreme cases, use checked_add (and propagate an error) when computing truncated_total / full_total too.
| .fold(Amount::ZERO, |acc, (_, txout)| acc + txout.value); | |
| let full_total = full_candidates | |
| .iter() | |
| .fold(Amount::ZERO, |acc, (_, txout)| acc + txout.value); | |
| .try_fold(Amount::ZERO, |acc, (_, txout)| { | |
| acc.checked_add(txout.value).ok_or_else(|| { | |
| anyhow::anyhow!("overflow while computing truncated UTXO total") | |
| }) | |
| })?; | |
| let full_total = full_candidates | |
| .iter() | |
| .try_fold(Amount::ZERO, |acc, (_, txout)| { | |
| acc.checked_add(txout.value).ok_or_else(|| { | |
| anyhow::anyhow!("overflow while computing full UTXO total") | |
| }) | |
| })?; |
| impl InscriberPolicy { | ||
| pub fn from_sats( | ||
| min_inscription_output_sats: u64, | ||
| min_change_output_sats: u64, | ||
| allow_unconfirmed_change_reuse: bool, | ||
| min_feerate_sat_vb: u64, | ||
| min_feerate_chained_sat_vb: u64, | ||
| max_feerate_sat_vb: u64, | ||
| escalation_step_sat_vb: u64, | ||
| ) -> Self { | ||
| Self { | ||
| min_inscription_output: Amount::from_sat(min_inscription_output_sats), | ||
| min_change_output: Amount::from_sat(min_change_output_sats), | ||
| allow_unconfirmed_change_reuse, | ||
| min_feerate_sat_vb, | ||
| min_feerate_chained_sat_vb, | ||
| max_feerate_sat_vb, | ||
| escalation_step_sat_vb, | ||
| } | ||
| } |
There was a problem hiding this comment.
InscriberPolicy::from_sats() accepts arbitrary values without validation. In particular, min_change_output_sats can be configured below the P2WPKH dust threshold, leading to creation of non-relayable transactions. Consider validating/clamping policy values (e.g., min change >= dust; min/max feerate ordering) at construction time so misconfiguration fails fast.
| let policy = InscriberPolicy::from_sats( | ||
| self.config.min_inscription_output_sats(), | ||
| self.config.min_change_output_sats(), | ||
| self.config.allow_unconfirmed_change_reuse(), | ||
| self.config.min_feerate_sat_vb(), | ||
| self.config.min_feerate_chained_sat_vb(), | ||
| self.config.max_feerate_sat_vb(), | ||
| self.config.escalation_step_sat_vb(), | ||
| ); | ||
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) |
There was a problem hiding this comment.
The InscriberPolicy::from_sats(...) construction is duplicated across multiple wiring layers. This duplication risks config drift if new policy fields are added later. Consider extracting a small helper (e.g., fn inscriber_policy_from_config(config: &ViaBtcSenderConfig) -> InscriberPolicy) in the node framework layer code to centralize the mapping.
| let policy = InscriberPolicy::from_sats( | ||
| self.config.min_inscription_output_sats(), | ||
| self.config.min_change_output_sats(), | ||
| self.config.allow_unconfirmed_change_reuse(), | ||
| self.config.min_feerate_sat_vb(), | ||
| self.config.min_feerate_chained_sat_vb(), | ||
| self.config.max_feerate_sat_vb(), | ||
| self.config.escalation_step_sat_vb(), | ||
| ); | ||
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) |
There was a problem hiding this comment.
The InscriberPolicy::from_sats(...) construction is duplicated across multiple wiring layers. This duplication risks config drift if new policy fields are added later. Consider extracting a small helper (e.g., fn inscriber_policy_from_config(config: &ViaBtcSenderConfig) -> InscriberPolicy) in the node framework layer code to centralize the mapping.
| let policy = InscriberPolicy::from_sats( | ||
| self.config.min_inscription_output_sats(), | ||
| self.config.min_change_output_sats(), | ||
| self.config.allow_unconfirmed_change_reuse(), | ||
| self.config.min_feerate_sat_vb(), | ||
| self.config.min_feerate_chained_sat_vb(), | ||
| self.config.max_feerate_sat_vb(), | ||
| self.config.escalation_step_sat_vb(), | ||
| ); | ||
| let inscriber = Inscriber::new_with_policy(client, &self.wallet.private_key, None, policy) |
There was a problem hiding this comment.
The InscriberPolicy::from_sats(...) construction is duplicated across multiple wiring layers. This duplication risks config drift if new policy fields are added later. Consider extracting a small helper (e.g., fn inscriber_policy_from_config(config: &ViaBtcSenderConfig) -> InscriberPolicy) in the node framework layer code to centralize the mapping.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use via_btc_client::inscriber::InscriberPolicy; | ||
| use zksync_config::ViaBtcSenderConfig; | ||
|
|
||
| pub(crate) fn inscriber_policy_from_config(config: &ViaBtcSenderConfig) -> anyhow::Result<InscriberPolicy> { | ||
| InscriberPolicy::from_sats( | ||
| config.min_inscription_output_sats(), | ||
| config.min_change_output_sats(), | ||
| config.allow_unconfirmed_change_reuse(), | ||
| config.min_feerate_sat_vb(), | ||
| config.min_feerate_chained_sat_vb(), | ||
| config.max_feerate_sat_vb(), | ||
| config.escalation_step_sat_vb(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
mod.rs no longer declares its submodules (e.g. pub mod aggregator;, pub mod manager;, etc.). Without these, the files in this directory won't be compiled and imports like super::inscriber_policy_from_config from aggregator.rs/manager.rs/vote_manager.rs will fail. Re-add the pub mod ... declarations (and keep inscriber_policy_from_config alongside them) so the layer modules remain part of the crate.
| fn calculate_selection_target( | ||
| input_count: u32, | ||
| fee_rate: u64, | ||
| policy: &InscriberPolicy, | ||
| ) -> Result<Amount> { | ||
| let commit_fee = InscriberFeeCalculator::estimate_fee( | ||
| input_count, | ||
| COMMIT_TX_P2TR_INPUT_COUNT, | ||
| COMMIT_TX_P2WPKH_OUTPUT_COUNT, | ||
| COMMIT_TX_P2TR_OUTPUT_COUNT, | ||
| vec![], | ||
| fee_rate, | ||
| )?; | ||
|
|
||
| let minimum_change_budget = std::cmp::max(MIN_CHANGE_BUFFER, policy.min_change_output); | ||
| let target = commit_fee | ||
| .checked_add(policy.min_inscription_output) | ||
| .and_then(|v| v.checked_add(minimum_change_budget)) | ||
| .ok_or_else(|| anyhow::anyhow!("Target amount overflow"))?; |
There was a problem hiding this comment.
calculate_selection_target() uses policy.min_inscription_output directly, but prepare_commit_tx_output() uses max(P2TR_DUST_LIMIT, policy.min_inscription_output). If a config sets min_inscription_output_sats below the dust limit, selection can underestimate the required amount and later fail when building the commit tx. Consider validating min_inscription_output_sats >= P2TR_DUST_LIMIT in InscriberPolicy::from_sats() and/or using the same max(P2TR_DUST_LIMIT, ...) logic inside the selection target calculation to keep them consistent.
| let mut utxos = vec![]; | ||
| // First 100 entries are too small to satisfy the target. | ||
| for vout in 0..100u32 { | ||
| utxos.push(( | ||
| OutPoint { txid: Txid::all_zeros(), vout }, | ||
| TxOut { value: Amount::from_sat(100), script_pubkey: script_pubkey.clone() }, | ||
| )); | ||
| } | ||
| // The 101st entry makes the full set sufficient. | ||
| utxos.push(( | ||
| OutPoint { txid: Txid::all_zeros(), vout: 100 }, | ||
| TxOut { value: Amount::from_sat(20_000), script_pubkey: script_pubkey.clone() }, | ||
| )); | ||
|
|
||
| let (selected, total) = select_utxos(utxos, 10, &InscriberPolicy::default()).unwrap(); | ||
| assert!(selected.iter().any(|(outpoint, _)| outpoint.vout == 100)); | ||
| assert!(total.to_sat() >= 20_000); |
There was a problem hiding this comment.
test_select_utxos_falls_back_to_full_set_when_truncated_prefix_is_insufficient doesn't actually exercise the fallback path: after sorting largest-first, the 20_000-sat UTXO will be in the first 100 candidates, so the truncated selection is likely to succeed without needing the full-set retry. To test the fallback reliably, adjust the fixture so the first 100 largest UTXOs are insufficient but the full set is sufficient (e.g., use many equal-valued UTXOs where 100 are insufficient but 101 are sufficient), and add an assertion that the truncated attempt would fail without the full-set retry.
| let mut utxos = vec![]; | |
| // First 100 entries are too small to satisfy the target. | |
| for vout in 0..100u32 { | |
| utxos.push(( | |
| OutPoint { txid: Txid::all_zeros(), vout }, | |
| TxOut { value: Amount::from_sat(100), script_pubkey: script_pubkey.clone() }, | |
| )); | |
| } | |
| // The 101st entry makes the full set sufficient. | |
| utxos.push(( | |
| OutPoint { txid: Txid::all_zeros(), vout: 100 }, | |
| TxOut { value: Amount::from_sat(20_000), script_pubkey: script_pubkey.clone() }, | |
| )); | |
| let (selected, total) = select_utxos(utxos, 10, &InscriberPolicy::default()).unwrap(); | |
| assert!(selected.iter().any(|(outpoint, _)| outpoint.vout == 100)); | |
| assert!(total.to_sat() >= 20_000); | |
| // Construct 101 equal-valued UTXOs. | |
| let mut utxos = vec![]; | |
| for vout in 0..101u32 { | |
| utxos.push(( | |
| OutPoint { txid: Txid::all_zeros(), vout }, | |
| TxOut { value: Amount::from_sat(1_000), script_pubkey: script_pubkey.clone() }, | |
| )); | |
| } | |
| // Choose a target such that 100 UTXOs are insufficient but 101 are sufficient. | |
| let target = 100_500u64; // 100 * 1_000 = 100_000 < target <= 101 * 1_000 = 101_000 | |
| // Simulate the truncated-prefix attempt: only the first 100 UTXOs. | |
| let truncated_utxos = utxos[..100].to_vec(); | |
| let truncated_result = select_utxos(truncated_utxos, target, &InscriberPolicy::default()); | |
| assert!( | |
| truncated_result.is_err(), | |
| "Selection over the truncated prefix should be insufficient and fail" | |
| ); | |
| // Now run selection over the full set, which should succeed via the fallback. | |
| let (selected, total) = select_utxos(utxos, target, &InscriberPolicy::default()).unwrap(); | |
| assert!(total.to_sat() >= target); |
| let age_seconds = (Utc::now().naive_utc() - last_inscription_history.created_at) | ||
| .num_seconds() | ||
| .max(0) as u64; | ||
|
|
||
| if age_seconds >= self.config.escalation_interval_sec() { | ||
| if let Some(request) = storage | ||
| .btc_sender_dal() | ||
| .get_inscription_request(inscription_id) | ||
| .await? | ||
| { | ||
| tracing::warn!( | ||
| "Stuck inscription {} for request {} (type {}) exceeded escalation interval {}s; retrying by sending a new commit+reveal transaction", | ||
| last_inscription_history.reveal_tx_id, | ||
| inscription_id, | ||
| request.request_type, | ||
| self.config.escalation_interval_sec() | ||
| ); | ||
| METRICS.retry_attempts.inc(); | ||
| self.send_inscription_tx(storage, &request).await?; | ||
| } |
There was a problem hiding this comment.
The new escalation retry sends a brand-new commit+reveal via send_inscription_tx() based only on the age of the last history entry. This can (a) create duplicate on-chain inscriptions if the original tx later confirms, and (b) hammer the network if send_inscription_tx() fails before persisting a new history row (the next poll will immediately retry again because created_at hasn't changed). Consider implementing a true fee-bump / replacement strategy (RBF) for the existing tx(s), or persisting retry metadata (e.g., last_retry_at / attempt counter) and applying backoff / rate-limiting to avoid rapid repeated retries and duplicated inscriptions.
|
/copilot review |
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
core/lib/via_btc_client/src/inscriber/mod.rs:516
- When
allow_unconfirmed_change_reuseis enabled, this reusesfifo_queue.front()(oldest) reveal-change output. But earlier in this function, reveal-change outputs for all entries except the last are marked as spent (i != context_queue_len - 1), so forcontext_queue_len > 1the front change output is already considered spent by a later pending inscription. Re-adding it here can lead to selecting an outpoint that this process already treats as spent (double-spend / invalid chain).
Prefer reusing the latest / still-unspent change output (e.g., fifo_queue.back()), or explicitly pick the single context change outpoint that is not present in spent_utxos.
if self.policy.allow_unconfirmed_change_reuse && context_queue_len > 0 {
if let Some(head_inscription) = self.context.fifo_queue.front() {
let reveal_change_output = head_inscription.inscriber_output.reveal_txid;
let reveal_change_output = OutPoint {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive policy management system for the Bitcoin inscriber, allowing for fine-grained control over transaction construction parameters. Key changes include new configuration options for minimum inscription and change output values, various feerate settings (min, chained, max, escalation step), and the ability to allow reuse of unconfirmed change outputs. The core Inscriber logic has been refactored to incorporate this InscriberPolicy, including a new UTXO selection algorithm that prioritizes largest-first and dynamically calculates fees, and an enhanced get_fee_rate function that considers network conditions, policy floors, and pending transaction depth. Additionally, the system now includes a mechanism to detect and retry stuck inscriptions based on a configurable escalation interval, and a new metric for tracking these retry attempts. Review feedback suggests optimizing UTXO selection by avoiding an unnecessary clone and simplifying the fee rate calculation by removing a redundant max operation.
| let full_candidates = utxos.clone(); | ||
| let truncated_candidates = utxos | ||
| .iter() | ||
| .take(MAX_UTXOS_TO_CONSIDER) | ||
| .cloned() | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| match select_utxos_from_candidates(truncated_candidates, fee_rate, policy) { | ||
| Ok(result) => Ok(result), | ||
| Err(err) => { | ||
| let truncated_total = utxos | ||
| .iter() | ||
| .take(MAX_UTXOS_TO_CONSIDER) | ||
| .try_fold(Amount::ZERO, |acc, (_, txout)| { | ||
| acc.checked_add(txout.value) | ||
| .ok_or_else(|| anyhow::anyhow!("overflow while computing truncated UTXO total")) | ||
| })?; | ||
| let full_total = full_candidates | ||
| .iter() | ||
| .try_fold(Amount::ZERO, |acc, (_, txout)| { | ||
| acc.checked_add(txout.value) | ||
| .ok_or_else(|| anyhow::anyhow!("overflow while computing full UTXO total")) | ||
| })?; | ||
|
|
||
| if full_total > truncated_total { | ||
| debug!( | ||
| "Retrying UTXO selection with full set after truncated candidate failure: truncated_total={} sats, full_total={} sats, err={}", | ||
| truncated_total.to_sat(), | ||
| full_total.to_sat(), | ||
| err | ||
| ); | ||
| select_utxos_from_candidates(full_candidates, fee_rate, policy) | ||
| } else { | ||
| Err(err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The utxos.clone() to full_candidates is inefficient, especially if the utxos vector is large. The original utxos vector can be used directly since it's owned by the function, avoiding the clone and an extra allocation. This also simplifies the logic in the Err arm.
let truncated_candidates = utxos
.iter()
.take(MAX_UTXOS_TO_CONSIDER)
.cloned()
.collect::<Vec<_>>();
match select_utxos_from_candidates(truncated_candidates, fee_rate, policy) {
Ok(result) => Ok(result),
Err(err) => {
let truncated_total = utxos
.iter()
.take(MAX_UTXOS_TO_CONSIDER)
.try_fold(Amount::ZERO, |acc, (_, txout)| {
acc.checked_add(txout.value)
.ok_or_else(|| anyhow::anyhow!("overflow while computing truncated UTXO total"))
})?;
let full_total = utxos.iter().try_fold(Amount::ZERO, |acc, (_, txout)| {
acc.checked_add(txout.value)
.ok_or_else(|| anyhow::anyhow!("overflow while computing full UTXO total"))
})?;
if full_total > truncated_total {
debug!(
"Retrying UTXO selection with full set after truncated candidate failure: truncated_total={} sats, full_total={} sats, err={}",
truncated_total.to_sat(),
full_total.to_sat(),
err
);
select_utxos_from_candidates(utxos, fee_rate, policy)
} else {
Err(err)
}
}
}| let effective = std::cmp::max( | ||
| floor, | ||
| std::cmp::min(self.policy.max_feerate_sat_vb, candidate), | ||
| ); |
There was a problem hiding this comment.
The std::cmp::max(floor, ...) call appears to be redundant. Given the logic that sets candidate, it's guaranteed to be greater than or equal to floor. Additionally, max_feerate_sat_vb is also checked to be >= floor. Therefore, min(self.policy.max_feerate_sat_vb, candidate) will also be >= floor. The outer max can be removed for simplification.
| let effective = std::cmp::max( | |
| floor, | |
| std::cmp::min(self.policy.max_feerate_sat_vb, candidate), | |
| ); | |
| let effective = std::cmp::min(self.policy.max_feerate_sat_vb, candidate); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let minimum_change_budget = std::cmp::max(MIN_CHANGE_BUFFER, policy.min_change_output); | ||
| let target = commit_fee | ||
| .checked_add(policy.min_inscription_output) | ||
| .and_then(|v| v.checked_add(minimum_change_budget)) | ||
| .ok_or_else(|| anyhow::anyhow!("Target amount overflow"))?; |
There was a problem hiding this comment.
calculate_selection_target uses a fixed MIN_CHANGE_BUFFER that is independent of the reveal transaction fee (which depends on fee_rate, inscription script size, and optional recipient output). This can let UTXO selection succeed but later fail during prepare_reveal_tx_output with "Reveal change output ... below minimum" even when additional UTXOs exist. Consider estimating the reveal fee (using the same fee calculator + script_size / output counts) and including it (plus the policy min change) in the selection target, instead of a hard-coded 10_000 sat buffer.
| ) -> anyhow::Result<()> { | ||
| 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 { |
There was a problem hiding this comment.
The pending-chain-depth guard uses pending_chain_depth > max_pending_chain_depth, which still allows sending when pending_chain_depth == max_pending_chain_depth and can push the context beyond the configured maximum. If max_pending_chain_depth is meant to be the maximum allowed pending depth, this should likely be >= so you don't exceed the cap after enqueueing a new inscription.
| if pending_chain_depth > max_pending_chain_depth { | |
| if pending_chain_depth >= max_pending_chain_depth { |
| ) -> anyhow::Result<()> { | ||
| 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 { |
There was a problem hiding this comment.
The pending-chain-depth guard uses pending_chain_depth > max_pending_chain_depth, which still allows sending when pending_chain_depth == max_pending_chain_depth and can push the context beyond the configured maximum. If max_pending_chain_depth is meant to be the maximum allowed pending depth, this should likely be >= so you don't exceed the cap after enqueueing a new inscription.
| if pending_chain_depth > max_pending_chain_depth { | |
| if pending_chain_depth >= max_pending_chain_depth { |
| let mut found = false; | ||
| for amount in 100u64..5000u64 { | ||
| let utxos = (0..101u32) | ||
| .map(|vout| { | ||
| ( | ||
| OutPoint { | ||
| txid: Txid::all_zeros(), | ||
| vout, | ||
| }, | ||
| TxOut { | ||
| value: Amount::from_sat(amount), | ||
| script_pubkey: script_pubkey.clone(), | ||
| }, | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let truncated_result = select_utxos_from_candidates( | ||
| utxos.iter().take(100).cloned().collect(), | ||
| 10, | ||
| &policy, | ||
| ); | ||
| let full_result = select_utxos(utxos, 10, &policy); | ||
|
|
||
| if truncated_result.is_err() && full_result.is_ok() { | ||
| let (selected, total) = full_result.unwrap(); | ||
| assert!(selected.len() >= 101); | ||
| assert!(total.to_sat() >= amount * 101); | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| assert!(found, "Expected to find a fixture where truncated selection fails but full-set fallback succeeds"); |
There was a problem hiding this comment.
This test brute-forces a value over a 4,900-iteration range to find a case where truncated selection fails but full-set fallback succeeds. This is relatively expensive for a unit test and may be brittle if fee estimation constants change (the loop may fail to find a fixture). Prefer constructing a deterministic fixture by computing an amount from calculate_selection_target(...) (or hard-coding a known-good amount) so the test remains fast and stable.
| let mut found = false; | |
| for amount in 100u64..5000u64 { | |
| let utxos = (0..101u32) | |
| .map(|vout| { | |
| ( | |
| OutPoint { | |
| txid: Txid::all_zeros(), | |
| vout, | |
| }, | |
| TxOut { | |
| value: Amount::from_sat(amount), | |
| script_pubkey: script_pubkey.clone(), | |
| }, | |
| ) | |
| }) | |
| .collect::<Vec<_>>(); | |
| let truncated_result = select_utxos_from_candidates( | |
| utxos.iter().take(100).cloned().collect(), | |
| 10, | |
| &policy, | |
| ); | |
| let full_result = select_utxos(utxos, 10, &policy); | |
| if truncated_result.is_err() && full_result.is_ok() { | |
| let (selected, total) = full_result.unwrap(); | |
| assert!(selected.len() >= 101); | |
| assert!(total.to_sat() >= amount * 101); | |
| found = true; | |
| break; | |
| } | |
| } | |
| assert!(found, "Expected to find a fixture where truncated selection fails but full-set fallback succeeds"); | |
| // Known-good fixture: this amount was chosen so that using only the first 100 | |
| // UTXOs is insufficient to meet the selection target, while using all 101 | |
| // UTXOs succeeds and triggers the fallback to the full candidate set. | |
| let amount: u64 = 1000; | |
| let utxos = (0..101u32) | |
| .map(|vout| { | |
| ( | |
| OutPoint { | |
| txid: Txid::all_zeros(), | |
| vout, | |
| }, | |
| TxOut { | |
| value: Amount::from_sat(amount), | |
| script_pubkey: script_pubkey.clone(), | |
| }, | |
| ) | |
| }) | |
| .collect::<Vec<_>>(); | |
| let truncated_result = select_utxos_from_candidates( | |
| utxos.iter().take(100).cloned().collect(), | |
| 10, | |
| &policy, | |
| ); | |
| let full_result = select_utxos(utxos, 10, &policy); | |
| assert!( | |
| truncated_result.is_err(), | |
| "Expected truncated selection over the first 100 UTXOs to fail" | |
| ); | |
| assert!( | |
| full_result.is_ok(), | |
| "Expected full-set selection over all 101 UTXOs to succeed" | |
| ); | |
| let (selected, total) = full_result.unwrap(); | |
| assert!(selected.len() >= 101); | |
| assert!(total.to_sat() >= amount * 101); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new InscriberPolicy to centralize configuration parameters for Bitcoin inscription, including minimum output values, feerates, and unconfirmed change reuse. These policy settings are integrated into the ViaBtcSenderConfig and used throughout the inscriber logic for UTXO selection, fee rate calculation, and output validation. The Inscriber now supports Largest-First UTXO selection, dynamic fee rate adjustments based on pending chain depth, and new guardrails in ViaBtcInscriptionManager to pause new inscription sends based on pending chain depth or low trusted balance. Additionally, a mechanism for retrying stuck inscriptions has been added. A minor improvement opportunity was noted regarding the InscriberPolicy::default() implementation, where constants should be reused instead of hardcoding values for min_inscription_output and min_change_output to ensure consistency.
| const MIN_INSCRIPTION_OUTPUT: Amount = Amount::from_sat(600); | ||
|
|
||
| /// Keep change outputs large enough to remain useful for follow-up reveal / chained spends. | ||
| const MIN_CHANGE_OUTPUT: Amount = Amount::from_sat(1_000); |
There was a problem hiding this comment.
The constants MIN_INSCRIPTION_OUTPUT and MIN_CHANGE_OUTPUT are defined, but their values are hardcoded again in the InscriberPolicy::default() implementation. To maintain consistency and prevent potential discrepancies, it's better to use the constants directly in the default() implementation.
const MIN_INSCRIPTION_OUTPUT: Amount = Amount::from_sat(600);
/// Keep change outputs large enough to remain useful for follow-up reveal / chained spends.
const MIN_CHANGE_OUTPUT: Amount = Amount::from_sat(1_000);
/// Minimum buffer for change output to ensure Reveal TX can be funded.
/// This accounts for Reveal TX fees plus safety margin.
const MIN_CHANGE_BUFFER: Amount = Amount::from_sat(10_000);
/// Maximum number of UTXOs to consider for selection (performance reasoning)
const MAX_UTXOS_TO_CONSIDER: usize = 100;
#[derive(Debug, Clone)]
pub struct InscriberPolicy {
pub min_inscription_output: Amount,
pub min_change_output: Amount,
pub allow_unconfirmed_change_reuse: bool,
pub min_feerate_sat_vb: u64,
pub min_feerate_chained_sat_vb: u64,
pub max_feerate_sat_vb: u64,
pub escalation_step_sat_vb: u64,
}
impl Default for InscriberPolicy {
fn default() -> Self {
Self {
min_inscription_output: MIN_INSCRIPTION_OUTPUT,
min_change_output: MIN_CHANGE_OUTPUT,
allow_unconfirmed_change_reuse: false,
min_feerate_sat_vb: 8,
min_feerate_chained_sat_vb: 20,
max_feerate_sat_vb: 80,
escalation_step_sat_vb: 5,
}
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive policy and fee management system for the BTC inscriber. It adds new configuration parameters to ViaBtcSenderConfig for controlling inscription output values, change output values, feerates (min, max, chained, escalation), and conditions for reusing unconfirmed change outputs. The Inscriber now uses an InscriberPolicy to guide UTXO selection with a "Largest-First" strategy, dynamically calculates fees based on network conditions and pending chain depth, and enforces guardrails to prevent excessive pending transactions or low trusted balances. The changes also include logic for retrying stuck inscriptions and updated balance reporting. A minor improvement opportunity was noted regarding the simplification of a fee calculation in get_fee_rate.
| floor | ||
| ); | ||
| } | ||
| let candidate = std::cmp::max(std::cmp::max(network_rate, floor), escalated); |
There was a problem hiding this comment.
The calculation for candidate can be simplified. Since escalated is defined as floor.saturating_add(...), it will always be greater than or equal to floor. Therefore, std::cmp::max(..., floor) is redundant within the nested max calls.
You can simplify this line to:
let candidate = std::cmp::max(network_rate, escalated);This improves readability without changing the logic.
| let candidate = std::cmp::max(std::cmp::max(network_rate, floor), escalated); | |
| let candidate = std::cmp::max(network_rate, escalated); |
|
/copilot review |
What ❔
Largest First UTXO selection
Why ❔
Optimized UTXO selection
Current Greedy implementation adds every UTXO, accumulates all value and counts all inputs
Largest First gives more value per input
Largest First over Branch and Bound prob not gonna work cause Commit TX change output
COMMIT_TX_CHANGE_OUTPUT_INDEX = 0is required to fund the Reveal TXThe Reveal TX uses this change output as fee paying input. No change or 0 change means the Reveal tx can't be funded
DEF needs to be peer reviewed
Checklist
zkstack dev fmtandzkstack dev lint.