hmm this compiles fine on my local machine but I didn't notice the warning "invalid feature rust_v_1_57 .." which causes CI to fail. Looks like this needs more work.

Should be using conditional configuration option not a feature (that means
#[cfg(rust_v_X_Y)]). There is another instance of using it as a feature in the code currently which is wrong.

We could also tack an _unchecked suffix onto it and then do something nasty (e.g. replace with 0) on invalid input.

Even without _unchecked I wouldn't be too opposed to having it silently replace with MAX_MONEY, or do an infinite loop, or something. It just feels bad to compiler-gate a method to deal with an error case which should never happen.

We could also just add 'unchecked` to the end and let the default behavior of overflow in release mode or panic in debug mode happen. I'm fine with that as long as unchecked is added. I think it could happen if applications don't sanitize user input.

or panic in debug mode happen

Well, we can't panic under any circumstances because we're trying to make a constfn.

We could overflow in both release and debug mode, I suppose.

I'm afk right now so I can't test, however I was thinking const fn will still panic in debug mode if for example u64::max * 2 is calculated, otherwise in release mode it silently overflows.

I tested on the rust playground and I guess since it's const fn the compiler won't allow overflows to compile. Although I'm not sure how the sementics work with const fn if user args are provided. Maybe that's not possible.

I tested on the rust playground and I guess since it's const fn the compiler won't allow overflows to compile. Although I'm not sure how the sementics work with const fn if user args are provided. Maybe that's not possible.

This is potentially very cool, if we can exploit that by transforming arbitrary "invalid inputs" into integer overflows.

How about this, it leaks the internal representation in the docs but every dev knows that bitcoin is stored in sats as a u64.

    /// Creates an [`Amount`] from a value representing a whole number of bitcoin.
    ///
    /// Internally computes the number of satoshis (ie, `btc * 100_000_000`)
    /// saturating at the numeric bounds of `u64`.
    pub const fn saturating_from_int_btc(btc: u64) -> Amount {
        Amount(btc.saturating_mul(100_000_000))
    }

and a unit test

    #[test]
    fn const_saturating_from_int_btc_overflow() {
        let got = Amount::saturating_from_int_btc(u64::MAX - 10);
        let want = Amount(u64::MAX);
        assert_eq!(got, want);
    }

OT: Why don't we have a const SATS_PER_BITCOIN instead of all the 100_000_000 in the code base?

OT: Why don't we have a const SATS_PER_BITCOIN instead of all the 100_000_000 in the code base?

Amount::ONE_BTC exists... maybe use that everywhere?

How about this

If we make a saturating one, I'd expect it to saturate to a whole BTC... maybe even 21 mil (MAX_BTC)

u64::MAX is kind of weird, since it's not a whole bitcoin.

How about this

If we make a saturating one, I'd expect it to saturate to a whole BTC... maybe even 21 mil (MAX_BTC)

Let's not have the whole "what does max bitcoin amount mean" debate here, its been argued before and is unlikely to be resolved IMO.

u64::MAX is kind of weird, since it's not a whole bitcoin.

I was under the impression that everyone knows that bitcoin (in general not just in rust-bitcoin) uses ints not floats so this behaviour is obvious but that's coming from the perspective of a low level C guy with a history of disliking floats - am I wrong?

I just think it's weird that

184467440737 will result in Amount(18446744073700000000)
but
184467440738 and above will result in Amount(18446744073709551615)

Since the whole point of the function is to turn 10 into 10 BTC worth of an Amount...

But again, I think this is just personal preference, and if it's documented anything is fine.

Cool, thanks for getting the bounds. So if I understand you clearly this leads to something like (excluding the exact name):

pub const: u64 MAX_EXPRESSIBLE_WHOLE_BITCOIN_IN_SATS = 18446744073700000000;

and we saturate to that. I agree with the sentiment, the reason I'd like to avoid this is because it provides an answer to the "what is the meaning of max bitcoin" question. Currently we implicitly define max bitcoin as u64::MAX sats and so far we have been unable to get any improvement on that, at least that is how I understood the debate.

What if, in this PR, we saturate to Amount::MAX, state as such in the docs, then leave the whole "what is Amount::MAX, and what does it even mean" discussion for later work on the amount module (which is getting a bunch of work at the moment). Would that be a good compromise?

Sounds perfect.

BTC_MAX (21 mil BTC) and MAX seems confusing though.

concept ACK from me on saturating as long as it's documented. The warning should probably be in bold, and we should briefly explain that we'd like to just panic but can't in a constfn at our current MSRV. I don't care where we saturate at. I mildly prefer we not provide an exposed constant for this because that will commit us to it. (Though OTOH if we commit to anything, u64::MAX seems like the most technically justifiable value.)

I also think that we should explore whether we can just use * rather than saturating_mul, and if that will give us the desired "panic in debug mode, overflow in release mode, refuse to compile in a compile-time-evaluated constfn" behavior.

You continue prodding finally tweaked my interest, it behaves as we want - see: #1883

I'm not convinced saturating is that useful, we should just use the array out of bounds trick to panic in const.

I also don't see why we should bother with saurating_mul. As I understand, const context evaluates when compiled, and if there is an overflow it's better to have a compiler error and not compile instead of saturating.

I'm not convinced saturating is that useful, we should just use the array out of bounds trick to panic in const.

It looks like there is a lint that prevents the array oob trick 🤦

If the OOB trick isn't possible, maybe feature gate this so it's only available in 1.57+?

You could either disable the lint or just remove the let binding.

Sorry for the delay, was afk for a bit

git grep _000_000

Oh nice, git grep is dope. Or better yet, git grep -n _000_000 so one can find the line num.

I use a shell alias to git grep -IPn --color=always

There are a bunch more places this new function could be used if you get super motivated (git grep _000_000).

This is the const version, so it's really only useful for other const functions.

Woops :) Yes its const but its also more terse. Not to worry, we can do it at another time.

A bunch of unrelated changes have snuck into the PR, can you remove them or explain them please? (The PR describtion is stale now too.)

Thanks man

Sorry about that. It looks like I pulled in some old state from master on accident.

ACK 9f7449b