-
Notifications
You must be signed in to change notification settings - Fork 8
Support for Child Pay For Parent (CPFP) #15
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: master
Are you sure you want to change the base?
Conversation
92150b5 to
5ad7b68
Compare
5ad7b68 to
b39710f
Compare
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.
Thanks for starting on this!
examples/cpfp.rs
Outdated
| .into_selection( | ||
| selection_algorithm_lowest_fee_bnb(FeeRate::from_sat_per_vb_unchecked(1), 100_000), | ||
| SelectorParams::new( | ||
| FeeRate::from_sat_per_vb_unchecked(30), // Higher fee rate for 10 sat/vB combined |
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 API should figure out a package feerate. What we have here is the feerate of the single transaction at the end.
CPFP doesn't actually need any "coin selection". We already know the inputs and outputs, we just need to figure out the fee needed to satisfy a given package feerate.
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.
Thank you for the review, I'll fix the package feerate.
…nce and fee efficiency
736ef39 to
204e0b5
Compare
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.
Thank you for working on this!
Looking through the PR, I realized that I’m not entirely happy with the current state of CanonicalUnspents. Trying to build a CPFP API around it feels a bit clunky. I’m planning to replace this structure with bdk_chain::CanonicalView (WIP), which should make things cleaner.
To move forward with this PR, I suggest the following plan:
Step 1 - Introduce standalone CPFP logic
Let’s first implement standalone CPFP logic that is decoupled from CanonicalUnspents.
pub struct CpfpParams {
pub package_fee: Amount,
pub package_weight: Weight,
pub inputs: Vec<Input>,
pub target_package_feerate: FeeRate,
pub output_script: ScriptSource,
}
impl CpfpParams {
fn into_selection(self) -> Result<Selection, CPFPError> {
todo!("complete me")
}
}Here, CpfpParams assumes the caller has constructed it correctly. We should document this clearly, and later (in a separate PR) introduce helper methods to construct it more safely.
The implementation of CpfpParams::into_selection will likely benefit from using bdk_coin_select::CoinSelector with [select_all](https://docs.rs/bdk_coin_select/latest/bdk_coin_select/struct.CoinSelector.html#method.select_all). You'll still need to do some math to determine the correct parameters. If you discover any missing functionality in bdk_coin_select`, feel free to upstream improvements.
Step 2 - Rebuild the example
Let's then rewrite common::Wallet::create_cpfp_tx using the CpfpParams logic. For simplicity, (since it's just an example), we can:
- Select the first owned output from each tx in
parent_txids. - Return an error if no owned outputs are found.
Step 3 - Add tests
I recommend starting with a few simple tests:
- Given one original tx with feerate
x, create CPFP tx withtarget_package_feerateofx. The resultant CPFP tx feerate should have feeratex. - Given one original tx with feerate
x, create a CPFP tx withtarget_package_feerateofx + y. The resulting package feerate should have a total feerate ofx + y(weight(original_tx + CPFP_tx) / fee(original_tx + CPFP_tx)).
After this PR lands, we can start discussing ways to make the API less manual and more ergonomic.
src/cpfp.rs
Outdated
| canon_utxos: &CanonicalUnspents, | ||
| target_package_feerate: FeeRate, | ||
| script_pubkey: &ScriptBuf, | ||
| plans: impl IntoIterator<Item = Plan>, |
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.
This API is a bit confusing. It implicitly requires the plans to be in the same order as selected_outpoints, introducing a hidden ordering invariant. This coupling feels unnecessary and makes the API harder to reason about.
Additionally, this couples the CPFPSet API with CanonicalUnspents...
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.
Okay. I will fix it
src/canonical_unspents.rs
Outdated
| const MAX_ANCESTORS: usize = 25; | ||
| if parent_txids.len() > MAX_ANCESTORS { | ||
| return Err(CPFPError::ExcessUnconfirmedAncestor); | ||
| } |
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.
This implies that parent_txids should be a linked chain of direct spends. This is not what we are going for. We really want parent_txids to be an arbitrary list of txids that we wish to fee-bump.
We want to iterate the ancestors of these parent_txids to accumulate the total fee of all unconfirmed txs. Confirmation status can be accessed via CanonicalUnspents::statuses.
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.
Okay, noted.
Thank you for the review. I'll make the necessary fixes. |
src/cpfp.rs
Outdated
| /// Create a new [CpfpParams] instance. | ||
| pub fn new( | ||
| package_fee: Amount, | ||
| package_weight: Weight, | ||
| inputs: impl IntoIterator<Item = impl Into<Input>>, | ||
| target_package_feerate: FeeRate, | ||
| output_script: crate::ScriptSource, | ||
| ) -> Self { | ||
| Self { | ||
| package_fee, | ||
| package_weight, | ||
| inputs: inputs.into_iter().map(Into::into).collect(), | ||
| target_package_feerate, | ||
| output_script, | ||
| } | ||
| } |
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.
I don't think we should have this as it's not adding any convenience.
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.
Alright, I have fixed that
| pub enum CpfpError { | ||
| /// Output value is below the dust threshold. | ||
| OutputBelowDustLimit, | ||
| /// Total input value is insufficient. | ||
| InsufficientInputValue, | ||
| /// No spendable outputs were found. | ||
| NoSpendableOutputs, | ||
| /// Failed to compute a valid fee for the child transaction. | ||
| InvalidFeeCalculation, | ||
| /// The package feerate (parent + child) is lower than the target feerate. | ||
| InsufficientPackageFeerate { | ||
| /// The actual feerate of the package. | ||
| actual: FeeRate, | ||
| /// The target feerate that the package should meet or exceed. | ||
| target: FeeRate, | ||
| }, | ||
| /// Output script is invalid | ||
| InvalidOutputScript, | ||
| } |
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.
InsufficientInputValue seem to be the only necessary error variant here.
OutputBelowDustLimitis essentially the same thing asInsufficientInputValue.NoSpendableOutputscan be replaced byInsufficientInputValue.InvalidFeeCalculationshould never happen since we are the ones calculating it. We just don't have enough input value.InsufficientPackageFeerateshould also never happen since we are the ones calculating it. The actual feerate and the target should be exactly the same.InvalidOutputScriptis out of scope here imo.
I think it will be useful to let the caller know how much value is missing. Let me know if you have any thoughts on this.
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.
I agree that InsufficientInputValue can cover all the error cases, and I've updated CpfpParams::into_selection to use a single InsufficientInputValue error variant with a missing field to indicate how much additional value is needed.
| /// This method calculates the required child transaction fee to achieve the | ||
| /// target package feerate and creates a selection with the appropriate inputs | ||
| /// and outputs. | ||
| pub fn into_selection(self) -> Result<Selection, CpfpError> { |
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.
Use the CoinSelector to calculate the cpfp_tx_weight. Everything else can be done with a simple formula:
cpfp_tx_fee = target_fee_rate * (package_weight + cpfp_tx_weight) - package_fee
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.
Okay, I have made the fix
# Conflicts: # examples/common.rs
This PR introduces Child-Pays-For-Parent (CPFP) support to
bdk-txby implementing a newCPFPSetstruct,CPFPParamsandcpfp_candidatesfunction.It also add the ancestor fee calculation for CPFP.
This implementation supports:
bumping multiple parent transaction with a single CPFP transaction
Unconfirmed ancestors are considered in fee rate calculation
Added
cpfp_candidatesMempool compliance of ≤25 ancestors count
I've signed all my commits
I ran
cargo fmtandcargo clippybefore committingFixes #14