VIP: Cost and usability Improvements to internal function calls

[maurelian]

Preamble

VIP: <to be assigned>
Title: reduce the cost and difficulty of code reuse in common situations
Author: maurelian
Status: Draft
Created: 2018-06-16
Requires (*optional): <VIP number(s)>
Replaces (*optional): <VIP number(s)>

Simple Summary

1. I shouldn't need to copy and paste the same code snippets into 5 functions to valid authorization.
2. I shouldn't feel like copying and pasting code is more efficient or cheaper.

Abstract

In order to call a within the same contract function, Vyper uses the CALL opcode, which send a new message to call functions within the same contract. This has some nice safety benefits.

Unfortunately it will result in developers using anti-patterns to save on gas, or access msg.sender in a function call.

Motivation

The benefit of using CALL to access code in the same contract is to create a new execution context, with no risk of side effects from memory access. I admit that I find this to be quite an elegant use of the EVM for safety.

Problems with using CALL

Unfortunately, there are two major issues with the use of the CALL opcode for calling functions within the contract:

1. Environment values change unexpectedly

At least two important environment opcodes will return different values: CALLER (ie. msg.sender) and CALLVALUE (ie. msg.value). This complicates the use of functions for permission checking. The following is a naive vyper translation of the common "ownable" pattern for a simple storage solidity contract.

owner: address
value: public(int128)

@public
def __init__():
    self.owner = msg.sender

@private
def checkOwner():
  assert msg.sender == self.owner # this will REVERT every time

@public
def writeValue(_value: int128):
    self.checkOwner()
    self.value = _value

2. Gas costs disincentivizes code reuse

The CALL opcode costs 700 gas (+ input data costs + function dispatching again). By contrast Solidity uses JUMP (2 gas) to call a function.

This imposes a large penalty on code reuse. Regardless Vyper's design philosophy favoring safety over gas efficiency, developers will respond to this incentive by simply copying and pasting boilerplate code. This is dangerous, and difficult to audit.

Specification:

I'm sorry, I don't have one specification. Below are some options I see for mitigating these issues.

From the Motivation section above, we have 3 parameters along which to analyze approaches to calling a function in the EVM.

1. Gas cost
2. Preservation of env variables
3. Safety benefits

1. Use DELEGATECALL instead of CALL

1. Gas cost: NEUTRAL (same as using CALL)
2. Env variables: GOOD (gives us back CALLER and CALLVALUE)
3. Memory Safety: GOOD. It does indeed reset memory.

Seems like a decent solution!

3. Memory Safety: BAD (executes the code in the same memory context, nullifying reasons for using CALL in the first place) ~I proposed this earlier, but I don't think this is a good solution. ~

2. Just use JUMP like Solidity

1. Gas cost: GOOD (Much lower gas cost)
2. Env variables: GOOD (Preserves CALLER and CALLVALUE)
3. Memory Safety: BAD (Greater risk of compiler bugs)

This is worth considering.
See also #330.

3. Implement a safer analog to Solidity's modifiers

These could have significant restrictions on them, like no access to MSTORE or SSTORE, and not accepting arguments. I assume this would be done by with JUMP, or just inlining the same code each time it's needed.

1. Gas cost: GOOD (Lower execution cost, but might increase size of contract if inlining is used)
2. Env variables: GOOD (Preserves CALLER and CALLVALUE)
3. Memory Safety: Depends on implementation

4. Create special variables which persist across message calls

Built-in vars like caller and callvalue can be created, which are magically passed to a function in a message call.

My code above would thus be:

@private
def checkOwner():
  assert _sender == self.owner # <- replaced msg.sender with a magical global variable `sender`

@public
def writeValue(_value: int128):
    self.checkOwner)
    self.value = _value

This feels a bit funky to me. Probably would be hard to implement safely, and obscures the true functioning of the EVM.

5. Status Quo

My problem in the code sample above can be addressed by passing the values I need as arguments:

@private
def checkOwner(_sender: address):
  assert _sender == self.owner # <- replaced msg.sender with _sender

@public
def writeValue(_value: int128):
    self.checkOwner(msg.sender) # <- pass msg.sender as an argument 
    self.value = _value

Maybe that's OK? But it requires a deeper understanding of how both the EVM and Vyper work.

Backwards Compatibility

Dependent on approach.

Copyright

Copyright and related rights waived via CC0

[haydenadams]

I do find myself avoiding internal calls in favor of reusing the same code in multiple places to save gas, which is probably not ideal. I don't personally know the tradeoffs well but If this can be done safely I'm all for it.

[ben-kaufman]

I get your point but I don't fully agree with that. Unlike other software, smart contracts' code must be explicit and easily readable. In most other cases, it's more important to not repeat yourself in the code but in smart contracts, I think readability of the function should be more important.
In the example about you are trying to mitigate the behavior of modifiers which Vyper intentionally excluded.

Modifiers - e.g. in Solidity you can do function foo() mod1 { ... }, where mod1 can be defined
elsewhere in the code to include a check that is done before execution, a check that is done after
execution, some state changes, or possibly other things. Vyper does not have this, because it makes > it too easy to write misleading code. mod1 just looks too innocuous for something that could add
arbitrary pre-conditions, post-conditions or state changes. Also, it encourages people to write code > where the execution jumps around the file, harming auditability. The usual use case for a modifier is > something that performs a single check before execution of a program; our recommendation is to
simply inline these checks as asserts.
This is the reason Vyper don't have modifiers like Solidity in the first place.

I am in favor of your fourth proposal of creating the caller and callvalue.

I would like to hear more opinions on that as code repetition is an insecure thing as well so I'm not completely sure what's more important yet, but I believe there's a room for improvement.

[haydenadams]

Idk if I agree that internal function calls have the same readability issues as Solidity modifiers. Also code repetition can feel very verbose and reduce readability. If you need to do a square root in five different functions, its probably bad to have the sqrt() implemented five times. But people will be tempted to do so if they save 1000 gas per function call. Discouragement through high gas cost makes sense on a protocol level (ie making sstore exepensive) but feels wrong for smart contract languages.

To be clear I don't think

1. Environment values change unexpectedly
   At least two important environment opcodes will return different values: CALLER (ie. msg.sender) and CALLVALUE (ie. msg.value).

is a big problem. I do however think

2. Gas costs disincentivizes code reuse
   The CALL opcode costs 700 gas (+ input data costs + function dispatching again). By contrast Solidity uses JUMP (2 gas) to call a function.

is bad. If using JUMP for internal calls can be done as safely as CALL, it should be used. I don't know the security tradeoffs at all, hopefully someone who does can chime in here!

[maurelian]

It's not only about readability, but maintainability as well. More mistakes will be made if 5 pieces of code have to change.

Take a look at these @private utilities I wrote for my ERC721 translation:

https://github.com/maurelian/ethereum-erc721/blob/vyperTranslation/contracts/tokens/NFToken.v.py#L102-L120

There are a total of 12 lines of code in the bodies, which can be written once, then reused 3 times in the standard. If they were written 3 times over, it would be very hard (for my brain at least) to read carefully and catch any differences between them.

[fubuloubu]

6th option?: an optimization function that will perform a JUMP instead of a CALL if it is able to be validated that the code jump cannot access anything outside of the called function. This would limit functions that make external calls from having advanced memory access while allowing safe internal calls to reduced their gas count. This will also incentivize isolation/reduction of external calls to improve gas costs.

[maurelian]

6th option?: an optimization function that will perform a JUMP instead of a CALL if it is able to be validated that the code jump cannot access anything outside of the called function.

This optimization means that source code using msg.sender would behave differently based on a decision by the compiler. It might work if the developer could specify a flag (@memsafe, @pure ?), indicating they would like to use JUMP and the compiler makes a check to ensure it's safe.

[maurelian]

To be clear I don't think "Environment values change unexpectedly" is a big problem.

@haydenadams No? IMO, the current behaviour of this code is highly non-intuitive, and will make a lot of common operations much more difficult.

@public
def getCaller() -> address:
    return msg.sender
    
@public
def alsoGetCaller() -> address:
    return self.getCaller() # always returns the contract's own address!

[haydenadams]

@maurelian

IMO, the current behaviour of this code is highly non-intuitive, and will make a lot of common operations much more difficult.

Whatever address calls a function is msg.sender within the scope of that function. That is the only rule right now. Its not a massive leap to realize that extends to self for internal calls. When a public function is called I don't think it should react differently if its being called by itself, another smart contract, or an external account.

"on the internet nobody knows you're a dog" -> "on Ethereum nobody knows you're ̶a̶ ̶s̶m̶a̶r̶t̶ ̶c̶o̶n̶t̶r̶a̶c̶t̶ yourself"

With account abstraction the divide between contracts and accounts will go from negligible to nonexistent. Contracts will get more complicated but also more versatile. Its pretty important that smart contract devs understand Ethereum account structure. I would prioritize simplicity of logic over what feels immediately intuitive here. Totally understand where you're coming from though!

This seems reasonable, and personally I like it:

@private
def checkOwner(_sender: address):
  assert _sender == self.owner # <- replaced msg.sender with _sender

@public
def writeValue(_value: int128):
    self.checkOwner(msg.sender) # <- pass msg.sender as an argument 
    self.value = _value

[jacqueswww]

We briefly discussed this in the bi-weekly meeting yesterday, but left it as unresolved. As it's quite a significant change, we have to study all the options carefully ;)

[maurelian]

As it's quite a significant change, we have to study all the options carefully ;)

@jacqueswww I certainly agree.

[maurelian]

@haydenadams it's straightforward to make small changes to my little example to make it work, but it's always going to be confusing.

Here's an other summation:

IF: a contract can call its own @public functions;
THEN: msg.sender is pretty much useless as a builtin variable, because it means something entirely different when called externally vs. internally. It's only useful when passed as an argument.

If the Vyper team decides to move towards the external and internal visibilities as in solidity, this would be fine. (added to OP)

Finally, from the Auditability principle:

Simplicity for the reader is more important than simplicity for the writer, and simplicity for readers with low prior experience with Vyper (and low prior experience with programming in general) is particularly important.

Outside of project contributors, requiring this level of nuance is far from simple for anyone reading code.

[jacqueswww]

I agree that it's confusing that msg.sender changes, and will be a major annoyance going forward.
Short/little fixes I will investigate:
1.) disable accessing msg.sender or msg.value in private functions.
2.) Investigate using DELETEGATECALL -> I was under the impression that this also resets the VM.