Failing test case for debugging constant inherited arguments

[fubuloubu]

See #315

Looking for help resolving this problem as the compiler should be passing through all the inherited arguments (e.g. msg.sender) in the context call to a function from within another function.

[DavidKnott]

@fubuloubu After doing some research it's not only an issue @constant but with normal functions as well. The issue is that the caller changes to the contract itself which is why the following code passes:

bal: num[address]

def __init__():
    self.bal[msg.sender] = 1
    self.bal[self] = 1

@constant
def get_my_bal() -> num:
    return self.bal[msg.sender]

def foo():
    assert self.bal[msg.sender] > 0

def bar():
    assert self.get_my_bal() > 0

I should be to the bottom of this shortly, looking into the opcode calls now.

[fubuloubu]

Yeah, I just added @constant because it made sense in the scenario that you would call smaller constant functions to access things commonly. Do you think I should simplify the scenario?

[DavidKnott]

It makes sense, I was just noting it.

[DavidKnott]

@fubuloubu I figured out the opcodes to get it working properly, mind if I make a PR or I can push to your repo if you'd prefer.

[fubuloubu]

Push to my repo probably best so this PR will pass CI

[DavidKnott]

Hey, I got msg.sender persist by using delegatecall but it broke internal functions because both use the CALLER opcode (in my implementation if msg.sender is still an outside address then internal functions will throw). Any ideas?

[fubuloubu]

@DavidKnott not sure I entirely understand what you are saying, but from what I understand the context shifts from the client (msg.sender) to the contract (self) when the call is made internally of function b from function a. Is that correct?

Curious as to why this context shift is necessary and what might be causing the shift if it is unintentional.

[fubuloubu]

Do you have a specific line you can point to where the context shift occurs?

[DavidKnott]

Yes, exactly. Basically when I change the context to keep client(msg.sender) internal doesn't work (because internal means it can only be called from within). I think I'm just implementing it wrong.

Yes, in Parser line 772

[fubuloubu]

Man that file is super large and needs to be cleaned up.

Hmm. I could see internal being implemented like inline in C where the compiled function referencing internal actually gets a copy of that code inserted atomically instead of a function created requiring the self caller context. Inlining that code probably wouldn't be too hard since we mostly use global variables to track state, and perhaps any locals can be either checked for collisions or name-mangled via function name to avoid namespace issues.

Summary:

1. internal doesn't generate any code in the final IR/Bytecode (ABI can still generate though?)
2. when a call is made to an internal function, the compiler overwrites that call with the code inlined
3. This has the effect of naturally precluding any calls to that code externally, either via clients or contract inheritance, because the method doesn't actually exist

Definitely a significant change proposal there worth discussion. @vbuterin, what do you think?

[fubuloubu]

@DavidKnott can you post what your currently solution is? (diff fmt?)

[DavidKnott]

Sure, though I think the change I made is wrong as I'm not sure how to get internal working with it. It's replacing line 772 with this:

['assert', ['delegatecall', ['gas'], ['address'],

DELEGATECALL is like CALL but persists the current values for sender and value.
@fubuloubu I'm hoping your proposal is overkill. I've been looking at the Solidity repo to see how they implement internal but don't know c++ so it's a slow journey.
And internal does generate byte code that checks the called here on line 385 of parser.py:

if internal:
    clampers.append(['assert', ['eq', 'caller', 'address']])

The above code checks that the caller address is the same as the contracts address.
Also thanks for getting your hands dirty in parser.py.

[fubuloubu]

Rebased this to latest updates.

[fubuloubu]

Analyzing the IR it appears that whenever msg.sender is used, caller is put in it's place in the produced code. As you noted, caller for an internal function call is set to the contract's address by definition. So, in order to get this to pass you would have to introduce a slightly different construct for msg.sender: instead of replacing with caller, it would need to retrieve the original calling party from the context (msg.sender stored as an additional field) in order to reference correctly in internal functions.

I believe this implicit context change is somewhat dangerous if the contract coder does not understand this nuance, e.g. using a function directly produces different results than if used internally inside another function. As an example:

balance: currency_value[address]
pending: {from: address, to: address, amt: currency_value}[num]

def pay(_to: address, _amt: currency_value):
    assert self.balance[msg.sender] >= _amt
    self.balance[msg.sender] -= _amt
    self.balance[_to] += _amt

def accept(tx_id: num):
    assert msg.sender == self.pending[tx_id].from
    self.pay(self.pending[tx_id].to, self.pending[tx_id].amt)
    self.pending[tx_id] = None # Delete
...

If you called pay() directly, you would be paying from your own account to the relevant parties account; however, if you called accept() on some requested transaction (stored in self.pending from some unimplemented billing function), it would actually pay from the contract's own account instead.

An obvious solution here is to specify both parties in the pay() transaction, but in the interest of code savings this can be a useful alternative. I could probably come up with a more complex example given time, but I hope my point is clear that msg.sender is set to in different calling contexts in different scenarios, and that it is a potential source of user bugs.

@vbuterin @DavidKnott thoughts?

[fubuloubu]

Figured it out. msg.sender is set to caller of that function, tx.origin tracks the initiator of the original transaction (outer level caller). This is in line 494-495 of parser.py here

I'm assuming the original intent was that tx.origin is tracking which address initiated the whole transaction, which includes all function calls made. so msg.sender is then misleading to me, because it sounds like it's talking about a user, but in reality it is tracking the caller of that function instead. I'm debating whether msg.caller would more clearly state the purpose behind that, and/or whether a better name for tx.origin might also help. Also had a random thought about whether msg.value would be passed through correctly to the inner function? I'll have to write a test case for that

@vbuterin @DavidKnott thoughts?

P.S. When I make the update from msg.sender to tx.origin the issue gets resolved, but gas estimation fails for the function call with the inner call.

[DavidKnott]

@fubuloubu I think Viper needs both, I prefer using msg.sender over tx.origin because it's easier to control. Tx.origin messes things up when you have contracts interacting with one another see (this and this). Basically msg.sender shouldn't change between function calls but should change between contract calls (I believes msg.value should behave similarly).

[fubuloubu]

Kind of revisited this for a bit today. @DavidKnott are you trying to say that there is a potential problem here with how msg.sender is working? If so, is that issue with viper, solidity, or EVM?

[DavidKnott]

@fubuloubu I'll look into this again tomorrow and get back to you tomorrow, right now I suspect it's a fixable issue with viper.

[fubuloubu]

Looking at these lines in parser.py again, It seems like it's making a call to something that is in EVM. Namely, I think that references to these opcodes. If solidity doesn't have this same problem, then I would agree something in Viper has to be wrong, but I'm not as familiar with Solidity.

That, or I am totally misunderstanding what the problem is here.

Let me know what you find.

[fubuloubu]

I copied the failing test case over to Solidity here and it seems to work fine. You must be right there is probably an issue with something on the Viper end.

Although, doesn't viper use a different implementation of the EVM?

[DavidKnott]

@fubuloubu I looked into how Solidity maintains msg.sender while supporting internal and though I'm not very familiar with C++ it seems to be similar to inlining as you mentioned above. So far, the one possible solution I've find is to not include unused internal functions in the evm code and then inserting them whene they're used within functions.

Generally speaking, I think the call opcode is meant to call other contracts functions as opposed to their own.

[DavidKnott]

@fubuloubu I talked to Vitalik and as of now the proper way to use msg.sender after the first function call is to pass it through as an argument. The other option would be to load functions directly through memory (Solidity does this) as opposed to calling them (mload vs call opcodes) as loading them wouldn't change the msg.sender. But, it was agreed that loading functions from memory would increase code complexity more than it's worth.

[fubuloubu]

I will close this PR then because there's nothing unique in this additional test case.