units: Make from_int_btc_const take a 16 bit integer by tcharding · Pull Request #4254 · rust-bitcoin/rust-bitcoin

tcharding · 2025-03-17T22:48:04Z

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 of corn for most use cases.

tcharding · 2025-03-17T22:49:35Z

Draft because patch 2 (trying to use the from_int_btc_cons functions for the associated consts) causes an error i do not understand.

For more information about this error, try `rustc --explain E0391`.
error: could not compile `bitcoin-units` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error[E0391]: cycle detected when caching mir of `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::from_sat` for CTFE
   --> units/src/amount/unsigned.rs:105:5
    |
105 |     pub const fn from_sat(satoshi: u64) -> Result<Amount, OutOfRangeError> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: ...which requires elaborating drops for `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::from_sat`...
   --> units/src/amount/unsigned.rs:106:22
    |
106 |         if satoshi > Self::MAX_MONEY.to_sat() {
    |                      ^^^^^^^^^^^^^^^
note: ...which requires simplifying constant for the type system `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::MAX_MONEY`...
   --> units/src/amount/unsigned.rs:82:5
    |
82  |     pub const MAX_MONEY: Self = Amount::from_int_btc_const(21_000_000);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating + checking `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::MAX_MONEY`...
   --> units/src/amount/unsigned.rs:82:5
    |
82  |     pub const MAX_MONEY: Self = Amount::from_int_btc_const(21_000_000);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: ...which again requires caching mir of `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::from_sat` for CTFE, completing the cycle
note: cycle used when const-evaluating + checking `amount::unsigned::<impl at units/src/amount/unsigned.rs:72:1: 72:12>::ONE_BTC`
   --> units/src/amount/unsigned.rs:78:5
    |
78  |     pub const ONE_BTC: Self = Amount::from_int_btc_const(1);
    |     ^^^^^^^^^^^^^^^^^^^^^^^
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

apoelstra · 2025-03-17T23:36:44Z

@tcharding without looking, my guess is that you've used MAX_MONEY in a constfn that is used to define MAX_MONEY.

tcharding · 2025-03-18T02:51:19Z

Ha, correct! Thanks man.

Kixunil

Not objecting to the PR but I'd prefer changing the outer match for code clarity.

Kixunil · 2025-03-18T06:33:48Z

units/src/amount/signed.rs

-    /// If `whole_bitcoin` is greater than `21_000_000`.
    #[allow(clippy::missing_panics_doc)]
-    pub const fn from_int_btc_const(whole_bitcoin: i32) -> Result<SignedAmount, OutOfRangeError> {
+    pub const fn from_int_btc_const(whole_bitcoin: i16) -> SignedAmount {


TODO: rename the function to from_i16_btc (or from_btc_i16)

(Can be postponed to a future PR.)

Will leave for someone else.

Kixunil · 2025-03-18T06:35:05Z

units/src/amount/signed.rs

-    pub const fn from_int_btc_const(whole_bitcoin: i32) -> Result<SignedAmount, OutOfRangeError> {
+    pub const fn from_int_btc_const(whole_bitcoin: i16) -> SignedAmount {
        let btc = whole_bitcoin as i64; // Can't call `into` in const context.
        match btc.checked_mul(100_000_000) {


Checked multiplication is pointless and annoying here. If it can't overflow, just use the normal * operator.

Kixunil · 2025-03-18T06:35:51Z

units/src/amount/unsigned.rs

+                Ok(amount) => amount,
+                Err(_) => panic!("unreachable - 65536 BTC is within range")
+            }
+            None => panic!("unreachable - mul by 65536 cannot overflow a u64"),


Same issues in this section as above.

Kixunil · 2025-03-18T06:39:28Z

units/src/amount/signed.rs

+    pub const ONE_BTC: Self = SignedAmount::from_int_btc_const(1);
    /// Exactly fifty bitcoin.
-    pub const FIFTY_BTC: Self = SignedAmount::from_sat_unchecked(50 * 100_000_000);
+    pub const FIFTY_BTC: Self = SignedAmount::from_int_btc_const(50);


Nice getting rid of these unchecked conversions!

Kixunil · 2025-03-18T07:47:30Z

units/src/amount/signed.rs

-                Err(_) => panic!("unreachable - 65536 BTC is within range")
-            }
-            None => panic!("unreachable - mul by 65536 cannot overflow a i64"),
+        let sats = btc * 100_000_000;


Did you mean to do this in previous commit?

Bother, my bad. Thanks man

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.

tcharding · 2025-03-18T08:09:52Z

I just squashed the whole thing into a single patch because I'm lazy right now.

apoelstra

ACK dfc2562; successfully ran local tests

apoelstra · 2025-03-18T14:14:33Z

I'm going to make another PR building on this one, and maybe we can just merge that instead.

apoelstra · 2025-03-18T15:33:20Z

#4256

Kixunil

ACK dfc2562

apoelstra · 2025-03-18T19:23:55Z

@tcharding ok if we close this in favor of #4256? It has a copy of this commit except that I changed the formatting on two lines (cargo +nightly fmt was otherwise clean), so if I merge this now then it'll cause a conflict.

In any case I'll hold off on merging til you respond.

Kixunil · 2025-03-18T20:18:23Z

OK, I'll wait with review of #4256 until Tobin responds too. Though since that one has no ACKs, how about you just rebase it on top of this one? I could then review it right away and this one can go in. (Though I think it messes up hash commits unles fast forward merge is used and we don't do those here? Maybe we should consider pijul, LOL.)

apoelstra · 2025-03-18T20:37:24Z

Though since that one has no ACKs, how about you just rebase it on top of this one?

I can do that, but then I can't do git rebase -x 'cargo +nightly fmt --check' master on the result and expect it to run cleanly, which is an extra bit of friction when I'm sanity-checking my own commits.

tcharding · 2025-03-18T20:42:05Z

Close/merge either one man. For the record you are always welcome to do surgery on my PRs and/or patches taking any bits you like. No attribution needed ever. Code is a liability to me not an asset - if it merges under your name hopefully if/when its found buggy I'll forget that I wrote it and think the bugs were yours.

Leaving you to close in case you need it open, so I don't have to think about the process.

apoelstra · 2025-03-18T20:45:30Z

if it merges under your name hopefully if/when its found buggy I'll forget that I wrote it and think the bugs were yours.

Lol!!

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

github-actions bot added the C-units PRs modifying the units crate label Mar 17, 2025

tcharding mentioned this pull request Mar 17, 2025

Enforce MAX_MONEY invariant in amount types #4157

Merged

tcharding force-pushed the 03-18-from-int-btc branch from f3335be to cb8ef19 Compare March 18, 2025 02:51

tcharding marked this pull request as ready for review March 18, 2025 02:51

Kixunil reviewed Mar 18, 2025

View reviewed changes

tcharding force-pushed the 03-18-from-int-btc branch from cb8ef19 to 1f4e76d Compare March 18, 2025 07:43

Kixunil reviewed Mar 18, 2025

View reviewed changes

tcharding force-pushed the 03-18-from-int-btc branch from 1f4e76d to dfc2562 Compare March 18, 2025 08:09

apoelstra approved these changes Mar 18, 2025

View reviewed changes

apoelstra mentioned this pull request Mar 18, 2025

Followups to #4157: remove Amount::from_sat_unchecked #4256

Merged

Kixunil approved these changes Mar 18, 2025

View reviewed changes

apoelstra closed this Mar 18, 2025

Conversation

tcharding commented Mar 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

apoelstra commented Mar 17, 2025

Uh oh!

tcharding commented Mar 18, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 18, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

Kixunil commented Mar 18, 2025

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Mar 17, 2025 •

edited

Loading

tcharding commented Mar 18, 2025 •

edited

Loading