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

feat: Add an error code on the metering middleware #1951

Merged
merged 1 commit into from
Dec 23, 2020

Conversation

jubianchi
Copy link
Contributor

@jubianchi jubianchi commented Dec 17, 2020

This is a proposition for #1931.

⚠️ Do not merge as-is. Wait for #1941 to get merged or to provide an alternative

Basically, the metering middleware now injects a new global, error_code with its own helper get_error_code which will allow the host to correctly handle metering errors. Now, a remaining_points == 0 and error_code != NoError means the execution of the guest module consumed all the gas points.

Review

  • Add a short description of the the change to the CHANGELOG.md file

@jubianchi
Copy link
Contributor Author

ping @webmaster128

@jubianchi jubianchi force-pushed the metering-manager-error-code branch from cd9ba3e to 15a34bf Compare December 17, 2020 16:16
@jubianchi jubianchi changed the title Metering manager error code feat: Add an error code on the metering middleware Dec 17, 2020
Copy link
Contributor

@webmaster128 webmaster128 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, some first thoughts

lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

The structure and design looks reasonable to me! I have some feedback about some of the implementation details though!

lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
@jubianchi jubianchi force-pushed the metering-manager-error-code branch 3 times, most recently from 20b43b8 to 6b21f17 Compare December 17, 2020 21:44
lib/middlewares/src/lib.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
@webmaster128
Copy link
Contributor

This PR covers all functionality required for #1931 – very cool. Freel free to reject my suggestions and personal preferences if you don't like them.

One more thing that I can think of (but don't need on my own), is the ability to reset the error. Imagine a use case where Wasm runs for a while and hits a safety limit. Now the user has the choice to resume the execution. Then it would be nice if you could increase the limit and reset the error.

@jubianchi jubianchi force-pushed the metering-manager-error-code branch from 6b21f17 to 84d2647 Compare December 17, 2020 23:08
@jubianchi
Copy link
Contributor Author

@webmaster128 @syrusakbary what do you think of reseting the error each time we call set_remaining_points?

@webmaster128
Copy link
Contributor

webmaster128 commented Dec 17, 2020

@webmaster128 @syrusakbary what do you think of reseting the error each time we call set_remaining_points?

I think there is two layers of abstraction here:

  1. getters/setters for the metering state variables
  2. a higher level concept of metering points that can be empty or exceeded

In 1., the values are independent and it it would be very confusing if they affect each other when setting them. However, we can think about exposing 2. to user, where

  • set_remaining_points resets the error
  • get_remaining_points returns an
    enum MeteringPoints {
        /// The given amount of points >= 0 is left 
        Remaining(u64),
        /// The limit was exceeded
        Depleted,
    }

This would remove the impossible state error = 1 and points > 0 from the user's handling code.

@jubianchi
Copy link
Contributor Author

@webmaster128 thanks for the suggestion.

I like the second proposition. @syrusakbary @nlewycky what do you think about that?

@jubianchi jubianchi force-pushed the metering-manager-error-code branch 2 times, most recently from 92fd1f9 to 780be36 Compare December 21, 2020 14:59
Copy link
Contributor

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Cool API

lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
examples/metering.rs Outdated Show resolved Hide resolved
@jubianchi jubianchi force-pushed the metering-manager-error-code branch from 780be36 to 9f4450f Compare December 21, 2020 16:30
@jubianchi jubianchi force-pushed the metering-manager-error-code branch from 9f4450f to 2b0d254 Compare December 22, 2020 13:54
@jubianchi
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Dec 22, 2020
@bors
Copy link
Contributor

bors bot commented Dec 22, 2020

@jubianchi jubianchi marked this pull request as ready for review December 22, 2020 17:01
@jubianchi jubianchi requested a review from Hywan as a code owner December 22, 2020 17:01
Copy link
Contributor

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Great work. Some ideas, feel free to dismiss. The only thing we should clean up one way or another is the exports.

lib/middlewares/src/lib.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
@jubianchi jubianchi force-pushed the metering-manager-error-code branch from cebbe48 to b382cfa Compare December 23, 2020 11:53
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
lib/middlewares/src/metering.rs Outdated Show resolved Hide resolved
@jubianchi jubianchi force-pushed the metering-manager-error-code branch from b382cfa to 3c33325 Compare December 23, 2020 20:43
@MarkMcCaskey
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 23, 2020

@bors bors bot merged commit 9fc9daa into master Dec 23, 2020
@bors bors bot deleted the metering-manager-error-code branch December 23, 2020 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants