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

conduition · 2023-11-09T19:21:12Z

Fixes a bug in #1864. The FeeRate::checked_mul_by_weight and by extension the FeeRate::fee_wu methods were returning fee amounts scaled up by a factor of 1000, since the internal representation in FeeRate is sats per 1000 weight units.

This PR fixes checked_mul_by_weight and its unit tests by scaling the output of these methods down by 1000, so that X sat/vbyte * Y vbytes = X * Y sats, instead of X * Y * 1000 sats as before.

Before

This code would pass without panic before this PR.

let weight = Weight::from_vb(3).unwrap();
let rate = FeeRate::from_sat_per_vb(3).unwrap();
assert_eq!(weight * rate, Amount::from_sat(9));
assert_eq!(rate.checked_mul_by_weight(weight), Some(Amount::from_sat(9000)));

After

let weight = Weight::from_vb(3).unwrap();
let rate = FeeRate::from_sat_per_vb(3).unwrap();
assert_eq!(weight * rate, Amount::from_sat(9));
assert_eq!(rate.checked_mul_by_weight(weight), Some(Amount::from_sat(9)));

Kixunil

Nice catch, thanks!

Could you please add the test from PR description?

bitcoin/src/blockdata/fee_rate.rs

yancyribbens · 2023-11-10T12:42:57Z

Good catch. I agree there is a missing multiple by 1,000. Keeping track of the internal types is one of the reasons why I would like to see something like the changes present here: #1787.

yancyribbens · 2023-11-10T12:37:45Z

bitcoin/src/blockdata/fee_rate.rs

@@ -209,8 +212,11 @@ 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();


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".

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

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.

yancyribbens · 2023-11-10T12:38:24Z

bitcoin/src/blockdata/fee_rate.rs

@@ -95,13 +97,14 @@ impl FeeRate {
    ///
    /// # Examples
    ///
-    /// ```no_run


Why was this no_run?

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.

yancyribbens · 2023-11-10T12:41:00Z

bitcoin/src/blockdata/fee_rate.rs

    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;


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.

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.

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

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)) }

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.

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

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.

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.

Yeah definitively better to not wait for #1787

apoelstra · 2023-11-10T20:32:09Z

ACK from me. If somebody is willing to make a backport PR I'd be happy to merge it so we can do a point release. I also put this on my TODO list but it's likely to be more than a couple weeks before I can do it, vs if it shows up in my Github notifications then I'll probably react quickly.

apoelstra

ACK 0c56131

Kixunil

ACK 6c6c08c

yancyribbens · 2023-11-11T08:04:43Z

Overall looks good. Thanks for putting in this fix.

tcharding · 2023-11-12T21:53:17Z

Can you squash the two patches together please mate. I'll do the backport then with the same patch.

tcharding · 2023-11-12T21:55:04Z

To explain myself; mainly I just didn't want a patch with simply "add second test case" to get merged. I'm "that guy" that always picks people up on basic git stuff, sorry.

tcharding · 2023-11-12T22:12:04Z

I created a backport by squashing this into a single patch, checking out tag bitcoin-0.31.0, creating a new branch 0.31.x, applying the squashed patch, and pushed it straight up to this repo. Is that how we do that or was I supposed to create the branch then PR with the new branch as target?

FTR when I squashed it the original commit author was maintained as @conduition.

Kixunil · 2023-11-12T22:25:30Z

@tcharding I actually prefer fixes like these to have test commits separated as it's easier to check that the test previously failed. I was also thinking of putting this into CI somehow.

tcharding · 2023-11-12T22:53:55Z

Yep I thought about that too, in that case all the tests need to go in a separate patch with a properly written commit log please @conduition.

Kixunil · 2023-11-13T12:46:26Z

Ah, yeah, I missed that the commit message is too brief. While I'm keeping my ACK, I'd be a bit happier if it were more detailed.

apoelstra · 2023-11-13T18:30:27Z

Ok to merge? I think this diff is short enough that I'm not going to get hung up on commit messages, and I agree with Kix that it's better to separate tests if possible, and it sounds like @tcharding was able to do the backport even though it involved futzing with the commit structure.

tcharding · 2023-11-13T19:51:54Z

Ok to merge IMO

conduition · 2023-11-14T16:10:39Z

Thanks all, glad to help. I'll keep the commit message stuff in mind for next time :)

tcharding · 2023-11-14T21:41:34Z

Thanks for the contribution man!

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

0c56131

Kixunil reviewed Nov 9, 2023

View reviewed changes

bitcoin/src/blockdata/fee_rate.rs Show resolved Hide resolved

yancyribbens suggested changes Nov 10, 2023

View reviewed changes

add second test case

6c6c08c

apoelstra approved these changes Nov 10, 2023

View reviewed changes

Kixunil approved these changes Nov 10, 2023

View reviewed changes

tcharding mentioned this pull request Nov 12, 2023

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

Closed

apoelstra merged commit ee787c6 into rust-bitcoin:master Nov 13, 2023
29 checks passed

conduition deleted the fix-checked-weight-fee-rate-mul branch November 14, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

conduition commented Nov 9, 2023

Kixunil left a comment

yancyribbens commented Nov 10, 2023

yancyribbens Nov 10, 2023 •

edited

conduition Nov 10, 2023

yancyribbens Nov 11, 2023

yancyribbens Nov 10, 2023

conduition Nov 10, 2023

yancyribbens Nov 10, 2023

conduition Nov 10, 2023

yancyribbens Nov 10, 2023

yancyribbens Nov 10, 2023 •

edited

conduition Nov 10, 2023

yancyribbens Nov 10, 2023

conduition Nov 10, 2023

Kixunil Nov 10, 2023

yancyribbens Nov 11, 2023

apoelstra commented Nov 10, 2023

apoelstra left a comment

Kixunil left a comment

yancyribbens commented Nov 11, 2023

tcharding commented Nov 12, 2023

tcharding commented Nov 12, 2023

tcharding commented Nov 12, 2023 •

edited

Kixunil commented Nov 12, 2023

tcharding commented Nov 12, 2023

Kixunil commented Nov 13, 2023

apoelstra commented Nov 13, 2023

tcharding commented Nov 13, 2023

conduition commented Nov 14, 2023

tcharding commented Nov 14, 2023

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

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

Conversation

conduition commented Nov 9, 2023

Before

After

Kixunil left a comment

Choose a reason for hiding this comment

yancyribbens commented Nov 10, 2023

yancyribbens Nov 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yancyribbens Nov 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Nov 10, 2023

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

yancyribbens commented Nov 11, 2023

tcharding commented Nov 12, 2023

tcharding commented Nov 12, 2023

tcharding commented Nov 12, 2023 • edited

Kixunil commented Nov 12, 2023

tcharding commented Nov 12, 2023

Kixunil commented Nov 13, 2023

apoelstra commented Nov 13, 2023

tcharding commented Nov 13, 2023

conduition commented Nov 14, 2023

tcharding commented Nov 14, 2023

yancyribbens Nov 10, 2023 •

edited

yancyribbens Nov 10, 2023 •

edited

tcharding commented Nov 12, 2023 •

edited