Skip to content

Conversation

@magik6k
Copy link
Collaborator

@magik6k magik6k commented Dec 7, 2022

On top of #10

Rewrite of #4

This PR makes it possible to define much more advanced gas cost rules

/// An interface that describes instruction costs.
pub trait Rules {
    /// Returns the cost for the passed `instruction`.
    ///
    /// Returning an error can be used as a way to indicate that an instruction
    /// is forbidden
    fn instruction_cost(&self, instruction: &Operator) -> Result<InstructionCost>;
}

/// Dynamic costs instructions.
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub enum InstructionCost {
    /// Charge fixed amount per instruction.
    Fixed(u64),

    /// Charge the specified amount of base miligas, plus miligas based on the
    /// last item on the stack. For memory.grow this is the number of pages.
    /// For memory.copy this is the number of bytes to copy.
    Linear(u64, NonZeroU32),
}

@magik6k magik6k force-pushed the feat/dynamic-charges-new branch 3 times, most recently from cb87418 to de82267 Compare December 8, 2022 15:48
@magik6k magik6k changed the title wip dynamic gas cost instructions dynamic gas cost instructions Dec 8, 2022
@magik6k magik6k force-pushed the feat/dynamic-charges-new branch from d921fdb to 4fdc4b0 Compare December 8, 2022 18:47
Base automatically changed from fix/missing-stacklimit-instrs to master December 9, 2022 05:46
Comment on lines +797 to +810
// is negative, and if the spec is followed exactly, those instructions may take
// very long to trap with a negative argument.
Copy link
Member

Choose a reason for hiding this comment

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

What if we just treat them as unsigned? I.e., just charge a lot of gas?

Copy link
Member

Choose a reason for hiding this comment

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

(if it's an i32, that is).

Copy link
Collaborator Author

@magik6k magik6k Dec 9, 2022

Choose a reason for hiding this comment

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

That can get all kinds of messy with overflows, but I can dabble with that idea for a bit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So apparently I64ExtendI32U already extends with all zero bits (so -1 becomes 0x00000000_ffffffff), so we can just do that and be fine.

You can see that with this demo: https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/Control_flow/call

Put this as js:

var url = "{%wasm-url%}";
await WebAssembly.instantiateStreaming(
  fetch(url),
  {
    env: {
      greet: function(v) {
        console.log(v);
      }
    }
  }
);

And this as wat

(module
  (import "env" "greet" (func $greet (param i64)))

  (func
    i32.const -1
    i64.extend_i32_u
    call $greet
  )

  (start 1)
)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there's a separate instruction for sign extending (I64ExtendI32I).

Copy link
Member

@Stebalien Stebalien Dec 9, 2022

Choose a reason for hiding this comment

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

Actually, are you sure this is signed? Wasm usually doesn't distinguish between the two.

InstructionCost::Linear(base, cost_per) => {
// Enforce that cost per unit fits in 31 bits
if cost_per.get() >= 0x8000_0000 {
return Err(anyhow!("cost per unit excedes the 0x80000000 limit"));
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess 32 bits plus 31 bits because our gas is technically signed (although I'd like to remove that...). But yeah, this is reasonable.

@magik6k magik6k force-pushed the feat/dynamic-charges-new branch from 7a72fa3 to 49670b5 Compare December 9, 2022 16:05
@magik6k
Copy link
Collaborator Author

magik6k commented Dec 9, 2022

(rebased on master)

@Stebalien Stebalien merged commit 3edfae2 into master Dec 9, 2022
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