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

Re-evaluate the necessity for all the contract cost types before testnet #981

Closed
jayz22 opened this issue Aug 2, 2023 · 3 comments · Fixed by #984
Closed

Re-evaluate the necessity for all the contract cost types before testnet #981

jayz22 opened this issue Aug 2, 2023 · 3 comments · Fixed by #984
Assignees

Comments

@jayz22
Copy link
Contributor

jayz22 commented Aug 2, 2023

The contract cost types were created since the beginning of Soroban to represent metering components of the Soroban host.
The cost definitions has undergone many iterations and currently these 31 cost types encompasses all the metering components. This issue is to reevaluate all the definitions to make them "testnet finalized".

Here are the list of criteria for the existence of a cost type (in accordance with the metering CAP):

  1. Significance:
    a. has either linear or large constant cost in either of {cpu, mem} dimensions
    b. or if the cost is a small constant, it can happen in high frequency
    c. the existence threshold for a constant cost is the cost of ChargeBudget (~100 cpu insns)
  2. Standalone: no double counting, i.e. no charging (or a different type) inside a charge
  3. Recursion prevention: if there is a recursion (loop) path of un-predetermined depth (iterations) in the host without encountering budget charging, we should add some charge.
  4. Ubiquity: this is for future proofing. Some code paths are ubiquitous, such as visiting a host object, or invoking a host function. Adding a metering charge at these "choke points" can prevent 1. someone finding a crack in the metering system to do infinite amount of work for free 2. we add extend the host functionality in the future but forget to add metering. These provide a minimal base coverage.
@jayz22 jayz22 self-assigned this Aug 2, 2023
@jayz22
Copy link
Contributor Author

jayz22 commented Aug 2, 2023

Questionable types based on above criteria:

  • VisitObject - keep. Although each visit object does small work. It is ubiquitous. If we didn't have it, object access/manipulation metering will be relying on the calling closure, which is easy to miss. Here we add some base cost for future proof.
  • ValXdrConv - remove. See comment below.
  • InvokeHostFunction - keep, but rename it to HostFunctionDiaptch. The work is kind of non-trivial (marshaling values and handle fuel transferring) thus satisfies significance. And it is also ubiquitous, all future host function implementations will share the same path.
  • InvokeVmFunction - keep, per significance.
  • GuardFrame - remove. Significant parts are context frame push and auth frame push, both already covered.
  • ChargeBudget - remove. See comment below.
  • HostMemCpy/HostMemCmp - keep. these are more sublinear, need to be evaluated after Consider using fixed-point integer for cost model parameters #824

@jayz22
Copy link
Contributor Author

jayz22 commented Aug 4, 2023

ChargeBudget Should be removed.

Why:

It is a small constant call that cannot happen recursively or in bulk -- fails the significance check.

It was originally included mainly because of metering of wasm instructions. Original comment:

        // NB: charging a cost-amount to the budgeting machinery itself seems to
        // cost a similar amount as a single WASM instruction; so it's quite
        // important to buffer WASM step counts before flushing to budgeting,
        // and we add a constant charge here for "the cost of budget-counting"
        // itself.

The worry was (based on my understanding of what @graydon wrote), if we charge wasm instruction too granularly, we would be calling charge_budget too often, each time incurring a non-trivial cost, and since charge_budget costs on the same level as a wasm insn, we should count it too. Basically charge_budget could be called in bulk, thus making it significant.

This is no longer the case. Now wasm instruction metering happens on the wasmi side and we are only charging the budget when crossing the host<->guest boundaries.

No other cost types can charge budget excessively.

(Note: one interesting potentially useful information being removed is how many times the meter was called. We can add that information back as a new field in the Budget::tracker.)

@jayz22
Copy link
Contributor Author

jayz22 commented Aug 4, 2023

ValXdrConv should be removed

Why:

Originally added mainly for recursion prevension -- in case there is a nested val path where we didn't charge for.

For normal non-object val, the conversion goes through env-common, and they are not charged (we cannot charge for it as they don't go through the host). Those are just 8 bytes copy so they are okay.

We charge for anything that involves the host object. So in from_host_obj and to_host_obj in every variant of object type, we make sure it is charged. Therefore all nested object conversion will be covered. And we don't need the ValXdrConv type.

Does this satisfy the "ubiquity" criteria?

One could argue ValXdrConv is ubiquitous and should be kept for future proofing. However there are a few considerations that goes the other way:

  • The HostObject types are finite, not subject to frequent changes and are compiler-checked by the enum match statement. We've already made sure all object<->ScObject conversions are metered (even an u64 object), and any future extensions should be easy to make sure of it as well.
  • There is already a ubiquitous charge for VisitObject.

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 a pull request may close this issue.

1 participant