Allow setting Additional GAS to be used in RpcInvoke for RpcServer. Expose overloaded ApplicationEngine.Run allowing a passed Snapshot. #397
Conversation
…verload allowing a passed Snapshot.
|
In case someone wants to test to see how much invoking a contract will cost -- decided to increase the limit to 1000 |
|
Ok, but shouldn’t it still be allowed to be overridden when calling ApplicationEngine.Run |
|
Just for RPC |
|
There are potentially other uses of ApplicationEngine.Run outside of the core that will want to override it. As for ways to proceed related to adding a setting for RPC here are options:
|
|
Thank you, looks good |
|
I am afraid that 1000 gas is not enough for an exchange with millions of addresses. I feel that the For dos attacks, the RPC server operator should turn on the firewall to block external access; the public server should use the RpcDisabled plugin to block these dangerous APIs. |
|
Then let an exchange increase it for their nodes only. People run public Rpc nodes, right? Or are you discouraging that? |
|
We can set by default 100.000 of GAS or more, but the feature is good for me |
|
Do you mean that the exchange can modify the maximum allowed gas by setting the config.json file? |
|
Yes, a configuration setting such as that seems appropriate. The StartRpc method of NeoSystem should pass the parameter along so it can be configured properly from neo-cli or whatever top level package is being used with the NEO core. I’ll adjust the PR for that sometime in the next couple days. |
|
Exactly this @erikzhang , the idea is set this max on config.json, anyone could set his max according to his necessities |
| { | ||
| using (Snapshot snapshot = Blockchain.Singleton.GetSnapshot()) | ||
| snapshot.PersistingBlock = persistingBlock ?? new Block |
There was a problem hiding this comment.
If snapshot.PersistingBlock is not null and persistentBlock is null, should we keep the original value instead of creating a new block?
There was a problem hiding this comment.
Good idea. I’ll make that change.
|
Do we have an estimate of when we will be able get this work merged in? |
|
I will finish up the change today and Erik can merge it as soon after as he
would like.
…On Thu, Oct 11, 2018 at 10:18 AM Maxwell Lasky ***@***.***> wrote:
Do we have an estimate of when we can get this work merged in?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#397 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AD6kxkSvpsvKMmrNkSwdcvbOOJpXz1h5ks5uj31mgaJpZM4W5Sdf>
.
|
|
It's ready for review again, and can be merged if @erikzhang doesn't have any further concerns. |
|
Lol, I think it is finally ready now. |
It was reported (#396) that invoke calls prior to 2.9.0 supported more than 10 GAS limit. There should be some limit, but not 10 since it helps batch results coming back to clients. This raises the limit to 300 GAS.
Note: It was reported Neon wallet currently makes invokes that use between 40 and 50 GAS.
Also, some users of neo core want to run against a snapshot that may not be the current block. I'm exposing a mechanism to do this with the overloaded ApplicationEngine.Run method.
Close #396