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

Compile pricing for testnet #113

Merged
merged 6 commits into from
Aug 17, 2023
Merged

Compile pricing for testnet #113

merged 6 commits into from
Aug 17, 2023

Conversation

rachel-bousfield
Copy link
Contributor

@rachel-bousfield rachel-bousfield commented Aug 16, 2023

This PR adds a basic 14 million gas charge to compile a Stylus program. In the future we'll price compilation based on the wasm, which will cut costs considerably for reasonable programs. In the interest of preventing DoS, however, we'll charge 14 million as a conservative back-stop.

Note that this PR includes all the plumbing necessary to introduce a complex pricing policy.

payForCompilation(burner burn.Burner, info *wasmPricingInfo) {
    return burner.Burn(11000000)
}

The above function doesn't do anything right now, but could use the info parameter to charge different amounts based on various factors like wasm size, memory footprint, number of functions, etc.

This PR makes additional changes, including

  • Diagramming the go stack when crossing into Rust
  • Adding additional constraints on the shape of wasms
  • An extra safety check on the ability to form Machine's during native / jit execution
  • Not using rayon in the JIT prover (not needed anyways due to block validation being embarrassingly parallel)

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #113 (7a07fd4) into stylus (6a8b0bf) will decrease coverage by 0.03%.
Report is 1 commits behind head on stylus.
The diff coverage is 30.31%.

@@            Coverage Diff             @@
##           stylus     #113      +/-   ##
==========================================
- Coverage   56.20%   56.17%   -0.03%     
==========================================
  Files         272      272              
  Lines       41274    41397     +123     
==========================================
+ Hits        23197    23255      +58     
- Misses      15707    15787      +80     
+ Partials     2370     2355      -15     

Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

No comments. Nice annotations, explainers, and good cleanup

@@ -1078,6 +1080,43 @@ impl Machine {
Ok(machine)
}

/// Produces a compile-only `Machine` from a user program.
/// Note: the machine's imports are stubbed, so execution isn't possible.
pub fn new_user_stub(
Copy link
Contributor

Choose a reason for hiding this comment

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

part of my onchain compilation PR was realising that we don't really need a machine for these things.. a module is enough.
See from_user_binary in machine.rs:
https://github.com/OffchainLabs/stylus/pull/95/files#diff-dd6bc6f8903cfa5ee9704487f1a234c34957f007b05cc7e8ae47e482f8ead0e3

(I can remove new_user_stub when I merge my PR later)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, we just need to make sure that whatever we do, we revert native execution if arbitrator fails (rn that includes creating a machine, so let’s keep this for now but remove in your PR)

}
return footprint, err

// compilation succeeds during proving, so failure should not be possible
Copy link
Contributor

Choose a reason for hiding this comment

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

during parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now those are the same thing, though your PR will change that. Def feel free to remove parse too :)

@@ -25,42 +25,68 @@ import (
"github.com/ethereum/go-ethereum/core/state"
"github.com/ethereum/go-ethereum/core/vm"
"github.com/ethereum/go-ethereum/log"
"github.com/offchainlabs/nitro/arbos/burn"
Copy link
Contributor

Choose a reason for hiding this comment

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

generally: some of the changes here will not be necessary in onchian-compilation world..
After these changes: stylus_compile is only called on compilation and not on callUserWasm. Also - styls_compile must compute the arbitrator machine to get the module hash, so it could just as well also compute compilation cost.

So after that stylus_parse_wasm will be removed and stylus_compile will return WasmPricingInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s works too once we have that, though do note you’ll need to bring a few things over the boundary in order to compute and assign gas deductions (like the memory model params).

@@ -382,20 +385,38 @@ const (
userOutOfStack
)

func (status userStatus) output(data []byte) ([]byte, error) {
func (status userStatus) toResult(data []byte, debug bool) ([]byte, string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, but I don't really like this..
msg is basically only used to print error message, right?
Can't we just do something like

func extraData(data []byte, status UserStatus, debug bool) string

And use that when printing error messages?

Copy link
Contributor Author

@rachel-bousfield rachel-bousfield Aug 17, 2023

Choose a reason for hiding this comment

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

I don’t love it either, but I’m trying to avoid extra variables that are only used in the debug case (we drop data depending on the kind of failure, so you have to move the .into argument to before the call and give it a new name)

@rachel-bousfield rachel-bousfield merged commit a5d27cd into stylus Aug 17, 2023
7 checks passed
@rachel-bousfield rachel-bousfield deleted the stylus-compile-safety branch August 17, 2023 21:34
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