rs-soroban-env icon indicating copy to clipboard operation
rs-soroban-env copied to clipboard

Consider forking into `Wasmi::Module` and add section-wise metering

Open jayz22 opened this issue 1 year ago • 4 comments

Follow up to https://github.com/stellar/rs-soroban-env/issues/821

The VmInstantiation's cpu cost is a linear function w.r.t. the size of the Wasm bytes. Calibration is done on the Wasm file containing maximum number of internal functions, in order to account for the worst case scenario (i.e. preventing hostile contracts). This means a good contract may be over charged by up to ~4x contract size.

One way to solve this is to do section-wise metering. I.e. breaking down the metering of Module::new (where parsing happens) into sections and have a separate cost component for each. Here is the natural place to do it https://github.com/stellar/wasmi/blob/soroban-wasmi-v0.30/crates/wasmi/src/module/parser.rs#L169-L185 This would require forking into Wasmi and inject into metering charging, which is something we've moved away from since https://github.com/stellar/rs-soroban-env/issues/683.

Note this is only to make the metering model more "fair" for normal contracts, it does not get around the fact that Vm instantiation is inherently costly and inefficient. https://github.com/stellar/rs-soroban-env/issues/827 tracks ways to make it more efficient, which is the much preferred approach.

jayz22 avatar Jun 07 '23 02:06 jayz22

Good find on section-wise metering! While we look into caching option in #827 , should we also file an issue for ParityTech to incorporate section-wise metering? That way at some point we will have both fair (section-wise) and efficient (caching etc) metering.

anupsdf avatar Jun 07 '23 04:06 anupsdf

We should not be treating wasmi upstream like it's immutable. I think modifying its instantiation code to invoke some callbacks ("Observer" pattern) should be fairly easy to upstream? We would do actual metering from those callbacks as long as we have the right arguments (this allows people to perform additional policies outside of our use cases).

MonsieurNicolas avatar Jun 07 '23 17:06 MonsieurNicolas

should we also file an issue for ParityTech to incorporate section-wise metering? That way at some point we will have both fair (section-wise) and efficient (caching etc) metering.

@anupsdf Just want to point out this has nothing to do with fuel metering (i.e. metering of the wasm execution). This is purely for the host and working under our metering framework. The way to do it is to add some callback into Wasmi that invokes our metering charging functions in the host (as @MonsieurNicolas pointed out).

It might be a little tricky to frame this as a general "Observer" callback to upstream though, since it will probably require some new constrains (trait impls) on the host (Wasmi is very particular about its own way of things). But I'll look into it.

jayz22 avatar Jun 07 '23 18:06 jayz22

This is an interesting possibility to explore, but not feature-work, and should be postponed until features are wrapped up.

graydon avatar Jun 07 '23 19:06 graydon