Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: FeeRate::checked_mul_by_weight should scale output down by 1000 #2182

Merged
merged 2 commits into from
Nov 13, 2023
Merged

fix: FeeRate::checked_mul_by_weight should scale output down by 1000 #2182

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c56131
fix: FeeRate::checked_mul_by_weight should scale output down by 1000
conduition Nov 9, 2023
6c6c08c
add second test case
conduition Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 18 additions & 7 deletions bitcoin/src/blockdata/fee_rate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,12 @@ impl FeeRate {

/// Checked weight multiplication.
///
/// Computes `self * rhs` where rhs is of type Weight. `None` is returned if an overflow
/// occurred.
/// Computes the absolute fee amount for a given [`Weight`] at this fee rate.
///
/// `None` is returned if an overflow occurred.
pub fn checked_mul_by_weight(self, rhs: Weight) -> Option<Amount> {
self.0.checked_mul(rhs.to_wu()).map(Amount::from_sat)
let sats = self.0.checked_mul(rhs.to_wu())?.checked_add(999)? / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, but wouldn't this be simpler read using checked_mul instead of checked_add?

suggest

let sats = self.0.checked_mul(rhs.to_wu())?.checked_mul(1000)?;

I don't really understand why the checked_add here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

let sats = self.0.checked_mul(rhs.to_wu())?.checked_mul(1000)?;

That would produce an incorrect result. I'm guessing you meant checked_div? Checked division isn't necessary as we're dividing by a fixed non-zero constant.

The .checked_add(999) call is there to ensure we compute a ceiling without overflowing. See the impl Mul<FeeRate> for Weight implementation below at L137 for an example that isn't overflow-safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, alright, this is confusing. It feels like what we really want is for rhs: Weight to instead be KWU instead of WU. Right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other-words, if fee_rate is in sats/kwu, then wouldn't the interface be simpler as: sats/kwu * kwu = sats

    pub fn checked_mul_by_weight(self, rhs: KWU) -> Option<Amount> {
        let sats = self.0.checked_mul(rhs)?;
        Some(Amount::from_sat(sats))
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like what we really want is for rhs: Weight to instead be KWU instead of WU. Right?

Yes i suppose that's a good way to visualize it, but we couldn't implement it that way. We'd have to represent KWU as a float (or a new struct type) to perform accurate arithmetic on weight-unit numbers less than 1000.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to represent KWU as a float (or a new struct type)

Yes, exactly, like here:
https://github.com/rust-bitcoin/rust-bitcoin/pull/1787/files#diff-c2ea40075e93ccd068673873166cfa3312ec7439d6bc5a4cbc03e972c7e045c4R16

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like #1787 also fixes the same bug as this PR. Although it's much bigger and has been held up since April so maybe a small patch like this PR might be appropriate until #1787 can be merged. I filed this PR because I got unexpected results back from FeeRate::fee_wu. #1787 is probably a better place to consider refactoring APIs; this PR is just a hotfix after all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely let's get this fix in here. No point waiting for #1787

I'd be in favor of making a point release as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitively better to not wait for #1787

Some(Amount::from_sat(sats))
}

/// Calculates fee by multiplying this fee rate by weight, in weight units, returning `None`
Expand All @@ -95,13 +97,14 @@ impl FeeRate {
///
/// # Examples
///
/// ```no_run
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this no_run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I think it was because the example didn't assert or panic, so running it would be pointless. Now that we have an assert_eq call it makes sense to run it.

/// ```
/// # use bitcoin::{absolute, transaction, FeeRate, Transaction};
/// # // Dummy transaction.
/// # let tx = Transaction { version: transaction::Version::ONE, lock_time: absolute::LockTime::ZERO, input: vec![], output: vec![] };
///
/// let rate = FeeRate::from_sat_per_vb(1).expect("1 sat/vbyte is valid");
/// let fee = rate.fee_wu(tx.weight());
/// let fee = rate.fee_wu(tx.weight()).unwrap();
/// assert_eq!(fee.to_sat(), tx.vsize() as u64);
/// ```
pub fn fee_wu(self, weight: Weight) -> Option<Amount> { self.checked_mul_by_weight(weight) }

Expand Down Expand Up @@ -209,12 +212,20 @@ mod tests {

#[test]
fn checked_weight_mul_test() {
let weight = Weight::from_wu(10);
let fee: Amount = FeeRate(10).checked_mul_by_weight(weight).expect("expected Amount");
let weight = Weight::from_vb(10).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this to from_vb instead of from_wu?

Suggest:

let weight = Weight::from_wu(10); 
let fee: Amount = FeeRate(10).checked_mul_by_weight(weight).expect("Amount not to overflow");

Also, the current expect messages suck (sorry). It should say something like "Amount not to overflow".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as for this change: intuition for the numbers of the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already two acks, and I think it's good to get this fixed asap, so no more changes needed. Although, I still feel like from_vb() makes this test more confusing. Keeping it all as Weight Units:

    #[test]
    fn checked_weight_mul_test() {
        let weight = Weight::from_wu(10);
        let fee: Amount = FeeRate(10).checked_mul_by_weight(weight).expect("expected Amount");
        assert_eq!(Amount::from_sat(1), fee);

        let fee = FeeRate(10).checked_mul_by_weight(Weight::MAX);
        assert!(fee.is_none());
    }

since we start at 10 Weight units, and then add 1_000, inside checked_mul, this results in 1,000 Wu * 10 sats / 1,000 Wu = 1 sat. The multiplication by 4 when using from_vb() doesn't add anything to this test and is diff noise.

let fee: Amount = FeeRate::from_sat_per_vb(10)
.unwrap()
.checked_mul_by_weight(weight)
.expect("expected Amount");
Kixunil marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(Amount::from_sat(100), fee);

let fee = FeeRate(10).checked_mul_by_weight(Weight::MAX);
assert!(fee.is_none());

let weight = Weight::from_vb(3).unwrap();
let fee_rate = FeeRate::from_sat_per_vb(3).unwrap();
let fee = fee_rate.checked_mul_by_weight(weight).unwrap();
assert_eq!(Amount::from_sat(9), fee);
}

#[test]
Expand Down