Overflow check for feerate traits #1849
Conversation
d45468b to
ef12c78
Compare
| type Output = Amount; | ||
|
|
||
| fn mul(self, rhs: Weight) -> Self::Output { rhs * self } | ||
| } |
There was a problem hiding this comment.
This absolutely cannot be removed - it's the primary way to compute the fee!
There was a problem hiding this comment.
OK good to know. Can you provide an example of how this is used? I tried to come up with a way to test this but found it confusing.
There was a problem hiding this comment.
You literally write let fee = fee_rate * weight;
| fn div(self, rhs: Weight) -> Self::Output { FeeRate(self.to_sat() * 1000 / rhs.to_wu()) } | ||
| fn div(self, rhs: Weight) -> Self::Output { | ||
| self.to_sat().checked_mul(1_000).map(|s| FeeRate(s / rhs.to_wu())) 8000 | ||
| } |
There was a problem hiding this comment.
I don't like this much. It's not idiomatic and perhaps we could actually guarantee that Weight is non-zero? Or have NonZeroWeight type?
There was a problem hiding this comment.
This is checking for a multiplication overflow. Although maybe it should also be performing a checked_div also in case of zero? I'm less concerned about this right now since I'm not using it atm. Just figured I'd add check for overflows while I'm at it.
|
|
||
| fn mul(self, rhs: FeeRate) -> Self::Output { | ||
| Amount::from_sat((rhs.to_sat_per_kwu() * self.to_wu() + 999) / 1000) | ||
| rhs.to_sat_per_kwu().checked_mul(self.to_wu()).map(|s| Amount::from_sat((s + 999) / 1_000)) |
There was a problem hiding this comment.
This one I think should be checked for overflow since FeeRate may be something provided as user input.
@Kixunil The issue is that something like |
|
Yeah, checked versions just like all other integers have make more sense. |
|
closing in favor of #1864 |
b03c24d Add a checked version of weight mul fee_rate (yancy) Pull request description: 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 a `checked_weight_mul` method to `FeeRate`. ACKs for top commit: apoelstra: ACK b03c24d Kixunil: ACK b03c24d Tree-SHA512: 231ade94291becadcea9ea2a40a5daf96b77f01a29cca2494d7bbe4f7de5b412fa8fc816ea249268569f5378410185d9349fd687533bf3a422a752997e107a2b
Add checks for overflow for fee_rate div and mul traits. Also I'm not 100% but I think there is a trait that isn't useful so I've removed it in a separate commit.