Skip to content

Make UInt panic on overflow if overflow_checks is enabled #408

Closed
@u59149403

Description

@u59149403

Currently UInt wraps on overflow. Please, make it panic on overflow if cfg(overflow_checks) is enabled, i. e. exactly same thing rustc does with standard numbers. Here is why.

First and foremost, ruint is used mainly for representing monetary values. Wrapping behavior is unnatural for monetary values. So, UInt should panic on overflow. But still some users may want to skip checks for speed reasons. So, we should provide way to configure behavior. The most natural thing to do is to make UInt panic when standard numbers panic and wrap when standard types wrap. So, we should use cfg(overflow_checks).

Of course, this is breaking change, so it should be done in 2.0.0.

I use alloy for managing my cryptocurrency, i. e. for transferring it around. Unnoticed overflows may cause loss of money, I absolutely don't want this. alloy directly uses ruint's UInt. So, I plan to create my wrapper around U256, which will panic on overflow. And I also will use clippy::arithmetic_side_effects to make sure that I will not use arithmetic directly on ruint/alloy's U256. But ideally this should be fixed in ruint itself

Activity

u59149403

u59149403 commented on Dec 15, 2024

@u59149403
Author
prestwich

prestwich commented on Dec 16, 2024

@prestwich
Collaborator

ruint is intended to support the modular arithmetic required for cryptographic applications. Adding overflow checks based on cfg(overflow_checks) would mean that consumers of the library could inadvertently break ruint when used as a subdep. E.g. if you used a hypothetical ruint-ecdsa, it would be broken if you enabled cfg(overflow_checks) for your project

Is there a reason that using the checked_* API is not sufficient for your project?

u59149403

u59149403 commented on Dec 17, 2024

@u59149403
Author

Is there a reason that using the checked_* API is not sufficient for your project?

As I said this requires me to depend on special clippy lints. But okay, this is your project, so feel free to close this issue if you want

prestwich

prestwich commented on Dec 17, 2024

@prestwich
Collaborator

when writing consensus code, I would strongly recommend always using checked arithmetic, even with primitive number types, rather than panicking. Accidentally leaving reachable panics in consensus code can lead to network-wide DOS

for now, closing this as wontfix

Yen

Yen commented on Feb 16, 2025

@Yen

when writing consensus code, I would strongly recommend always using checked arithmetic, even with primitive number types, rather than panicking. Accidentally leaving reachable panics in consensus code can lead to network-wide DOS

for now, closing this as wontfix

Would it be possible to reassess this as I don't think it's the correct behaviour. I fully agree that people shouldn't be relying on behaviour like this to catch errors, but I was running into issues with overflows and had a hard time debugging the issue as the assumption was that the behaviour here would be the same as the standard rust types. I think the decision of the way to do things should not made here as it has already been decided by the rust team with the standard types. I would argue that replicating the default rust behaviour is far more important than trying to enforce good practice with a different behaviour in this library that nobody will know about unless they are hit by this case wile debugging and end up on this issue like I did.

I think the above comment for using checked arithmetic is a very reasonable thing to say as an argument against the rust standard types behaviour, but here it's far more important to have consistent behaviour with those types and to panic in debug.

Yen

Yen commented on Feb 16, 2025

@Yen

For reference, the issue I had debugging was that I was debugging under the rust assumption that in debug mode, overflows will panic. It took a long time checking many other parts of the code before I realised that it was overflowing, and it was the different behaviour in this library that broke that assumption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    wontfixThis will not be worked on

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @Yen@prestwich@u59149403

        Issue actions

          Make UInt panic on overflow if `overflow_checks` is enabled · Issue #408 · recmo/uint