Enforce MAX_MONEY invariant in amount types#4157
Enforce MAX_MONEY invariant in amount types#4157apoelstra merged 1 commit intorust-bitcoin:masterfrom
Conversation
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
|
In 6bd4ca6: Looks good (using diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs
index a62e1e59..43528eb1 100644
--- a/units/src/amount/unsigned.rs
+++ b/units/src/amount/unsigned.rs
@@ -56,7 +56,13 @@ impl Amount {
/// Constructs a new [`Amount`] with satoshi precision and the given number of satoshis.
///
/// Caller to guarantee that `satoshi` is within valid range. See [`Self::MAX_MONEY`].
- pub const fn from_sat_unchecked(satoshi: u64) -> Amount { Self(satoshi) }
+ pub const unsafe fn from_sat_unchecked(satoshi: u64) -> Amount {
+ if satoshi > 21_000_000 * 100_000_000 {
+ core::hint::unreachable_unchecked()
+ }
+
+ Self(satoshi)
+ }
/// Gets the number of satoshis in this [`Amount`].
///and confirmed that, after fixing all the call sites, everything compiled ok even in a const context. So I think this change is good to go (I had been worried that we were closing off the future ability to mark the function unsafe, but it looks like nope.) Also, can we move 166785d (last commit) to a separate PR? Most of the other commits are obvious and unobjectionable and I worry that we'll have a bunch of review comments on the last one which will make the whole PR hard to think about. |
BTW @Kixunil elsewhere you asked me for an example of where you might replace "tricky" unsafe code with a branch and an |
| impl SignedAmount { | ||
| /// The zero amount. | ||
| pub const ZERO: Self = SignedAmount(0); | ||
| pub const ZERO: Self = SignedAmount::from_sat_unchecked(0); |
There was a problem hiding this comment.
When unwrap() is available, I think we should just use that. Although it doesn't look like that will be until MSRV 1.83 https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.unwrap
There was a problem hiding this comment.
In the past we have just added TODOs in the code, although preferably there's a better way to remind future us to change these.
units/src/amount/unsigned.rs
Outdated
| /// Warning! | ||
| /// | ||
| /// This type implements several arithmetic operations from [`core::ops`]. | ||
| /// To prevent errors due to overflow or underflow when using these operations, |
There was a problem hiding this comment.
underflow pertains to floats: https://en.wikipedia.org/wiki/Arithmetic_underflow
Arithmetic underflow can occur when the true result of a floating-point operation is smaller in magnitude (that is, closer to zero) than the smallest value representable as a normal floating-point number in the target datatype.
Better to only use overflow here to save confusion
There was a problem hiding this comment.
This code is just moved. Can you raise an issue if you want to see docs fixes for this.
There was a problem hiding this comment.
Yep, can do. I didn't realize this was a code move.
units/src/amount/unsigned.rs
Outdated
| /// To prevent errors due to overflow or underflow when using these operations, | ||
| /// it is advised to instead use the checked arithmetic methods whose names | ||
| /// start with `checked_`. The operations from [`core::ops`] that [`Amount`] | ||
| /// implements will panic when overflow or underflow occurs. Also note that |
There was a problem hiding this comment.
Another use of underflow here too
There was a problem hiding this comment.
As above, code move only.
units/src/amount/unsigned.rs
Outdated
| /// start with `checked_`. The operations from [`core::ops`] that [`Amount`] | ||
| /// implements will panic when overflow or underflow occurs. Also note that | ||
| /// since the internal representation of amounts is unsigned, subtracting below | ||
| /// zero is considered an underflow and will cause a panic if you're not using |
There was a problem hiding this comment.
I think we had this conversation before and it was decided that when subtracting below zero, this is still technically an overflow.
|
In 1702fdd commit message s/weather/whether |
units/src/amount/signed.rs
Outdated
| match parse_signed_to_satoshi(s, denom).map_err(|error| error.convert(true))? { | ||
| // (negative, amount) | ||
| (false, sat) if sat > SignedAmount::MAX.to_sat() as u64 => Err(ParseAmountError( | ||
| (false, sat) if sat > SignedAmount::MAX_MONEY.to_sat() as u64 => Err(ParseAmountError( |
There was a problem hiding this comment.
My feeling is that MAX is more idiomatic, although I'm more opinionated that MAX_MONEY be left out of the docs. Using it in the code doesn't matter as much, but also matching the docs isn't a bad thing.
|
In ef92709 commit message s/Whole bitcoin can non in/Whole bitcoin can not in |
|
Crap, I just noticed I've been commenting on the wrong PR. Comments apply to #4164 |
|
Thanks for the review @yancyribbens I rebase and used them in the other PR. |
94756db to
00a8c1e
Compare
|
There are a few changes to the |
Enforcing the MAX_MONEY invariant is quite involved because it means multiple things: - Constructing amounts is now fallible - Converting from unsigned to signed is now infallible - Taking the absolute value is now infallible - Integer overflow is illuminated in various places Details: - Update from_sat to check the invariant - Fix all docs including examples - Use the unchecked constructor in test code - Comment any other use of the unchecked constructor - Deprecate unchecked_abs - Fail serde (using the horrible string error variant) - Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it. - Use ? in rustdoc examples (required by Rust API guidlines) - Remove TryFrom<Amount> for SignedAmount because the conversion is now infallible. Add a From impl. - Fix the arbitrary impls - Maintain correct formatting - Remove private check_max function as its no longer needed
00a8c1e to
ab4ea7c
Compare
| /// If `whole_bitcoin` is greater than `21_000_000`. | ||
| #[allow(clippy::missing_panics_doc)] | ||
| pub const fn from_int_btc_const(whole_bitcoin: i32) -> SignedAmount { | ||
| pub const fn from_int_btc_const(whole_bitcoin: i32) -> Result<SignedAmount, OutOfRangeError> { |
There was a problem hiding this comment.
In ab4ea7c:
We should either
- Change this to use
i64rather thani32since it can error anyway - Change this to use
i16so that it can't error.
I'm leaning toward the latter. The point of this method is to be a convenient way to construct Amounts with fixed values. 65536 is roughly 5000 million US dollars right now and probably in excess of any fixed values users might care about.
There was a problem hiding this comment.
Change this to use i64 rather than i32 since it can error anyway
It would be better the just remove the checked_mul instead since the None branch will never be reached as is.
There was a problem hiding this comment.
I don't think we can remove it in const code.
There was a problem hiding this comment.
I don't think we can remove it in const code.
I'm not sure why that would be. Maybe we are miscommunicating. This is the change I had in mind which compiles fine:
pub const fn from_int_btc_const(whole_bitcoin: i32) -> Result<SignedAmount, OutOfRangeError> {
let btc = whole_bitcoin as i64; // Can't call `into` in const context.
let amount = btc * 100_000_000;
SignedAmount::from_sat(amount)
}There was a problem hiding this comment.
Di you try compiling what you are suggesting?
There was a problem hiding this comment.
Really this should be unreachable! but that doesn't work in const code.
There was a problem hiding this comment.
Hows this?
pub const fn from_int_btc_const(whole_bitcoin: u16) -> Amount {
let btc = whole_bitcoin as u64; // Can't call `into` in const context.
match btc.checked_mul(100_000_000) {
Some(amount) => match Amount::from_sat(amount) {
Ok(amount) => amount,
Err(_) => panic!("unreachable - 65536 BTC is within range")
}
None => panic!("unreachable - mul by 65536 cannot overflow a u64"),
}
}There was a problem hiding this comment.
FTR I originally used * and was asked in review to use checked_mul
I don't recall that, although I do recall Kix asking to remove the checked_mul in the PR leading up to this.
The panic exists in both cases, the current code is better IMO because it explicitly says why this cannot panic and saves readers of the code wondering why we use * and if it can ever wrap/panic, then having to think about it to satisfy themself.
It's debatable which is more readable. Although it's certainly worse in performance to use checked_mul.
Hows this?
I would say worse. I see that you are explicitly saying in the panic! message that this is unreachable although that seems unnecessary. It's more code for the eyes to read and impacts performance negatively.
There was a problem hiding this comment.
@tcharding I find checked_whatever with such matches horribly unreadable. We have to use it sometimes in const code, so I tolerate those cases but if something can be written in const with * instead, it should be. If you want to explain why it doesn't overflow a comment is both shorter and more readable.
Although it's certainly worse in performance to use
checked_mul.
No, it isn't certain. It depends on where it's expanded and if it gets inlined and how hard is the code for compiler to understand. I get that you got burned by it in your code but that has no relation to other code that's primarily supposed to be const where overflow is always runtime-checked and if you can confidently call unreachable! the compiler can almost certainly figure it out as well even in non-const.
There was a problem hiding this comment.
No, it isn't certain. It depends on where it's expanded and if it gets inlined and how hard is the code for compiler to understand.
Ok noted. Then I would restate that it is certainly the equivalent or worse performance but never performs better.
|
Gonna ack and merge this. I will make a followup which:
|
|
Awesome, thanks. |
Sure, if you're online then go ahead and do it. Otherwise I will do it in 12 hours or so. |
|
Can you also address Yancy's comment about getting rid of a panic? |
|
I won't get back to this till Monday mate. |
|
Happy Monday. Heads up that I did not do this. I spent the whole weekend optimizing the Nix garbage collector. Sorry. |
That sounds fun :D |
|
It was :) I have a working implementation which is a huge speedup but it has behavior around cyclic references that I don't fully understand, so I need to refactor and comment it until I do. |
|
Garbage collecting is a dirty job but somebody's got to do it :) |
(Numbers added by me) I did (2) in #4254. I'm going to leave (1) because I did that once before and it got nacked and I'm a bit prickly already this morning so just recognizing my weaknesses. (3) I don't know what you have in mind, can you either explain or hack it up? |
|
@tcharding I can do both but since @apoelstra said he will do it I'll focus elsewhere unless he retracts that. I can't comment on (1) since I didn't see the nacked PR, in case of (3) the explanation is to just implement the niche-filling optimization which will make overflow unsound, not just broken. |
2958521 amount: remove from_sat_unchecked (Andrew Poelstra) d0d7a15 amount: move MIN/MAX constants and constructors inside the privacy boundary (Andrew Poelstra) 004d073 amount: return i64 from parse_signed_to_satoshi (Andrew Poelstra) 3370c14 amount: stop using from_sat_unchecked in tests (Andrew Poelstra) 05c8b04 tests: replace Amount::from_sat_unchecked with from_sat.unwrap (Andrew Poelstra) beaa2db amount: add from_sat_i32 and from_sat_u32 methods for small constants (Andrew Poelstra) 82d9d1a amount: rename `from_int_btc_const` unctions to hungarian ones (Andrew Poelstra) 2ec1c2a units: Make from_int_btc_const take a 16 bit integer (Tobin C. Harding) Pull request description: This does some followups to #4157. It pulls in #4254, which changes the signature of `Amount::from_int_btc_const` and also replaces a `checked_mul` which can't ever fail with a `*`. It then renames `from_int_btc_const` to `from_btc_u16`/`from_btc_i16` as appropriate, since these names reflect what the function does. It also adds an analogous method for making `Amount`s from sats rather than BTC. Then it systematically removes every instance of `from_sat_unchecked`. There wind up being 7 instances in tests that I just added unwraps() to, plus one instance where we had `let sat = Amount::from_sat_unchecked` which I also added an unwrap() to. In the real code, there was one instance (`Amount::to_signed`) where I needed to add an `expect`. Every other instance I was able to remove, by refactoring logic to use the error case as appropriate. ACKs for top commit: tcharding: ACK 2958521 Kixunil: ACK 2958521 Tree-SHA512: e2c2eb19acdd60da5524f6700e66c140b8e113bec7f2418a2505aeefdbf742eff216fb580891a3ea53255156786f5a6b365b8fb6553e60e42b18d4115d705dd2
|
NACK I just noticed this got merged when for the first time in a few years I finally make some time to contribute to rust-bitcoin 😅 I don't think this change makes sense in principle and created an issue to revert this before release: #4273 It especially doesn't make sense without the addition of a type like |
Enforcing the
MAX_MONEYinvariant is quite involved because it means multiple things:Details:
from_satto check the invariantunchecked_abs?in rustdoc examples (required by Rust API guidlines)TryFrom<Amount> for SignedAmountbecause the conversion is now infallible. Add aFromimpl.check_maxfunction as its no longer neededClose #620