Introduce ‘max_gas_burnt_view’ client config and command line flag - #4381
Conversation
|
I haven’t yet fully tested that the limit actually takes hold. Any hints how to do that welcome. :) |
I think the best way will be add a pytest (in pytest dir), where you can write a python test to override config.json and command line options, and send rpc request to check the setting is effective |
I’m struggling a bit with figuring out an RPC to test the setting. |
|
The check failure looks related to actix/actix-web#2185. I’ve tried updating |
bowenwang1996
left a comment
There was a problem hiding this comment.
I am a bit unsure whether it is a good idea to make max_gas_burnt_view part of RuntimeConfig since RuntimeConfig is part of the protocol right now and mixing it with non-protocol related config could cause confusion
|
It already is part of the RuntimeConfig (and is included in genesis config). This change will make it so that the actual value used by the runtime will be overridden. |
|
oh I didn't realize that. @evgenykuzyakov what was the motivation to have it as part of |
bowenwang1996
left a comment
There was a problem hiding this comment.
If you want to properly test this change, you can first write a contract that has some method that costs more than 200Tgas to run and then either write a python test or a unit in process_blocks.rs to call that method through a view call.
I encountered this fail in my PR too, one way of fix this is to pin actix-http: 7dcfd0d and also update Cargo.lock 6fa4bd8 |
frol
left a comment
There was a problem hiding this comment.
Overall looks good to me, so I unblock the PR, but please address the comments about the Indexer example.
| genesis_config: GenesisConfig, | ||
| genesis_runtime_config: Arc<RuntimeConfig>, | ||
| /// Runtime configuration. Note that it may be slightly different than | ||
| /// `genesis_config.runtime_config`. Consider `max_gas_burnt_view` value | ||
| /// which may be configured per node. | ||
| runtime_config: Arc<RuntimeConfig>, |
There was a problem hiding this comment.
I feel that config handling here accumulated quite a bit of technical debt, as we now have two separate paths that "patch" config: one with max_gas_burnt, and another with RuntimeConfig::from_protocol_version (which is especcially confusing, as it relies on hidden global state). I have a stalled PR which cleans parts of this up in #4263 (comment).
This isn't strictly related to the PR, so it isn't a blocker, but I want to raise awareness and signal that straightening this up is non-urgent, but important ;-)
|
Added tests though also added a |
olonho
left a comment
There was a problem hiding this comment.
Overall looks good, modulo potential argument refactoring and subtle serialized format change, I'm unsure about.
The ‘max_gas_burnt_view’ affects the RPCs only and therefore doesn’t need to be part of the consensus. In other words, all nodes could have this value customised rather than using an agreed upon value from genesis configuration. Introduce a ‘max_gas_burnt_view’ field to ClientConfig and a corresponding flag to ‘init’ and ‘run’ subcommands. When present, it will override the limit used by the node ignoring whatever is stored in genesis configuration. Fixes: near#3080
The ‘max_gas_burnt_view’ affects the RPCs only and therefore doesn’t
need to be part of the consensus. In other words, all nodes could
have this value customised rather than using an agreed upon value
from genesis configuration.
Introduce a ‘max_gas_burnt_view’ field to ClientConfig and
a corresponding flag to ‘init’ and ‘run’ subcommands. When present,
it will override the limit used by the node ignoring whatever is
stored in genesis configuration.
Fixes: #3080