Add integer operations for locktime Height types#2181
Add integer operations for locktime Height types#2181tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
58a4834 to
4e530cd
Compare
30c6022 to
e5bbfd6
Compare
|
I added a comment to #1639 that I think relates to this PR. |
|
I think it makes sense to add two heights. In fact I think it makes more sense than it does to add a height and an integer. |
|
Consider this train of thought.:
At one stage I implemented both |
|
I disagree with point 3. For convenience's sake I think we should have both, but conceptually |
|
No sweat, I'll add both in. |
e5bbfd6 to
91d472b
Compare
|
I think this is very similar to |
|
Good points, I'll re-spin. Thanks |
91d472b to
c6f93e4
Compare
|
Won't get green CI until rebased on top of #2188 because |
|
There's no need to actually use the std |
To improve ergonomics it would be good to support a subset of integer
operations that make sense for the two locktime heights.
- Add `checked_*` and `saturating_*` functions for add, sub, div, mul
- Add functions to increment (add 1) and decrement (sub 1) for each
`Height` type. Call through to the respective `saturating_` function.
- Implement `ops::*` traits, `Add`, `Mul`, `Sub`, `Div` (as well as
`AddAssign` and friends).
It makes sense to add a number of blocks onto both absolute and relative
heights so we implement, for example:
- `ops::Add<u32> for Height`
- `ops::Add<Height> for u32`
- `ops::AddAssign<u32> for Height`
It also makes sense to add a `relative::Height` to an `absolute::Height`
but not two absolute heights together. so we implement:
- relative height:
- `ops::Add for Height`
- `ops::AddAssign for Height`
- absolute height:
- `ops::Add<relative::Height> for Height`
- `ops::AddAssign<relative::Height> for Height`
Same for other operations.
We can implement `Add<u16>` and since the max value of
`relative::Height` and `u16` is the same we get the same unsurprising
behavior in relation to overflow and panic.
For the `absolute` type `absolute::Height::MAX` is less that the inner
ints max value.
As suggested here:
rust-bitcoin#1639 (comment)
implement `ops` traits by unconditionally panicing if overflow occurs.
Done as part of rust-bitcoin#1639 and also to support the calculate next target stuff.
c6f93e4 to
a8d5203
Compare
|
hah, good point. Used |
Kixunil
left a comment
There was a problem hiding this comment.
NACK, this has several problems.
|
|
||
| /// Decrements height by 1, saturating at `Height::MIN`. | ||
| #[inline] | ||
| pub const fn decrement(self) -> Height { self.saturating_sub(1) } |
There was a problem hiding this comment.
These seem arbitrary. There's a reason Rust doesn't have the ++ operator. I suggest having a method called offset that just accepts an int.
| /// | ||
| /// Computes self + rhs, returning `None` if result would overflow `Height::MAX`. | ||
| #[inline] | ||
| pub const fn checked_add(self, rhs: u32) -> Option<Self> { |
There was a problem hiding this comment.
This is not the same type which is unusual and possibly a foot gun but I understand something like that could be handy. I would call it offset to distinguish it from the usual operations.
| /// | ||
| /// Computes self - rhs, returning `None` if result would overflow `Height::MIN`. | ||
| #[inline] | ||
| pub const fn checked_sub(self, rhs: u32) -> Option<Height> { |
There was a problem hiding this comment.
neg_offset or other name?
| } | ||
| } | ||
|
|
||
| impl ops::Add<u32> for Height { |
There was a problem hiding this comment.
This absolutely goes against the idea of having strong newtypes. I really, really, really don't like this.
| fn sub(self, rhs: Height) -> Self::Output { rhs - self } | ||
| } | ||
|
|
||
| impl ops::Mul<relative::Height> for Height { |
There was a problem hiding this comment.
This is definitely wrong. What does multiplying an instant with duration even mean? Or blocks squared? Even if we wanted it it'd need HeightSquared output type.
| } | ||
|
|
||
| impl ops::Mul for Height { | ||
| type Output = Height; |
There was a problem hiding this comment.
This shouldn't exists because there's no HeaightSquared (Or BlockchainArea?)
| } | ||
|
|
||
| impl ops::Div for Height { | ||
| type Output = Height; |
There was a problem hiding this comment.
This could actually exist returning a unitless ratio - the tye should then be u16 though, not Height.
| type Output = Height; | ||
| #[inline] | ||
| fn div(self, rhs: u16) -> Self::Output { Height(self.0 / rhs) } | ||
| } |
| } | ||
|
|
||
| impl ops::Div<Height> for u16 { | ||
| type Output = Height; |
There was a problem hiding this comment.
Unitless.
Edit: just noticed this should be blocks^-1, not unitless. I don't think inverse block count is used anywhere.
| impl ops::DivAssign<u16> for Height { | ||
| #[inline] | ||
| fn div_assign(&mut self, rhs: u16) { self.0.div_assign(rhs) } | ||
| } |
There was a problem hiding this comment.
*Assigns need to be according to the above.
|
hmm, thanks for the review man. These ops PRs are thorny. I'll come back to this. |
To improve ergonomics it would be good to support a subset of integer operations that make sense for the two locktime heights.
checked_*andsaturating_*functions for add, sub, div, mulHeighttype. Call through to the respectivesaturating_function.ops::*traits,Add,Mul,Sub,Divfor two heights as well as height and an integer (on both sides of the equation). Also the assign traits.For operations from
opswe have some additional considerations.relative::HeightWe can implement
Add<u16>and since the max value ofrelative::Heightandu16is the same we get the same unsurprising behavior in relation to overflow and panic.absolute::HeightFor the
absolutetypeabsolute::Height::MAXis less that the inner ints max value.As suggested here: #1639 (comment) implement
opstraits by unconditionally panicing if overflow occurs.Done as part of #1639 and also to support the calculate next target stuff.