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.

Sign up for GitHub

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

Jump to bottom

Strongly typed gas units, and fix two related bugs #562

Merged
merged 2 commits into from
May 15, 2022
Merged

Strongly typed gas units, and fix two related bugs #562

merged 2 commits into from
May 15, 2022

Conversation

Stebalien
Copy link
Member

Motivation: we had two bugs in gas units (milligas versus gas):

  1. Consensus fault charges from lotus (only applies to nv15).
  2. Window post scaling charge.

This fixes those bugs, and adds a new "safe" gas type that lets us automatically avoid:

  1. Multiplying gas by gas. Only Gas * (i32/i64) is implemented.
  2. Adding gas to non-gas. Only Gas + Gas is implemented.
  3. Swapping gas/milligas.
  4. Not using saturating arithmetic. All gas operations use saturating arithmetic internally.

The automatic saturating arithmetic also makes the code significantly cleaner.

@Stebalien Stebalien requested a review from magik6k May 15, 2022 19:27
@Stebalien Stebalien mentioned this pull request May 15, 2022
Merged
magik6k
magik6k approved these changes May 15, 2022
View reviewed changes
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Looks good, one nitpick

fvm/src/gas/mod.rs Outdated Show resolved Hide resolved
@Stebalien Stebalien force-pushed the fix/price-list-snafu branch 2 times, most recently from 0e92fb3 to e109382 Compare May 15, 2022 20:35
@Stebalien Stebalien enabled auto-merge May 15, 2022 20:39
@Stebalien
feat: strongly typed gas units
5baa787 
This adds a new gas type to avoid things like:

1. Multiplying gas by gas (not possible now).
2. Swapping gas/milligas.
3. Not using saturating arithmetic.

This change also caught and fixed a bug where the "extra" consensus
fault fee from lotus was being charged as milligas, not gas.

Motivation: the previous commit that fixed a bug in window post cost
scaling.
@Stebalien Stebalien merged commit 5d3a7b1 into master May 15, 2022
@Stebalien Stebalien deleted the fix/price-list-snafu branch May 15, 2022 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magik6k magik6k magik6k approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Stebalien @magik6k