[REVIEW] issue244 delegatecall

[benjaminbollen]

this is for communication, review and testing purposes only. Do not merge this into master, until we have properly reviewed and tested

[benjaminbollen]

so reasoning is:

• act on storage of callee
• preserve caller
• run code from destination
• do not transfer value to destination

[VoR0220]

Did you look at this yet? https://github.com/ethereum/go-ethereum/search?utf8=%E2%9C%93&q=delegatecall This has everywhere that is delegate call related in some way and could be an excellent reference point.

[VoR0220]

Not entirely correct. See here. https://github.com/ethereum/go-ethereum/blob/b7e3dfc5a2bc7e2f4d653fbe0ec9774277a10643/core/execution.go#L41

[VoR0220]

Just change these notes here please @benjaminbollen then it should be ready to merge.

[benjaminbollen]

done

[silasdavis]

I'm going to state my understanding of this operation and see if other people agree. "Caller" and "Callee" can get a bit confusing because we an intermediate account (named B below; the delegate) that is both caller and callee.

Suppose we have 3 accounts: A, B, and C with the following psuedo-code contracts attached:

A:

int n := 0
address addr := nil

CALL B

B:

int n := 0
address addr := nil

DELEGATECALL C

C:

int n := 0
address addr := nil

n++
addr = msg.sender

Then the result of invoking A (message or transaction) should be the following sequence:

Stack	A	B	C
[]	n = 0, addr = nil	n = 0, addr = nil	n = 0, addr = nil
[CALL B]	n = 0, addr = nil	n = 0, addr = nil	n = 0, addr = nil
[DELEGATECALL C, CALL B]	n = 0, addr = nil	n = 1, addr = A	n = 0, addr = nil

That is B acts a 'delegate' for A in terms of the message sender and value, but like with CALLCODE it is B's storage that gets updated by the code in C.

Is this correct?

[silasdavis]

// [ -->] Different to the normal CALL or CALLCODE, the value does not need to be transferred to the callee. [<-- CORRECT?]

I need clarification on what this means, and if it is indeed correct...

[benjaminbollen]

That is B acts a 'delegate' for A in terms of the message sender and value, but like with CALLCODE it is > B's storage that gets updated by the code in C.
Is this correct?

yes, that is also how i understand it and the pseudo-code example is consistent.

[benjaminbollen]

// [ -->] Different to the normal CALL or CALLCODE, the value does not need to be transferred to the > callee. [<-- CORRECT?]
I need clarification on what this means, and if it is indeed correct...

in the code, opcodes CALL and CALLCODE will start a new stack and memory by calling the funnction Call, which atomically transfers the value from caller to callee; then calls the function call which executes the ethereum opcodes;

I understand that the value sent with the message (starting a new stack+memory) does not require the value sent from A to B to be passed to the delegated contract C; a few notes:

• whether or not to pass on this value from B to C; does not affect the succes of e-pm test app35
• go-ethereum code - from my reading - does also not pass this value to the delegated contract C

[compleatang]

the key diff between CALL and DELEGATECALL is the storage and memory zone in which the code operates. namely CALL will execute in the storage and memory zone of the CALLEE while DELEGATECALL will execute the in the storage and memory zone of the CALLER.

[benjaminbollen]

so in the proposed impl here; for switch CALL, CALLCODE, DELEGATECALL it essential hinges on this:

if op == CALLCODE {
    [...]
    ret, err = vm.Call(callee, callee, acc.Code, args, value, gas)
} else if op == DELEGATECALL {
    [...]               
    ret, err = vm.DelegateCall(caller, callee, acc.Code, args, value, gas)
} else {
    [...]
    ret, err = vm.Call(callee, acc, acc.Code, args, value, gas)
}

https://github.com/eris-ltd/eris-db/pull/247/files#diff-cad6213f6a08c87e4110c4006789e2b7R832

where then vm.DelegateCall does not transfer the value from caller to callee, although that - as referred to above, here does not mean much because it is currently called with caller==callee

[compleatang]

I'm pretty sure there's a few nuances in the proposed vm.DelegateCall which are not captured in geths -> https://github.com/ethereum/go-ethereum/blob/b7e3dfc5a2bc7e2f4d653fbe0ec9774277a10643/core/execution.go#L138-L169

[VoR0220]

The original EIP with all the specifications. ethereum/EIPs#23

[silasdavis]

I just want to emphasise the difference between DELEGATECALL and CALLCODE in the example I gave before. I think the following execution would occur with CALLCODE:

Stack	A	B	C
[]	n = 0, addr = nil	n = 0, addr = nil	n = 0, addr = nil
[CALL B]	n = 0, addr = nil	n = 0, addr = nil	n = 0, addr = nil
[CALLCODE C, CALL B]	n = 0, addr = nil	n = 1, addr = B	n = 0, addr = nil

Do we agree?

[benjaminbollen]

Yes, I agree that the stored address would be now stored as B as the message sender.

[benjaminbollen]

@pietv can you review whether for this PR to merge into master we want VERSION="0.12.0-rc2 or simply VERSION=0.12.0 (rc labelling still has me confused)

[pietv]

@benjaminbollen it's "0.12.0" for #master. without rc.

[silasdavis]

We keeping this as todo?

[benjaminbollen]

It is arguably redundant as value cannot be passed to another contract if it did not exist; and value is atomically swapped. However, should a contract rely on CALLVALUE Im at this point not confident it would not be possible to produce bytecode that would put a malignent value on the stack and as such provide a hidden way to corrupt seemingly valid solidity.

[silasdavis]

So other than my suggestion for a rewording of the comment describing the crucial implementation fact for DELEGATECALL, this looks good to me!

Great work Ben