Conversation
| EXECUTOR_PORT='4000' | ||
| EXECUTOR_FUEL_PRICE_PER_UNIT='1' | ||
| EXECUTOR_MIN_FEE_PER_REQUEST='10' | ||
| EXECUTOR_MAX_CACHED_MODULES='0' |
There was a problem hiding this comment.
This var should be added in docker-compose.yml
| m.log.Warn("Manager: State root mismatch for app %d, expected %x, got %x. Checking if it is a REORG.", appID, localStateRoot, stateRoot) | ||
|
|
||
| isReorg, err := m.checkIfReorg(stateRoot) | ||
| isReorg, err := m.checkIfReorg(appID, stateRoot) |
There was a problem hiding this comment.
Why the reorg check is at app-level instead of chain-level? It should be enough to have a single timer. In addition, we would need a way to reset all the expired timers, otherwise an app could get another reorg with an old expired timer and wrongly assume that the timer is related to the same reorg and so triggering a dangerous rollback
There was a problem hiding this comment.
You are right, fixed. I also added a unit test for this case.
pkg/wasm/wasmtime_runtime.go
Outdated
| @@ -203,6 +211,10 @@ func (r *WasmtimeRuntime) getOrLoadModule(ctx context.Context, appId common.Appl | |||
There was a problem hiding this comment.
It is safer to take here directly a write lock, instead of a read one. In any case the write lock must be taken after RUnlock and if you're unlucky the module is evicted before regaining the write lock again.
There was a problem hiding this comment.
I modified this code so now it should be ok
There was a problem hiding this comment.
There is still the read lock, that is useless in this case
pkg/wasm/wasmtime_runtime.go
Outdated
| } | ||
| for r.accessOrder.Len() > r.maxCachedModules { | ||
| back := r.accessOrder.Back() | ||
| if back == nil { |
There was a problem hiding this comment.
Is it possible that back is nil? r.accessOrder.Len() must be at least 2
pkg/wasm/wasmtime_runtime.go
Outdated
|
|
||
| r.log.Info("Runtime: Setting maxCachedModules from %d to %d", r.maxCachedModules, max) | ||
| r.maxCachedModules = max | ||
| r.evictIfNeeded() |
There was a problem hiding this comment.
It is probably safer to avoid calling evictIfNeeded here and let just the "loadmodule" path to take care of cleaning modules. This way we avoid the risk of having 2 go routines that can modify the list of modules at the same time
| return &adminMessage{Type: keyAttestationRequest, Target: "executor"} | ||
| } | ||
|
|
||
| func buildGetWasmCacheSize() (*adminMessage, error) { |
There was a problem hiding this comment.
Nitpick: I have noticed that all the build* functions return an error, even when it is not needed, like in this case. The only exception is buildKeyAttestation. If it could be important that all these functions have the same signature, then buildKeyAttestation should be updated, otherwise the error can be removed from function where it is not needed.
There was a problem hiding this comment.
Ok, I opted for keeping the same signature
cmd/admincli/README.md
Outdated
|
|
||
| Changes the WASM module LRU cache limit at runtime. If the new limit is lower | ||
| than the current number of cached modules, excess modules are evicted | ||
| immediately (least recently used first). Evicted modules are transparently |
There was a problem hiding this comment.
This is no longer true, excess modules are evicted next time a module is loaded
pkg/executor/executor.go
Outdated
| if cc, ok := e.runtime.(moduleCacheController); ok { | ||
| cc.SetMaxCachedModules(req.MaxCachedModules) | ||
| } else { | ||
| e.log.Warn("Executor: set_wasm_cache_size ignored (runtime does not support module caching)") |
There was a problem hiding this comment.
In this case, shouldn't it return an error? If not, at least it should return 0 instead of req.MaxCachedModules, otherwise is misleading
There was a problem hiding this comment.
Ok, the same also applied to the get cmd
pkg/wasm/wasmtime_runtime.go
Outdated
| @@ -203,6 +211,10 @@ func (r *WasmtimeRuntime) getOrLoadModule(ctx context.Context, appId common.Appl | |||
There was a problem hiding this comment.
There is still the read lock, that is useless in this case
2 participants
Update Manager and Executor to support multiple applications.
See https://horizenlabs.atlassian.net/browse/HZN-2803