Remove fork in chain #4771
Remove fork in chain #4771
Conversation
📝 WalkthroughWalkthroughReplaces per-block VM multi-state forking with direct in-memory statedb references during DAG block execution, adjusts epoch retrieval to read from the in-memory statedb, and adds a pre-apply fork check in block sync to fork the chain to a block's parent when head id != block.parent_hash before verification/apply. Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce a new multi-component sequential flow warranting a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
2211-2225: Add validation that state is positioned at selected parent's state root before execution.The comment on line 2211 mentions "Fork statedb from selected parent's state root," but the code now takes direct references to
self.statedbwithout validation. Before line 2229 where execution begins, add a check that verifiesself.statedb.0andself.statedb.1are positioned at theselected_parent's state roots using theparent_block_infoalready retrieved. Without this validation, execution could proceed with misaligned state if the selected block changed during prior block connections, causing silent state root failures.Also update the comment on line 2211 to reflect that direct references are used instead of forking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chain/src/chain.rs(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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: 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.
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:
sync/src/tasks/block_sync_task.rschain/src/chain.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-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:
sync/src/tasks/block_sync_task.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:
sync/src/tasks/block_sync_task.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/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:
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-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/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 (1)
vm2/chain/src/lib.rs (1)
get_epoch_from_statedb(53-56)
🔇 Additional comments (1)
chain/src/chain.rs (1)
2230-2268: State positioning pattern partially implemented—add validation inexecute_dag_blockto fail fast if state roots don't match selected parent.The code correctly uses direct
statedbreferences to avoid repeated state forking during execution and relies on pre-positioning. However,execute_dag_blockdoes not validate that the chain's state is positioned at the selected parent's state roots before executing—it silently assumes correct positioning. The sync task correctly implements the pre-positioning pattern viafork()before callingapply_block(), but this validation gap inexecute_dag_blockitself means mid-execution state misalignment could occur undetected.Add an assertion or early return in
execute_dag_block(after line 2215) to verify thatstatedb.state_root()andstatedb2.state_root()match the parent's state roots from storage, similar to how sync tasks validate viafork().
jackzhhuang
force-pushed
the
no-fork-state
branch
from
513f503 to
8c145cd
Compare
jackzhhuang
force-pushed
the
no-fork-state
branch
from
8c145cd to
7191010
Compare
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)
2246-2253: CRITICAL: Direct state modifications risk corrupting chain state.The code directly modifies
self.statedbthroughapply_write_set,commit, andflushoperations (lines 2246-2253, 2417-2418). Sincestatedbis a reference toself.statedb.0, these modifications affect the BlockChain instance's state in-place.If
self.statedbis not correctly positioned at the selected parent's state roots (which is not validated per the previous comment), these operations will corrupt the chain state by applying changes from the wrong base state.The previous forking approach created an isolated copy, preventing such corruption. The new direct-reference approach requires either:
- Guaranteed correct positioning (via validation as suggested above), OR
- Reverting to isolated execution context to prevent side effects on self.statedb
Without one of these safeguards, executing blocks on incorrect state will persist corrupted data to storage, potentially breaking the chain.
Based on learnings, the design expects correct state positioning before execution, but the current implementation lacks the necessary validation to ensure this.
Also applies to: 2417-2418
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chain/src/chain.rssync/src/tasks/block_sync_task.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- sync/src/tasks/block_sync_task.rs
🧰 Additional context used
🧠 Learnings (10)
📓 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: 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: 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.
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`.
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-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: 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:
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: 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-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/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 (2)
vm2/chain/src/lib.rs (1)
get_epoch_from_statedb(53-56)executor/src/block_executor.rs (1)
block_execute(65-147)
| 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)?; | ||
| let epoch = get_epoch_from_statedb(statedb2)?; |
There was a problem hiding this comment.
CRITICAL: Missing state positioning validation before execution.
Based on learnings, the design requires that self.statedb must be positioned at the selected parent's state roots BEFORE execute_dag_block is called, and execution should validate this precondition. However, no such validation exists.
The code uses direct references to self.statedb without verifying they match the selected parent's state roots. If the current chain head differs from selected_parent (line 2172), self.statedb will be at the wrong state, causing incorrect execution results.
This is especially problematic because verify_with_verifier (lines 502-510) creates a temporary chain positioned at the parent for verification, but execution happens on the original self without repositioning.
🔎 Add state positioning validation
let statedb = &self.statedb.0;
let statedb2 = &self.statedb.1;
+
+// Validate that statedb is positioned at selected parent's state roots
+let parent_multi_state = self.storage.0.get_vm_multi_state(selected_parent)?;
+ensure!(
+ statedb.state_root() == parent_multi_state.state_root1() &&
+ statedb2.state_root() == parent_multi_state.state_root2(),
+ "statedb not positioned at selected parent's state roots. Expected: ({:?}, {:?}), Actual: ({:?}, {:?})",
+ parent_multi_state.state_root1(), parent_multi_state.state_root2(),
+ statedb.state_root(), statedb2.state_root()
+);
// Get epoch from forked statedb (read from VM2's statedb)
let epoch = get_epoch_from_statedb(statedb2)?;Based on learnings, this validation is essential to prevent executing blocks on incorrect state, which would corrupt the chain.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In chain/src/chain.rs around lines 2221-2225, the code reads self.statedb and
statedb.1 and calls get_epoch_from_statedb without ensuring self.statedb is
positioned at the selected_parent's state roots; add a precondition check that
validates the current statedb's state root (and any related roots like
receipts/root hashes used by execution) matches selected_parent's expected state
roots before proceeding. If they don't match, either reposition the chain's
statedb to the selected_parent (call the existing repositioning/checkout routine
or create a temporary positioned statedb) or return an error so
execute_dag_block is not run against the wrong state; make the same
validation/assertion visible where verify_with_verifier uses a temp chain to
ensure both verification and execution operate on identically positioned state.
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
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.