Cache transaction output #4772
Cache transaction output #4772
Conversation
WalkthroughAdds a thread-safe global transaction-output cache, plumbs cached VM1/VM2 outputs through OpenedBlock.finalize, miner insertion, and chain/executor execution paths that can consume precomputed outputs via new "with_outputs" APIs, removing cache entries after use. Changes
Sequence Diagram(s)sequenceDiagram
actor Miner
participant OpenedBlock
participant GlobalCache
participant Chain (Node)
participant Executor
participant ChainState
rect rgba(200,220,255,0.9)
note right of Miner: Block finalization & caching
Miner->>OpenedBlock: finalize()
OpenedBlock-->>Miner: (BlockTemplate, vm1_outs, vm2_outs)
Miner->>GlobalCache: insert_outputs(txn_acc_root, vm1_outs, vm2_outs)
end
rect rgba(220,255,220,0.9)
note right of Chain: Block import/execution (cache hit)
Chain->>GlobalCache: get(txn_acc_root)
GlobalCache-->>Chain: CachedBlockOutputs (vm1/vm2)
Chain->>Executor: choose block_execute_with_outputs / execute_transactions_with_outputs
Executor->>ChainState: apply pre-computed write sets & events
Executor-->>Chain: BlockExecutedData
Chain->>GlobalCache: remove(txn_acc_root)
end
rect rgba(255,240,200,0.9)
note right of Chain: Block import/execution (cache miss)
Chain->>GlobalCache: get(txn_acc_root)
GlobalCache-->>Chain: None
Chain->>Executor: block_execute / execute_transactions (full run)
Executor->>ChainState: execute & compute outputs
Executor-->>Chain: BlockExecutedData (and produced outputs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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)
2272-2280: Write set application after cached execution may cause state inconsistency.When using cached outputs (lines 2241-2245), the
block_execute_with_outputsfunction already applies write sets internally (per the relevant snippet fromvm2/executor/src/block_executor.rs). However, lines 2273-2280 apply write sets again fromexecuted_data.write_sets.For cached execution,
executed_datacomes fromblock_execute_with_outputswhich returnsBlockExecutedDatawithwrite_setspopulated. Applying them again would be a double-apply causing incorrect state.Please verify whether
block_execute_with_outputsalready applies write sets internally. If so, the write set loop (lines 2273-2280) should be conditional on whether cached outputs were used, or the loop should be removed entirely ifexecuted_data.write_setsis always applied within the executor.#!/bin/bash # Check if block_execute_with_outputs applies write sets internally rg -n --type rust 'apply_write_set' vm2/executor/src/block_executor.rs -B 2 -A 2 echo "" # Check VM1 executor's block_execute_with_outputs rg -n --type rust 'apply_write_set' executor/src/block_executor.rs -B 2 -A 2
🧹 Nitpick comments (6)
chain/open-block/src/vm2.rs (1)
105-105: Consider the performance impact of cloning transaction outputs.Cloning
TransactionOutputmay be expensive when it contains large write sets, events, or table info. Since the output is consumed immediately after on line 106 viapush_txn_and_state2, consider restructuring the code to cache the output before consuming it, or extract only the necessary fields for caching to avoid the full clone overhead.executor/src/block_executor.rs (1)
149-203: Significant code duplication withblock_executefunction.The
block_execute_with_outputsfunction duplicates most of the logic fromblock_execute(lines 65-147), differing only in the source of transaction outputs. This creates a maintenance burden where bug fixes or enhancements must be applied to both functions.Consider extracting the common logic (state application, commit handling, TransactionInfo construction) into a shared helper function that both
block_executeandblock_execute_with_outputscan use.Example refactoring approach
fn process_outputs<S: ChainStateReader + ChainStateWriter>( chain_state: &S, txns: &[Transaction], outputs: Vec<TransactionOutput>, ) -> ExecutorResult<BlockExecutedData> { // Common logic extracted here } pub fn block_execute<S: ChainStateReader + ChainStateWriter>( chain_state: &S, txns: Vec<Transaction>, block_gas_limit: u64, vm_metrics: Option<VMMetrics>, ) -> ExecutorResult<BlockExecutedData> { let outputs = execute_block_transactions(...)?; process_outputs(chain_state, &txns, outputs) } pub fn block_execute_with_outputs<S: ChainStateReader + ChainStateWriter>( chain_state: &S, txns: Vec<Transaction>, outputs: Vec<TransactionOutput>, ) -> ExecutorResult<BlockExecutedData> { process_outputs(chain_state, &txns, outputs) }vm2/executor/src/block_executor.rs (1)
101-154: Code duplication exists in VM2 executor as well.Similar to the VM1 executor, this function duplicates most of the logic from
block_execute(lines 36-99). The same refactoring suggestion applies here: extract common output-processing logic into a shared helper function to reduce duplication and improve maintainability.chain/src/txn_output_cache.rs (3)
68-106: Remove unnecessary clones in insert methods.The
outputs.clone()calls on lines 75, 87, and thevm1_outputs.clone(),vm2_outputs.clone()on lines 100-101 are unnecessary. Since these values are moved intoCachedBlockOutputs, you can pass them directly without cloning, avoiding extra allocations.🔎 Apply this diff to remove unnecessary clones:
pub fn insert_vm1_outputs( &self, block_id: HashValue, state_root: HashValue, outputs: Vec<TransactionOutput1>, ) { let key = CacheKey::new(block_id, state_root); - let cached = Arc::new(CachedBlockOutputs::new_vm1(outputs.clone())); + let cached = Arc::new(CachedBlockOutputs::new_vm1(outputs)); self.cache.insert(key, cached.clone()); self.cache_by_block_id.insert(block_id, cached); } pub fn insert_vm2_outputs( &self, block_id: HashValue, state_root: HashValue, outputs: Vec<TransactionOutput2>, ) { let key = CacheKey::new(block_id, state_root); - let cached = Arc::new(CachedBlockOutputs::new_vm2(outputs.clone())); + let cached = Arc::new(CachedBlockOutputs::new_vm2(outputs)); self.cache.insert(key, cached.clone()); self.cache_by_block_id.insert(block_id, cached); } pub fn insert_outputs( &self, block_id: HashValue, state_root: HashValue, vm1_outputs: Option<Vec<TransactionOutput1>>, vm2_outputs: Option<Vec<TransactionOutput2>>, ) { let cached = Arc::new(CachedBlockOutputs { - vm1_outputs: vm1_outputs.clone(), - vm2_outputs: vm2_outputs.clone(), + vm1_outputs, + vm2_outputs, }); let key = CacheKey::new(block_id, state_root); self.cache.insert(key, cached.clone()); self.cache_by_block_id.insert(block_id, cached); }
68-106: Consider potential transient inconsistency between dual caches.The cache maintains two separate DashMaps (
cacheandcache_by_block_id), and insert/remove operations update them sequentially. While unlikely to cause issues in practice, concurrent readers might observe transient inconsistency—finding an entry in one cache but not the other.If strict consistency is required, consider wrapping the dual inserts/removes in a higher-level synchronization primitive, or document that transient inconsistency is acceptable.
212-259: Consider expanding test coverage.The current tests cover basic insert, get, and remove operations for VM1 outputs. Consider adding tests for:
- VM2 outputs and dual VM1/VM2 insertion via
insert_outputsevict_if_neededbehavior- Helper methods like
get_vm1_outputs_by_block_idandget_vm2_outputs_by_block_id- Concurrent access patterns (multiple threads inserting/reading)
📜 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 (14)
chain/Cargo.toml(1 hunks)chain/open-block/src/lib.rs(5 hunks)chain/open-block/src/vm2.rs(1 hunks)chain/src/chain.rs(4 hunks)chain/src/lib.rs(1 hunks)chain/src/txn_output_cache.rs(1 hunks)executor/src/block_executor.rs(2 hunks)executor/src/lib.rs(1 hunks)miner/src/create_block_template/block_builder_service.rs(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)txpool/src/test.rs(1 hunks)vm2/chain/src/lib.rs(2 hunks)vm2/executor/src/block_executor.rs(2 hunks)vm2/executor/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 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-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:
executor/src/lib.rstxpool/src/test.rschain/src/lib.rsvm2/chain/src/lib.rsexecutor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rssync/src/tasks/block_sync_task.rsvm2/executor/src/block_executor.rschain/open-block/src/lib.rschain/src/chain.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:
executor/src/lib.rsminer/src/create_block_template/block_builder_service.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:
executor/src/lib.rstxpool/src/test.rschain/src/lib.rsexecutor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rssync/src/tasks/block_sync_task.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:
txpool/src/test.rschain/src/lib.rsvm2/chain/src/lib.rsexecutor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rsvm2/executor/src/block_executor.rschain/open-block/src/vm2.rschain/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:
txpool/src/test.rschain/src/lib.rsvm2/chain/src/lib.rsexecutor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rschain/open-block/src/vm2.rschain/open-block/src/lib.rschain/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/lib.rsvm2/chain/src/lib.rsexecutor/src/block_executor.rsminer/src/create_block_template/block_builder_service.rssync/src/tasks/block_sync_task.rsvm2/executor/src/block_executor.rschain/open-block/src/vm2.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:
chain/src/lib.rsvm2/chain/src/lib.rsminer/src/create_block_template/block_builder_service.rssync/src/tasks/block_sync_task.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/src/lib.rsvm2/chain/src/lib.rsexecutor/src/block_executor.rschain/open-block/src/vm2.rschain/open-block/src/lib.rschain/src/txn_output_cache.rschain/src/chain.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-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.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.rssync/src/tasks/block_sync_task.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.rschain/open-block/src/vm2.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-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 (7)
executor/src/lib.rs (5)
executor/src/block_executor.rs (3)
block_execute(65-147)block_execute_with_outputs(150-203)get_logger_balance_amount(31-33)vm2/executor/src/block_executor.rs (2)
block_execute(36-99)block_execute_with_outputs(102-154)rpc/client/src/lib.rs (1)
get_logger_balance_amount(796-799)rpc/server/src/module/debug_rpc.rs (1)
get_logger_balance_amount(101-103)rpc/api/src/debug/mod.rs (1)
get_logger_balance_amount(46-46)
chain/src/lib.rs (1)
chain/src/txn_output_cache.rs (1)
global_txn_output_cache(208-210)
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)
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)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (1)
global_txn_output_cache(208-210)
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)
chain/src/chain.rs (6)
chain/src/txn_output_cache.rs (2)
global_txn_output_cache(208-210)len(173-175)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/chain/src/lib.rs (1)
get_epoch_from_statedb(62-65)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)
🔇 Additional comments (18)
sync/src/tasks/block_sync_task.rs (1)
355-357: Pre-fork logic ensures correct parent state before block application.The fork-before-apply logic correctly positions the chain state at the block's parent before applying the block. This aligns with the requirement that state must be initialized at the selected parent's state roots before execution.
Ensure that when cached transaction outputs are consumed later in the execution path, they correspond to the correct parent state established by this fork operation.
Based on learnings, the BlockChain instance's state must be positioned at the selected parent's state roots BEFORE execution is called.
txpool/src/test.rs (1)
282-282: Test correctly updated for new finalize API.The test appropriately destructures the triple return value from
finalize()and ignores the VM outputs that are not needed for this test case.chain/Cargo.toml (1)
4-4: Appropriate dependency for concurrent caching.The
dashmapdependency is well-suited for the concurrent transaction output cache implementation, providing lock-free concurrent hash map functionality.executor/src/lib.rs (1)
8-9: Public API correctly expanded.The new
block_execute_with_outputsfunction is appropriately exported to make the cached-output execution path available to consumers of this crate.miner/src/create_block_template/block_builder_service.rs (1)
621-634: Remove incorrect assumption about dual state_root parameters in insert_outputs.The
insert_outputsmethod signature takes only onestate_rootparameter, not two. It accepts:block_id,state_root,vm1_outputs, andvm2_outputs. Therefore, the concern about "using the same state root for both parameters" is not applicable to this method's design.The code at lines 621-634 passes
template.state_rootfor both theblock_idandstate_rootarguments, which is the current implementation pattern. While this may warrant review (consider whether a block hash should be used asblock_idinstead), the specific concern about dual state roots was based on a misunderstanding of the method signature.Likely an incorrect or invalid review comment.
chain/src/lib.rs (1)
6-15: LGTM!The new
txn_output_cachemodule is properly declared as public and the key types are correctly re-exported for downstream consumption. The global accessor pattern aligns with the caching strategy for VM outputs.vm2/chain/src/lib.rs (1)
27-34: LGTM!The new
execute_transactions_with_outputsfunction correctly delegates to the executor'sblock_execute_with_outputs, enabling cache-backed execution paths for VM2. The signature appropriately accepts pre-computed outputs alongside transactions.vm2/executor/src/lib.rs (1)
8-8: LGTM!The re-export correctly exposes
block_execute_with_outputsat the crate root, making the outputs-aware execution API available to consumers likevm2/chain.chain/open-block/src/lib.rs (4)
58-60: LGTM!The new cache fields for VM1 and VM2 outputs are correctly typed and appropriately scoped within
OpenedBlock. Using distinct types for each VM's outputs maintains type safety.
128-129: LGTM!Fields properly initialized to empty vectors, ready to accumulate outputs during transaction processing.
233-233: Output cloning is necessary here.The
output.clone()is required sinceoutputis consumed bypush_txn_and_stateon line 234. This is the correct approach to cache the output before it's moved.
340-346: Breaking API change:finalize()return type updated.The return type changed from
Result<BlockTemplate>toResult<(BlockTemplate, Vec<TransactionOutput>, Vec<starcoin_vm2_types::transaction::TransactionOutput>)>. All callers have been updated to destructure the tuple correctly (chain/src/chain.rs:453, miner/src/create_block_template/block_builder_service.rs:611, txpool/src/test.rs:282).chain/src/chain.rs (6)
7-7: LGTM!Import of
global_txn_output_cachecorrectly added for cache access in execution paths.
453-453: LGTM!The
finalize()call is updated to destructure the new tuple return type. Outputs are correctly ignored here sincecreate_block_templatedoesn't need to cache outputs (the miner's block_builder_service handles caching per the AI summary).
2222-2226: Using statedb references directly is intentional.Based on prior learnings, the
BlockChaininstance's state must be positioned at the selected parent's state roots beforeexecute_dag_blockis called. The design avoids performance overhead of repeated state forking during execution.
2233-2261: VM1 cached execution path looks correct.The logic properly:
- Checks for cached outputs before execution
- Falls back to standard
block_executewhen cache miss- Uses
block_execute_with_outputswhen cache hitThe info logging helps with debugging cache utilization.
2289-2316: VM2 cached execution path follows the same pattern.The fallback logic mirrors VM1's approach. Same observation about consistency with the cache key applies.
2318-2321: Cache removal prevents memory leaks and stale entries.Good practice to remove cached outputs after consumption. The guard
if cached_outputs.is_some()ensures removal only occurs when cache was used.
jackzhhuang
force-pushed
the
cache-transaction-output
branch
from
3ebfaf0 to
afbaca5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
chain/src/chain.rs (2)
2234-2262: Consider reducing code duplication in cached execution paths.The cached vs non-cached execution logic is repeated with slight variations. Consider extracting a helper function to reduce duplication.
🔎 Example refactor
// Helper could look like: fn execute_vm1_transactions( statedb: &ChainStateDB, transactions: Vec<Transaction>, cached_outputs: Option<&Arc<Vec<TransactionOutput>>>, gas_limit: u64, vm_metrics: Option<VMMetrics>, ) -> Result<BlockExecutedData> { if let Some(outputs) = cached_outputs { starcoin_executor::block_execute_with_outputs( statedb, transactions, outputs.as_ref().clone(), ) } else { starcoin_executor::block_execute( statedb, transactions, gas_limit, vm_metrics, ) } }
2243-2246: CloningArc<Vec<...>>contents may be unnecessary overhead.
vm1_outputs.as_ref().clone()andvm2_outputs.as_ref().clone()clone the entireVec. If theblock_execute_with_outputsAPI could accept&[TransactionOutput]orArc<Vec<...>>directly, this clone could be avoided for better performance with large transaction sets.Also applies to: 2298-2301
chain/src/txn_output_cache.rs (1)
92-151: Consider adding concurrency tests for thread-safe cache.The tests cover basic functionality but lack concurrency scenarios. Since
TransactionOutputCacheusesDashMapfor thread-safe access, consider adding tests that:
- Insert from multiple threads simultaneously
- Concurrent get/remove operations
- Race conditions between insert and remove
This would increase confidence in the thread-safety guarantees.
Example concurrency test pattern
#[test] fn test_concurrent_access() { use std::thread; let cache = Arc::new(TransactionOutputCache::new()); let mut handles = vec![]; for i in 0..10 { let cache_clone = Arc::clone(&cache); handles.push(thread::spawn(move || { let txn_root = HashValue::random(); cache_clone.insert_outputs(txn_root, None, None); let result = cache_clone.get(txn_root); assert!(result.is_some()); cache_clone.remove(txn_root); })); } for handle in handles { handle.join().unwrap(); } }
📜 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 (14)
chain/Cargo.toml(1 hunks)chain/open-block/src/lib.rs(6 hunks)chain/open-block/src/vm2.rs(3 hunks)chain/src/chain.rs(4 hunks)chain/src/lib.rs(1 hunks)chain/src/txn_output_cache.rs(1 hunks)executor/src/block_executor.rs(2 hunks)executor/src/lib.rs(1 hunks)miner/src/create_block_template/block_builder_service.rs(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)txpool/src/test.rs(1 hunks)vm2/chain/src/lib.rs(2 hunks)vm2/executor/src/block_executor.rs(2 hunks)vm2/executor/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- miner/src/create_block_template/block_builder_service.rs
- executor/src/block_executor.rs
- txpool/src/test.rs
- executor/src/lib.rs
- vm2/chain/src/lib.rs
- vm2/executor/src/lib.rs
- chain/Cargo.toml
🧰 Additional context used
🧠 Learnings (12)
📓 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: 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:
vm2/executor/src/block_executor.rschain/open-block/src/lib.rschain/src/chain.rssync/src/tasks/block_sync_task.rschain/open-block/src/vm2.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:
vm2/executor/src/block_executor.rschain/open-block/src/lib.rschain/src/chain.rschain/open-block/src/vm2.rschain/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/lib.rschain/src/chain.rssync/src/tasks/block_sync_task.rschain/open-block/src/vm2.rschain/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/lib.rschain/src/chain.rschain/open-block/src/vm2.rschain/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/open-block/src/lib.rschain/src/txn_output_cache.rschain/src/chain.rschain/open-block/src/vm2.rschain/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.rschain/src/chain.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/chain.rssync/src/tasks/block_sync_task.rschain/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/chain.rssync/src/tasks/block_sync_task.rschain/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-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.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
🧬 Code graph analysis (2)
chain/src/chain.rs (4)
chain/src/txn_output_cache.rs (2)
global_txn_output_cache(88-90)len(66-68)vm2/chain/src/lib.rs (1)
get_epoch_from_statedb(62-65)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)
chain/src/lib.rs (1)
chain/src/txn_output_cache.rs (1)
global_txn_output_cache(88-90)
⏰ 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 (18)
sync/src/tasks/block_sync_task.rs (1)
355-357: State positioning before DAG block execution is correctly implemented.The fork operation before
apply_blockensures the chain state is positioned at the block's parent before execution, which aligns with DAG requirements and avoids inefficient mid-execution state forking. The implementation is efficient:fork()andstatedb.fork()use cheap Arc clones with metadata updates only—no state tree copying or expensive I/O. The operation triggers only on out-of-order blocks (whenchain.status().head().id() != block.parent_hash()), not during sequential processing. The pattern is consistent across the codebase (also used in the parallel executor).chain/src/lib.rs (1)
6-13: LGTM!The new
txn_output_cachemodule and its re-exports follow the existing pattern in this file. The public API surface is appropriate for cross-module cache access.chain/open-block/src/vm2.rs (3)
45-48: LGTM!The caching of VM2 outputs before consumption by
push_txn_and_state2is correctly implemented. The clone is necessary sinceinto_inner()consumes the output.
102-111: LGTM!Consistent caching pattern for user transaction outputs. The cache is populated only for
Keepstatus transactions, which is correct.
153-156: LGTM!Block epilogue output caching follows the same correct pattern as the other transaction types.
vm2/executor/src/block_executor.rs (1)
101-154: LGTM!The
block_execute_with_outputsfunction correctly mirrors the existingblock_executelogic, applying pre-computed outputs instead of re-executing transactions. The state commitment strategy (first and last transactions) is consistent with the existing implementation. Based on learnings, this intermediate commit pattern is acceptable since block-level atomicity is maintained at a higher level.chain/open-block/src/lib.rs (5)
58-60: LGTM!The new cache fields for VM1 and VM2 outputs are properly typed and positioned within the struct.
128-129: LGTM!Cache vectors are correctly initialized as empty in the constructor.
232-234: LGTM!VM1 output caching in
push_txnsfollows the same pattern as VM2 - clone before consumption bypush_txn_and_state.
273-276: LGTM!Block metadata transaction output is correctly cached before being consumed.
342-348: Breaking API change:finalize()now returns a tuple.The return type changed from
Result<BlockTemplate>toResult<(BlockTemplate, Vec<TransactionOutput>, Vec<starcoin_vm2_types::transaction::TransactionOutput>)>. All call sites have been properly updated to handle the new return type.chain/src/chain.rs (5)
7-7: LGTM!Import of
global_txn_output_cacheis correctly placed alongside other crate imports.
453-453: LGTM!The finalize return value is correctly destructured, and the cached outputs are intentionally ignored here since they're only needed for miner-side caching, not for template creation during chain operations.
2222-2226: State access relies on precondition being met.The code accesses
self.statedb.0andself.statedb.1directly rather than forking. Based on learnings, this is correct because "the BlockChain instance's state must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called." Ensure callers maintain this invariant.
2228-2231: Cache key usage looks correct.Using
header.txn_accumulator_root()as the cache key is appropriate since it uniquely identifies the transaction list for a block. This addresses the concern from previous reviews about incorrect cache key parameters.
2319-2322: LGTM!Cache entries are correctly removed after use, preventing memory leaks and ensuring one-time use of cached outputs.
chain/src/txn_output_cache.rs (2)
11-28: LGTM! Arc wrapping is appropriate for cache sharing.The
CachedBlockOutputsdesign correctly usesArcto enable cheap cloning of cached outputs across multiple readers without duplicating the underlying vectors.
85-90: LGTM! Standard lazy-initialized global cache pattern.The use of
once_cell::sync::Lazyensures thread-safe initialization of the global cache. This is idiomatic Rust for singleton patterns.
jackzhhuang
force-pushed
the
cache-transaction-output
branch
from
afbaca5 to
441ea03
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
chain/src/chain.rs (1)
2228-2322: Critical: Cache leak on error paths requires scope-based cleanup.This issue was identified in a previous review and remains unaddressed. Between
cache.get()(line 2231) andcache.remove()(line 2321), there are at least 9 error propagation points (?operators) where execution can exit the function without removing the cache entry:
- Block execution calls (lines 2242, 2248, 2256, 2297, 2303, 2311)
- State operations (lines 2275-2280)
- Plus additional verification and state management operations further down
Impact: In production, any execution failure (invalid transaction, state error, verification failure) will leave the cache entry indefinitely, causing unbounded memory growth.
Recommendation: Implement a scope guard pattern to ensure cleanup on both success and error paths:
Proposed scope-guard solution
// At cache lookup site (around line 2230) struct CacheGuard { cache: &'static TransactionOutputCache, key: HashValue, } impl Drop for CacheGuard { fn drop(&mut self) { self.cache.remove(self.key); } } let cache = global_txn_output_cache(); let cached_outputs = cache.get(header.txn_accumulator_root()); let _guard = cached_outputs.as_ref().map(|_| CacheGuard { cache, key: header.txn_accumulator_root(), }); // Remove explicit cache.remove() at line 2321 // The guard will clean up automatically on scope exit (success or error)chain/src/txn_output_cache.rs (1)
34-77: Critical: Add capacity limits and LRU eviction to prevent unbounded growth.This issue was identified in a previous review. The cache has no size limits or eviction strategy, which combined with the error-path memory leak (see
chain/src/chain.rsreview), creates guaranteed unbounded memory growth:
- Miner inserts outputs during block template creation
- Chain executor attempts to use cached outputs
- On execution failure (bad state, transaction error, verification failure),
remove()is never called- No capacity limits or eviction means entries accumulate indefinitely
Immediate risks:
- No upper bound on memory consumption
- No recovery mechanism for leaked entries
- Production blockchain will experience memory exhaustion over time
Recommended mitigation
+use std::sync::atomic::{AtomicUsize, Ordering}; + +// Configuration constants +const DEFAULT_MAX_CAPACITY: usize = 1000; +const EVICTION_BATCH_SIZE: usize = 100; + pub struct TransactionOutputCache { cache: DashMap<HashValue, CachedBlockOutputs>, + max_capacity: AtomicUsize, } impl TransactionOutputCache { pub fn new() -> Self { Self { cache: DashMap::new(), + max_capacity: AtomicUsize::new(DEFAULT_MAX_CAPACITY), } } pub fn insert_outputs( &self, txn_accumulator_root: HashValue, vm1_outputs: Option<Vec<TransactionOutput1>>, vm2_outputs: Option<Vec<TransactionOutput2>>, ) { + // Evict oldest entries if at capacity (LRU approximation) + if self.cache.len() >= self.max_capacity.load(Ordering::Relaxed) { + warn!("Cache at capacity {}, evicting {} entries", self.cache.len(), EVICTION_BATCH_SIZE); + let mut count = 0; + self.cache.retain(|_, _| { + count += 1; + count > EVICTION_BATCH_SIZE + }); + } let cached = CachedBlockOutputs::new(vm1_outputs, vm2_outputs); self.cache.insert(txn_accumulator_root, cached); } + + pub fn set_max_capacity(&self, capacity: usize) { + self.max_capacity.store(capacity, Ordering::Relaxed); + } }Additionally, consider adding a background cleanup task or TTL-based expiration for entries older than a configurable threshold (e.g., 5 minutes).
🧹 Nitpick comments (1)
executor/src/block_executor.rs (1)
149-203: Consider adding balance event logging for consistency.The new
block_execute_with_outputsfunction omits the large balance event logging present in the originalblock_executefunction (lines 92-105). When using cached outputs, transfers exceeding the configured threshold won't be logged, creating an inconsistency in observability between cached and non-cached execution paths.Proposed addition
Add the balance logging logic after line 167:
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 {
📜 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 (14)
chain/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
🚧 Files skipped from review as they are similar to previous changes (5)
- txpool/src/test.rs
- chain/open-block/src/vm2.rs
- vm2/executor/src/lib.rs
- chain/src/lib.rs
- chain/Cargo.toml
🧰 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: 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/chain.rsexecutor/src/block_executor.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rsvm2/chain/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:
miner/src/create_block_template/block_builder_service.rsexecutor/src/lib.rschain/src/chain.rsexecutor/src/block_executor.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.rschain/src/chain.rsexecutor/src/block_executor.rschain/src/txn_output_cache.rschain/open-block/src/lib.rsvm2/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:
miner/src/create_block_template/block_builder_service.rschain/src/chain.rsexecutor/src/block_executor.rschain/open-block/src/lib.rsvm2/chain/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:
miner/src/create_block_template/block_builder_service.rsvm2/executor/src/block_executor.rschain/src/chain.rsexecutor/src/block_executor.rschain/open-block/src/lib.rsvm2/chain/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:
miner/src/create_block_template/block_builder_service.rsexecutor/src/lib.rsvm2/executor/src/block_executor.rschain/src/chain.rsexecutor/src/block_executor.rssync/src/tasks/block_sync_task.rschain/open-block/src/lib.rsvm2/chain/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:
miner/src/create_block_template/block_builder_service.rschain/src/chain.rssync/src/tasks/block_sync_task.rsvm2/chain/src/lib.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-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-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.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
🧬 Code graph analysis (6)
miner/src/create_block_template/block_builder_service.rs (1)
chain/src/txn_output_cache.rs (1)
global_txn_output_cache(88-90)
executor/src/lib.rs (5)
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)rpc/client/src/lib.rs (1)
get_logger_balance_amount(796-799)rpc/server/src/module/debug_rpc.rs (1)
get_logger_balance_amount(101-103)rpc/api/src/debug/mod.rs (1)
get_logger_balance_amount(46-46)
vm2/executor/src/block_executor.rs (2)
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)
chain/src/chain.rs (1)
chain/src/txn_output_cache.rs (1)
global_txn_output_cache(88-90)
executor/src/block_executor.rs (3)
vm2/executor/src/block_executor.rs (2)
block_execute_with_outputs(102-154)default(26-33)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)types/src/transaction/mod.rs (2)
gas_used(102-104)status(106-108)
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)
⏰ 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 (6)
sync/src/tasks/block_sync_task.rs (1)
355-357: Fork behavior is correct and aligns with cache design; concerns are unwarranted.The conditional fork at lines 355-357 correctly ensures the chain state is positioned at the block's parent before executing, as required. Analysis confirms:
Fork is lightweight: Creates a new BlockChain instance by cloning Arc references to shared storage—not an expensive deep copy operation.
Cache consistency is maintained: The transaction output cache is keyed by
txn_accumulator_root(which uniquely identifies the transaction list), not by state root. This means fork operations, which reposition the chain head without affecting the transaction list, do not invalidate cached outputs. The cache design explicitly supports lookups before block execution.State positioning is correct: The fork properly repositions state at the parent block before apply operations, aligning with the requirement that state must be initialized at the parent's state roots before block execution.
The cache is consumed immediately after use (removed at line 2152 in
execute_dag_block), so it functions as a temporary optimization rather than persistent storage—further eliminating consistency concerns across fork boundaries.executor/src/lib.rs (1)
7-10: LGTM! Clean API expansion.The new
block_execute_with_outputsexport is properly grouped with the existingblock_executefunction, maintaining a consistent public API surface for cached output execution.miner/src/create_block_template/block_builder_service.rs (1)
611-634: LGTM! Cache insertion is properly implemented.The finalization result is correctly destructured to extract VM1/VM2 outputs, and the cache key (
template.txn_accumulator_root) matches the retrieval key used inchain.rs. The conditional wrapping (non-empty outputs asSome(...), empty asNone) is appropriate.chain/open-block/src/lib.rs (1)
59-60: LGTM! Output caching is correctly integrated.The caching strategy properly captures VM1 outputs at each successful transaction execution point:
- Cached only on
TransactionStatus::Keep(lines 233, 275)- Includes both user transactions and block metadata
- Finalize signature updated to return cached outputs alongside the template
The cached outputs will accurately reflect the final block contents.
Also applies to: 128-129, 233-233, 274-275, 342-388
vm2/chain/src/lib.rs (1)
27-34: LGTM! Clean delegation to executor.The new
execute_transactions_with_outputsfunction properly delegates to the VM2 block executor'sblock_execute_with_outputs, maintaining a consistent layer separation.vm2/executor/src/block_executor.rs (1)
101-154: The review concern about table_infos is based on incorrect assumptions about VM2's architecture and does not apply.VM2's
TransactionOutputfundamentally differs from VM1's: it does not includetable_infosin its structure. Theinto_inner()method returns(WriteSet, Vec<ContractEvent>, u64, TransactionStatus, TransactionAuxiliaryData), where the ignored fifth value isTransactionAuxiliaryData(auxiliary error information), not table metadata. Since VM2 transactions do not produce table_infos, there is nothing to collect or merge intoBlockExecutedData.txn_table_infos. The function's handling of output is correct for VM2's design.Likely an incorrect or invalid review comment.
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
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.