Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make UInt panic on overflow if overflow_checks is enabled #408

Closed
u59149403 opened this issue Dec 15, 2024 · 4 comments
Closed

Make UInt panic on overflow if overflow_checks is enabled #408

u59149403 opened this issue Dec 15, 2024 · 4 comments
Labels
wontfix This will not be worked on

Comments

@u59149403
Copy link

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

@u59149403
Copy link
Author

@prestwich
Copy link
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
Copy link
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
Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants