issue-476: return One256 for EXTCODESIZE on native contracts #476

VoR0220 · 2017-02-08T04:20:45Z

Fixes problem with not being able to get at the Snative contract.

Signed-off-by: RJ Catalano rj@monax.io

silasdavis · 2017-02-08T11:56:30Z

Could you be a bit clearer with "problem with not being able to get at the Snative contract.", what problem prompted your investigation here?

It does seem like there can be a problem here and your solution will in some sense work. However this is a special case of a broader problem. That is making sure we have some special case handling for snatives. The other obvious place this will be an issue is EXTCODECOPY:

https://github.com/eris-ltd/eris-db/blob/master/manager/eris-mint/evm/vm.go#L583

in fact EXTCODESIZE seems like it probably is designed be used in tandem with EXTCODECOPY to copy contract code around. We would need to handle some special case there too at least.

What is the context of use here? Pretending a native contract exists and has code length 1 may make certain code execute, but it doesn't make it make any sense, seems ad hoc, and does nothing to allow you to treat native contracts as if they have code accessible in contract-space. It seems like the best thing to do here might be to just make in an error. But we need more context about where this was encountered.

Another place where we don't check whether we are dealing with an snative, and perhaps we should is in CREATE: https://github.com/eris-ltd/eris-db/blob/master/manager/eris-mint/evm/vm.go#L763

benjaminbollen

This will need a proper understanding of the motivating changes in solc; if for snative contracts we would need to return a non-error; then we need to first understand what should be returned;

benjaminbollen · 2017-02-08T11:48:59Z

manager/eris-mint/evm/vm.go

@@ -565,13 +565,17 @@ func (vm *VM) call(caller, callee *Account, code, input []byte, value int64, gas
 			}
 			acc := vm.appState.GetAccount(addr)
 			if acc == nil {
-				return nil, firstErr(err, ErrUnknownAddress)
+				if _, ok := registeredNativeContracts[addr]; !ok {


Thus far we have not returned a code size for native contracts. Is this motivated by a change in solidity, where it newly checks the code length before going to a contract. We have not had a problem before calling our snative contracts. Can you document and elaborate?

silasdavis · 2017-02-08T16:24:33Z

manager/eris-mint/evm/vm.go

+				if _, ok := registeredNativeContracts[addr]; !ok {
+					return nil, firstErr(err, ErrUnknownAddress)
+				}
+				stack.Push64(int64(1))


More idiomatic and a little more efficient to use stack.Push(One256) here

silasdavis · 2017-02-08T16:34:33Z

Here's a fragment of solc 4.4.0 compiled bytecode that helps illuminate what the solidity compiler is up to:

      EXTCODESIZE 			perm.rm_role(addr, role)
      ISZERO 			perm.rm_role(addr, role)
      PUSH [ErrorTag] 			perm.rm_role(addr, role)
      JUMPI 			perm.rm_role(addr, role)
      PUSH 32			perm.rm_role(addr, role)
      GAS 			perm.rm_role(addr, role)
      SUB 			perm.rm_role(addr, role)
      CALL 			perm.rm_role(addr, role)
      ISZERO 			perm.rm_role(addr, role)
      PUSH [ErrorTag] 			perm.rm_role(addr, role)
      JUMPI 			perm.rm_role(addr, role)
      POP 			perm.rm_role(addr, role)
      POP 			perm.rm_role(addr, role)
      POP 			perm.rm_role(addr, role)
      PUSH 40			perm.rm_role(addr, role)

Here's the relevant line in the solidity compiler that generates this type of code: https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/ExpressionCompiler.cpp#L1662

The check amounts to an existence check for the contract/contract code before running it. I am not certain why this is, but it seems like in the current pricing model it might be better to check the code exists before calling empty code. Worth checking.

In this compiler case returning any truthy value seems to be the right thing to do, and 1 is probably the most elegant.

There is a worry with how this interacts with EXTCODECOPY. It could easily try and copy non-existing bytes into allocated space. However as a point of reassurance, a quick grep of the solidity compiler code does not seem to show any use of EXTCODECOPY by the the solidity code generator.

If someone is deliberately trying to access snative code from an EXTCODECOPY then we have a problem, but an error is probably the right thing to do there from the time being. We could return stub code, but that seems like it would cause more pain than it might prevent. EXTCODECOPY is a low-level thing, and I think solidity is our main concern...

silasdavis · 2017-02-08T17:24:23Z

manager/eris-mint/evm/vm.go

+			} else {
+				code := acc.Code
+				l := int64(len(code))
+				stack.Push64(l)


again, I think this should be stack.Push(One256) it saves doing the conversion to use the constant, and it is used elsewhere

This is the actual code length for normal contracts that are not natives.

It should stay as it is.

benjaminbollen · 2017-02-08T17:33:34Z

LGTM

Signed-off-by: RJ Catalano <rj@monax.io>

…CallsFromSolc issue-476: return One256 for EXTCODESIZE on native contracts

VoR0220 mentioned this pull request Feb 8, 2017

Wip attempted fix at jump i #477

Closed

benjaminbollen suggested changes Feb 8, 2017

View reviewed changes

silasdavis suggested changes Feb 8, 2017

View reviewed changes

silasdavis reviewed Feb 8, 2017

View reviewed changes

benjaminbollen approved these changes Feb 8, 2017

View reviewed changes

benjaminbollen changed the base branch from develop to release-0.16 February 8, 2017 17:38

benjaminbollen changed the title ~~fixes snative calls from solc~~ issue-476: return One256 for EXTCODESIZE on native contracts Feb 8, 2017

RJ Catalano and others added 4 commits February 8, 2017 11:40

fixes snative calls from solc

c7933f0

Signed-off-by: RJ Catalano <rj@monax.io>

use one256 for slightly better efficiency

f75283e

Signed-off-by: RJ Catalano <rj@monax.io>

add error for codecopy

fed9a15

Signed-off-by: RJ Catalano <rj@monax.io>

add error on EXTCODECOPY native contract and fix One256 push

9e141fa

VoR0220 force-pushed the fixesSNativeCallsFromSolc branch from 8dcb9d0 to 9e141fa Compare February 8, 2017 17:41

silasdavis approved these changes Feb 8, 2017

View reviewed changes

silasdavis merged commit 45fbfdd into hyperledger-archives:release-0.16 Feb 8, 2017

silasdavis added a commit to silasdavis/burrow that referenced this pull request Mar 9, 2019

Merge pull request hyperledger-archives#476 from VoR0220/fixesSNative…

325e91d

…CallsFromSolc issue-476: return One256 for EXTCODESIZE on native contracts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue-476: return One256 for EXTCODESIZE on native contracts #476

issue-476: return One256 for EXTCODESIZE on native contracts #476

VoR0220 commented Feb 8, 2017

silasdavis commented Feb 8, 2017 •

edited

Loading

benjaminbollen left a comment

benjaminbollen Feb 8, 2017

silasdavis Feb 8, 2017

silasdavis commented Feb 8, 2017

silasdavis Feb 8, 2017

VoR0220 Feb 8, 2017

VoR0220 Feb 8, 2017

benjaminbollen commented Feb 8, 2017

issue-476: return One256 for EXTCODESIZE on native contracts #476

issue-476: return One256 for EXTCODESIZE on native contracts #476

Conversation

VoR0220 commented Feb 8, 2017

silasdavis commented Feb 8, 2017 • edited Loading

benjaminbollen left a comment

Choose a reason for hiding this comment

benjaminbollen Feb 8, 2017

Choose a reason for hiding this comment

silasdavis Feb 8, 2017

Choose a reason for hiding this comment

silasdavis commented Feb 8, 2017

silasdavis Feb 8, 2017

Choose a reason for hiding this comment

VoR0220 Feb 8, 2017

Choose a reason for hiding this comment

VoR0220 Feb 8, 2017

Choose a reason for hiding this comment

benjaminbollen commented Feb 8, 2017

silasdavis commented Feb 8, 2017 •

edited

Loading