-
Notifications
You must be signed in to change notification settings - Fork 3
feat(via_btc_client): implement Largest-First UTXO selection #333
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
e2a788c
b6a22d7
91f0103
2bc96f7
7019c70
526fc71
403f38a
8311aee
10ed39a
293e6bc
a351a10
93ed235
459f28c
13530a5
73bf931
6327233
372ea0a
560c30f
d819d6e
6a45737
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,6 +65,83 @@ const BROADCAST_RETRY_COUNT: u32 = 3; | |||||||||||||
| // https://bitcointalk.org/index.php?topic=5453107.msg62262343#msg62262343 | ||||||||||||||
| const P2TR_DUST_LIMIT: Amount = Amount::from_sat(330); | ||||||||||||||
|
|
||||||||||||||
| /// 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. | ||||||||||||||
| 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 target = commit_fee | ||||||||||||||
| .checked_add(P2TR_DUST_LIMIT) | ||||||||||||||
| .and_then(|v| v.checked_add(MIN_CHANGE_BUFFER)) | ||||||||||||||
| .ok_or_else(|| anyhow::anyhow!("Target amount overflow"))?; | ||||||||||||||
| Ok(target) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Selects UTXOs using Largest-First: sorts by value descending, picks until target met. | ||||||||||||||
| /// Dynamically recalculates fees as inputs are added. Ensures change for Reveal TX. | ||||||||||||||
| fn select_utxos( | ||||||||||||||
| mut utxos: Vec<(OutPoint, TxOut)>, | ||||||||||||||
| fee_rate: u64, | ||||||||||||||
| ) -> Result<(Vec<(OutPoint, TxOut)>, Amount)> { | ||||||||||||||
| if utxos.is_empty() { | ||||||||||||||
| return Err(anyhow::anyhow!("No UTXOs available for selection")); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Sort by value descending (largest first) | ||||||||||||||
| utxos.sort_by(|a, b| b.1.value.cmp(&a.1.value)); | ||||||||||||||
|
|
||||||||||||||
| // Limit UTXOs to consider for performance | ||||||||||||||
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); | ||||||||||||||
|
|
||||||||||||||
|
||||||||||||||
| // Limit UTXOs to consider for performance | |
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); |
Outdated
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.
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); |
Outdated
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.
select_utxos() truncates the UTXO set to MAX_UTXOS_TO_CONSIDER after sorting. If the wallet has >100 UTXOs and the largest 100 don’t cover the target, this will return Insufficient funds even when the remaining smaller UTXOs would make the balance sufficient. Consider either removing the truncation, or only applying it after first verifying that the truncated set still contains enough total value (and otherwise falling back to selecting from the full set).
| // Limit UTXOs to consider for performance | |
| utxos.truncate(MAX_UTXOS_TO_CONSIDER); |
Outdated
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.
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.
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.
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.
Outdated
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.
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)); |
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 comment on
MIN_CHANGE_BUFFER/calculate_selection_targetsays 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 affectprepare_reveal_tx_outputfee). 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 passinginscription_data.script_sizeand recipient info into selection), or (b) adjust the comment to avoid implying the Reveal TX fees are actually covered by this constant.