Add a checked version of weight mul fee_rate #1864
Conversation
dcadc1a to
856796d
Compare
bitcoin/src/blockdata/fee_rate.rs
Outdated
| /// | ||
| /// Computes `self * rhs` where rhs is of type Weight. `None` is returned if an overflow | ||
| /// occured. | ||
| pub fn checked_weight_mul(self, rhs: Weight) -> Option<Amount> { |
There was a problem hiding this comment.
Maybe checekd_mul_by_weight would sound better?
There was a problem hiding this comment.
+1 I rescind that +1. Caveat, I've not been following the fee rate / weight stuff super closely but I was surprised to see FeeRate::checked_mul returns Self as expected but FeeRate::checked_weight_mul returns Amount (so FeeRate * Weight == Amount) - so this return value is a fee, right? Forgive me if this has been discussed but should the function name express what the calculation means instead of just what it is doing?
There was a problem hiding this comment.
Yeah, I thought about it but I couldn't quickly find a name for that and thought being consistent is nice. If you have some name you like feel free to write it. :)
There was a problem hiding this comment.
I'm fine with checekd_mul_by_weight.
@tcharding I was thinking this is simply the checked version of the this trait which also returns Amount. If you have an idea of where to place a helper function called fee() or something like that which does the same thing that works also.
There was a problem hiding this comment.
I think just use checked_mul_by_weight and leave any further investigation of the API to #1639
856796d to
b03c24d
Compare
…down by 1000 6c6c08c add second test case (conduition) 0c56131 fix: FeeRate::checked_mul_by_weight should scale output down by 1000 (conduition) Pull request description: 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. ```rust 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 ```rust 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))); ``` ACKs for top commit: Kixunil: ACK 6c6c08c Tree-SHA512: aa7b0b6237d9b18057dd7c5df12740f87bd9601acc0cac588b73a2d7a1c35b1581532d0de888230296623fa166baff4b9df89d1f2f73399f44a24dcb9c75eac4
Add a checked version of fee_rate * weight. While I like the trait version of just being able to multiply
feerate * weight, it's not really very useful imo since a large input feerate could cause an overflow. Instead of changing the trait in #1849 (not idiomatic enough I guess) I added achecked_weight_mulmethod toFeeRate.