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

Remove any unchecked arithmetic #170

Closed
5 tasks done
Voxelot opened this issue Jul 13, 2022 · 3 comments · Fixed by #729
Closed
5 tasks done

Remove any unchecked arithmetic #170

Voxelot opened this issue Jul 13, 2022 · 3 comments · Fixed by #729
Assignees
Labels
bug Something isn't working fuel-vm Related to the `fuel-vm` crate. tech-debt

Comments

@Voxelot
Copy link
Member

Voxelot commented Jul 13, 2022

In order to prevent overflows and unpredictable behaviors, all arithmetic (esp gas or free balances) should use checked_* operations rather than std ops like AddAssign. In cases when we know that it is not possible to fail or the context around is valid, then we can ignore or expect the operation.

The main idea is to make the code safe and keep API as simple as possible. If the input from the user may invalidate the state/behavior, then we need to return an Option or Error.

It is better to do this change crate by crate to avoid conflicts with other PRs:

@Dentosal
Copy link
Member

Just to elaborate on this: there are checked_*, overflowing_*, saturating_* and multiple other variants. The appropriate operation should be used. A comment (e.g. spec link) on why a specific variant was selected with overflowing_* or saturating_* operations wouldn't hurt either.

For optimization, there are also unstable and unsafe unchecked_* operations, which assume overflow cannot occur.

@Voxelot Voxelot moved this to Todo in Fuel Network Jul 15, 2022
@Voxelot
Copy link
Member Author

Voxelot commented Aug 8, 2022

Looks like clippy has a lint for unchecked math, cargo clippy -- -D clippy::integer_arithmetic which results in 135 errors currently.

Once we've addressed this issue, we can add clippy::integer_arithmetic to our set of disallowed lints.

@Voxelot Voxelot added the bug Something isn't working label Aug 8, 2022
vlopes11 added a commit that referenced this issue Sep 15, 2022
This module aims to centralize all arithmetic operations so they can be
easily updated whenever required.

It is desirable to run all protocol checked operations as unchecked
code, but then it will be feasible only after we have comprehensive test
coverage.

Closes #214
Relates to #170
@Voxelot Voxelot removed this from Fuel Network Nov 1, 2022
@ghost ghost self-assigned this Nov 1, 2022
@mitchmindtree mitchmindtree added the fuel-vm Related to the `fuel-vm` crate. label Dec 9, 2022
@MitchTurner MitchTurner self-assigned this Aug 11, 2023
@MitchTurner
Copy link
Member

After spending a couple of days on this, it's not clear the best approach. In terms of capturing the error effectively, it makes sense to return Result from any function that has checked arithmetic. Theoretically there is internal logic that could never go awry during healthy behavior, and for those functions we could #[allow(clippy::arithmetic_side_effects)], but in reality its very hard to tell which those are. For the rest of the functions, making them fallible (returning Result) clutters up the code a lot.

It was when those Results started bleeding into infallible functions in other crates (from fuel-merkle into fuel-tx) I really started questioning the value of these changes.

It seems more valuable to have thorough fuzzing/prop testing than be able to catch the error telling us the chain is bricked.

With all that in mind, I'm going to put this story back and work on higher-priority issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fuel-vm Related to the `fuel-vm` crate. tech-debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants