-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow contracts to load script at runtime dynamically #2756
Conversation
Can we at least postpone it after 3.3.0? |
Sure. |
3.3.0 is released. Now we can review this. |
I wouldn't do this at all. Because:
Is there any real problem that requires this as a part of solution? |
I can do it even today. public void Eval(byte[] script)
{
ByteString nef = BuildNef(script);
string manifest = BuildManifest(script);
var contract = ContractManagement.Deploy(nef, manifest);
Contract.Call(contract.Hash, "main", CallFlags.All);
} So it at least doesn't bring new problems.
I'm trying to build a dapp with permission management. Usually, when we need to set an administrator, we must explicitly tell the contract which one our administrator account is. But if our administrator account is not fixed, such as consensus address or committee address, there is no way to express it by a fixed address. With dynamic script loading, we can tell the contract the logic of "how to get the admin address". This is equivalent to a contract that can store a piece of logic, not just data. I think it's an improvement, and I didn't expect any security issues with this. If there is, we can limit it, but at least we should discuss it. |
This contract will stay on-chain and the invocation will cost you quite some GAS. Real dynamic code is a different problem to me.
What prevents this logic to be implemented as a simple contract method? Of a contract that can be updated, if need be. |
Because I am a user, not the contract owner. My plan is to allow the users to store logic in a contract. |
Imagine a contract with many users, and each user stores data in the contract. Each user can designate administrators for their data. And allow administrators not to be fixed, but to pass through a logic. An example of a non-fixed administrator is a consensus address or committee address. |
@erikzhang Is right, we can do it right now... |
We don't store it explicitly, but this can be done (and there can be an event with it like in current #2764), it all boils down to an address anyway. And it's contract-derived, following well-known update logic. Users that really want this can deploy regular contracts and use their addresses.
The contract deployed can be destroyed in the same transaction, making it somewhat ephemeral, that's true (but even in this case there will be some events generated). This loophole can be easily closed though. |
But how do you tell the contract that "I want the committee address to be the administrator of my data"? And how do you specify arbitrary logic? For example, "the multi-sig address of the top 10 richest neo holders". Don't forget that I am not the owner of the contract, I'm just a user. With this update, I can tell the contract that, "I can't tell you exactly which address to be the administrator, but I can tell the logic of how to obtain it." |
One can deploy contract V and use V as the administrator address in contract A (I suppose A is just doing |
Yes, it works. But deploying a contract is a bit expensive, and usually the logic of getting an address is very simple logic, and it's not worth deploying a contract for this. |
And, by contrast, specifying a contract V is actually more dangerous. For example, if I specify that V is |
What's the use case for this feature? Regardless of what does or does not work today, what is the problem this is intended to solve? How would we see developers using this feature? Note, I'm with @roman-khimov - this seems very dangerous. |
The script is loaded in a different context, so it's not dangerous. This is the same as calling a contract, but saves the cost of deploying a contract. |
I'm not sure that's enough protection. Can you provide an example of how this might be used? Without a use case, I don't see why we would add this Also, would a dynamically loaded script be able to store data and raise notifications? Data seems like it would be an issue since there is no entry in the contract table. |
Suppose there is a DAO contract that allows users to create an organization and set up administrative accounts according to the organization's rules. Since users can create arbitrary rules for the organization, the contract cannot know the specific content of the rules in advance, so these rules must be described in dynamic scripts. |
This looks a good feature, in fact, we have been always supporting this like the opeval. |
Go? |
I am double checking with @igormcoelho some details, soon we will test here. |
BTW, its cost is a feature to me. This PR allows to deploy some cheap shim contract that will accept new code as data, store it as data and invoke as needed. Any updates to this code will also be silent (unlike regular contract updates). So it will make #2788 problem much worse. Dynamic code won't be able to use And how about loading the entry script once again as dynamic one to try having some fun with I don't think anything changed on my side, this feature will bring more problems than it'll solve. Also, from a pure development cycle perspective we're supposed to be rolling 3.4.0 ASAP now (#2787) and merging this right before the release is also questionable to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested @superboyiii ?
I wanted to suggest a script hash check to forbid (re)loading the entry script completely, but then I thought that dynamic code can add Some brave souls may also try invoking dynamic code in the verification context. Or maybe not brave, but just not careful enough to see that it happens in some contract internals. This might also be an interesting experience, although not very likely to happen. Just some random thoughts, sorry for the noise. |
@roman-khimov You convinced me. So how about only allowing dynamic scripts to have the |
@erikzhang,sounds good, then, it really comes as we previously pushed in the past. @igormcoelho has highlighted it again as well.
|
That will greatly reduce the attack surface, so it's an improvement for sure (that also allows to simplify some of the implementation details). But I'm still not convinced we need it. It's a cool feature, no doubt about it, but:
So to me it's a choice between a number of not completely understood risks and a problem that affects something like three users and can be solved in other way. To me it's not worth doing. We need more use cases with wider potential audience and we need more of methodical research on this topic before accepting this. |
We can merge #2796 first. |
# Conflicts: # src/Neo/Network/P2P/Payloads/Conditions/CalledByEntryCondition.cs # src/Neo/SmartContract/ExecutionContextState.cs
Done. I don't think there is any risk now. |
Tested OK. |
If there are no objections, I merge it. |
From our side we are in agreement. |
No description provided.