Followups to #4157: remove Amount::from_sat_unchecked by apoelstra · Pull Request #4256 · rust-bitcoin/rust-bitcoin

apoelstra · 2025-03-18T15:11:23Z

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

The `from_int_btc_const` constructors are specifically designed for easily creating amount types in const context but currently they return an error which is annoying to handle in const context. If we make the `whole_bitcoin` parameter a 16 bit integer this gives us a nicer const constructor with the downside that it can only create values upto a maximum of - unsigned: 65_536 - signed: 32_767 That is plenty high enough for most use cases. Then use the new `from_int_btc_const` in associated consts. Note that because `from_sat` checks max (and min) values we must define max and min from sats directly.

github-actions · 2025-03-18T15:12:29Z

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

coveralls · 2025-03-18T15:14:29Z

Pull Request Test Coverage Report for Build 13931957355

Details

83 of 93 (89.25%) changed or added relevant lines in 8 files are covered.
3 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.006%) to 83.089%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
units/src/amount/mod.rs	11	12	91.67%
units/src/amount/unsigned.rs	18	21	85.71%
units/src/amount/signed.rs	15	21	71.43%

Files with Coverage Reduction	New Missed Lines	%
units/src/amount/mod.rs	1	88.78%
units/src/amount/signed.rs	1	81.05%
units/src/amount/unsigned.rs	1	83.33%

Totals
Change from base Build 13923187550:	0.006%
Covered Lines:	21786
Relevant Lines:	26220

💛 - Coveralls

apoelstra · 2025-03-18T15:25:01Z

API breaks are as expected: renaming from_int_btc_const and deleting from_sat_unchecked.

apoelstra · 2025-03-18T15:25:43Z

Oh, oops, we need to deprecate from_int_btc: https://docs.rs/bitcoin/latest/bitcoin/struct.Amount.html#method.from_int_btc

Edit done

github-actions · 2025-03-18T15:32:23Z

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

yancyribbens · 2025-03-18T15:46:28Z

units/src/amount/signed.rs

-            None => panic!("cannot overflow in i64"),
+        let sats = btc * 100_000_000;
+
+        match SignedAmount::from_sat(sats) {


I tend to agree that just a comment saying why this won't panic is more readable. Otherwise I might read this and think why is there an err(_) arm.

I guess performance doesn't matter here since it's const context? There are some Acks for this PR so I know it's not going to be changed now, I'm just mostly curious for my own understanding.

yancyribbens · 2025-03-18T16:09:09Z

units/src/amount/unsigned.rs

        ///
-        /// Caller to guarantee that `satoshi` is within valid range. See [`Self::MAX`].
-        pub const fn from_sat_unchecked(satoshi: u64) -> Amount { Self(satoshi) }
+        /// Accepts an `u32` which is guaranteed to be in range for the type, but which can only


Suggested change

/// Accepts an `u32` which is guaranteed to be in range for the type, but which can only

/// Accepts a `u32` which is guaranteed to be in range for the type, but which can only

Can you please post what commit you're commenting on? Github shows 3 or 4 lines of context and nothing else so it is very hard to figure out what change you're talking about without further hints.

It's 61c54aa

But it's unrelated, so not needed for this PR. Could be added to the typos list.

It's just a minor nit on grammar: beaa2db#diff-0ced1794ac364cd494a4351ae6baed7b4d14d3dc673b945f3eae8f83e4e93844R62

Should be a instead of an since followed by something that is or sounds like a vowel.

nm this is now i32

should still be a I think but feel free to disregard my nit picking of course.

yancyribbens · 2025-03-18T16:14:48Z

units/src/amount/unsigned.rs

    /// in const context.
    #[allow(clippy::missing_panics_doc)]
-    pub const fn from_int_btc_const(whole_bitcoin: u16) -> Amount {
+    pub const fn from_btc_u16(whole_bitcoin: u16) -> Amount {


Should this be u32? The idea being that u32 max is greater than max number of bitcoins 21 million? u16 is 65,535 which isn't enough to represent every bitcoin.

Ok, I guess you mean to have this for cases where only a small number of bitcoin are represented and to use from_btc for other cases? It seems confusing but if that's the intent then it's not a mistake.

Ah I see, this allows for a restricted bounds since u32 max would be 429496729500000000 sats which is greater than the maximum number sats. Sorry, didn't think that through before.

yancyribbens · 2025-03-18T16:18:57Z

units/src/amount/unsigned.rs

+    #[allow(clippy::missing_panics_doc)]
    pub fn to_signed(self) -> SignedAmount {
-        SignedAmount::from_sat_unchecked(self.to_sat() as i64) // Cast ok, signed amount and amount share positive range.
+        SignedAmount::from_sat(self.to_sat() as i64) // Cast ok, signed amount and amount share positive range.


The comment and the expect message are saying the same thing. Maybe just move the comment into the expect message?

No, they're not. The comment is about the cast. The expect is about the call to from_sat.

Ok, I see that now.

Kixunil

Cool! The PR improved more things than I expected. Just a few suggestions to make it even better.

Kixunil · 2025-03-18T16:07:07Z

units/src/amount/signed.rs

+    /// in const context.
+    #[allow(clippy::missing_panics_doc)]
+    #[deprecated(since = "TBD", note = "renamed to `from_btc_i16`")]
+    pub const fn from_int_btc_const(whole_bitcoin: i16) -> SignedAmount {


This also changed type so wouldn't it be better to backport the deprecation?

Ah, you mean, backport only the deprecation but not the type change?

To be clearer:

Deprecate the from_int_btc in 0.32.x and backport the new from_btc_i16 to 0.32.x

Then just delete from_int_btc in master since its type changed (and the old type signature is untenable because of the MAX_MONEY changes)

Yes, that's what I meant.

Changed to just delete the old method. Separately we should backport a deprecation of it.

units/src/amount/mod.rs

Kixunil · 2025-03-18T16:37:16Z

units/src/amount/unsigned.rs

+                return Err(ParseAmountError(ParseAmountErrorInner::OutOfRange(
+                    OutOfRangeError::negative(),
+                ))),
+        };


Yeah, if we returned SignedAmount this entire thing would simplify to if is_neg { return ... }

Kixunil · 2025-03-18T16:43:10Z

units/src/amount/unsigned.rs

        ///
-        /// Caller to guarantee that `satoshi` is within valid range. See [`Self::MAX`].
-        pub const fn from_sat_unchecked(satoshi: u64) -> Amount { Self(satoshi) }
+        /// Accepts an `u32` which is guaranteed to be in range for the type, but which can only


It's 61c54aa

But it's unrelated, so not needed for this PR. Could be added to the typos list.

Kixunil · 2025-03-18T16:47:18Z

units/src/amount/unsigned.rs

    pub fn to_signed(self) -> SignedAmount {
-        SignedAmount::from_sat_unchecked(self.to_sat() as i64) // Cast ok, signed amount and amount share positive range.
+        SignedAmount::from_sat(self.to_sat() as i64) // Cast ok, signed amount and amount share positive range.
+            .expect("range of Amount is within range of SignedAmount")


Honestly, this is the kind of thing where I'd prefer unchecked (by turning it private instead of deleting). The compiler doesn't understand our invariant (even niche-filling optimization that I came up with cannot convey it perfectly, just some other upper bound), so this won't get optimized out in non-trivial code and it's the kind of thing where one wouldn't expect a performance hit.

But it's fine to leave it as-is for now. Even wait until someone complains.

If you think the compiler won't understand this, we could do a match and an unsafe core::hint::unreachable_unchecked. I'd rather do that than introduce a from_sat_unchecked for a single call site.

Regardless, I think let's leave it for now.

OK, that's a reasonable approach.

We have `from_int_btc_const` on both `Amount` and `SignedAmount` because the "real" `from_int_btc` is generic over the integer type it accepts, which means that it cannot be a constfn. But we do want a constfn. However, just because `from_int_btc_const` exists for the sake of constfn doesn't mean that that's what it *is*. So rename these methods to reflect what they *are*.

We have a ton of calls to `from_sat_unchecked` for small constants which were clearly in range, e.g. in fee.rs. Add a new constfn for these cases. Don't bother making a generic Into<u32>/Into<u16> variant because there isn't an obvious name for it. There are 7 instances where we're using this method with values that are out of range, which we leave as from_sat_unchecked for now.

There are only 7 instances of this so just call .unwrap() on each one.

There is no need for this. It's a 2-line diff to change it.

This private function is used for string-parsing an amount. It returns a sign boolean and a u64, but its surrounding logic can be simplified if it just returns a i64 (which is OK now since the range of `Amount` fits into the range of i64). Along the way we eliminate some instances of from_sat_unchecked. Annoyingly we still need the bool to distinguish -0 from +0; when parsing Amount we reject -0 (and we have tests for this). This causes a couple error constructors to no longer be used outside of unit tests. May be worth looking at whether we need them, or whether we should be using them in parse_signed_to_satoshi.

…undary It's conceptually a bit tortured to have an `Amount` type defined in a private module, with an _unchecked method allowing you to set values out of range, which needs to be used outside of the module to *define* the range and the constructors that check it. Move the constants and constructors inside the privacy module, where they can be written directly. This is easier to understand and eliminates a couple _unchecked calls.

Turns out we don't even need this. It is only used in one place now and we can just stick an .expect there.

github-actions · 2025-03-18T19:35:17Z

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

Kixunil

ACK 2958521

Kixunil · 2025-03-18T21:23:37Z

units/src/amount/unsigned.rs

-            )));
-        }
-        Ok(Self::from_sat_unchecked(sats))
+        Self::try_from(amount).map_err(|e| ParseAmountError(ParseAmountErrorInner::OutOfRange(e)))


Hmm, the error should be unreachable here since we return early if it's negative. It's not trivial to prove. The only somewhat sensible idea I have is something like this:

/// Like SignedAmount but can represent -0 /// Returned from `parse_signed_to_satoshi` enum FatAmount { Positive(Amount), Negative(Amount), } // in parse_signed_to_satoshi if is_negative { FatAmount::Negative(ret) } else { FatAmount::Positive(ret) } // in Amount::from_str_in match amount { FatAmount::Positive(amount) => Ok(amount), FatAmount::Negative(_) => Err(...), } // in SignedAmount::from_str_in match amount { FatAmount::Positive(amount) => Ok(amount.into()), FatAmount::Negative(amount) => Ok(-SignedAmount::from(amount)), }

Though it's a bit heavy, so if you don't think it's worth it it's fine.

I think it's too heavy. As in the other case, we can use unreachable_unchecked if we want to try to optimize this. But I'd rather not do that now since it's just an optimization and considerably increases the review difficulty.

For now I can replace the map_err with an expect though, since you're right that the error path is actually unreachable now.

Well, it's also a proof that it won't panic or do something weird (for some cases of "weird"). And as my recent findings suggest, doing too many expects is not really that great. :)

But still, I don't mind either map_err or expect in this PR. This can be improved later.

I'm gonna leave it as is to avoid losing your ACK.

apoelstra

On e71aa08 successfully ran local tests

tcharding · 2025-03-19T02:52:01Z

ACK 2958521

apoelstra

On 61c54aa successfully ran local tests

yancyribbens · 2025-03-19T16:18:26Z

units/src/amount/mod.rs

 fn parse_signed_to_satoshi(
    mut s: &str,
    denom: Denomination,
-) -> Result<(bool, u64), InnerParseError> {


It's strange to see u64 replaced by SignedAmount in the diff. I'd have expected to see i64 replaced by SignedAmount.

It's explained in the commit message. I suggest you read them.

I read through them yesterday although I guess this one is new today. Today I was paging through the diff to see what change although I should pay closer attention to what commit messages changed as well.

Discussions also often provide insight.

yancyribbens · 2025-03-19T16:23:42Z

units/src/amount/mod.rs

+    if is_negative {
+        ret = -ret;
+    }
+    Ok((is_negative, ret))


I'm confused why is_negative returned. Doesn't SighnedAmount contain the information about if it's negative using two's compliment?

Yes, but not if it's negative in decimal notation.

That's literally a comment that says it's need for negative zero which I failed to read. Funny since the exclusion of negative zero was also added by me.

s/That's/There's

tcharding · 2025-03-20T04:09:27Z

Gentle bump bro - 12 hours come on now :)

Kixunil · 2025-03-20T09:05:12Z

As long as no other PR comes in that would break this and trigger re-review of this large change I'm fine with waiting. :)

(added P-hight to express that)

apoelstra · 2025-03-20T13:22:55Z

Gentle bump bro - 12 hours come on now :)

Sorry. I queued up two earlier versions of this, so I wound up testing 24 commits for this, plus 6 for #4248, and overnight I leave "away from keyboard" mode on so when it finished tests it statred testing other things instead of merging.

apoelstra

On 2958521 successfully ran local tests

tcharding · 2025-03-20T21:06:14Z

No stress man, I was just winding you up.

ca6c607 Adhere to sanity rules for amount types (Tobin C. Harding) 6c614d9 units: Fix panic message (Tobin C. Harding) Pull request description: This is a follow up to #4256 - onwards and upwards! - Patch 1: Fix the incorrect BTC value in panic message - Patch 2: Strictly adhere to the sanity rules (#4090) Close: #4140 ACKs for top commit: apoelstra: ACK ca6c607; successfully ran local tests Tree-SHA512: 6d7fd60830e1a0f6d6262ab02ec6e297b095d0fe8fb7737563979652e4a3b4a9477a79982201c42b08e2555fd23dc5c430549966b534bdf45f40621ae81da83a

github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-units PRs modifying the units crate test labels Mar 18, 2025

github-actions bot added the API break This PR requires a version bump for the next release label Mar 18, 2025

apoelstra force-pushed the 2025-03--followup-4157 branch from e71aa08 to 61c54aa Compare March 18, 2025 15:30

apoelstra mentioned this pull request Mar 18, 2025

units: Make from_int_btc_const take a 16 bit integer #4254

Closed

yancyribbens reviewed Mar 18, 2025

View reviewed changes

Kixunil reviewed Mar 18, 2025

View reviewed changes

apoelstra added 7 commits March 18, 2025 19:27

tests: replace Amount::from_sat_unchecked with from_sat.unwrap

05c8b04

There are only 7 instances of this so just call .unwrap() on each one.

amount: stop using from_sat_unchecked in tests

3370c14

There is no need for this. It's a 2-line diff to change it.

amount: remove from_sat_unchecked

2958521

Turns out we don't even need this. It is only used in one place now and we can just stick an .expect there.

apoelstra force-pushed the 2025-03--followup-4157 branch from 61c54aa to 2958521 Compare March 18, 2025 19:34

Kixunil approved these changes Mar 18, 2025

View reviewed changes

apoelstra commented Mar 19, 2025

View reviewed changes

tcharding approved these changes Mar 19, 2025

View reviewed changes

tcharding mentioned this pull request Mar 19, 2025

units: Improve code enforcing privacy boundry #4258

Merged

apoelstra commented Mar 19, 2025

View reviewed changes

yancyribbens reviewed Mar 19, 2025

View reviewed changes

Kixunil added the P-high High priority label Mar 20, 2025

apoelstra commented Mar 20, 2025

View reviewed changes

apoelstra merged commit e0be90d into rust-bitcoin:master Mar 20, 2025
24 checks passed

apoelstra deleted the 2025-03--followup-4157 branch March 20, 2025 17:32

	/// Accepts an `u32` which is guaranteed to be in range for the type, but which can only
	/// Accepts a `u32` which is guaranteed to be in range for the type, but which can only

Conversation

apoelstra commented Mar 18, 2025

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

coveralls commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13931957355

Details

💛 - Coveralls

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

apoelstra commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yancyribbens Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Mar 18, 2025 •

edited

Loading

apoelstra commented Mar 18, 2025 •

edited

Loading

yancyribbens Mar 19, 2025 •

edited

Loading