Remove subjective eos-vm-oc limits for whitelisted accounts #1010
Conversation
…lso prioritize compilation of whitelisted accounts when applying blocks.
libraries/chain/apply_context.cpp
Outdated
| @@ -1099,7 +1103,7 @@ action_name apply_context::get_sender() const { | |||
| // Compute trx | baseline, OC for eosio.* | |||
| // Read only trx | OC | |||
| bool apply_context::should_use_eos_vm_oc()const { | |||
| return receiver.prefix() == config::system_account_name // "eosio"_n, all cases use OC | |||
| return is_eos_vm_oc_whitelisted() // all cases use OC | |||
There was a problem hiding this comment.
Probably you don't need this all cases use OC comment.
| uint64_t get_cache_size() const { return cache_size; } | ||
| uint64_t get_threads() const { return threads; } | ||
|
|
||
| std::optional<rlim_t> get_cpu_limit() const { return !whitelisted ? cpu_limit : std::optional<rlim_t>{}; } |
There was a problem hiding this comment.
Add a comment saying null cpu_limit means no limit.
libraries/chain/wasm_interface.cpp
Outdated
| @@ -92,7 +92,9 @@ namespace eosio { namespace chain { | |||
| const chain::eosvmoc::code_descriptor* cd = nullptr; | |||
| chain::eosvmoc::code_cache_base::get_cd_failure failure = chain::eosvmoc::code_cache_base::get_cd_failure::temporary; | |||
| try { | |||
| const bool high_priority = context.get_receiver().prefix() == chain::config::system_account_name; | |||
| // Not high priority with producing a block since we want validators to have a higher probability of having | |||
There was a problem hiding this comment.
Can you reword this comment so it is easier to follow?
| @@ -126,7 +126,9 @@ const code_descriptor* const code_cache_async::get_descriptor_for_code(bool high | |||
| _outstanding_compiles_and_poison.emplace(*nextup, false); | |||
| std::vector<wrapped_fd> fds_to_pass; | |||
| fds_to_pass.emplace_back(memfd_for_bytearray(codeobject->code)); | |||
| FC_ASSERT(write_message_with_fds(_compile_monitor_write_socket, compile_wasm_message{ *nextup, _eosvmoc_config }, fds_to_pass), "EOS VM failed to communicate to OOP manager"); | |||
| auto msg = compile_wasm_message{ *nextup, _eosvmoc_config }; | |||
| msg.eosvmoc_config.whitelisted = high_priority; | |||
There was a problem hiding this comment.
Why set whitelisted to be high_priority? They don't mean the same thing.
There was a problem hiding this comment.
Good point. For now we do want whitelisted accounts to be high_priority, but maybe clearer to use two different variables.
| failure = get_cd_failure::permanent; // Compile will not start | ||
| return nullptr; | ||
| } | ||
| _blacklist.erase(ct); |
There was a problem hiding this comment.
Should return nullptr now or continue?
There was a problem hiding this comment.
It should continue. We do not want to blacklist whitelisted account contracts.
| : std::numeric_limits<uint64_t>::max(); | ||
| } | ||
|
|
||
| bool whitelisted = false; |
There was a problem hiding this comment.
This really feels like we're dual purposing a struct that represents global static configuration with something that represents a per-compile input.
My first thought on how to resolve this commingling is to refactor out the subjective limits to their own struct
struct subjective_compile_limits {
std::optional<rlim_t> cpu_limit {20u};
//...
std::optional<size_t> generated_code_size_limit {16u*1024u*1024u};
};Then make config be something like
struct config {
uint64_t cache_size = 1024u*1024u*1024u;
//...
subjective_compile_limits non_whitelisted_limits;
};And then make compile_wasm_message not take a config but rather a limits struct. e.g. maybe
struct compile_wasm_message {
code_tuple code;
eosvmoc::subjective_compile_limits limits;
};Then when sending a compile_wasm_message, can decide how to populate limits based on whether it's whitelisted or not. (you maybe could also make limits an optional here)
…ify eosvmoc::config to have a non_whitelisted_limits.
|
Note:start |
eos-vm-oc limitsfor whitelisted accountseosvmoc::configResolves #984