I'm starting to think from_sats_unchecked should just be removed once unwrap() can be used in const context.

Yes, we've agreed elsewhere on removing it. If anyone wants to keep it I want it to be unsafe.

Or at least make the constructor private. I'm fine with having the checked function in encapsulated module.

Note to self: this change makes the docs in result stale. After #4179 goes in include a change that updates the docs since NumOpResult is now used in ops impls that return an Amount not just in ops on amounts.

Since I had to rebase to fix the review suggestions I just grabbed the new result rustdocs from #4179 (comment) and rolled it into the fee change in 89751fc

Good idea, we can later just change this to |x| Amount::from_sat(x).unwrap.
But it's repeated in multiple tests so why not just go all the way and define the function only once for the whole module? We can then just change one definition and it'll work fine.

No worries, I can do that.

Since we've agreed to remove the unchecked constructor we shouldn't use it here.

I was trying to leave the _unchecked discussion out of scope for this PR because I didn't follow the conversation closely. Also since we cannot use a fallible constructor in const AFAIK then it seems reasonable ATM to use the _unchecked constructor. Is this too much code churn?

The result of that discussion was to remove the function. You don't have to make the change right now but adding more calls to it if we know we will change them for sure just adds noise for no reason. We're better off just dropping the commit that adds them, because those lines will change in any case.

we cannot use a fallible constructor in const AFAIK

We can for a while, our MSRV allowed that like year ago if not more. But also we don't have any fallible constructor in this commit so this is non-issue.

Is this too much code churn?

Yes, because that churn has literally zero value. So churn-to-value ration approaches infinity.

But also we don't have any fallible constructor in this commit so this is non-issue.

Come on now bro, this whole PR is preparation for enforcing the MAX_MONEY invariant - you are having a lend of me.

Just to re-cap. The amount module is currently borked. Normal patching procedure does not apply because we are not trying to maintain the non-existent broken-ness, nor are we trying to maintain a releaseable state.

The new sanity policy is to have one public constructor that accesses the inner field. So if we are leaving the _unchecked issue out of scope for this PR it makes logical sense to use from_sat_unchecked as the one public function.

Furthermore I don't know what you mean about using a fallible constructor in const for associated consts, there is no unwrap in const and we can't touch the inner field directly?

It is extremely difficult to go from the current broken state to the state we want to release without doing many many changes. I've done my best to make the changes at least discreet and kind of sane. I expected there to be a shit fight to get this in but 'don't change that line if we are going to change it again later' is just a pain in the arse. And I've been breaking it up and re putting it together for three months. Can we just merge something and iterate please.

Furthermore I don't know what you mean about using a fallible constructor in const for associated consts, there is no unwrap in const and we can't touch the inner field directly?

Well, you can still workaround the missing unwrap, right? For example, consts can be written as:

    pub const ZERO: Self = match SignedAmount::from_sat(0) {                                                                                                               │   |
        Ok(s) => SignedAmount::ZERO,                                                                                                                                       │75 |     pub const ZERO: Self = match SignedAmount::from_sat(0) {
        Err(e) => panic!("oops")                                                                                                                         │   |     ^^^^^^^^^^^^^^^^^^^^
    };

Would be nice if we could just bump to 1.83 and avoid that since it's an ugly hack. I don't think we should add it though and then pull it later because that's kind of annoying to deprecate for downstream imo.

Correct but that is way too ugly for associated consts IMO for very little additional value.

Correct but that is way too ugly for associated consts IMO for very little additional value.

We already have a lot of this going on with the understanding that it's temporary until things are available in MSRV

I object to uselessly modifying the same bunch of lines twice. It's not helpful in any way to change these to use the _unchecked version if we know for sure we will be changing them back. It just wastes review time.

And regarding unwrapping in consts, just define a private const fn unwrap<T>(opt: Option<T>) -> T and then type unwrap(Amount::from_sat(whatever)). Or maybe consider the macro with compile-time check I suggested.

Good idea.

Or at least make the constructor private. I'm fine with having the checked function in encapsulated module.

::MAX looks a lot like u64::MAX and for Bitcoin devs an amount is intrinsically a 64 byte unsigned integer.

Maybe, but not for Rust devs, where MAX represents maximum value a type can hold. I find it an argument for MAX.

The compiler will prevent any mixups. But we should deprecate anything in older versions that unconditionally produces amounts > 21M BTC.

I accidentally commented on the followup PR, although I agree that MAX is better than MAX_MONEY here.

The compiler will prevent any mixups. But we should deprecate anything in older versions that unconditionally produces amounts > 21M BTC.

Are you suggesting deprecating parse_signed_to_satoshi? It would be a good idea to look into modifying the function so it doesn't produce values greater than 21 BTC. Although, it's private so no need to deprecate and also that would slow down this PR so better to look into it later prob.

That one doesn't produce high amounts unconditionally.

So 2 votes for using MAX and not MAX_MONEY. @apoelstra do you want to weigh in, if not I'll change it to that.

BTW in any case I think we'll need to deprecate Amount::MAX in the old version saying it's creating absurdly large amounts and its semantics will change.

Only use MAX and at the same time deprecate MAX - you've lost me?

I think he's pointing out that if you're using Amount::MAX for values greater than MAX_MONEY now, and the the API changes to limit Amount::MAX to MAX_MONEY that could be a surprise (since this const is public).

Yeah MAX is fine.

Now uses MAX everywhere and references this thread in the commit log to justify the change.

@yancyribbens exactly!

I think there is the possibility for catastrophe changing MAX from u64::MAX to MAX_MONEY without deprecation. Someone could be using this constant thinking that what they are getting is u64::MAX doing some computations etc. After upgrading they are going to get a completely different result without any warning. However, that change to MAX was done a while ago: #3719

FTR, this bad name got in and IIRC I was complaining about it.

not to pile on with critiques, although I don't know why whole_bitcoin. But that doesn't pertain to this PR.

We likely all need to go over the amount module again with a fine tooth comb and a bunch of issues will fall out. Can we leave rename till then?

Sure, this was just a side note.

Since it never overflows, might as well use *. But it'll still panic later.

Since it never overflows,

It can over flow. 21 million * 100000000 is greater than i32::MAX. It would _not_overflow had this been left as i64.

? It's literally converted at the beginning.

Oh I see. Sorry I was looking at the function as it exists on master before this change.

Yeah, good call that this won't overflow so the checked_mul isn't needed.

Since it never overflows, might as well use *. But it'll still panic later.

I swear last time you said "might as well use checked arithmetic to make it explicit that it can't overflow" - either way its super easy to later change in a smaller PR - can we leave it as is?

I myself have done backflips like that in subsequent reviews so no hard feelings!

I swear last time you said "might as well use checked arithmetic to make it explicit that it can't overflow"

I think we're talking apples and oranges. I'm certain I definitely didn't say such thing about arithmetic operators. I think what you have in mind is my suggestion to use Amount::from_sat(42).unwrap() in tests rather than unwrap_unchecked but in this case we're not handling the value returned from from_sat but the value that goes into it.

But yes, we can keep it.

You can probably avoid this by using *.

Didn't we agree that these are ridiculously unlikely and not worth it? I can't remember.

Didn't we agree that these are ridiculously unlikely and not worth it

Since it's already here, I don't see a downside to being cautious here.

It can only happen if something is attempting to produce a script that is 175000000 times longer than the block size. To my knowledge, everyone just uses one of the standard block templates, so it's valid by construction.

Doing it like this makes the next change (making from_sat fallible) way easier. Last time I did not do that and it turned out too messy. So this is way cleaner. We can always re-visit if we don't want the Option and prefer to panic.

How does it make it way easier if you can simply use from_sat(x).unwrap()?

Anyway, don't take this as blocking, just curiosity.

Its easier because one doesn't have to reason about what values cause a panic - which I did last time and still got wrong.

Anyway, don't take this as blocking, just curiosity.

Cool, thanks.

Oh, I see. OK.

Opened #4221

We should address this before release, but fine to leave it open during refactors.

Yeah a downside to this change is it is disruptive.

There is a mixture of the use of Self:: and Amount:: or SignedAmount:: in signed, for no obvious reason.

Self is generally better because it protects against renames of types or added generics and such. But I don't think it'll ever be done here. I don't think this code is worth changing.

It was more of a general comment than pulling out that one occurrence. It's just one example where the same function in unsigned and signed are different pub const fn from_sat_unchecked(satoshi: i64) -> SignedAmount { SignedAmount(satoshi) }. The functions throughout seem to vary in which they use.

But it can be changed later, or not at all.

Good, thanks. Promoted to #4210

There are a few cases of this that were missed.

Thanks man: #4211