Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace the contract method name with an offset #1335

Closed
erikzhang opened this issue Dec 6, 2019 · 15 comments · Fixed by #1481
Closed

Replace the contract method name with an offset #1335

erikzhang opened this issue Dec 6, 2019 · 15 comments · Fixed by #1481
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@erikzhang
Copy link
Member

If we avoid the Main method and we index the Offset in the ABI for all methods, we can avoid the initial switch too and start in the right place at the beginning.

What do you think @erikzhang @lightszero?

Originally posted by @shargon in #1094 (comment)

@erikzhang erikzhang added the Discussion Initial issue state - proposed but not yet accepted label Dec 6, 2019
@erikzhang
Copy link
Member Author

This is an interesting proposal.

Usually we call a contract like this:

Call(hash, method, args)

Do you mean that we change to this:

Call(hash, offset, args)

This will indeed be faster. But I can think of some drawbacks, such as the inability to hide private methods.

Originally posted by @erikzhang in #1094 (comment)

@shargon
Copy link
Member

shargon commented Dec 6, 2019

What do you think @lightszero?

@igormcoelho
Copy link
Contributor

igormcoelho commented Dec 6, 2019

Looks interesting! I see two different proposals being discussed, the ability of directly calling on ABI by absolute position and the call-by-offset...
On ABI we could put absolute value for entrypoint, couldn't we? Example: ABI "transfer()" = 0x0010 (absolute position on NVM). And after executing such method, would it return and directly finish the entrypoint, right? It's like having multiple entrypoints, in different locations, indexed by string operation. Looks promising. (By default ABI "Main()" = 0x0000, on practice).
Regarding private methods, as long as ABI itself is generated by compiler, it doesn't change much from current situation, as script and ABI themselves control the execution flow. When we start allowing random offsetting, then yes, we could probably break encapsulation very easily. I think Shargon was aware of this, when he mentioned: "If we allow only the offset if it is inside the ABI, we can hide them.". I agree with that. But in my opinion, Call would continue to be Call(method_name), and ABI would provide offset directly, we clearly cannot allow users to jump into any random position of NVM.

@shargon
Copy link
Member

shargon commented Dec 6, 2019

we can have an array with the valid offsets, and only check that it is in the range. I prefer to cache this inside the storage

@roman-khimov
Copy link
Contributor

From the code I've seen one method check is ~20 bytes of code (including method name push), if we have 10 methods that's roughly 200 bytes of code out of ~9K of the average contract size (based on mainnet stat at the height of 4076134 with 141 contracts deployed). And it's a simple push-compare-jump code that shouldn't be visible in the overall contract execution time.

It also adds some complexity to the compiler and its relations with developers, because before that it's simple for both of them --- you have a Main with known parameters and you're free to do whatever you want with it (and compiler just generates code), now if you're to automatically make this entry-point table there should be some agreement between the developer and compiler on what is exported and what is not (and compiler should be more careful when generating code). It can also be a little more complicated to do (and not to forget!) some generic intro/outro code that has to be executed on every contract invocation irrespective of the method being called.

One benefit that I see here at the same time is a common automatic contract interface specification (in terms of available methods), but I doubt that it would be enough to be a real spec, because methods are methods, but all the rest of the arguments are just an array at the moment and the contract needs to have documentation for them anyway.

@melanke
Copy link

melanke commented Jan 17, 2020

I may be saying something a little naive but couldn't we add a C# annotation above the method to mark that method as an entry point to the Smart Contract? I believe it would be a lot easier to develop contracts that way. Something like this:

@ContractMethod
public void MyMethod() {

}

@lightszero
Copy link
Member

make a contract more like a dll?good point

@lightszero
Copy link
Member

我来多解释一下“make a contract more like a dll“

现在使用的合约方式是这样:
合约只有一个固定的入口点,offset 0
调用约定在abi文件里
由入口点零 对 第一个参数 name 做分支,确定要调用合约的什么功能

如果我们只是单纯的在入口点0里面把用name来做分支改为用index来做分支
而把name到index的映射放在abi文件中,那这就有点像c语言的头文件和库的关系
这已经被证明是一种低效的方式,而且安全性很糟糕。
vbi里面错误的映射乃至恶意的映射,都会导致灾难性的后果

所以我们应该做一个整体的设计
1.可以有多个入口点,我们不再限制必须由入口点0启动
这样根本就可以删除offset0的 switch逻辑,你要调哪个函数,直接从函数偏移开始执行
2.abi文件的功能被直接整合在nef中
定义和实现不能分离,你说你要调用”abc"函数,我不需要到nef外面去找abc函数的offset是多少,就在nef文件中就有信息
3.nef应该自签名,加强作为一个文件格式的安全性

让nef文件更像一个dll,我们用工具可以直接看到nef里面有几个可调用的函数,他们的参数,他的入口点。我们调用一个函数的时候,虚拟机可以直接从函数的offset开始执行。

@lightszero
Copy link
Member

@Tommo-L help,need translate

@Tommo-L
Copy link
Contributor

Tommo-L commented Mar 3, 2020

to EN:

Let me explain "make a contract more like a DLL"

Currently, the way to invoke the contract is as follows:
The contract has only one fixed entry point offset 0, which is specified in the abi file. By making a branch judgment on the first parameter name at entry position of 0, to determine which function of the contract is to be called.

If we just replace name by index for branching judgment at the entry position of 0, and save the mapping from name to index in abi file, it's like the relationship between the C header file and the library. This has proven to be an inefficient and unsafe way. Incorrect or even malicious mappings in the abi file will lead to disastrous consequences.

So we should make a complete design:

  1. There can be multiple entry points, no longer restricted to start from the entrypoint 0.
    In this way, we can delete the switch logic of Main method. No matter which function you want to call, just execute directly from the function's offset.
  2. The functionality of the abi file should be integrated into the nef file.
    Definition and implementation cannot be separated. If want to call abc method, we don't need to go outside the nef file to find the offset of abc method, just read it from the nef file.
  3. The nef file should be self-signed to enhance security as a file format.

To make the nef file more like a dll, we can use some tools to see which the nef has callable functions, their parameters, and its entry point. When we want to call a function, vm can start execution directly from the offset of the function.

@Tommo-L
Copy link
Contributor

Tommo-L commented Mar 12, 2020

Sine we already have three files: abi, manifest.json, nef. And manifest.json already contrains the abi information. I think we can remove the abi file and extend manifest.json with offset field and remove entryPoint field.

{
	"groups": [],
	"features": {
		"storage": true,
		"payable": true
	},
	"abi": {
		"hash": "0x13f98375a2a5bf2c8ab9a71b7c1a66d0a96f82bf",
		"methods": [{
			"name": "main",   //<--- only save public method
			"parameters": [{
				"name": "account",
				"type": "ByteArray"
			}],
			"returnType": "Integer",
			"offset": "0x00"  // <------ add offset field
		}]
	},
	"permissions": [{
		"contract": "*",
		"methods": "*"
	}],
	"trusts": [],
	"safeMethods": []
}

Then, when we want to invoke a contract, just call by this way. In System.Contract.Call syscall, we'll replace the method name by offset which stored in manifest.json file. And we also have the ability to hide private method.

Call(hash, method, args)

@erikzhang
Copy link
Member Author

neo-project/proposals#109

@Tommo-L
Copy link
Contributor

Tommo-L commented Mar 15, 2020

If we use this way, should we add a special method for VerificationTrigger?

@shargon
Copy link
Member

shargon commented Mar 15, 2020

should we add a special method for VerificationTrigger?

Yes, also it should be safer

@erikzhang
Copy link
Member Author

verify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants