-
Notifications
You must be signed in to change notification settings - Fork 300
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
Add fee_amount() and fee_rate() functions to PsbtUtils trait #728
Conversation
0608644
to
7a9ed14
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.
Tested ACK. Just have one question.
src/psbt/mod.rs
Outdated
@@ -37,15 +46,35 @@ impl PsbtUtils for Psbt { | |||
None | |||
} | |||
} | |||
|
|||
fn fee_amount(&self) -> u64 { |
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'm wondering why the output_amount
is calculated using the unsigned_tx
outputs and not the PSBT outputs
.
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 PSBT and transactions inputs and outputs should all be the same. But your questions made me think about moving these functions to the TransactionDetails struct, or maybe as new util trait on Transaction. Will noodle around with that and see how it works out.
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.
Also I just remembered the reason I needed to use PSBT to calculate the fee_amount
is because it contains the full details for the transaction inputs, which includes the input amounts, if you only look at the Transaction structure it contains essentially "pointers" to the unspent outputs that become the inputs (the OutPoint
) and this structure does not contain the input amounts. So I'm leaving these helper functions on the PsbtUtils
trait.
Linking #524 to 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.
Concept ACK. It will be helpful to get the feerate in the unsigned stage.
And if I understand correctly, the ask of #524 is to have the fee rate in unsigned transaction specifically, which is a special annoying problem. As we don't know the size before fully signing it. Its straight forward to calculate the fee rate from a signed tx just from the transaction data itself. We know both the fee and the size after signing.
I think one way could be to return the FeeRate
in the TransactionDetails
itself from create_tx()
function. The fee rate value exists in the create_tx()
function, we just happen to throw it away after coinselection. here
Lines 848 to 855 in 0a3734e
let coin_selection = coin_selection.coin_select( | |
self.database.borrow().deref(), | |
required_utxos, | |
optional_utxos, | |
fee_rate, | |
outgoing + fee_amount, | |
&drain_script, | |
)?; |
So instead why not just keep the value alive and store it in the TransactionDetails
.
Edge Case: If I understand correctly the interplay between fee_rate
and fee_amount
happening here
Lines 743 to 770 in 0a3734e
let (fee_rate, mut fee_amount) = match params | |
.fee_policy | |
.as_ref() | |
.unwrap_or(&FeePolicy::FeeRate(FeeRate::default())) | |
{ | |
//FIXME: see https://github.com/bitcoindevkit/bdk/issues/256 | |
FeePolicy::FeeAmount(fee) => { | |
if let Some(previous_fee) = params.bumping_fee { | |
if *fee < previous_fee.absolute { | |
return Err(Error::FeeTooLow { | |
required: previous_fee.absolute, | |
}); | |
} | |
} | |
(FeeRate::from_sat_per_vb(0.0), *fee) | |
} | |
FeePolicy::FeeRate(rate) => { | |
if let Some(previous_fee) = params.bumping_fee { | |
let required_feerate = FeeRate::from_sat_per_vb(previous_fee.rate + 1.0); | |
if *rate < required_feerate { | |
return Err(Error::FeeRateTooLow { | |
required: required_feerate, | |
}); | |
} | |
} | |
(*rate, 0) | |
} | |
}; |
It seems if the user sets a FeePolicy::FeeAmount(fee)
then fee_rate
will always be 0, and in that case we should have that value as None
in the TrasnactionDetails
.
Another idea.. We have a Bad idea: The wallet might have inputs for which it can't produce satisfaction. |
@rajarshimaitra you're right that the initial request was for an estimate of the fee rate before signing. Since I didn't see an easy way to do that so I went for giving the exact fee rate of the PSBT after it's finalized. I didn't think of using the |
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.
@notmandatory Noted.. This can be done in separate PR..
tACK 7a9ed14
Although I am thinking if its much useful to provide this as a separate function, as its mostly straight forward calculation from any signed transaction data. But I guess having extra utility never hurts..
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.
utACK 7a9ed14
I've also added this PR to the milestone, as it seems super close to getting in :)
BTW, I didn't add a CHANGELOG message since I'm assuming we'll have #544 in for this next release. If not I'll need to update CHANGELOG before this goes in. |
7a9ed14
to
8ee055d
Compare
rebased to pickup ci and other changes |
8ee055d
to
7d07e48
Compare
7d07e48
to
9f4bcc0
Compare
PsbtUtils.fee_amount(), calculates the PSBT total transaction fee amount in Sats. PsbtUtils.fee_rate(), calculates the PSBT FeeRate, the value is only accurate AFTER the PSBT is finalized.
9f4bcc0
to
ab41679
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.
ACK ab41679
I've also checked the coveralls report and it looks like all the added lines are actually covered by tests! I mean, I was expecting it but it's nice to see the report match the expectation :)
34987d5 Make psbt mod public and add required docs (Steve Myers) Pull request description: ### Description Make psbt mod public and add required docs. The module needs to be public so `bdk-ffi` can expose the new PSBT `fee_amount()` and `fee_rate()` functions. ### Notes to the reviewers I should have done this as part of #728. ### Changelog notice Make psbt module public to expose PsbtUtils trait to downstream projects. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [ ] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: rajarshimaitra: Concept + tACK 34987d5 Tree-SHA512: 99e91e948bccb7593a3da3ac5468232103d4ba90ad4e5888ef6aebb0d16511ad3a3286951779789c05587b4bb996bc359baa28b0f4c3c55e29b24bfc12a10073
Description
The purpose of the PR is to provide a more convenient way to calculate the transaction fee amount and fee rate for a PSBT. This PR adds
fee_amount
andfee_rate
functions to the existingPsbtUtils
trait and implements them forPartiallySignedBitcoinTransaction
. Thefee_rate
value is only valid if the PSBT it is called on is fully signed and finalized.See related discussion: bitcoindevkit/bdk-ffi#179
Changelog
Added
Notes to the reviewers
Ideally I'd like
fee_rate
to return anOption
and returnNone
if the PSBT isn't finalized. But I'm not quite sure how to determine if a PSBT is finalized without having aWallet
and running it through the finalize code first. Or there might be a way to fill in missing signatures with properly sized fake data prior to calculating the fee rate. For now I think it's enough to do this simple approach with usage warning in the rust docs.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: