Add feature to enable panicking arithmetic

[kana-sama]

Is your feature request related to a problem? Please describe.
We migrated from primitive_types to ruint and one of issues that we have it that sometimes we have unexpected overflows. Previously we could handle it because primitive_types panics on overflows but now we just get silent overflows.

Describe the solution you'd like
Feature flag that when enabled replaces current logic with one that panic on invalid operations.

Describe alternatives you've considered
Newtypes with another impls. But we lose compatibility with libraries like alloy without explicit coercions.

[prestwich]

Related to #408.

Adding this as a feature would mean that consumers of the library could inadvertantly cause subdeps relying on overflows to always panic. i.e. you would end up with library consumers of ruint that are mutually exclusive within a dep tree. So unfortunately, adding it as a feature flag is not an option

We've considered changing the behavior in v2 to match rust's primitives, however, this also leads to a situation where libraries incorrectly rely on overflow checks that vanish in --release builds, which would be a significant source of bugs for consensus and cryptographic applications

The only reasonable choice seems to be to have the basic operator impls ALWAYS overflow, or NEVER overflow, and to not allow downstream consumers to adjust that behavior. I am reluctant to change that behavior, even in a hypothetical v2

One thing we would consider is a checked_uint! { } proc macro to translate in-line math from c = a + b to c = a.checked_add(b).unwrap(). I am unsure when we could prioritize that, but it seems like it would solve your problem?

[Yen]

Assuming the default behaviour was changed to be the same as the default rust primitives and have a debug assert, this does not actually change anything for release build binaries. I think doing this as a new major release should not cause too many unintended bugs in dependent applications as they have to explicitly upgrade their dependency to get it, and if they were depending on the behaviour, that would be caught in tests as a result since those run with debug asserts on.

The only case where this would cause hidden bugs would be where a dependent explicitly updates their version to the new major release, is relying on this overflow behaviour, and does not have test coverage to catch it. I think the test coverage alone should filter out the possibility of it affecting consensus and cryptographic applications as you mention.

Even in the case where the above case would happen, it doesn't affect release builds and so would not impact production environments at all. It would eventually be caught in a debug build or test and updated accordingly.

[prestwich]

I think you're missing my core point, which is that for the target consumers (cryptographic and consensus applications), allowing conditional compilation to change this behavior at all is undesirable. While all choices have footguns, choosing a specific behavior and preserving it for all profiles and feature combinations has the fewest footguns for these applications

I also strongly recommend using explicit function calls instead of implied trait invocations via operators if your logic requires specific behavior

[Yen]

I totally understand that changing behaviour is bad, but this is also something acceptable in a major release where people should be reading what actually changed to warrant it. It is also only an issue for the dependent that directly uses the library who likely knows why and how they are using it. (I cannot make alloy use a new major version of the dependency f.ex, that's up to them to correctly manage.)

I understand that this is the current behaviour, but this is probably the wrong behaviour if we could go back in time. You can look at the rationale for why debug asserts are the correct behaviour when it was formally defined in rust itself. I also understand that changing behaviour when it can impact downstream users without their knowledge is very bad. But in this case, as it would be a major update, it should not be something that we have to worry about as this is the whole point behind major updates being breaking.

My final point is that even if you are worried about bugs slipping in, they will not ever impact production environments. It would be unfortunate for someone to have their tests passing one day, update, and now they fail, but that's the whole point really. The actual damage this could cause in the worst case is minimal from my understanding with this change only ever affecting debug builds.

I also strongly recommend using explicit function calls instead of implied trait invocations via operators if your logic requires specific behavior

Totally agree here, but we are discussing what happens when you don't do that. If you believe this is the answer to this problem, then would you be opposed to deprecating the implementation of the operator traits entirely so that future users of the library don't make the mistake of using them with no downside of breaking existing builds?

[prestwich]

changing the behavior in a major release is fine, and could be considered

Changing the behavior via conditional compilation like this issue and #408 request is not, because the human that chooses the library's assumption is not the human that chooses the conditional compilation flags, and two different libraries may require different assumptions in order to function properly, preventing their use in the same binary. Preventing compile-time changes to this behavior decreases the amount of ruint-specific knowledge that dependents need to communicate to authors of transitive dependents

Given that, there are only two reasonable options for the operators: always or never overflow. I'm not convinced that switching from always to never is a good idea

regardless, this specific issue is wontfix, and @DaniPopes has helpfully added a checkbox to the 2.0 tracking issue, so we can continue the discussion when 2.0 is on the table :)

@kana-sama feel free to open a separate issue if you think a checked_uint! macro would be helpful for your purposes :)

[Yen]

Apologies for getting off topic of this issue, you are correct that I was referring to changing the default and it would be a major release issue.

Regarding the current issue however, could you clarify why having a feature flag to enable this is not possible? The nature of feature flag collision in rust dependencies is not specific to ruint, every other rust package that has a feature flag falls into this category but it does not stop them from adding it. If a feature flag is relevant to users of downstream packages then forwarding feature flags will be added where necessary. I am not sure I understand why ruint is special in this case in that users of the library can't be trusted to manage dependency feature flags correctly?

[prestwich]

well this is what i mentioned earlier

two different libraries may require different assumptions in order to function properly, preventing their use in the same binary

because rust feature flags are intended to be strictly additive, the Cargo dependency resolver will dedupe dependencies and enable the union of feature flags. This means that any library in your dep tree could change critical behavior of ruint which may be relied on by any other library. This is Bad

suppose we added a flag "panicking_arithmetic". lib1 relies on overflowing. lib2 enables panicking_arithmetic and relies on not overflowing. Now lib1 spontaneously panics during normal operation, and lib1 and lib2 can never be used in the same binary, despite both having valid assumptions, and being entirely naive to eachother. This is Bad

How does lib1 communicate to the end user (the human building a binary composed of multiple libraries) that installing lib2 (or some other set of unknown libraries) will cause it to become non-functional? How does the end user debug this issue? How do they resolve it? They really can't resolve it. This is Bad

which goes back to the point: allowing conditional compilation to change this behavior at all is undesirable. Rust's feature system is not designed for behavior modification, only for additive features

[Pzixel]

because rust feature flags are intended to be strictly additive, the Cargo dependency resolver will dedupe dependencies and enable the union of feature flags. This means that any library in your dep tree could change critical behavior of ruint which may be relied on by any other library. This is Bad

That's not entirely true. Take chrono for example, The rkyv-16, rkyv-32 and rkyv-64 features are mutually exclusive. If in your example lib1 relies on rkyv-16 and lib 2 on rkyv-32, you get into the same situation.

To prevent silent errors you can just make two features - panicking-math and wrapping-math. When both enabled it would have two conflicting trait impls, therefore protecting against unexpected feature combination.

As for crypto libraries, I think they should value correctness over performance, and overflowing without explicit wrappin_add is probably ill advised. That's of course not for the current ruint, but 2.0 seems to really need this.

[prestwich]

because rust feature flags are intended to be strictly additive, the Cargo dependency resolver will dedupe dependencies and enable the union of feature flags. This means that any library in your dep tree could change critical behavior of ruint which may be relied on by any other library. This is Bad

That's not entirely true. Take chrono for example, The rkyv-16, rkyv-32 and rkyv-64 features are mutually exclusive. If in your example lib1 relies on rkyv-16 and lib 2 on rkyv-32, you get into the same situation

I'm just repeating what's in the docs. The Cargo dev's intentions and recommendations are clearly spelled out, and the next section continues to discourage mutually exclusive features while documenting your proposed solution. Yes, quite a few packages choose to do it anyway. No, that's not a reason to do it here