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

Deny clippy::arithmetic_side_effects #725

Merged
merged 16 commits into from
Apr 24, 2024
Merged

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Apr 20, 2024

Work towards #170

This does the changes for all crates other than fuel-merkle, which will be done in a separate PR.

Denies the following lints:

  • clippy::arithmetic_side_effects
  • clippy::cast_sign_loss
  • clippy::cast_possible_truncation
  • clippy::cast_possible_wrap

The following bugs were fixed:

  • UtxoId::from_str now rejects inputs with multiple 0x prefixes
  • array and numeric wrapper types like AssetId and BlockHeight now reject extra bytes in their from_str implementation (these were ignored before)

Some improvements were also done:

  • from_str error messages now indicate which type caused the error

Some changes introduce Rust-level panics where normally an overflow would have only panicked on debug mode.

Memory offsets of several types were previously calculated using unchecked operations. This PR changes those to use saturating_* operations instead. This means that given incorrect consensus parameters, some operations that previously overflowed silently know instead give memory offsets outside VM ram, causing a VM-level panic on access. This is strictly an improvement over previous behavior, but still leaves a lot to be desired. I'm not sure if it's actually possible to do much better, though. Maybe with custom types? The current approach feels like the most sensible tradeoff.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

@Dentosal Dentosal added bug Something isn't working breaking A breaking api change tech-debt fuel-vm Related to the `fuel-vm` crate. fuel-asm Related to the `fuel-asm` crate. fuel-tx Related to the `fuel-tx` crate. fuel-crypto Related to the `fuel-crypto` crate. fuel-types Related to the `fuel-types` crate. fuel-storage Related to the `fuel-storage` crate. labels Apr 20, 2024
@Dentosal Dentosal self-assigned this Apr 20, 2024
@Dentosal Dentosal marked this pull request as ready for review April 20, 2024 02:15
@Dentosal Dentosal requested a review from a team April 20, 2024 02:15
fuel-tx/src/transaction/consensus_parameters.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/consensus_parameters/gas.rs Outdated Show resolved Hide resolved
@@ -84,7 +84,7 @@ impl CommonMetadata {
.iter()
.map(|input| {
let i = offset;
offset += input.size();
offset = offset.saturating_add(input.size());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that later, we should handle values that are too huge in the into_checked_basic, but metadata is something that we calculate before verifying primary validity rules. So maybe it makes sense to return an overflow error here. It is non-breaking since precompute already returns an error=)

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validation in 042db35. Might still be a breaking change, since I created new ValidityError variants for these. The errors didn't seem to be a good match with this.

fuel-tx/src/transaction/types/create.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/types/upgrade.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/types/upload.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/types/utxo_id.rs Show resolved Hide resolved
fuel-types/src/canonical.rs Outdated Show resolved Hide resolved
@Dentosal Dentosal requested a review from xgreenx April 22, 2024 17:13
@Dentosal Dentosal requested a review from xgreenx April 23, 2024 13:31
@Dentosal Dentosal added this pull request to the merge queue Apr 24, 2024
Merged via the queue into master with commit 865e1b9 Apr 24, 2024
38 checks passed
@Dentosal Dentosal deleted the dento/deny-unchecked-arithmetic branch April 24, 2024 16:04
@Dentosal Dentosal mentioned this pull request Apr 25, 2024
5 tasks
@xgreenx xgreenx mentioned this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change bug Something isn't working fuel-asm Related to the `fuel-asm` crate. fuel-crypto Related to the `fuel-crypto` crate. fuel-storage Related to the `fuel-storage` crate. fuel-tx Related to the `fuel-tx` crate. fuel-types Related to the `fuel-types` crate. fuel-vm Related to the `fuel-vm` crate. tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants