Cache state db #4773
Cache state db #4773
Conversation
📝 WalkthroughWalkthroughAdds a global per-block state cache, threads dual-VM execution outputs through open-block/finalize (FinalizedBlock), exposes execution-with-outputs APIs, integrates cache lookups into chain execution and miner caching, and updates shutdown and test flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Miner
participant Opened as OpenedBlock
participant Finalized as FinalizedBlock
participant Cache as GlobalCache
participant Chain
participant Exec as Executor
Miner->>Opened: push transactions (record VM1 & VM2 outputs/infos/events)
Opened-->>Miner: finalize() → Finalized (template + statedb(s) + executed_data(s))
Miner->>Cache: insert(template.txn_accumulator_root, CachedBlockState)
Chain->>Cache: lookup(txn_accumulator_root)
alt cache hit (complete)
Cache-->>Chain: cached statedb & executed_data (VM1 & VM2)
Chain->>Exec: block_execute_with_outputs(txns, cached outputs)
else cache miss / partial
Chain->>Exec: execute_block_transactions(txns) -> fresh VM1 & VM2 execution
end
Exec-->>Chain: BlockExecutedData (VM1 & VM2)
Chain->>Chain: apply state roots, flush state, finalize block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chain/src/chain.rs (1)
2443-2471: Cached block execution skips populating txn_table_infos, creating persistent metadata divergenceIn
execute_dag_block, when the full cache hit condition is met (allCachedBlockStatefieldsSome),executed_dataandexecuted_data2sourced fromFinalizedBlockare used directly. However,FinalizedBlock::finalizeconstructs both as:txn_table_infos: BTreeMap::new(), // TODO: track table infos if neededIn contrast, non-cached execution paths (
block_execute,execute_transactions) populatetxn_table_infosby appending transaction outputs, then callstorage.save_table_infos(txn_table_infos)to persist them.This creates divergence: nodes executing the same block from cache persist empty
txn_table_infos, while nodes re-executing without cache persist populated table metadata. Consensus hashes match, but stored metadata differs.Recommended fix: Populate
txn_table_infosduringOpenedBlockexecution and pass real data toFinalizedBlockconstruction instead of empty maps.
🧹 Nitpick comments (6)
benchmark_stats_20251208_125237.txt (1)
1-133: Consider removing this benchmark artifact from the repository.This appears to be a development/testing artifact containing benchmark results. Typically, such files should not be committed to version control unless there's a specific documentation purpose. Consider:
- Moving to a
docs/benchmarks/directory if needed for reference- Adding to
.gitignoreif it's generated during development- Removing from the PR if committed unintentionally
miner/src/create_block_template/block_builder_service.rs (1)
8-11: Finalize path caching looks coherent; be aware of stale entriesThe way you cache FinalizedBlock data under
template.txn_accumulator_rootand later consume it inBlockChain::execute_dag_blockis consistent (same key, correct ordering for VM1/VM2 outputs, correct use ofArcfor state DBs and executed data). No correctness issues spotted.One thing to keep in mind: templates that are never turned into committed blocks will leave their entries in
GLOBAL_BLOCK_STATE_CACHE/GLOBAL_TXN_OUTPUT_CACHEindefinitely. Over time, in a busy miner this can accumulate a lot ofChainStateDBandTransactionOutputdata. You’re currently relying solely on the DAG execute path toremoveentries.Consider adding some form of pruning (size bound / age-based eviction / periodic sweeping) or at least metrics so you can monitor cache growth and act before it becomes a memory problem.
Also applies to: 612-651
executor/src/block_executor.rs (1)
13-13: block_execute_with_outputs mirrors block_execute; consider tightening invariantsThe new
block_execute_with_outputshelper correctly mirrorsblock_executesemantics (status handling, gas accounting, write-set application, and commit-on-first-or-last) and should integrate cleanly with the cache path.Two suggestions:
- To guard against mismatched inputs, it’s worth enforcing (at least in debug) that
txns.len() == outputs.len()rather than silently truncating tooutputs.len():debug_assert_eq!(txns.len(), outputs.len());
- Long-term, you might want to factor the common “consume (txn, output) pairs into BlockExecutedData” logic into a shared internal helper to reduce duplication between
block_executeandblock_execute_with_outputs.Also applies to: 149-203
vm2/executor/src/block_executor.rs (1)
12-13: VM2 block_execute_with_outputs is consistent; same note on length invariantThe VM2
block_execute_with_outputsimplementation is consistent with the existingblock_execute(status handling, write-set application, commit-on-first-or-last, and BlockExecutedData structure).As with the VM1 variant, I’d recommend a debug assertion that
txns.len() == outputs.len()to catch cache / wiring bugs early instead of silently ignoring extra transactions or outputs:debug_assert_eq!(txns.len(), outputs.len());Also applies to: 101-154
chain/src/chain.rs (1)
4-9: Cache helper wiring is mostly correct; note that cached state DBs are not yet surfaced
execute_block_transactions_with_cacheis wired correctly to:
- Prefer cached VM1/VM2 outputs when present.
- Fall back to normal
block_execute/execute_transactionsotherwise.- Respect VM1 gas usage when deciding VM2’s gas budget.
Currently the
(Option<Arc<ChainStateDB>>, Option<Arc<ChainStateDB2>>)tuple components are alwaysNone; the function never returns a cached state DB. That’s fine for now but makes the signature a bit misleading.If you don’t plan to return pre-applied state DBs from here, consider either:
- Dropping the extra return values, or
- Implementing the “cached state DB” path inside this helper so
execute_dag_blockdoesn’t need to know about it.Also applies to: 2169-2264
chain/src/txn_output_cache.rs (1)
178-237: Tests cover TransactionOutputCache, consider adding BlockStateCache testsThe existing tests exercise
TransactionOutputCacheand the global txn output cache well (insert, get, remove, clear). There are no analogous tests forBlockStateCache.Given
BlockStateCachewill carry ChainStateDB arcs and executed data, it would be useful to have at least one basic test ensuring:
insert/get/removebehave as expected, andclearempties the map.This will help catch any future refactors that accidentally break the state-cache path.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockstarcoin.logis excluded by!**/*.log
📒 Files selected for processing (15)
benchmark_stats_20251208_125237.txtchain/Cargo.tomlchain/open-block/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rschain/src/lib.rschain/src/txn_output_cache.rsexecutor/src/block_executor.rsexecutor/src/lib.rsminer/src/create_block_template/block_builder_service.rssync/src/tasks/block_sync_task.rstxpool/src/test.rsvm2/chain/src/lib.rsvm2/executor/src/block_executor.rsvm2/executor/src/lib.rs
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/src/lib.rssync/src/tasks/block_sync_task.rsminer/src/create_block_template/block_builder_service.rsvm2/chain/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rsexecutor/src/block_executor.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/src/lib.rssync/src/tasks/block_sync_task.rsminer/src/create_block_template/block_builder_service.rstxpool/src/test.rsvm2/chain/src/lib.rschain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/src/lib.rssync/src/tasks/block_sync_task.rsvm2/executor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/lib.rstxpool/src/test.rsvm2/chain/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rsexecutor/src/block_executor.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/lib.rsvm2/executor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rstxpool/src/test.rsvm2/chain/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rsexecutor/src/block_executor.rschain/Cargo.tomlchain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/src/lib.rsvm2/chain/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rsexecutor/src/block_executor.rschain/Cargo.tomlchain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
chain/src/lib.rssync/src/tasks/block_sync_task.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/lib.rstxpool/src/test.rsexecutor/src/block_executor.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/src/lib.rsminer/src/create_block_template/block_builder_service.rsvm2/chain/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rsexecutor/src/block_executor.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-16T01:30:06.195Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4658
File: vm2/vm-runtime/src/parallel_executor/mod.rs:0-0
Timestamp: 2025-09-16T01:30:06.195Z
Learning: In the Starcoin codebase, the Display schema for AccessPath is stable, making string-based matching via format!("{}", path) a reliable approach for identifying specific resource paths like the STC transaction fee aggregator.
Applied to files:
chain/src/lib.rs
📚 Learning: 2025-04-01T08:16:02.035Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4447
File: sync/src/tasks/block_sync_task.rs:608-620
Timestamp: 2025-04-01T08:16:02.035Z
Learning: In the `fetch_blocks_in_batch` method of `BlockSyncTask`, the filter logic is designed to include block headers where at least one parent exists in the local store but has not yet been integrated into the chain.
Applied to files:
sync/src/tasks/block_sync_task.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
miner/src/create_block_template/block_builder_service.rsexecutor/src/lib.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
chain/open-block/src/vm2.rschain/Cargo.toml
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
executor/src/block_executor.rschain/open-block/src/lib.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (11)
chain/src/lib.rs (1)
chain/src/txn_output_cache.rs (2)
global_block_state_cache(112-114)global_txn_output_cache(174-176)
sync/src/tasks/block_sync_task.rs (2)
genesis/src/lib.rs (1)
block(272-274)types/src/block/mod.rs (1)
block(1128-1130)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (6)
global_block_state_cache(112-114)global_txn_output_cache(174-176)new(23-31)new(49-61)new(70-74)new(125-129)
executor/src/lib.rs (2)
executor/src/block_executor.rs (4)
block_execute(65-147)block_execute_with_outputs(150-203)get_logger_balance_amount(31-33)set_logger_balance_amount_once(24-28)vm2/executor/src/block_executor.rs (2)
block_execute(36-99)block_execute_with_outputs(102-154)
vm2/chain/src/lib.rs (2)
executor/src/block_executor.rs (1)
block_execute_with_outputs(150-203)vm2/executor/src/block_executor.rs (1)
block_execute_with_outputs(102-154)
chain/open-block/src/vm2.rs (2)
vm2/vm-types/src/transaction/mod.rs (2)
txn_info(966-968)events(790-792)chain/src/chain.rs (5)
events(762-762)events(778-778)events(1592-1595)events(2623-2623)events(2633-2633)
chain/src/chain.rs (4)
chain/src/txn_output_cache.rs (4)
global_block_state_cache(112-114)global_txn_output_cache(174-176)len(91-93)len(152-154)executor/src/block_executor.rs (2)
block_execute_with_outputs(150-203)block_execute(65-147)chain/open-block/src/lib.rs (1)
new(92-167)vm2/chain/src/lib.rs (3)
execute_transactions_with_outputs(27-34)execute_transactions(13-25)get_epoch_from_statedb(62-65)
vm2/executor/src/lib.rs (2)
executor/src/block_executor.rs (2)
block_execute(65-147)block_execute_with_outputs(150-203)vm2/executor/src/block_executor.rs (2)
block_execute(36-99)block_execute_with_outputs(102-154)
executor/src/block_executor.rs (2)
vm2/vm-types/src/transaction/mod.rs (7)
write_set(786-788)events(790-792)gas_used(794-796)gas_used(902-904)status(641-647)status(798-800)status(906-908)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
chain/src/txn_output_cache.rs (4)
chain/src/chain.rs (5)
new(89-101)executed_data(742-752)executed_data(961-965)executed_data(2606-2616)executed_data2(2515-2519)chain/open-block/src/lib.rs (1)
new(92-167)executor/src/block_executor.rs (1)
default(45-53)vm2/executor/src/block_executor.rs (1)
default(26-33)
chain/open-block/src/lib.rs (1)
types/src/block/mod.rs (1)
block(1128-1130)
🔇 Additional comments (12)
chain/Cargo.toml (1)
4-4: LGTM! Dependency additions support the caching infrastructure.The additions of
dashmap(concurrent hash map for caching) andstarcoin-vm2-executor(VM2 execution with outputs) align with the PR's dual-VM caching mechanism.Also applies to: 38-38
vm2/executor/src/lib.rs (1)
8-8: LGTM! API expansion aligns with the caching architecture.Re-exporting
block_execute_with_outputsprovides the public API for executing blocks with pre-computed outputs, enabling the dual-VM caching mechanism described in the PR.txpool/src/test.rs (1)
282-286: LGTM! Test correctly refactored for FinalizedBlock structure.The test properly adapts to the new
FinalizedBlocktype returned byfinalize(), accessing state roots and block data through thetemplatefield. The functional behavior remains unchanged.executor/src/lib.rs (1)
8-9: LGTM! API expansion supports caching and mirrors VM2 interface.Re-exporting
block_execute_with_outputsandBlockExecutedDataprovides the public API for executing blocks with pre-computed outputs and accessing execution results, enabling the caching infrastructure.chain/src/lib.rs (1)
6-6: LGTM! Public API properly exposes the caching infrastructure.The module and type exports provide external access to the dual-VM caching mechanism. The global cache accessors follow a singleton pattern for coordinated access across the codebase.
Also applies to: 13-16
vm2/chain/src/lib.rs (1)
9-9: LGTM! VM2 execution path correctly implements cached execution.The new
execute_transactions_with_outputsfunction provides a parallel execution path that reuses pre-computed outputs, properly delegating to the block executor. The implementation is consistent with the existingexecute_transactionspattern.Also applies to: 27-34
sync/src/tasks/block_sync_task.rs (1)
355-357: The fork operation is already conditional and correctly positioned for state verification.The check
if self.chain.status().head().id() != block.parent_hash()ensures the fork only occurs when necessary—it doesn't fork during normal linear chain progression. The fork beforeapply_with_verifieris necessary because verification itself requires the chain state to be positioned at the block's parent state roots (consistent with the learning that state must be correctly initialized before block execution). Sinceverify_with_verifiertakes&mut self, it likely reads parent-related state during validation, so the fork cannot be delayed until after verification.chain/open-block/src/vm2.rs (1)
16-19: VM2 output caching and execution tracking look soundThe added VM2 fields (
cached_vm2_outputs,vm2_txn_infos,vm2_txn_events) and their population in:
initialize(BlockMetadata),push_txns2(kept user txns),finalize_block_epilogue(BlockEpilogue),- and
push_txn_and_state2(txn_info/events tracking)produce an output list and execution metadata that align with the transaction order produced by
build_block_transactions. That matches whatFinalizedBlockexpects.No correctness issues noticed here. The cloned outputs and txn_infos are extra allocations but necessary for the caching path; acceptable given their one-time use per block.
Also applies to: 46-48, 71-87, 126-157, 168-215
chain/open-block/src/lib.rs (3)
44-88: FinalizedBlock structure and OpenedBlock fields are aligned with caching designThe new
FinalizedBlockwrapper (template, vm1_outputs, vm2_outputs, statedb/statedb2, executed_data{1,2}) plus the extra fields onOpenedBlock(cached_vm1_outputs,cached_vm2_outputs, vm1/vm2 txn_infos, events, write_sets) provide exactly the data needed by the miner and chain caches.Construction in
OpenedBlock::newinitializes all these tracking fields and keeps previous behaviour (state, accumulators, gas tracking) unchanged. No obvious correctness issues.Also applies to: 137-162
232-276: VM1 output caching order matches VM1 execution orderIn
push_txnsandexecute_block_meta_vm1you push:
- BlockMetadata output once (in
execute_block_meta_vm1), and- each kept user transaction’s output (in
push_txns),into
cached_vm1_outputsin the same order as they are applied and as they appear in the block (BlockMetadatafollowed by user txns). This matches the transaction list used later byblock_execute_with_outputs, so the VM1 output cache should align cleanly with block execution.
331-376: VM1 execution tracking (infos/events/write_sets) is correct and consistent
push_txn_and_statenow:
- Stores each
write_setinvm1_write_sets,- Pushes the corresponding
TransactionInfoand events intovm1_txn_infosandvm1_txn_events,- Maintains the accumulator as before.
This is exactly what
FinalizedBlock::finalizeneeds to reconstructBlockExecutedData1for the cache path, and mirrors what the executor would have produced. Looks good.chain/src/txn_output_cache.rs (1)
15-62: CachedBlockOutputs / CachedBlockState types look appropriateThe cache value types (
CachedBlockOutputswrapping optional Arc<Vec<...>> andCachedBlockStatewrapping optional Arc<ChainStateDB{,2}> plus BlockExecutedData{1,2}) are structured sensibly:
Arcusage is appropriate to share heavy data across threads.Optionallows partial caches (outputs only, or state only) if needed.- Constructors wrap plain inputs cleanly.
No correctness issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
chain/src/chain.rs (2)
2319-2323: Verify statedb is positioned at selected parent's state before execution.The code uses
&self.statedb.0and&self.statedb.1directly without forking from the selected parent's state roots. Per the retrieved learning, the design expects statedb to be initialized/positioned at the selected parent's state roots BEFOREexecute_dag_blockis called.Ensure callers (e.g.,
executemethod at line 1392) guarantee theBlockChaininstance is correctly positioned at the parent's state before invokingexecute_dag_block. If the incoming block's parent differs from the current head, execution may run on the wrong base state.
2387-2400: Redundant write_set re-application in non-cached execution path.The executor functions (
block_execute,execute_transactions,block_execute_with_outputs) already apply and commit write_sets to the passed-in state DB. Lines 2389-2396 redundantly apply these same write_sets again. Per thevm2/executor/src/block_executor.rssnippet (lines 173-179),block_execute_with_outputscallschain_state.apply_write_set(write_set.clone())andchain_state.commit()internally.This double-application is unnecessary and potentially wasteful (extra commits). The cached path (lines 2382-2386) correctly only flushes.
🔎 Proposed fix: remove the manual loop and only flush
} else { - // Apply write_sets to self.statedb - for write_set in executed_data.write_sets.clone() { - statedb - .apply_write_set(write_set) - .map_err(BlockExecutorError::BlockChainStateErr)?; - statedb - .commit() - .map_err(BlockExecutorError::BlockChainStateErr)?; - } - // Flush self.statedb + // VM1/VM2 executors have already applied and committed write_sets. + // Only flush to persist state tree nodes. statedb.flush()?; statedb2.flush()?; }chain/open-block/src/lib.rs (1)
421-436: Emptytxn_table_infoswill cause table metadata loss for cached blocks.Both
BlockExecutedData1andBlockExecutedData2havetxn_table_infos: BTreeMap::new()(lines 426, 435) with TODO comments. When these executed data structs are used via the cache path inexecute_dag_block, the empty table_infos get persisted viastorage.save_table_infos(), losing table metadata that the non-cached executor would have captured.Track table_infos during transaction execution (from
TransactionOutput::into_inner()which returnstable_infosas first element), or treat the cache as incomplete and force re-execution when table_infos are missing.🔎 Suggested tracking in push_txn_and_state
fn push_txn_and_state( &mut self, txn_hash: HashValue, output: TransactionOutput, root_state_calc: bool, ) -> Result<(Option<HashValue>, HashValue)> { let (state, _state2) = &mut self.state; - let (_table_infos, write_set, events, gas_used, status) = output.into_inner(); + let (table_infos, write_set, events, gas_used, status) = output.into_inner(); + + // Track table_infos for later use + self.vm1_table_infos.extend(table_infos.into_iter().map(|(k, v)| (k, v))); + // ... rest of method }Add field to OpenedBlock:
vm1_table_infos: BTreeMap<TableHandle, TableInfo>,
🧹 Nitpick comments (2)
chain/src/chain.rs (2)
2172-2264: Return values for cached statedbs are alwaysNone; consider removing unused tuple elements.The function signature returns
Option<Arc<ChainStateDB>>andOption<Arc<ChainStateDB2>>(3rd and 4th tuple elements), but line 2263 always returns(None, None). The caller at lines 2381-2386 checks these values, but the condition will never be true. Either:
- The statedb caching via this function is incomplete, or
- The return signature and caller checks should be simplified
If caching is handled elsewhere (via
cached_statefromglobal_block_state_cache), consider simplifying this function to return only(BlockExecutedData, BlockExecutedData2).🔎 Suggested simplification if statedb caching is handled elsewhere
fn execute_block_transactions_with_cache( statedb: &ChainStateDB, statedb2: &ChainStateDB2, transactions: &[Transaction], transactions2: &[starcoin_vm2_types::transaction::Transaction], epoch: &Epoch, cached_outputs: &Option<CachedBlockOutputs>, block_id: HashValue, vm_metrics: Option<VMMetrics>, -) -> Result<( - BlockExecutedData, - BlockExecutedData2, - Option<Arc<ChainStateDB>>, - Option<Arc<ChainStateDB2>>, -)> { +) -> Result<(BlockExecutedData, BlockExecutedData2)> { // ... execution logic unchanged ... - Ok((executed_data, executed_data2, None, None)) + Ok((executed_data, executed_data2)) }
2553-2556: Double flush in non-cached execution path.In the non-cached path,
statedb.flush()andstatedb2.flush()are called at lines 2398-2399, then again at lines 2555-2556. While not incorrect, this is redundant I/O.Consider removing the earlier flush (lines 2398-2399) since the final flush here handles persistence.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
chain/open-block/src/lib.rschain/src/chain.rsminer/src/create_block_template/block_builder_service.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- miner/src/create_block_template/block_builder_service.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
chain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
chain/open-block/src/lib.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/chain.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
🧬 Code graph analysis (2)
chain/open-block/src/lib.rs (3)
vm2/vm-types/src/transaction/mod.rs (2)
write_set(786-788)events(790-792)vm/types/src/transaction/mod.rs (2)
write_set(683-685)events(691-693)types/src/multi_state.rs (2)
state_root1(35-37)state_root2(39-41)
chain/src/chain.rs (4)
chain/src/txn_output_cache.rs (4)
global_block_state_cache(112-114)global_txn_output_cache(174-176)len(91-93)len(152-154)executor/src/block_executor.rs (2)
block_execute_with_outputs(150-203)block_execute(65-147)vm2/executor/src/block_executor.rs (2)
block_execute_with_outputs(102-154)block_execute(36-99)vm2/chain/src/lib.rs (2)
execute_transactions_with_outputs(27-34)execute_transactions(13-25)
🔇 Additional comments (10)
chain/src/chain.rs (3)
7-7: LGTM on import additions.The new imports for
global_block_state_cacheandglobal_txn_output_cacheproperly support the cache integration.
453-455: LGTM on finalize rename.The rename from
templatetofinalizedand access via.templatealigns with the newFinalizedBlockreturn type.
2402-2405: LGTM on cache cleanup.Proper cleanup of output cache after use prevents unbounded cache growth.
chain/open-block/src/lib.rs (7)
10-41: LGTM on import additions.The new imports properly support the
FinalizedBlockstruct and execution data tracking.
44-55: LGTM onFinalizedBlockstructure.The struct properly aggregates all data needed for caching: block template, VM1/VM2 outputs, state databases, and executed data. Arc wrapping for statedbs enables shared ownership across cache and consumers.
78-87: LGTM on newOpenedBlockfields for execution data tracking.The fields properly track VM1 outputs/infos/events/write_sets and VM2 outputs/infos/events. The asymmetry (no
vm2_write_sets) correctly reflects thatBlockExecutedData2doesn't include write_sets.
265-265: LGTM on caching VM1 outputs.Correctly caches the output before it's consumed by
push_txn_and_state. Only kept transactions are cached, which is the correct behavior.
306-308: LGTM on caching BlockMetadata output.Correctly caches the BlockMetadata transaction output, ensuring the first transaction is included in cached outputs for replay.
346-372: LGTM on execution data tracking.Properly tracks write_sets (cloned before consumption), txn_infos, and events for later aggregation into
BlockExecutedDataduring finalization.
438-446: LGTM onFinalizedBlockconstruction.The struct properly aggregates the template, cached outputs, state databases, and executed data for downstream caching use.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chain/src/chain.rs (1)
2545-2548: Redundant flush calls detected.Lines 2547-2548 flush
statedbandstatedb2unconditionally, but these were already flushed at lines 2390-2391. This results in double flushing which is wasteful.If the intent is to ensure flush after verification passes, consider restructuring to avoid the redundant flush.
🧹 Nitpick comments (4)
chain/src/txn_output_cache.rs (2)
91-111: Eviction is non-deterministic but acceptable for this use case.The eviction strategy removes arbitrary entries since
DashMap::iter()has no guaranteed order. While not ideal, this is acceptable because:
- The cache is small (16 entries max)
- It's for block templates being mined, where temporal locality matters less
- The comment at line 93 documents this behavior
For production observability, consider adding a metric counter for eviction events.
233-291: LGTM - Tests cover core cache operations.The tests validate insert, get, remove, clear, and global cache access. Consider adding a test for the eviction behavior when exceeding
max_size.chain/src/chain.rs (2)
2169-2171: Consider placing the helper function closer to its usage or in a separate module.The helper function is defined after the
ChainWriterimpl block, which is unconventional. In Rust, it's more common to place helper functions either:
- Before the impl block that uses them, or
- In a separate private module
This is a minor organizational suggestion.
2182-2187: Return type includes unused statedb fields that are never populated.The
execute_block_transactions_with_cachefunction declaresOption<Arc<ChainStateDB>>andOption<Arc<ChainStateDB2>>in its return type but always returns(None, None)at line 2263. The callers at lines 2354 and 2367 always receiveNonefor these fields. The non-None values destructured intocached_statedbandcached_statedb2(line 2335) come exclusively from the full cache hit path (lines 2346-2351), not from the function. Either refactor the function to return cached statedbs when the full cache path is used, or simplify the return type.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
chain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
chain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
chain/open-block/src/lib.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (2)
chain/src/chain.rs (1)
chain/src/txn_output_cache.rs (8)
global_block_state_cache(144-146)global_txn_output_cache(229-231)len(123-125)len(207-209)new(30-38)new(56-68)new(78-80)new(158-160)
chain/open-block/src/lib.rs (3)
vm2/vm-types/src/transaction/mod.rs (7)
write_set(786-788)events(790-792)gas_used(794-796)gas_used(902-904)status(641-647)status(798-800)status(906-908)vm/vm-runtime/src/move_vm_ext/session.rs (1)
events(148-155)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
🔇 Additional comments (14)
chain/src/txn_output_cache.rs (4)
1-21: LGTM - Cache module setup with sensible defaults.The imports and constants are appropriate. The cache sizes of 16 entries are reasonable for a mining-focused cache where only a small number of block templates are typically in flight.
22-39: LGTM - CachedBlockOutputs correctly wraps outputs in Arc.The
new()constructor wraps the vectors inArcto enable cheap cloning and shared ownership across threads, which is appropriate for a concurrent cache.
41-69: LGTM - CachedBlockState captures all necessary execution artifacts.The struct correctly captures both VM1 and VM2 statedbs and their respective executed data, enabling full execution skip when cache hits.
113-121: Consider usinggetwithRef::clone()to avoid holding the read lock.The current implementation clones inside the closure which is correct, but an alternative pattern using
mapis slightly more idiomatic:pub fn get(&self, txn_accumulator_root: HashValue) -> Option<CachedBlockState> { self.cache.get(&txn_accumulator_root).map(|v| v.clone()) }This is already what you have - looks good!
chain/src/chain.rs (3)
453-455: LGTM - Clean adaptation to FinalizedBlock return type.The change correctly extracts the template from the new
FinalizedBlockstruct and maintains the existing return signature.
2335-2377: Cache lookup and execution flow is correct.The logic properly:
- Attempts to retrieve cached state first (removes from cache to take ownership)
- Falls back to output cache for partial caching
- Uses full execution when no cache is available
The info-level logging for cache hits is appropriate for debugging performance.
2394-2397: LGTM - Output cache cleanup after use.Properly removes the output cache entry after consumption to prevent stale data accumulation.
chain/open-block/src/lib.rs (7)
45-56: LGTM - FinalizedBlock captures all necessary caching artifacts.The struct properly includes:
- Block template for mining
- VM1/VM2 outputs for transaction output caching
- Arc-wrapped statedbs for state caching
- Executed data for both VMs to skip re-execution
This enables the full caching workflow introduced in this PR.
79-90: LGTM - Execution tracking fields added to OpenedBlock.The new fields properly track all execution artifacts needed to construct
BlockExecutedData1andBlockExecutedData2during finalization:
- VM1: txn_infos, events, write_sets, table_infos
- VM2: txn_infos, events (note correctly mentions VM2 doesn't track table_infos)
342-377: LGTM - Properly extracts and tracks execution data during push.This addresses the previous review concern about empty
txn_table_infos. The implementation:
- Extracts
table_infosfrom the output viainto_inner()(line 343)- Merges them into
vm1_table_infosusingappend()(line 350)- Tracks
write_setfor later use (line 353)- Accumulates
txn_infoandevents(lines 376-377)This ensures VM1 table_infos are properly preserved in
BlockExecutedData1.
444-452: LGTM - FinalizedBlock construction is complete.All tracked data is properly transferred to the
FinalizedBlock, enabling downstream caching in the miner and chain execution paths.
269-274: LGTM - VM1 outputs cached during transaction processing.The output is cloned and cached before being consumed by
push_txn_and_state, ensuring the data is available forFinalizedBlock.
309-312: LGTM - BlockMetadata output also cached.Correctly caches the block metadata transaction output alongside user transaction outputs.
435-442: Remove verification note—implementation is correct.The empty
txn_table_infosfield is intentionally designed. Downstream code inchain.rsproperly accesses and mergesexecuted_data2.txn_table_infoswith VM1 data via a chain operation, safely handling the empty map without issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
chain/src/txn_output_cache.rs (1)
82-99: Eviction strategy uses arbitrary ordering, not LRU or recency-based.
DashMap::iter()provides no ordering guarantees; entries are yielded in arbitrary hash-table order. Line 88's.take(max_size / 2)may evict recently inserted or frequently accessed entries instead of the oldest. In a DAG scenario with multiple competing blocks or delayed block application, this can evict useful cache entries before they're consumed.Consider:
- Tracking insertion timestamps and evicting oldest entries (LRU approximation)
- Using a dedicated LRU crate like
lrualongside DashMap for ordering- Documenting the arbitrary eviction behavior if acceptable for the use case
chain/src/chain.rs (1)
2172-2212: Document state mutation side effects in helper function.
execute_block_transactionscallsblock_executeandexecute_transactions, both of which apply and commit write_sets directly to the passedstatedbandstatedb2references as side effects. This is correct per the architecture (learnings confirm executors handle state commits), but the function signature and comment don't make the mutation explicit.🔎 Suggested documentation improvement
-/// Helper function to execute block transactions +/// Helper function to execute block transactions. +/// Side effect: Executors apply and commit write_sets to statedb and statedb2 in-place. +/// The returned BlockExecutedData state_root fields reflect the committed states. fn execute_block_transactions( statedb: &ChainStateDB, statedb2: &ChainStateDB2,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
chain/open-block/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rschain/src/lib.rschain/src/txn_output_cache.rsminer/src/create_block_template/block_builder_service.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- chain/src/lib.rs
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rsminer/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rsminer/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rsminer/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
chain/open-block/src/vm2.rschain/src/chain.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/open-block/src/lib.rschain/src/chain.rsminer/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
chain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
chain/open-block/src/lib.rsminer/src/create_block_template/block_builder_service.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (3)
chain/open-block/src/vm2.rs (2)
vm2/vm-types/src/transaction/mod.rs (2)
txn_info(966-968)events(790-792)chain/src/chain.rs (5)
events(762-762)events(778-778)events(1592-1595)events(2548-2548)events(2558-2558)
chain/src/chain.rs (5)
chain/src/txn_output_cache.rs (3)
global_block_state_cache(133-135)new(36-48)new(69-71)vm2/vm-runtime/src/starcoin_vm.rs (1)
execute_block_transactions(1129-1321)vm2/vm-types/src/on_chain_config/mod.rs (1)
epoch(132-134)executor/src/block_executor.rs (1)
block_execute(65-147)vm2/executor/src/block_executor.rs (1)
block_execute(36-99)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (3)
global_block_state_cache(133-135)new(36-48)new(69-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (9)
chain/src/txn_output_cache.rs (2)
50-56: LGTM: Defensive completeness check prevents partial cache usage.
is_complete()ensures all four fields are populated before a cache entry is used, preventing partial/incomplete data from being applied during execution. This guards against cache corruption or incomplete finalization.
131-135: LGTM: Global cache accessor is clean and thread-safe.The
Lazy<BlockStateCache>pattern withglobal_block_state_cache()accessor provides a clean singleton for process-wide caching.DashMapensures thread-safe concurrent access without explicit locking.chain/open-block/src/vm2.rs (1)
175-201: LGTM: VM2 execution data tracking aligns with caching flow.Lines 175-177 document the VM2 output structure (no write_sets field in
BlockExecutedData2), and lines 200-201 capturetxn_infoandeventsfor later use inFinalizedBlock.executed_data2. This mirrors the VM1 bookkeeping pattern and complements the new caching pathway introduced in this PR.miner/src/create_block_template/block_builder_service.rs (1)
611-633: LGTM: Finalized block caching integrated cleanly into mining flow.The refactor from
templatetofinalized(line 611) extracts theBlockTemplateat line 619 while preserving the existing callback API. Lines 625-632 cache the full finalized state (both VM1/VM2 statedb and executed_data) keyed bytxn_accumulator_root, enabling the executor to skip re-execution when this mined block arrives. Caching before the callback (line 636) ensures the entry is available when needed.chain/open-block/src/lib.rs (3)
44-55: LGTM: FinalizedBlock struct provides complete execution artifacts for caching.The struct captures all data required to skip re-execution: the
BlockTemplate, both VM statedbs (with write_sets already applied and committed but not flushed), andBlockExecutedDatafor VM1/VM2. This design enables the mining-to-execution fast path introduced in this PR.
333-344: LGTM: Table_infos and write_sets tracked for VM1 execution.Lines 333-341 extract and accumulate
table_infos(merging updates byTableHandle), and line 344 captures thewrite_set. This addresses the past review concern about missing table_infos in cached execution, ensuring VM1'sBlockExecutedDatais complete.
417-433: LGTM: BlockExecutedData construction is complete for both VMs.VM1's
executed_data(lines 418-424) includestxn_infos,txn_events,txn_table_infos, andwrite_sets. VM2'sexecuted_data2(lines 428-433) omitswrite_sets(not part ofBlockExecutedData2) and uses emptytxn_table_infosper the VM2 design (line 427 comment). This aligns with the dual-VM architecture and provides complete data for caching.chain/src/chain.rs (2)
2273-2315: LGTM: Cache retrieval and fallback execution logic is sound.Lines 2275-2276 retrieve and remove the cached entry by
txn_accumulator_root. Lines 2280-2291 use the cachedstatedb,statedb2, andexecuted_dataifis_complete()is true, avoiding re-execution. Lines 2293-2314 fall back to live execution if the cache is partial or missing. The tuple unpacking at line 2279 ensurescached_statedbandcached_statedb2areSomeonly when the fast path is taken, enabling correct flush logic downstream (lines 2319-2330).
2317-2330: LGTM: Flush logic correctly distinguishes cached vs non-cached paths.Lines 2319-2324 flush the cached statedbs when
cached_statedbandcached_statedb2areSome(only set when cache hit and complete, per lines 2289-2290). Lines 2325-2329 flushself.statedbotherwise (cache miss or partial, executors have already applied/committed write_sets per lines 2326-2327 comment). This addresses the past review concern about flush logic and is now correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
chain/src/chain.rs (1)
2267-2271: Fix misleading comment and add state positioning validationThe comment at line 2257 incorrectly states "Fork statedb from selected parent's state root" but the code at lines 2267-2268 does not fork—it directly references
self.statedb. According to the design intent from PR 4722, the BlockChain instance's state must be pre-positioned at the selected parent's state roots beforeexecute_dag_blockis called, with validation to fail if state is incorrectly positioned.The code currently:
- Uses statedb directly without verification it's at the parent's state root
- Has a misleading comment claiming a fork that doesn't occur
- Relies entirely on callers to ensure correct state positioning with no defensive validation
Either position state at the parent's state root before line 2267 with validation, or remove the misleading comment and document that callers must ensure correct state positioning.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chain/src/chain.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/src/chain.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/src/chain.rs
🧬 Code graph analysis (1)
chain/src/chain.rs (5)
chain/src/txn_output_cache.rs (1)
global_block_state_cache(133-135)vm2/vm-runtime/src/starcoin_vm.rs (1)
execute_block_transactions(1129-1321)executor/src/executor.rs (2)
execute_block_transactions(24-31)execute_transactions(14-20)executor/src/block_executor.rs (1)
block_execute(65-147)vm2/executor/src/block_executor.rs (1)
block_execute(36-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (2)
chain/src/chain.rs (2)
7-7: LGTM - Cache import addedThe import of
global_block_state_cacheis necessary for the new caching functionality inexecute_dag_block.
2169-2212: LGTM - Well-structured helper functionThe type alias and helper function are well-implemented:
- Type alias improves readability for dual-VM execution
- Helper correctly handles empty VM1 transactions with dummy BlockExecutedData
- Gas limit calculation ensures VM2 gets the remaining gas budget after VM1 execution
- Return tuple provides both VM1 and VM2 execution results
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
chain/src/txn_output_cache.rs (2)
82-98: Consider using an LRU or FIFO eviction policy instead of arbitrary iteration order.The current eviction logic uses
iter().take(max_size / 2)which depends onDashMap's internal iteration order—not insertion time or access time. This can unpredictably evict recently inserted or frequently accessed entries.For more deterministic behavior, consider one of:
- LRU: Track access time and evict least-recently-used entries
- FIFO: Track insertion order and evict oldest entries
- Time-based TTL: Evict entries older than a threshold
💡 Example: Insertion-order tracking with a VecDeque
Add an insertion order tracker:
pub struct BlockStateCache { cache: DashMap<HashValue, CachedBlockState>, + insertion_order: std::sync::Mutex<std::collections::VecDeque<HashValue>>, max_size: usize, }Update eviction to remove oldest entries:
pub fn insert(&self, txn_accumulator_root: HashValue, state: CachedBlockState) { // Evict entries if cache is full if self.cache.len() >= self.max_size { - let to_remove: Vec<_> = self - .cache - .iter() - .take(self.max_size / 2) - .map(|entry| *entry.key()) - .collect(); + let mut order = self.insertion_order.lock().unwrap(); + let to_remove: Vec<_> = order.drain(..self.max_size / 2).collect(); for key in to_remove { self.cache.remove(&key); } log::debug!( "BlockStateCache evicted entries, new size: {}", self.cache.len() ); } + let mut order = self.insertion_order.lock().unwrap(); + order.push_back(txn_accumulator_root); self.cache.insert(txn_accumulator_root, state); }Also update
remove()andclear()to maintain the insertion order.
13-15: Consider adding cache observability metrics.To better understand cache effectiveness and tune the size limit, consider adding metrics such as:
- Cache hit/miss rate
- Eviction count and rate
- Current cache size over time
This would help diagnose if the 16-entry limit is appropriate for the DAG mining workload, or if it needs adjustment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
chain/src/lib.rschain/src/txn_output_cache.rsnode/Cargo.tomlnode/src/node.rs
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
node/Cargo.toml
📚 Learning: 2025-09-14T01:27:57.535Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: rpc/server/src/module/debug_rpc.rs:87-90
Timestamp: 2025-09-14T01:27:57.535Z
Learning: For debug/dev commands in the Starcoin codebase, the preference is to keep the implementation simple without excessive validation or access control checks, focusing on functionality rather than restrictive safeguards.
Applied to files:
node/Cargo.toml
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
node/src/node.rschain/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
node/src/node.rschain/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-07-03T03:21:32.104Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: vm/types/src/block_metadata/mod.rs:47-48
Timestamp: 2025-07-03T03:21:32.104Z
Learning: In the starcoin repository, the BlockMetadata structure changes are part of a clean slate implementation with no legacy data that needs to be deserialized, so backward compatibility concerns for field type changes are not applicable.
Applied to files:
node/src/node.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
node/src/node.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
node/src/node.rschain/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
node/src/node.rschain/src/lib.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/lib.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/src/lib.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/src/lib.rs
📚 Learning: 2025-09-16T01:30:06.195Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4658
File: vm2/vm-runtime/src/parallel_executor/mod.rs:0-0
Timestamp: 2025-09-16T01:30:06.195Z
Learning: In the Starcoin codebase, the Display schema for AccessPath is stable, making string-based matching via format!("{}", path) a reliable approach for identifying specific resource paths like the STC transaction fee aggregator.
Applied to files:
chain/src/lib.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (2)
node/src/node.rs (1)
chain/src/txn_output_cache.rs (1)
clear_global_block_state_cache(140-143)
chain/src/lib.rs (1)
chain/src/txn_output_cache.rs (2)
clear_global_block_state_cache(140-143)global_block_state_cache(133-135)
🔇 Additional comments (10)
node/Cargo.toml (1)
15-15: LGTM!The addition of
starcoin-chaindependency is necessary to enable the global cache management functionality in the node shutdown flow.node/src/node.rs (2)
16-16: LGTM!Import added to support global cache cleanup during node shutdown.
447-449: Correct shutdown ordering: cache cleanup before registry shutdown.Clearing the global block state cache before registry shutdown ensures that database connections held by cached state objects are released cleanly, preventing lock file issues on restart.
chain/src/lib.rs (2)
6-6: LGTM!Module declaration exposes the transaction output cache as part of the chain crate's public API.
13-15: LGTM!Public re-exports enable downstream crates (like the node crate) to access cache management functions and types.
chain/src/txn_output_cache.rs (5)
1-15: LGTM!Module setup and imports are appropriate. The cache size limit of 16 entries is conservative, which is reasonable for the initial implementation.
17-57: LGTM!
CachedBlockStatestructure cleanly encapsulates both VM1 and VM2 state along with their execution data. Theis_complete()method provides a convenient validation check.
102-129: LGTM!The
get,remove,len,is_empty,clear, andDefaultimplementations are straightforward and correct.
131-143: LGTM!Global singleton with clear documentation about its purpose during shutdown. The logging in
clear_global_block_state_cache()aids debugging.
145-193: LGTM!Unit tests cover the core operations: insert, get, remove, and global cache access. These provide good coverage of the basic cache semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
chain/src/txn_output_cache.rs (1)
146-194: Consider adding an eviction test.The existing tests cover basic insert/get/remove operations, but there's no test that validates the eviction behavior when the cache exceeds
max_size. Adding a test that inserts more thanMAX_BLOCK_STATE_CACHE_SIZEentries and verifies eviction would increase confidence in the cache's behavior under load.Example eviction test
#[test] fn test_block_state_cache_eviction() { let cache = BlockStateCache::with_capacity(4); let mut roots = vec![]; // Insert 5 entries into a cache with capacity 4 for _ in 0..5 { let txn_root = HashValue::random(); roots.push(txn_root); let state = CachedBlockState::new(None, None, None, None); cache.insert(txn_root, state); } // Cache should have evicted some entries to stay near capacity assert!(cache.len() <= 4); // At least one entry should still be present assert!(!cache.is_empty()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chain/src/txn_output_cache.rsnode/src/node.rs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
node/src/node.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
node/src/node.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
node/src/node.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
node/src/node.rschain/src/txn_output_cache.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
node/src/node.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (2)
node/src/node.rs (1)
chain/src/txn_output_cache.rs (1)
clear_global_block_state_cache(137-144)
chain/src/txn_output_cache.rs (2)
executor/src/block_executor.rs (1)
default(45-53)vm2/executor/src/block_executor.rs (1)
default(26-33)
🔇 Additional comments (5)
node/src/node.rs (2)
16-16: LGTM: Cache cleanup import added.The import of
clear_global_block_state_cacheintegrates the cache cleanup mechanism into the node shutdown flow.
447-448: Cache clearing order is safe—services do not access the cache during shutdown.The global block state cache is cleared before
registry.shutdown_system_sync(), which is correct. Services only access the cache during normal block processing in the miner and chain modules. Thestopped()lifecycle handlers (called during shutdown) only unsubscribe from events and do not attempt cache lookups, so cache misses are not a practical concern here.chain/src/txn_output_cache.rs (3)
1-15: LGTM: Imports and cache size constant.The imports are appropriate for the caching implementation. A limit of 16 entries is reasonable for a mining block template cache.
131-144: LGTM: Global cache singleton and clear function.The global singleton uses
Lazyfor thread-safe initialization, andclear_global_block_state_cache()logs the entry count before clearing, which aids observability during shutdown.
50-56: The concern aboutis_complete()is not supported by the codebase. The system design requires both VMs to be executed and cached together. The only production construction ofCachedBlockState(inblock_builder_service.rs) always populates all four fields withSome()values. The strict check inis_complete()is appropriate and correctly prevents use of partial cache entries.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
chain/src/txn_output_cache.rs (1)
80-100: Doc comment is misleading; eviction is arbitrary, not age-based.The comment on line 81 says "older entries are evicted" but
DashMap::iter()provides no ordering guarantees. The iteration order is arbitrary, not based on insertion time. Consider updating the comment to accurately reflect the behavior:- /// Insert cached state using txn_accumulator_root as key. - /// If cache exceeds max_size, older entries are evicted. + /// Insert cached state using txn_accumulator_root as key. + /// If cache exceeds max_size, arbitrary entries are evicted (not LRU/FIFO).Note: The race condition and non-deterministic eviction issues were flagged in previous reviews. Given the small cache size (16 entries) and the specific use case (miner block templates), the impact is limited, but the doc comment should at least be accurate.
🧹 Nitpick comments (1)
chain/src/txn_output_cache.rs (1)
162-209: Tests cover basic operations; consider adding eviction test.The existing tests validate core CRUD functionality and correctly handle potential test interference via delta checks. Consider adding a test that validates eviction behavior when the cache exceeds
max_size:🔎 Suggested test for eviction
#[test] fn test_block_state_cache_eviction() { let cache = BlockStateCache::with_capacity(4); // Insert more than max_size entries for _ in 0..6 { let txn_root = HashValue::random(); let state = CachedBlockState::new(None, None, None, None); cache.insert(txn_root, state); } // Cache should have evicted entries and not exceed max_size + 1 assert!(cache.len() <= 4); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
chain/src/lib.rschain/src/txn_output_cache.rsminer/src/create_block_template/block_builder_service.rsnode/src/node.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- node/src/node.rs
- chain/src/lib.rs
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
miner/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
miner/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
miner/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-07-03T03:21:32.104Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: vm/types/src/block_metadata/mod.rs:47-48
Timestamp: 2025-07-03T03:21:32.104Z
Learning: In the starcoin repository, the BlockMetadata structure changes are part of a clean slate implementation with no legacy data that needs to be deserialized, so backward compatibility concerns for field type changes are not applicable.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
miner/src/create_block_template/block_builder_service.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (1)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (4)
global_block_state_cache(136-138)is_node_shutting_down(140-142)new(36-48)new(69-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (5)
miner/src/create_block_template/block_builder_service.rs (3)
8-11: LGTM!Imports correctly bring in the global cache, shutdown flag, and cached state type from the chain module.
541-543: Shutdown guards are well-placed.The shutdown checks at each major processing stage (block creation, VM1/VM2 transaction processing, epilogue, finalization, template notification) ensure graceful termination without leaving work half-done. This pattern prevents wasted computation during node shutdown.
Also applies to: 569-571, 592-594, 614-616, 628-630, 640-642
632-658: Finalization and caching logic is correct.The flow properly:
- Obtains the finalized result containing all execution outputs
- Extracts the template for the callback
- Caches the complete state (both StateDbs and executed data) before notifying
Using
txn_accumulator_rootas the cache key is appropriate since theblock_idisn't known until mining completes (it includes the nonce).chain/src/txn_output_cache.rs (2)
17-57: Well-designed cache entry structure.The documentation clearly explains the caching purpose. Using
Optionfor all fields allows partial states whileis_complete()provides a convenient validation check.
131-160: Global utilities are correctly implemented.The
Lazyinitialization ensures the cache is created once on first access. The shutdown flag usesSeqCstordering which is conservative but ensures visibility across all threads. Theclear_global_block_state_cache()correctly couples shutdown signaling with cache clearing.
jackzhhuang
force-pushed
the
cache-state-db
branch
from
9ba729b to
e5e82dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
chain/src/txn_output_cache.rs (2)
82-100: Race condition in eviction logic (known issue).As noted in past reviews, the size check (line 84) and insert (line 99) are not atomic. Concurrent inserts can all observe
len >= max_size, all evict entries, and all insert. This can cause:
- Cache temporarily exceeding
max_size- More entries evicted than necessary
Given the small cache size (16) and low contention (only miner inserts), this is low risk but worth documenting or addressing in future.
85-90: Eviction order is non-deterministic (known issue).
DashMap::iter()provides arbitrary iteration order. The eviction doesn't target oldest or least-useful entries. For this use case (small cache, mined blocks), arbitrary eviction is acceptable but not ideal.Consider tracking insertion order if LRU-like behavior becomes important.
🧹 Nitpick comments (5)
rpc/client/tests/client_server_test.rs (1)
79-80: Consider verifying the increased wait time is necessary.The 10-second wait (increased from 3 seconds) likely accommodates the new global state cache clearing, dual-VM execution shutdown, and Rayon thread pool coordination introduced in this PR. While this change is reasonable given the added cleanup complexity, it increases test runtime by 7 seconds.
Consider verifying that 10 seconds is the minimum necessary duration, or explore a more deterministic approach such as polling for shutdown completion with a timeout:
💡 Optional: More robust shutdown wait pattern
// Example pattern (if shutdown status API is available): let mut attempts = 0; while attempts < 100 && !is_fully_shutdown() { std::thread::sleep(Duration::from_millis(100)); attempts += 1; } if attempts >= 100 { warn!("Shutdown wait timed out after 10s"); }Note: This is only beneficial if there's a way to query shutdown completion status. If not available, the current fixed sleep is acceptable given the new shutdown complexity.
Based on learnings, using
std::thread::sleepfor intentional blocking delays in tests aligns with the preferred approach in this codebase.vm2/executor/src/block_executor.rs (1)
100-154: Function correctly implements cached execution path.The
block_execute_with_outputsfunction properly mirrors theblock_executelogic but uses pre-computed outputs, enabling state cache reuse. The control flow, error handling, and boundary commit logic (lines 130-138) are consistent with the existing function.Optional: Consider extracting shared logic to reduce duplication
Lines 100-154 largely duplicate the core processing loop from
block_execute(lines 55-95). You could extract the common logic into a helper function that both paths call:fn process_outputs<S: ChainStateReader + ChainStateWriter + Sync>( chain_state: &S, txns: &[Transaction], outputs: Vec<TransactionOutput>, ) -> ExecutorResult<BlockExecutedData> { let mut executed_data = BlockExecutedData::default(); let last_index = outputs .len() .checked_sub(1) .ok_or_else(|| BlockExecutorError::BlockTransactionZero)?; for (index, (txn, output)) in txns .iter() .take(outputs.len()) .zip(outputs.into_iter()) .enumerate() { // ... shared processing logic ... } executed_data.state_root = chain_state.state_root(); Ok(executed_data) } pub fn block_execute<S: ChainStateReader + ChainStateWriter + Sync>( chain_state: &S, txns: Vec<Transaction>, block_gas_limit: u64, vm_metrics: Option<VMMetrics>, ) -> ExecutorResult<BlockExecutedData> { let txn_outputs = do_execute_block_transactions( chain_state, txns.clone(), Some(block_gas_limit), vm_metrics, )?; process_outputs(chain_state, &txns, txn_outputs) } pub fn block_execute_with_outputs<S: ChainStateReader + ChainStateWriter + Sync>( chain_state: &S, txns: Vec<Transaction>, outputs: Vec<TransactionOutput>, ) -> ExecutorResult<BlockExecutedData> { process_outputs(chain_state, &txns, outputs) }This reduces maintenance burden and ensures both paths stay synchronized.
executor/src/block_executor.rs (2)
148-203: Function correctly implements cached execution path for VM1.The
block_execute_with_outputsfunction properly mirrors the VM1block_executelogic but uses pre-computed outputs. The VM1-specific handling ofwrite_sets(line 196) is correctly preserved.Optional: Extract shared logic to reduce duplication
Lines 148-203 largely duplicate the core processing loop from
block_execute(lines 84-143). Consider extracting the common processing logic into a helper function that both execution paths can call, similar to the suggestion for the VM2 version. This would reduce maintenance burden and ensure consistency.
167-167: Consider adding balance event logging for consistency.In
block_execute(lines 92-105), large STC balance events are logged for monitoring. Theblock_execute_with_outputsfunction skips this logging entirely.If balance event monitoring is important for observability, consider adding the same logging logic here:
let txn_hash = txn.id(); let (mut table_infos, write_set, events, gas_used, status) = output.into_inner(); + let balance_amount = get_logger_balance_amount(); + let _ = events.iter().any(|e| { + if let Ok(balance_event) = BalanceEvent::try_from(e) { + let res = balance_event.token_code() == &G_STC_TOKEN_CODE.clone() + && balance_event.amount() + > STCUnit::STC.value_of(balance_amount as u128).scaling(); + if res { + warn!("Logging Event: txn_hash {}, {}", txn_hash, balance_event); + } + res + } else { + false + } + }); match status {Alternatively, if cached execution shouldn't trigger monitoring alerts (to avoid duplicate logs), document this design decision with a comment.
chain/src/chain.rs (1)
2172-2212: Potential underflow in VM2 gas limit calculation.Line 2207 subtracts
vm1_gas_usedfromepoch.block_gas_limit(). If VM1 somehow used more gas than the limit (edge case), this could underflow. Consider usingsaturating_sub.🔎 Proposed fix
- epoch.block_gas_limit() - vm1_gas_used, + epoch.block_gas_limit().saturating_sub(vm1_gas_used),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockstarcoin.logis excluded by!**/*.log
📒 Files selected for processing (18)
benchmark_stats_20251208_125237.txtchain/Cargo.tomlchain/open-block/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rschain/src/lib.rschain/src/txn_output_cache.rsexecutor/src/block_executor.rsexecutor/src/lib.rsminer/src/create_block_template/block_builder_service.rsnode/Cargo.tomlnode/src/node.rsrpc/client/tests/client_server_test.rssync/src/tasks/block_sync_task.rstxpool/src/test.rsvm2/chain/src/lib.rsvm2/executor/src/block_executor.rsvm2/executor/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- benchmark_stats_20251208_125237.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- chain/src/lib.rs
- vm2/chain/src/lib.rs
- sync/src/tasks/block_sync_task.rs
- vm2/executor/src/lib.rs
- txpool/src/test.rs
- executor/src/lib.rs
- node/Cargo.toml
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/open-block/src/vm2.rsvm2/executor/src/block_executor.rschain/Cargo.tomlminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/src/chain.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/open-block/src/vm2.rsvm2/executor/src/block_executor.rsnode/src/node.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/src/chain.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/open-block/src/vm2.rsvm2/executor/src/block_executor.rsnode/src/node.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/src/chain.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/open-block/src/vm2.rsexecutor/src/block_executor.rschain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
chain/open-block/src/vm2.rschain/Cargo.tomlnode/src/node.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
chain/open-block/src/vm2.rschain/Cargo.tomlexecutor/src/block_executor.rschain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
node/src/node.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/src/chain.rschain/open-block/src/lib.rs
📚 Learning: 2025-07-03T03:21:32.104Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: vm/types/src/block_metadata/mod.rs:47-48
Timestamp: 2025-07-03T03:21:32.104Z
Learning: In the starcoin repository, the BlockMetadata structure changes are part of a clean slate implementation with no legacy data that needs to be deserialized, so backward compatibility concerns for field type changes are not applicable.
Applied to files:
node/src/node.rsminer/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
node/src/node.rsminer/src/create_block_template/block_builder_service.rschain/src/chain.rsrpc/client/tests/client_server_test.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
node/src/node.rsminer/src/create_block_template/block_builder_service.rschain/src/chain.rschain/src/txn_output_cache.rschain/open-block/src/lib.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
executor/src/block_executor.rschain/open-block/src/lib.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-28T08:35:37.355Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
Applied to files:
chain/src/chain.rs
🧬 Code graph analysis (8)
chain/open-block/src/vm2.rs (2)
vm2/vm-types/src/transaction/mod.rs (2)
txn_info(966-968)events(790-792)chain/src/chain.rs (5)
events(762-762)events(778-778)events(1592-1595)events(2541-2541)events(2551-2551)
vm2/executor/src/block_executor.rs (3)
vm2/vm-types/src/transaction/mod.rs (8)
default(705-707)write_set(786-788)events(790-792)gas_used(794-796)gas_used(902-904)status(641-647)status(798-800)status(906-908)vm/types/src/transaction/mod.rs (5)
write_set(683-685)events(691-693)gas_used(695-697)status(623-629)status(699-701)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
node/src/node.rs (1)
chain/src/txn_output_cache.rs (2)
clear_global_block_state_cache(152-160)reset_node_shutdown_flag(148-150)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (4)
global_block_state_cache(136-138)is_node_shutting_down(140-142)new(36-48)new(69-71)
executor/src/block_executor.rs (2)
vm2/vm-types/src/transaction/mod.rs (7)
write_set(786-788)events(790-792)gas_used(794-796)gas_used(902-904)status(641-647)status(798-800)status(906-908)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
chain/src/txn_output_cache.rs (2)
executor/src/block_executor.rs (1)
default(45-53)vm2/executor/src/block_executor.rs (1)
default(26-33)
chain/open-block/src/lib.rs (2)
chain/src/chain.rs (13)
execute_block_transactions(2172-2212)std(806-806)std(2578-2578)new(89-101)events(762-762)events(778-778)events(1592-1595)events(2541-2541)events(2551-2551)executed_data(742-752)executed_data(961-965)executed_data(2524-2534)executed_data2(2440-2444)chain/src/txn_output_cache.rs (2)
new(36-48)new(69-71)
rpc/client/tests/client_server_test.rs (1)
rpc/client/src/lib.rs (1)
sleep(776-779)
🔇 Additional comments (22)
chain/Cargo.toml (1)
4-4: LGTM! Dependency additions align with caching infrastructure.The addition of
dashmap(concurrent HashMap for state caching) andstarcoin-vm2-executor(VM2 execution data structures) are appropriate for the per-block state cache and dual-VM execution paths introduced in this PR.Also applies to: 38-38
node/src/node.rs (2)
16-16: LGTM! Proper initialization of shutdown flag on node launch.Calling
reset_node_shutdown_flag()at the start oflaunchensures the global shutdown flag is cleared tofalsewhen the node starts, which is essential for correct behavior after node restarts.Also applies to: 192-192
448-448: LGTM! Proper cache cleanup during shutdown.The
clear_global_block_state_cache()call at the start ofshutdown_systemensures:
- The shutdown flag is set (preventing new cache operations)
- The global block state cache is properly cleared before registry shutdown
This maintains correct cache lifecycle and prevents stale data on restart.
chain/open-block/src/vm2.rs (2)
175-177: Clear documentation of VM2 design difference.The comment appropriately clarifies that VM2's
BlockExecutedData2does not include awrite_setsfield, distinguishing it from VM1's design.
198-201: LGTM! Proper VM2 execution data tracking.The code correctly records VM2 transaction info and events by pushing them into
vm2_txn_infosandvm2_txn_events, enabling downstream finalization to aggregate both VM1 and VM2 execution data.executor/src/block_executor.rs (1)
160-164: This function is currently unused in the VM1 executor codebase.The code at lines 160-164 does use
.take(outputs.len()).zip()as described, which silently handles length mismatches by truncating to the smaller collection. However, the functionblock_execute_with_outputsinexecutor/src/block_executor.rshas no callers in the VM1 codebase—it is only exported fromexecutor/src/lib.rs. The only actual call site is invm2/chain/src/lib.rs, which calls the VM2 version of this function.If this function is intended for future use or should be removed, clarify in the implementation. If it will be used, add explicit validation to ensure length equality rather than relying on silent truncation:
if txns.len() != outputs.len() { return Err(BlockExecutorError::InvalidOutputCount { expected: txns.len(), actual: outputs.len() }); }chain/open-block/src/lib.rs (5)
44-55: LGTM! Well-structured FinalizedBlock for caching.The struct cleanly encapsulates all data needed for cache reuse: the block template, both VM statedbs (with write_sets already applied), and execution data for both VMs. This enables completely skipping re-execution when the mined block is received.
333-344: Good: Correct table_infos merging.Using
BTreeMap::appendcorrectly moves entries from the output's table_infos into the accumulatedvm1_table_infos. Later entries with the sameTableHandlewill overwrite earlier ones, which is the desired behavior.
435-441: LGTM! FinalizedBlock correctly constructed.The finalize method properly packages all necessary data for caching: template, Arc-wrapped statedbs, and both VM's executed data. The statedbs have write_sets applied but not flushed, ready for cache consumers to flush.
78-86: The VM2 execution data is already being populated correctly. Bothvm2_txn_infosandvm2_txn_eventsare tracked inpush_txn_and_state2, which is called from thepush_txns2transaction processing path invm2.rs. The VM1 and VM2 paths are symmetric in their data tracking approach.
417-433: VM2 intentionally does not track table_infos in its TransactionOutput.The code correctly sets
txn_table_infos: BTreeMap::new()for VM2 on line 432. Unlike VM1, VM2'sTransactionOutputstructure does not include atable_infosfield—this is a design difference between the two VMs. No additional action needed.miner/src/create_block_template/block_builder_service.rs (2)
540-543: Good: Shutdown guards enable graceful termination.The early return on shutdown prevents unnecessary work when the node is stopping. This pattern is correctly applied before each major processing step.
632-658: LGTM! Correct cache population order.The flow is correct: finalize → extract template → cache state → notify callback. The cache is populated with complete data (all fields
Some) which matches whatis_complete()checks in the consumer.chain/src/chain.rs (4)
453-455: LGTM! Clean integration with FinalizedBlock.The template extraction from FinalizedBlock is straightforward and the excluded transactions are correctly merged.
2267-2271: Verify statedb is positioned at correct parent state.Per learnings, the BlockChain's statedb must be initialized at the selected parent's state roots before
execute_dag_blockis called. The code referencesself.statedb.0andself.statedb.1directly. Confirm the caller ensures proper state positioning before invocation.Based on learnings, this design is intentional to avoid performance overhead of repeated state forking.
2279-2315: Cache hit/miss logic is correct.The code properly distinguishes:
- Complete cache hit → use cached executed_data and statedbs
- Partial cache → fall back to execution
- No cache → execute normally
The
is_complete()check ensures all four fields are present before using cached data.
2317-2330: Flush logic correctly handles both paths.Per previous discussion: when using cached statedbs from the miner, those need to be flushed. When not using cache, the executors (
block_execute/execute_transactions) have already applied and committed write_sets toself.statedb, so only flush is needed.This addresses the past review concern about the flush condition.
chain/src/txn_output_cache.rs (5)
1-15: LGTM! Clear module documentation and appropriate cache size.The cache size of 16 is reasonable for mined blocks. The comments clearly explain the purpose and design rationale.
23-57: LGTM! CachedBlockState properly encapsulates cached data.The
is_complete()method provides a clean check for full cache availability, enabling the consumer to distinguish complete hits from partial cache states.
131-138: LGTM! Global singleton properly initialized.Using
once_cell::Lazyensures thread-safe lazy initialization of the global cache.
140-160: LGTM! Shutdown helpers are well-designed.The
SeqCstordering ensures proper visibility across threads. Setting the shutdown flag before clearing the cache inclear_global_block_state_cacheensures consistent state during shutdown.
162-209: LGTM! Good test coverage for core cache operations.Tests cover insert/get, remove, and global cache interactions. Consider adding a test for eviction behavior when cache exceeds max_size.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
chain/src/txn_output_cache.rs (1)
84-92: Non-deterministic eviction order remains unresolved.As flagged in previous reviews,
DashMap::iter()provides no ordering guarantees (not LRU, FIFO, or insertion order). Usingtake(max_size / 2)evicts arbitrary entries, which may include recently inserted or frequently accessed cache entries. This can lead to suboptimal cache performance, especially during mining template generation bursts.Consider one of:
- Track insertion timestamps and evict oldest entries
- Use an LRU-aware structure (e.g.,
lrucrate with a DashMap wrapper)- Add access-order tracking for frequently used entries
- Document that eviction is best-effort/arbitrary if acceptable for this workload
🧹 Nitpick comments (5)
chain/src/txn_output_cache.rs (5)
47-53: Consider validating consistency between VM1 and VM2 data inis_complete().The method only checks field presence but doesn't verify that the VM1 and VM2 data are related or consistent (e.g., same transaction count, matching state roots). While the current implementation may be sufficient, consider adding assertions or validation to catch potential mismatches during cache insertion.
82-82: Handle potential mutex poisoning more gracefully.The
unwrap()oneviction_lock.lock()will panic if the mutex is poisoned (e.g., if a previous thread panicked while holding the lock). While unlikely, this could cause cascading failures during cache operations.Consider logging and either propagating the error or using
expect()with a descriptive message.🔎 Potential fix using expect with context
- let _guard = self.eviction_lock.lock().unwrap(); + let _guard = self.eviction_lock.lock() + .expect("BlockStateCache eviction_lock poisoned");
152-160: Mixing concerns: cache clearing and shutdown coordination.The function
clear_global_block_state_cache()both clears the cache and sets the global shutdown flag (line 153). This couples two separate concerns—cache lifecycle and node shutdown state—which may confuse callers who only want to clear the cache without signaling shutdown.Consider one of:
- Rename to
shutdown_and_clear_cache()to clarify dual purpose- Split into two functions:
clear_global_block_state_cache()(only clears) and a higher-level shutdown coordinator that calls both- Document the shutdown side effect prominently if the coupling is intentional
56-123: Consider adding observability metrics for cache performance.The cache lacks instrumentation for hit/miss rates, eviction frequency, and size trends. These metrics would be valuable for:
- Tuning
MAX_BLOCK_STATE_CACHE_SIZE- Detecting anomalies (e.g., unexpectedly high miss rates)
- Understanding mining template caching effectiveness
Consider adding:
- Cache hit/miss counters in
get()andremove()- Eviction counter in
insert()- Periodic logging or metrics export of cache statistics
162-210: Consider expanding test coverage for edge cases.Current tests cover basic insert/get/remove operations but miss:
- Eviction behavior when cache exceeds
MAX_BLOCK_STATE_CACHE_SIZEis_complete()method validation- Concurrent access patterns (multiple threads inserting/evicting)
- Cache behavior with complete vs incomplete
CachedBlockStateWhile not critical, these tests would increase confidence in the caching logic under stress.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chain/src/txn_output_cache.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
chain/src/txn_output_cache.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
chain/src/txn_output_cache.rs
🧬 Code graph analysis (1)
chain/src/txn_output_cache.rs (2)
executor/src/block_executor.rs (1)
default(45-53)vm2/executor/src/block_executor.rs (1)
default(26-33)
🔇 Additional comments (1)
chain/src/txn_output_cache.rs (1)
80-100: Race condition resolved with eviction lock.The double-checked locking pattern using
eviction_lockcorrectly addresses the race condition flagged in previous reviews. Multiple threads now synchronize on eviction, preventing the cache from exceedingmax_sizeduring concurrent inserts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm2/types/src/error/block_executor.rs (1)
36-37: LGTM! Clear error variant for output count validation.The new
InvalidOutputCountvariant is well-structured with appropriate field types and a descriptive error message that clearly communicates the mismatch.Optional: Consider enhancing message specificity
For even greater clarity, you could make explicit what
expectedandactualrepresent:- #[error("the number of transaction outputs is not equal to the number of transactions, expected length: {expected}, actual length: {actual}")] + #[error("the number of transaction outputs is not equal to the number of transactions, expected: {expected} transactions, actual: {actual} outputs")]However, the current message is already quite clear in context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vm2/executor/src/block_executor.rsvm2/types/src/error/block_executor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- vm2/executor/src/block_executor.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
jackzhhuang
force-pushed
the
cache-state-db
branch
from
6bb22e7 to
2e6df3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @chain/src/chain.rs:
- Around line 2267-2330: The cached-path flushes cached_db/cached_db2 but never
updates self.statedb, so the Chain's active state remains stale; after
confirming cached_statedb and cached_statedb2 are Some, assign the cached Arcs
back into self.statedb (e.g., self.statedb = (cached_db.clone(),
cached_db2.clone()) or set the tuple fields) so the chain uses the cached state
for subsequent operations; also avoid holding long-lived &self.statedb borrows
(the local statedb/statedb2 should be clones or obtained after possible
assignment) to satisfy Rust borrow rules.
🧹 Nitpick comments (5)
rpc/client/tests/client_server_test.rs (1)
80-80: Extended wait time for node shutdown and cache cleanup.The 7-second increase (3s → 10s) accommodates the new per-block state cache clearing and dual-VM execution lifecycle introduced in this PR. The test has a 120-second timeout, so this is acceptable.
Optional: Consider verifying the minimum required wait time
If 10 seconds proves consistently sufficient, consider whether a shorter duration (e.g., 5-7 seconds) might work with the new cache management, or document why 10 seconds is necessary. This could be validated with repeated test runs.
vm2/chain/src/lib.rs (1)
27-34: LGTM! Clean delegation pattern.The new
execute_transactions_with_outputsfunction properly delegates to the executor layer and maintains separation of concerns. Error handling is appropriately deferred toblock_execute_with_outputs.📝 Optional: Consider adding a doc comment
Adding a brief doc comment would clarify when to use this function versus
execute_transactions:+/// Execute transactions with pre-computed outputs from cache. +/// Use this when outputs are already available to skip re-execution. pub fn execute_transactions_with_outputs( statedb: &ChainStateDB, transactions: Vec<Transaction>, outputs: Vec<TransactionOutput>, ) -> ExecutorResult<BlockExecutedData> {vm2/executor/src/block_executor.rs (1)
118-160: Implementation correctly handles pre-computed outputs.The function properly:
- Applies write sets to chain state
- Commits at first and last transaction indices (per established pattern)
- Creates
TransactionInfoentries with appropriate state roots- Handles all transaction status cases (Keep/Discard/Retry)
- Updates final state root
♻️ Consider extracting shared logic
Lines 118-157 duplicate significant logic from
block_execute(lines 55-95). Consider extracting the shared transaction processing loop into a helper function to reduce duplication and improve maintainability:fn process_transaction_output<S: ChainStateReader + ChainStateWriter>( chain_state: &S, index: usize, last_index: usize, txn_hash: HashValue, output: TransactionOutput, executed_data: &mut BlockExecutedData, ) -> ExecutorResult<()> { let (write_set, events, gas_used, status, _) = output.into_inner(); match status { TransactionStatus::Discard(status) => { Err(BlockExecutorError::BlockTransactionDiscard(status, txn_hash)) } TransactionStatus::Keep(status) => { chain_state .apply_write_set(write_set) .map_err(BlockExecutorError::BlockChainStateErr)?; let txn_state_root = if index == last_index || index == 0 { Some( chain_state .commit() .map_err(BlockExecutorError::BlockChainStateErr)?, ) } else { None }; let t = TransactionInfo::new(txn_hash, txn_state_root, events.as_slice(), gas_used, status); executed_data.txn_infos.push(t); executed_data.txn_events.push(events); Ok(()) } TransactionStatus::Retry => Err(BlockExecutorError::BlockExecuteRetryErr), } }Both
block_executeandblock_execute_with_outputscould then call this helper in their respective loops.chain/src/txn_output_cache.rs (2)
79-100: Consider eviction policy for production use.The eviction logic uses double-checked locking correctly to prevent races. However, the eviction policy (removing arbitrary first half of entries via
take()) is non-deterministic and doesn't prioritize recently/frequently used entries.For the current mining use case with a small cache (16 entries), this simple approach is acceptable. However, if the cache grows or usage patterns change, consider implementing LRU or LFU eviction.
162-210: Tests cover basic functionality; consider adding concurrency tests.The tests validate core cache operations. For a more robust test suite, consider adding:
- Concurrent access tests (multiple threads inserting/reading)
- Eviction behavior tests (filling cache beyond max_size)
- Cache hit/miss scenarios
For the initial implementation, the current tests are sufficient.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockstarcoin.logis excluded by!**/*.log
📒 Files selected for processing (19)
benchmark_stats_20251208_125237.txtchain/Cargo.tomlchain/open-block/src/lib.rschain/open-block/src/vm2.rschain/src/chain.rschain/src/lib.rschain/src/txn_output_cache.rsexecutor/src/block_executor.rsexecutor/src/lib.rsminer/src/create_block_template/block_builder_service.rsnode/Cargo.tomlnode/src/node.rsrpc/client/tests/client_server_test.rssync/src/tasks/block_sync_task.rstxpool/src/test.rsvm2/chain/src/lib.rsvm2/executor/src/block_executor.rsvm2/executor/src/lib.rsvm2/types/src/error/block_executor.rs
✅ Files skipped from review due to trivial changes (1)
- benchmark_stats_20251208_125237.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- chain/src/lib.rs
- node/src/node.rs
- vm2/types/src/error/block_executor.rs
- node/Cargo.toml
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
vm2/chain/src/lib.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/open-block/src/vm2.rstxpool/src/test.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
vm2/chain/src/lib.rsvm2/executor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rsexecutor/src/lib.rsexecutor/src/block_executor.rschain/open-block/src/vm2.rstxpool/src/test.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
vm2/chain/src/lib.rsvm2/executor/src/block_executor.rschain/Cargo.tomlminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/open-block/src/vm2.rstxpool/src/test.rschain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
vm2/chain/src/lib.rsexecutor/src/block_executor.rschain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
vm2/chain/src/lib.rschain/Cargo.tomlminer/src/create_block_template/block_builder_service.rsexecutor/src/block_executor.rschain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
vm2/chain/src/lib.rsminer/src/create_block_template/block_builder_service.rstxpool/src/test.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rschain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
vm2/executor/src/block_executor.rsexecutor/src/block_executor.rschain/open-block/src/lib.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
chain/Cargo.tomlminer/src/create_block_template/block_builder_service.rschain/open-block/src/vm2.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
miner/src/create_block_template/block_builder_service.rsexecutor/src/lib.rsexecutor/src/block_executor.rstxpool/src/test.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rschain/src/chain.rs
📚 Learning: 2025-07-03T03:21:32.104Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: vm/types/src/block_metadata/mod.rs:47-48
Timestamp: 2025-07-03T03:21:32.104Z
Learning: In the starcoin repository, the BlockMetadata structure changes are part of a clean slate implementation with no legacy data that needs to be deserialized, so backward compatibility concerns for field type changes are not applicable.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
miner/src/create_block_template/block_builder_service.rsexecutor/src/lib.rsrpc/client/tests/client_server_test.rschain/src/chain.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
miner/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-04-01T08:16:02.035Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4447
File: sync/src/tasks/block_sync_task.rs:608-620
Timestamp: 2025-04-01T08:16:02.035Z
Learning: In the `fetch_blocks_in_batch` method of `BlockSyncTask`, the filter logic is designed to include block headers where at least one parent exists in the local store but has not yet been integrated into the chain.
Applied to files:
sync/src/tasks/block_sync_task.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
chain/src/chain.rs
📚 Learning: 2025-11-26T05:13:52.832Z
Learnt from: simonjiao
Repo: starcoinorg/starcoin PR: 4762
File: storage/src/db_storage/mod.rs:138-168
Timestamp: 2025-11-26T05:13:52.832Z
Learning: In rust-rocksdb, the Cache type is a thin FFI wrapper around RocksDB's C++ cache. The C++ side manages lifetime through reference counting. When Cache is passed to BlockBasedOptions::set_block_cache, the C++ RocksDB increments its internal reference count, so the cache remains alive even after the Rust Cache wrapper is dropped. Therefore, creating a Cache locally in a function and passing it to set_block_cache is safe—the local Rust wrapper can be dropped without causing use-after-free.
Applied to files:
chain/src/chain.rschain/src/txn_output_cache.rs
📚 Learning: 2025-09-28T08:35:37.355Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
Applied to files:
chain/src/chain.rs
🧬 Code graph analysis (10)
vm2/chain/src/lib.rs (4)
vm/e2e-tests/src/account.rs (1)
transaction(161-163)chain/src/chain.rs (4)
transactions(991-994)executed_data(742-752)executed_data(961-965)executed_data(2524-2534)vm2/executor/src/block_executor.rs (1)
block_execute_with_outputs(102-160)executor/src/block_executor.rs (1)
block_execute_with_outputs(150-203)
vm2/executor/src/lib.rs (2)
vm2/executor/src/block_executor.rs (2)
block_execute(36-99)block_execute_with_outputs(102-160)executor/src/block_executor.rs (2)
block_execute(65-147)block_execute_with_outputs(150-203)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (4)
global_block_state_cache(136-138)is_node_shutting_down(140-142)new(33-45)new(67-69)
executor/src/lib.rs (1)
executor/src/block_executor.rs (2)
get_logger_balance_amount(31-33)set_logger_balance_amount_once(24-28)
executor/src/block_executor.rs (3)
vm2/executor/src/block_executor.rs (1)
block_execute_with_outputs(102-160)vm2/vm-types/src/transaction/mod.rs (7)
write_set(786-788)events(790-792)gas_used(794-796)gas_used(902-904)status(641-647)status(798-800)status(906-908)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
chain/open-block/src/vm2.rs (2)
vm2/vm-types/src/transaction/mod.rs (2)
txn_info(966-968)events(790-792)chain/src/chain.rs (5)
events(762-762)events(778-778)events(1592-1595)events(2541-2541)events(2551-2551)
chain/open-block/src/lib.rs (3)
chain/src/chain.rs (11)
execute_block_transactions(2172-2212)new(89-101)events(762-762)events(778-778)events(1592-1595)events(2541-2541)events(2551-2551)executed_data(742-752)executed_data(961-965)executed_data(2524-2534)executed_data2(2440-2444)vm2/chain/src/lib.rs (1)
execute_transactions(13-25)chain/src/txn_output_cache.rs (2)
new(33-45)new(67-69)
rpc/client/tests/client_server_test.rs (1)
rpc/client/src/lib.rs (1)
sleep(776-779)
chain/src/chain.rs (8)
vm2/vm-runtime/src/starcoin_vm.rs (1)
execute_block_transactions(1129-1321)executor/src/executor.rs (2)
execute_block_transactions(24-31)execute_transactions(14-20)rpc/api/src/types.rs (1)
transactions(964-972)vm2/vm-types/src/on_chain_config/mod.rs (1)
epoch(132-134)vm2/vm-types/src/on_chain_resource/epoch.rs (1)
epoch(163-165)vm2/executor/src/block_executor.rs (1)
block_execute(36-99)executor/src/block_executor.rs (1)
block_execute(65-147)vm2/vm-types/src/transaction/mod.rs (1)
block_id(962-964)
chain/src/txn_output_cache.rs (3)
chain/open-block/src/lib.rs (1)
new(91-165)vm2/executor/src/block_executor.rs (1)
default(26-33)executor/src/block_executor.rs (1)
default(45-53)
🔇 Additional comments (28)
txpool/src/test.rs (1)
282-286: LGTM! Correctly adapts to the new FinalizedBlock API.The test correctly updates to the new finalization flow where
finalize()returns aFinalizedBlockcontaining atemplatefield. The changes properly access state roots and block construction throughfinalized.template, aligning with the dual-VM execution architecture introduced in this PR.chain/Cargo.toml (2)
4-4: LGTM! DashMap enables concurrent cache access.Adding
dashmapas a workspace dependency is appropriate for implementing the global per-block state cache, as it provides thread-safe concurrent HashMap operations needed for cache lookups across multiple execution contexts.
38-38: LGTM! VM2 executor integration for dual-VM execution.Adding
starcoin-vm2-executorenables the dual-VM execution flow with pre-computed outputs, which is central to this PR's caching mechanism.executor/src/block_executor.rs (1)
13-13: LGTM! TransactionOutput import enables cached execution.The import of
TransactionOutputis necessary for the newblock_execute_with_outputsfunction to consume pre-computed outputs.sync/src/tasks/block_sync_task.rs (1)
355-357: No issues found with fork error handling or design pattern.The pre-apply fork in
apply_blockis correctly implemented. The?operator ensures that iffork()fails, the assignment toself.chainnever executes, so no inconsistent state is possible—the original chain reference remains unchanged on error. Additionally,fork()creates a newBlockChaininstance rather than mutating the caller, so the pattern is sound.The pre-apply fork correctly positions the state at the parent before execution, which aligns with the cache-aware execution model. Forking occurs only on parent mismatch, not on every block—this is expected behavior for DAG sync with non-linear block ordering, not a performance problem.
Likely an incorrect or invalid review comment.
vm2/executor/src/lib.rs (1)
8-8: LGTM! Clean API expansion.The re-export of
block_execute_with_outputscorrectly extends the public API to support execution with pre-computed outputs, aligning with the new caching strategy.executor/src/lib.rs (1)
7-9: LGTM! Consistent API extension.The addition of
block_execute_with_outputsto the public API maintains consistency with the VM2 executor path and supports the new outputs-aware execution flow.vm2/chain/src/lib.rs (1)
9-9: Import addition is appropriate.Adding
TransactionOutputto support the new outputs-aware execution path.chain/open-block/src/vm2.rs (2)
175-177: Clarifying comment is helpful.The comment appropriately documents the intentional difference between VM2 and VM1 execution paths regarding write_set tracking.
198-201: LGTM! Proper VM2 execution bookkeeping.Recording VM2 transaction info and events enables dual-VM execution tracking. The placement after
TransactionInfo2creation is logical and consistent with the execution flow.vm2/executor/src/block_executor.rs (2)
12-12: Import addition is correct.Adding
TransactionOutputis necessary to support the new outputs-aware execution path.
101-117: LGTM! Proper validation of inputs.The function signature is clear, and the length validation at lines 112-116 correctly prevents index mismatches between transactions and outputs. The
InvalidOutputCounterror provides actionable information.chain/open-block/src/lib.rs (5)
44-55: LGTM: Well-designed FinalizedBlock structure.The struct correctly bundles all necessary data for caching: template, state databases, and VM execution results. The documentation clearly explains the optimization goal.
78-86: LGTM: Appropriate tracking fields for dual-VM execution.The tracking fields correctly capture VM1 and VM2 execution data. The comment on line 86 helpfully documents that VM2 uses TransactionAuxiliaryData instead of table_infos, explaining the asymmetry.
154-159: LGTM: Proper initialization of tracking fields.All tracking fields are correctly initialized to empty collections.
333-368: LGTM: Correct accumulation of VM1 execution data.The code properly extracts and tracks table_infos, write_sets, txn_infos, and events from transaction outputs. The use of
BTreeMap::appendon line 341 correctly merges table_infos, keeping the latest value for duplicate keys.
380-441: All callers offinalize()have been properly updated to handleFinalizedBlock.The return type change from
BlockTemplatetoFinalizedBlockis correctly handled across all callers. The implementation bundles the template alongside VM execution data (statedb, statedb2, executed_data, executed_data2), and all three call sites properly extract the needed fields from the wrapper struct.miner/src/create_block_template/block_builder_service.rs (4)
8-11: LGTM: Appropriate imports for caching functionality.The imports correctly bring in the cache infrastructure and shutdown control from the chain module.
541-543: LGTM: Appropriate shutdown checks for graceful termination.The shutdown checks at key stages of block building ensure the system can terminate cleanly without wasting resources on work that will be discarded. The placement balances responsiveness with avoiding excessive checking overhead.
Also applies to: 569-571, 592-594, 614-616, 628-630, 640-642
632-644: LGTM: Correct handling of FinalizedBlock.The code properly adapts to the new
finalize()return type, extracting the template for use while retaining the full finalized data for caching.
646-658: The caching logic is correct; no uniqueness concerns apply.Using
txn_accumulator_rootas the cache key is intentional and properly designed. As documented inchain/src/txn_output_cache.rs, this value uniquely identifies a block's transaction list (not the block itself). Since transaction execution is deterministic, blocks with identical transaction lists will always produce identical execution results, making this a suitable and efficient cache key. No collision risk exists.chain/src/chain.rs (3)
7-7: LGTM: Clean imports and type aliasing.The import and type alias improve code clarity for dual-VM execution.
Also applies to: 2169-2169
2171-2212: LGTM: Well-structured dual-VM execution helper.The helper function correctly sequences VM1 and VM2 execution, properly managing the gas budget between them. The default case for empty VM1 transactions is handled appropriately.
2273-2315: LGTM: Correct cache lookup with appropriate fallbacks.The cache integration properly handles all cases:
- Complete cache hit: reuses cached state and execution data
- Partial/missing cache: falls back to normal execution
The use of
remove()ensures each cached entry is consumed only once, preventing stale data reuse.chain/src/txn_output_cache.rs (4)
14-54: LGTM: Well-designed CachedBlockState structure.The struct appropriately uses
Optionto represent potentially partial cache entries, andis_complete()provides a clear validation method. The documentation explains the optimization goal clearly.
56-77: LGTM: Appropriate concurrent data structure.Using
DashMapfor the cache enables lock-free concurrent reads while maintaining thread safety. Theeviction_lockcorrectly coordinates eviction to prevent races.
102-110: LGTM: Correct get and remove semantics.The methods properly handle DashMap's guard-based access. Cloning in
get()is necessary and cheap due toArc-wrapped data.
131-160: LGTM: Proper global state management.The global cache and shutdown flag are correctly implemented with appropriate synchronization primitives. The
clear_global_block_state_cachefunction correctly sets the shutdown flag before clearing, ensuring no new entries are added during cleanup.
| let statedb = &self.statedb.0; | ||
| let statedb2 = &self.statedb.1; | ||
|
|
||
| // Get epoch from forked statedb (read from VM2's statedb) | ||
| let epoch = get_epoch_from_statedb(&statedb2)?; | ||
|
|
||
| // Execute VM1 transactions | ||
| let executed_data = if !transactions.is_empty() { | ||
| starcoin_executor::block_execute( | ||
| &statedb, | ||
| transactions.clone(), | ||
| epoch.block_gas_limit(), | ||
| self.vm_metrics.clone(), | ||
| )? | ||
| } else { | ||
| BlockExecutedData { | ||
| state_root: statedb.state_root(), | ||
| txn_infos: vec![], | ||
| txn_events: vec![], | ||
| txn_table_infos: BTreeMap::new(), | ||
| write_sets: vec![], | ||
| } | ||
| }; | ||
| let epoch = get_epoch_from_statedb(statedb2)?; | ||
|
|
||
| // Check for cached state (StateDB + BlockExecutedData) | ||
| // If cache hit, we can skip execution and apply_write_set entirely | ||
| let state_cache = global_block_state_cache(); | ||
| let cached_state = state_cache.remove(header.txn_accumulator_root()); | ||
|
|
||
| // Execute or use cached data | ||
| let (executed_data, executed_data2, cached_statedb, cached_statedb2) = | ||
| if let Some(cached) = cached_state { | ||
| if cached.is_complete() { | ||
| info!( | ||
| "Using cached StateDB and executed data for block {:?}", | ||
| block_id | ||
| ); | ||
| ( | ||
| cached.executed_data.unwrap(), | ||
| cached.executed_data2.unwrap(), | ||
| cached.statedb, | ||
| cached.statedb2, | ||
| ) | ||
| } else { | ||
| // Partial cache - fall back to execution | ||
| let (data, data2) = execute_block_transactions( | ||
| statedb, | ||
| statedb2, | ||
| &transactions, | ||
| &transactions2, | ||
| &epoch, | ||
| self.vm_metrics.clone(), | ||
| )?; | ||
| (data, data2, None, None) | ||
| } | ||
| } else { | ||
| // No cache - execute normally | ||
| let (data, data2) = execute_block_transactions( | ||
| statedb, | ||
| statedb2, | ||
| &transactions, | ||
| &transactions2, | ||
| &epoch, | ||
| self.vm_metrics.clone(), | ||
| )?; | ||
| (data, data2, None, None) | ||
| }; | ||
|
|
||
| // Apply write sets for VM1 | ||
| for write_set in executed_data.write_sets { | ||
| statedb | ||
| .apply_write_set(write_set) | ||
| .map_err(BlockExecutorError::BlockChainStateErr)?; | ||
| statedb | ||
| .commit() | ||
| .map_err(BlockExecutorError::BlockChainStateErr)?; | ||
| // If we used cached statedb, we need to flush the cached ones | ||
| // Otherwise, we only need to flush self.statedb (executor already applied write_sets) | ||
| if cached_statedb.is_some() && cached_statedb2.is_some() { | ||
| // Flush cached statedbs | ||
| let cached_db = cached_statedb.as_ref().unwrap(); | ||
| let cached_db2 = cached_statedb2.as_ref().unwrap(); | ||
| cached_db.flush()?; | ||
| cached_db2.flush()?; | ||
| } else { | ||
| // Note: write_sets are already applied and committed by executor functions | ||
| // (block_execute / execute_transactions), we only need to flush here | ||
| statedb.flush()?; | ||
| statedb2.flush()?; | ||
| } |
There was a problem hiding this comment.
Critical: Self.statedb not updated when using cached state.
There's a state management issue in the cache path:
- Lines 2267-2268 borrow references to
self.statedb.0andself.statedb.1 - When cache is used (lines 2319-2324), the code flushes
cached_dbandcached_db2 - However,
self.statedb.0andself.statedb.1are never updated with the cached state
This means after executing a cached block, the BlockChain instance's state databases remain at the parent's state, not advancing to the new block's state. This will cause incorrect state for subsequent operations.
Suggested fix: After using cached statedbs, update self.statedb:
if cached_statedb.is_some() && cached_statedb2.is_some() {
// Flush cached statedbs
let cached_db = cached_statedb.as_ref().unwrap();
let cached_db2 = cached_statedb2.as_ref().unwrap();
cached_db.flush()?;
cached_db2.flush()?;
+ // Update self.statedb to the cached state
+ self.statedb = (cached_db.clone(), cached_db2.clone());
} else {Note: Cloning Arc<ChainStateDB> is cheap since it's just incrementing a reference count.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let statedb = &self.statedb.0; | |
| let statedb2 = &self.statedb.1; | |
| // Get epoch from forked statedb (read from VM2's statedb) | |
| let epoch = get_epoch_from_statedb(&statedb2)?; | |
| // Execute VM1 transactions | |
| let executed_data = if !transactions.is_empty() { | |
| starcoin_executor::block_execute( | |
| &statedb, | |
| transactions.clone(), | |
| epoch.block_gas_limit(), | |
| self.vm_metrics.clone(), | |
| )? | |
| } else { | |
| BlockExecutedData { | |
| state_root: statedb.state_root(), | |
| txn_infos: vec![], | |
| txn_events: vec![], | |
| txn_table_infos: BTreeMap::new(), | |
| write_sets: vec![], | |
| } | |
| }; | |
| let epoch = get_epoch_from_statedb(statedb2)?; | |
| // Check for cached state (StateDB + BlockExecutedData) | |
| // If cache hit, we can skip execution and apply_write_set entirely | |
| let state_cache = global_block_state_cache(); | |
| let cached_state = state_cache.remove(header.txn_accumulator_root()); | |
| // Execute or use cached data | |
| let (executed_data, executed_data2, cached_statedb, cached_statedb2) = | |
| if let Some(cached) = cached_state { | |
| if cached.is_complete() { | |
| info!( | |
| "Using cached StateDB and executed data for block {:?}", | |
| block_id | |
| ); | |
| ( | |
| cached.executed_data.unwrap(), | |
| cached.executed_data2.unwrap(), | |
| cached.statedb, | |
| cached.statedb2, | |
| ) | |
| } else { | |
| // Partial cache - fall back to execution | |
| let (data, data2) = execute_block_transactions( | |
| statedb, | |
| statedb2, | |
| &transactions, | |
| &transactions2, | |
| &epoch, | |
| self.vm_metrics.clone(), | |
| )?; | |
| (data, data2, None, None) | |
| } | |
| } else { | |
| // No cache - execute normally | |
| let (data, data2) = execute_block_transactions( | |
| statedb, | |
| statedb2, | |
| &transactions, | |
| &transactions2, | |
| &epoch, | |
| self.vm_metrics.clone(), | |
| )?; | |
| (data, data2, None, None) | |
| }; | |
| // Apply write sets for VM1 | |
| for write_set in executed_data.write_sets { | |
| statedb | |
| .apply_write_set(write_set) | |
| .map_err(BlockExecutorError::BlockChainStateErr)?; | |
| statedb | |
| .commit() | |
| .map_err(BlockExecutorError::BlockChainStateErr)?; | |
| // If we used cached statedb, we need to flush the cached ones | |
| // Otherwise, we only need to flush self.statedb (executor already applied write_sets) | |
| if cached_statedb.is_some() && cached_statedb2.is_some() { | |
| // Flush cached statedbs | |
| let cached_db = cached_statedb.as_ref().unwrap(); | |
| let cached_db2 = cached_statedb2.as_ref().unwrap(); | |
| cached_db.flush()?; | |
| cached_db2.flush()?; | |
| } else { | |
| // Note: write_sets are already applied and committed by executor functions | |
| // (block_execute / execute_transactions), we only need to flush here | |
| statedb.flush()?; | |
| statedb2.flush()?; | |
| } | |
| let statedb = &self.statedb.0; | |
| let statedb2 = &self.statedb.1; | |
| // Get epoch from forked statedb (read from VM2's statedb) | |
| let epoch = get_epoch_from_statedb(statedb2)?; | |
| // Check for cached state (StateDB + BlockExecutedData) | |
| // If cache hit, we can skip execution and apply_write_set entirely | |
| let state_cache = global_block_state_cache(); | |
| let cached_state = state_cache.remove(header.txn_accumulator_root()); | |
| // Execute or use cached data | |
| let (executed_data, executed_data2, cached_statedb, cached_statedb2) = | |
| if let Some(cached) = cached_state { | |
| if cached.is_complete() { | |
| info!( | |
| "Using cached StateDB and executed data for block {:?}", | |
| block_id | |
| ); | |
| ( | |
| cached.executed_data.unwrap(), | |
| cached.executed_data2.unwrap(), | |
| cached.statedb, | |
| cached.statedb2, | |
| ) | |
| } else { | |
| // Partial cache - fall back to execution | |
| let (data, data2) = execute_block_transactions( | |
| statedb, | |
| statedb2, | |
| &transactions, | |
| &transactions2, | |
| &epoch, | |
| self.vm_metrics.clone(), | |
| )?; | |
| (data, data2, None, None) | |
| } | |
| } else { | |
| // No cache - execute normally | |
| let (data, data2) = execute_block_transactions( | |
| statedb, | |
| statedb2, | |
| &transactions, | |
| &transactions2, | |
| &epoch, | |
| self.vm_metrics.clone(), | |
| )?; | |
| (data, data2, None, None) | |
| }; | |
| // If we used cached statedb, we need to flush the cached ones | |
| // Otherwise, we only need to flush self.statedb (executor already applied write_sets) | |
| if cached_statedb.is_some() && cached_statedb2.is_some() { | |
| // Flush cached statedbs | |
| let cached_db = cached_statedb.as_ref().unwrap(); | |
| let cached_db2 = cached_statedb2.as_ref().unwrap(); | |
| cached_db.flush()?; | |
| cached_db2.flush()?; | |
| // Update self.statedb to the cached state | |
| self.statedb = (cached_db.clone(), cached_db2.clone()); | |
| } else { | |
| // Note: write_sets are already applied and committed by executor functions | |
| // (block_execute / execute_transactions), we only need to flush here | |
| statedb.flush()?; | |
| statedb2.flush()?; | |
| } |
🤖 Prompt for AI Agents
In @chain/src/chain.rs around lines 2267 - 2330, The cached-path flushes
cached_db/cached_db2 but never updates self.statedb, so the Chain's active state
remains stale; after confirming cached_statedb and cached_statedb2 are Some,
assign the cached Arcs back into self.statedb (e.g., self.statedb =
(cached_db.clone(), cached_db2.clone()) or set the tuple fields) so the chain
uses the cached state for subsequent operations; also avoid holding long-lived
&self.statedb borrows (the local statedb/statedb2 should be clones or obtained
after possible assignment) to satisfy Rust borrow rules.
| /// Execute block with pre-computed outputs (from cache) | ||
| pub fn block_execute_with_outputs<S: ChainStateReader + ChainStateWriter>( | ||
| chain_state: &S, | ||
| txns: Vec<Transaction>, | ||
| outputs: Vec<TransactionOutput>, | ||
| ) -> ExecutorResult<BlockExecutedData> { | ||
| let mut executed_data = BlockExecutedData::default(); | ||
| let last_index = outputs | ||
| .len() | ||
| .checked_sub(1) | ||
| .ok_or_else(|| BlockExecutorError::BlockTransactionZero)?; | ||
| for (index, (txn, output)) in txns | ||
| .iter() | ||
| .take(outputs.len()) | ||
| .zip(outputs.into_iter()) | ||
| .enumerate() | ||
| { | ||
| let txn_hash = txn.id(); | ||
| let (mut table_infos, write_set, events, gas_used, status) = output.into_inner(); | ||
| match status { | ||
| TransactionStatus::Discard(status) => { | ||
| return Err(BlockExecutorError::BlockTransactionDiscard( | ||
| status, txn_hash, | ||
| )); | ||
| } | ||
| TransactionStatus::Keep(status) => { | ||
| chain_state | ||
| .apply_write_set(write_set.clone()) | ||
| .map_err(BlockExecutorError::BlockChainStateErr)?; | ||
| let txn_state_root = if index == last_index || index == 0 { | ||
| Some( | ||
| chain_state | ||
| .commit() | ||
| .map_err(BlockExecutorError::BlockChainStateErr)?, | ||
| ) | ||
| } else { | ||
| None | ||
| }; | ||
| executed_data.txn_infos.push(TransactionInfo::new( | ||
| txn_hash, | ||
| txn_state_root, | ||
| events.as_slice(), | ||
| gas_used, | ||
| status, | ||
| )); | ||
| executed_data.txn_events.push(events); | ||
| executed_data.txn_table_infos.append(&mut table_infos); | ||
| executed_data.write_sets.push(write_set); | ||
| } | ||
| TransactionStatus::Retry => return Err(BlockExecutorError::BlockExecuteRetryErr), | ||
| }; | ||
| } | ||
| executed_data.state_root = chain_state.state_root(); | ||
| Ok(executed_data) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the executor files in the repository
find . -name "block_executor.rs" -type f | head -20Repository: starcoinorg/starcoin
Length of output: 173
🏁 Script executed:
# Search for the BlockExecutorError enum definition
rg -n "enum BlockExecutorError" --type rust -A 30Repository: starcoinorg/starcoin
Length of output: 4141
🏁 Script executed:
# Check for balance event logging patterns
rg -n "LOGGER_BALANCE_AMOUNT|balance.*log" --type rustRepository: starcoinorg/starcoin
Length of output: 1234
🏁 Script executed:
# Check the actual file at lines 149-203 to see the implementation
if [ -f "executor/src/block_executor.rs" ]; then
echo "=== executor/src/block_executor.rs (lines 149-203) ===" && sed -n '149,203p' executor/src/block_executor.rs
elif [ -f "vm2/executor/src/block_executor.rs" ]; then
echo "=== vm2/executor/src/block_executor.rs (lines 149-203) ===" && sed -n '149,203p' vm2/executor/src/block_executor.rs
fiRepository: starcoinorg/starcoin
Length of output: 2248
🏁 Script executed:
# Search for block_execute function to find balance logging
rg -n "fn block_execute" --type rust -A 60 | grep -A 60 "block_execute[^_]"Repository: starcoinorg/starcoin
Length of output: 8856
🏁 Script executed:
# Check imports and error enum in VM1 executor
head -50 executor/src/block_executor.rs | grep -E "use|import|BlockExecutorError"Repository: starcoinorg/starcoin
Length of output: 851
🏁 Script executed:
# Check full BlockExecutorError enum in types/src/error/mod.rs
cat -n types/src/error/mod.rs | head -50Repository: starcoinorg/starcoin
Length of output: 1683
🏁 Script executed:
# Verify the complete error definition in VM1 types
rg -n "^pub enum BlockExecutorError" types/src/error/mod.rs -A 30Repository: starcoinorg/starcoin
Length of output: 1194
🏁 Script executed:
# Check if InvalidOutputCount needs to be added to VM1 error types
rg -n "InvalidOutputCount" types/src/error/mod.rsRepository: starcoinorg/starcoin
Length of output: 46
🏁 Script executed:
# Check how block_execute_with_outputs is called in the codebase
rg -n "block_execute_with_outputs" --type rust -B 3 -A 3Repository: starcoinorg/starcoin
Length of output: 2189
🏁 Script executed:
# Look for any callers that might pass mismatched lengths
rg -n "block_execute_with_outputs" executor/src/block_executor.rs -A 1 -B 1Repository: starcoinorg/starcoin
Length of output: 228
🏁 Script executed:
# Find where block_execute_with_outputs is called in VM1 (executor-based chain)
rg -n "block_execute_with_outputs" chain/src/ --type rust -B 5 -A 5Repository: starcoinorg/starcoin
Length of output: 46
🏁 Script executed:
# Check if there's similar validation in VM2 block_execute_with_outputs
sed -n '102,140p' vm2/executor/src/block_executor.rsRepository: starcoinorg/starcoin
Length of output: 1573
🏁 Script executed:
# Verify if there are any docs or comments explaining why balance logging is omitted in block_execute_with_outputs
rg -n "block_execute_with_outputs|pre-computed" executor/src/block_executor.rs -B 5 -A 2Repository: starcoinorg/starcoin
Length of output: 363
🏁 Script executed:
# Check what callers pass to block_execute_with_outputs (if any) and how they ensure matching lengths
rg -rn "block_execute_with_outputs" . --type rust | grep -v "pub fn block_execute_with_outputs" | grep -v "\.rs:8:" | head -20Repository: starcoinorg/starcoin
Length of output: 480
Add input validation with appropriate error handling.
The block_execute_with_outputs function lacks input validation that should ensure txns.len() == outputs.len(). Currently, take(outputs.len()) silently ignores extra transactions, which could mask bugs. VM2's implementation includes this validation.
However, the suggested fix references BlockExecutorError::InvalidOutputCount, which does not exist in the VM1 error types (types/src/error/mod.rs). Add validation using OtherError or another appropriate existing variant:
pub fn block_execute_with_outputs<S: ChainStateReader + ChainStateWriter>(
chain_state: &S,
txns: Vec<Transaction>,
outputs: Vec<TransactionOutput>,
) -> ExecutorResult<BlockExecutedData> {
+ if txns.len() != outputs.len() {
+ return Err(BlockExecutorError::OtherError(Box::new(anyhow::anyhow!(
+ "transaction count {} does not match output count {}",
+ txns.len(),
+ outputs.len()
+ ))));
+ }
let mut executed_data = BlockExecutedData::default();The balance event logging omission (lines 92–105 in block_execute) is intentional for pre-computed outputs but should be documented in a comment explaining why it's skipped.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.