fix: disable memiavl cache when optimistic execution is enabled(backport) #1890
Conversation
songgaoye
requested review from
JayT106 and
thomas-nguy
and removed request for
a team
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR updates GitHub Actions workflows by pinning external actions to specific commit SHAs, updates Go dependencies and build tooling (Nix, gomod2nix), adds legacy codec registration for historical transaction support, enhances memiavl and versiondb with batch writing and idempotency features, extends integration tests with batch transaction signing workflows, and adds documentation and configuration updates across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Store as Store/Cache
participant OptExec as Optimistic Execution
participant VersionDB as VersionDB
participant MemiAVL as MemiAVL
Note over App: Initialize application
App->>VersionDB: Check if versiondb.enable
alt versiondb enabled
VersionDB-->>App: setupVersionDB, capture qmsVersion
else versiondb disabled
App->>App: qmsVersion = 0
end
App->>OptExec: Check optimisticExecutionEnabled
alt optimistic execution enabled
OptExec-->>App: Enable in base app options
end
App->>Store: Decide on cache strategy
alt block-stm or optimistic execution enabled
Store->>MemiAVL: Skip memiavl cache
else both disabled
Store->>MemiAVL: Enable memiavl cache
end
App->>App: RegisterUpgradeHandlers(cdc, qmsVersion)
App->>Store: Set appropriate loader (MaxVersionStoreLoader or default)
Note over App: Loader selection based on version and upgrade metadata
App-->>App: Application ready
sequenceDiagram
participant Test as Integration Test
participant CLI as CosmosCLI
participant Node as Cronos Node
participant Batch as Batch TX Builder
participant Receipt as eth_getBlockReceipts
Test->>Batch: Prepare deploy, transfer1, transfer2 transactions
Batch->>Batch: Sign each transaction
Test->>CLI: build_batch_tx_signed(signed_txs)
CLI->>Node: Submit Cosmos batch transaction
Node-->>CLI: tx hash
Test->>Node: Wait for transaction confirmation
Node-->>Test: Transaction receipts
Test->>Receipt: Fetch block receipts via eth_getBlockReceipts
Receipt-->>Test: Receipts with blockNumber, transactionIndex, cumulativeGasUsed
Test->>Test: Validate receipt fields against expected values
Note over Test: Assert transactionIndex alignment and gas tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The diff spans multiple disparate subsystems (workflows, app core logic, memiavl internals, integration tests, nix configuration, documentation) with heterogeneous change types (logic refactoring, new features, dependency bumps, test additions). While individual cohorts may be straightforward, the breadth and interconnections require careful cross-file reasoning to ensure consistency across app initialization, storage handling, upgrade semantics, and test coverage. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
app/app.go
Outdated
| }) | ||
|
|
||
| blockSTMEnabled := cast.ToString(appOpts.Get(srvflags.EVMBlockExecutor)) == "block-stm" | ||
| optimisticExecutionEnabled := true |
There was a problem hiding this comment.
should you set the boolean somewhere?
There was a problem hiding this comment.
it's always true( in L449 SetOptimisticExecution). I use this variable because I want to disable memiavl cache.
There was a problem hiding this comment.
alternatively, is there a way to disable optimistic execution?
There was a problem hiding this comment.
good catch!. i can add a variable like srvflags.EVMBlockExecutor disable optimistic execution.
|
should the PR to |
app/app.go
Outdated
|
|
||
| // enable optimistic execution | ||
| baseAppOptions = append(baseAppOptions, baseapp.SetOptimisticExecution()) | ||
| if optimisticExecutionEnabled { |
There was a problem hiding this comment.
optimistic execution is crucial in enabling our quick blocktime though, would it be bad if we disable it?
There was a problem hiding this comment.
We should not recommend it but there is no harm to provide the option
sure, will update. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
songgaoye
changed the title
|
please also update the changelog 🙏 |
app/app.go
Outdated
| }) | ||
|
|
||
| blockSTMEnabled := cast.ToString(appOpts.Get(srvflags.EVMBlockExecutor)) == "block-stm" | ||
| // default enable optimisticExecution |
There was a problem hiding this comment.
Prefer wording The default value of optimisticExecution is enabled.
|
in favor of #1892, close it. |
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores