Benchmark the block execution #4751
Conversation
WalkthroughThis PR adds comprehensive dual-VM testing and benchmarking infrastructure. It introduces a new starcoin-execute-bench crate for performance measurement, extends tx-factory with VM2 support and dedicated pressure test entry points for vm1/vm2, implements batch transaction signing/unlocking APIs across account and RPC layers, increases transaction pool default capacity limits, simplifies the parallel executor by removing gas-limit handling, updates network configuration for Halley, and includes supporting integration tests and deployment updates. Changes
Sequence DiagramsequenceDiagram
participant Main as TX-Factory Main
participant TxnMocker
participant AcctSvc as Account Service
participant TxPool as TxPool Service
participant RpcClient as RPC Client
participant VM as VM Executor
Main->>TxnMocker: new(vm_version, accounts)
TxnMocker->>RpcClient: get_or_create_accounts
RpcClient->>AcctSvc: create accounts
AcctSvc-->>RpcClient: AccountInfo[]
RpcClient-->>TxnMocker: accounts ready
Main->>Main: spawn pressure_vm1/vm2 thread
rect rgb(200, 220, 240)
Note over TxnMocker,VM: Pressure Test Loop
TxnMocker->>TxnMocker: check factory_status
alt stress path
TxnMocker->>TxnMocker: stress_test(accounts)
TxnMocker->>TxnMocker: gen_and_submit_transfer_txn (batched)
TxnMocker->>AcctSvc: unlock_account_in_batch
TxnMocker->>AcctSvc: sign_txn_in_batch
AcctSvc-->>TxnMocker: SignedTxn[]
else normal path
TxnMocker->>TxnMocker: gen_and_submit_txn (single)
TxnMocker->>AcctSvc: unlock_account
TxnMocker->>AcctSvc: sign_txn
AcctSvc-->>TxnMocker: SignedTxn
end
TxnMocker->>RpcClient: submit_transactions (or submit_transactions2)
RpcClient->>TxPool: import_txns
TxPool->>VM: execute
VM-->>TxPool: execution_result
TxPool-->>RpcClient: HashValue[]
RpcClient-->>TxnMocker: submitted
TxnMocker->>TxnMocker: recheck_sequence_number on error
TxnMocker->>TxnMocker: sleep(interval)
end
Note over Main: stopping_signal set → threads exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
jackzhhuang
force-pushed
the
vm2benchmark-bin
branch
from
538d37e to
942a3de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/src/txpool_config.rs (1)
18-19: Update outdated default value comments.The documentation comments reference old default values (4096 and 128) that no longer match the actual defaults returned by the methods (40960000 and 1280000).
Apply this diff to update the comments:
- /// Maximal number of transactions in the pool. default to 4096 + /// Maximal number of transactions in the pool. default to 40960000 max_count: Option<u64>,- /// Maximal number of transactions from single sender. default to 128 + /// Maximal number of transactions from single sender. default to 1280000 max_per_sender: Option<u64>,Also applies to: 23-24
chain/src/chain.rs (1)
2186-2238: Based on my analysis of the code and the script output, the critical issue raised in the review comment is valid and confirmed.Here's the evidence:
No state positioning before execution: The script output shows
execute_dag_blockis called at line 1399 with no preceding state repositioning code.Method doesn't reposition state: The code snippet (lines 2186-2238) shows:
- Line 2186 comment promises "Fork statedb from selected parent's state root"
- Lines 2148-2149 retrieve
parent_block_infobut never use it to position state- Lines 2199 and 2234 execute directly on
self.statedb.0andself.statedb.1State alignment issue: In DAG mode,
self.status.head(where statedb is positioned) differs fromselected_parent(block.parent_hash() at line 2147), causing execution on incorrect state.Validation requirement unmet: According to the retrieved learning, the execution should validate and fail if state is not correctly positioned—but there's no such validation.
Critical: State not positioned at selected parent before execution.
The comment at Line 2186 states "Fork statedb from selected parent's state root," but the code does not reposition
self.statedb.0orself.statedb.1to the selected parent's state roots before execution. Lines 2199 and 2234 execute transactions directly onself.statedb, which remains positioned atself.status.head—not atselected_parent(Line 2147). In DAG mode, these differ, causing execution on incorrect state.Add state positioning logic or add validation that fails if state is not correctly positioned before the execution at lines 2199 and 2234.
🧹 Nitpick comments (5)
config/src/lib.rs (1)
509-516: Consider fixing the grammar in the error message.The error message reads "should success" but should be "should succeed". This same issue exists in other similar functions in this file (e.g., line 506, 522, 530, 542).
Apply this diff to fix the grammar:
- Self::load_with_opt(&opt).expect("Auto generate test config should success.") + Self::load_with_opt(&opt).expect("Auto generate test config should succeed.")Note: You may want to fix the same grammatical error in the other helper functions as well.
chain/src/chain.rs (1)
2423-2426: Note the TODO: k parameter update frequency.The code updates the DAG k parameter on every block using
self.epoch.max_uncles_per_block(). The TODO at Line 2423 correctly notes that k should ideally reload only on new epochs rather than every block, though the current implementation is consistent with how epoch is maintained (updated at Line 2118 inconnect()).sync/Cargo.toml (1)
80-80: Use workspace dependency for plotters.The
plotters = "0.3"dependency uses a hardcoded version instead of the workspace pattern. For consistency with other dependencies in this file and across the project, consider defining plotters in the workspace dependencies and referencing it here with{ workspace = true }.Apply this change:
In the root
Cargo.toml, add to[workspace.dependencies]:+plotters = "0.3"Then in
sync/Cargo.toml:-plotters = "0.3" +plotters = { workspace = true }cmd/tx-factory/src/main.rs (1)
187-189: Redundant VM version validation.The
vm_versionis validated twice:
- Lines 187-189: Initial validation after parsing
- Lines 267-268: In the else branch after the conditional dispatch
The second check at lines 267-268 is unreachable because:
- The first check ensures
vm_versionis 1 or 2- The conditional at line 244 handles
vm_version == 1- The conditional at line 255 handles
vm_version == 2- There's no way to reach line 267
Remove the unreachable panic at lines 267-268:
} else if vm_version == 2 { start_vm2_pressure_test( tx_mocker, round_num, account_num, batch_size, interval, transfer_account_size, is_stress, stopping_signal, ) - } else { - panic!("vm version must be 1 or 2"); };Alternatively, refactor to use a match expression for clearer exhaustiveness checking:
- let handle = if vm_version == 1 { + let handle = match vm_version { + 1 => { start_vm1_pressure_test( tx_mocker, round_num, account_num, batch_size, interval, transfer_account_size, is_stress, stopping_signal, ) - } else if vm_version == 2 { + } + 2 => { start_vm2_pressure_test( tx_mocker, round_num, account_num, batch_size, interval, transfer_account_size, is_stress, stopping_signal, ) - } else { - panic!("vm version must be 1 or 2"); + } + _ => unreachable!("vm_version already validated"), };Also applies to: 267-268
vm2/account/api/src/service.rs (1)
194-208: Inconsistent error handling - returnsErrinstead of panic.The
sign_txn_in_batchimplementation returnsErr(anyhow::anyhow!(...))for unexpected response types (lines 204-206), while most other methods in this file usepanic!for the same scenario (e.g., lines 117, 127, 145, 172, 190, 222, 246).For consistency with other methods in this file, consider using
panic!instead:async fn sign_txn_in_batch( &self, raw_txn: Vec<RawUserTransaction>, ) -> Result<Vec<SignedUserTransaction>> { let response = self .send(AccountRequest::SignTxnInBatch { txns: raw_txn }) .await??; if let AccountResponse::SignedTxnList(txns) = response { Ok(txns) } else { - Err(anyhow::anyhow!( - "unexpected response type: expected SignedTxnList" - )) + panic!("Unexpected response type.") } }Note: While returning an error is more graceful, consistency within a single file is valuable for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(2 hunks)chain/src/chain.rs(5 hunks)cmd/tx-factory/src/lib.rs(1 hunks)cmd/tx-factory/src/main.rs(7 hunks)cmd/tx-factory/src/mocker.rs(1 hunks)cmd/tx-factory/src/pressure_vm1.rs(1 hunks)cmd/tx-factory/src/pressure_vm2.rs(1 hunks)cmd/tx-factory/src/txn_generator.rs(4 hunks)config/src/genesis_config/mod.rs(1 hunks)config/src/lib.rs(1 hunks)config/src/txpool_config.rs(2 hunks)kube/manifest/starcoin-halley.yaml(2 hunks)node/src/lib.rs(2 hunks)rpc/api/generated_rpc_schema/txpool.json(2 hunks)rpc/api/src/txpool/mod.rs(2 hunks)rpc/client/src/lib.rs(7 hunks)rpc/client/src/remote_state_reader2/mod.rs(2 hunks)rpc/server/Cargo.toml(1 hunks)rpc/server/src/module/debug_rpc.rs(2 hunks)rpc/server/src/module/txpool_rpc.rs(3 hunks)scripts/release.sh(1 hunks)sync/Cargo.toml(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)sync/starcoin-execute-bench/Cargo.toml(1 hunks)sync/starcoin-execute-bench/src/main.rs(1 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)test-helper/src/lib.rs(1 hunks)test-helper/src/node.rs(1 hunks)txpool/src/tx_pool_service_impl.rs(4 hunks)vm2/account/api/src/message.rs(3 hunks)vm2/account/api/src/service.rs(3 hunks)vm2/account/src/account_manager.rs(2 hunks)vm2/rpc/api/src/account_api.rs(2 hunks)vm2/rpc/server/src/account_rpc.rs(2 hunks)vm2/service/account/src/service.rs(1 hunks)vm2/vm-runtime/src/starcoin_vm.rs(1 hunks)vm2/vm-types/src/token/stc.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (22)
📓 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: 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.
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: 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-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-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-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.rsrpc/client/src/remote_state_reader2/mod.rschain/src/chain.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:
cmd/tx-factory/src/pressure_vm1.rsrpc/api/src/txpool/mod.rscmd/tx-factory/src/lib.rscmd/tx-factory/src/pressure_vm2.rstest-helper/src/lib.rsrpc/api/generated_rpc_schema/txpool.jsonsync/tests/full_build_and_execute_test.rscmd/tx-factory/src/txn_generator.rscmd/tx-factory/src/main.rsrpc/server/src/module/txpool_rpc.rsrpc/client/src/lib.rscmd/tx-factory/src/mocker.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:
cmd/tx-factory/src/pressure_vm1.rscmd/tx-factory/src/pressure_vm2.rstest-helper/src/lib.rssync/tests/full_build_and_execute_test.rssync/starcoin-execute-bench/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:
cmd/tx-factory/src/pressure_vm1.rscmd/tx-factory/src/pressure_vm2.rscmd/tx-factory/src/main.rssync/starcoin-execute-bench/Cargo.toml
📚 Learning: 2025-09-13T14:15:32.756Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/starcoin/src/account/sign_multisig_txn_cmd.rs:61-63
Timestamp: 2025-09-13T14:15:32.756Z
Learning: In Starcoin codebase PR #4655 (Clap v3 to v4 upgrade), maintainer welbon confirmed the preference for minimal configuration changes during the upgrade, specifically declining suggestions to add num_args or value_delimiter attributes to maintain CLI compatibility with existing behavior, even when it might mean less optimal UX compared to new Clap v4 features.
Applied to files:
kube/manifest/starcoin-halley.yamlsync/Cargo.toml
📚 Learning: 2025-09-13T14:13:05.713Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/resource-code-exporter/src/main.rs:42-44
Timestamp: 2025-09-13T14:13:05.713Z
Learning: In the Starcoin codebase Clap v3 to v4 upgrade (PR #4655), the maintainer welbon prefers minimal configuration changes to ensure compatibility, even if it means slight UX differences, rather than adding additional attributes like num_args or value_delimiter.
Applied to files:
kube/manifest/starcoin-halley.yaml
📚 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:
kube/manifest/starcoin-halley.yaml
📚 Learning: 2025-09-28T08:11:05.036Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: testsuite/features/cmd.feature:71-71
Timestamp: 2025-09-28T08:11:05.036Z
Learning: The sign-multisig-txn command in Starcoin automatically creates the output directory if it doesn't exist using std::fs::create_dir_all, so specifying non-existent paths with --output-dir is safe and will not cause failures.
Applied to files:
scripts/release.sh
📚 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:
rpc/client/src/remote_state_reader2/mod.rsrpc/client/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:
test-helper/src/lib.rsrpc/server/src/module/debug_rpc.rsnode/src/lib.rscmd/tx-factory/src/main.rsconfig/src/txpool_config.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:
rpc/server/src/module/debug_rpc.rsvm2/vm-types/src/token/stc.rssync/Cargo.tomlchain/src/chain.rssync/tests/full_build_and_execute_test.rscmd/tx-factory/src/main.rsvm2/vm-runtime/src/starcoin_vm.rssync/starcoin-execute-bench/Cargo.tomlrpc/server/Cargo.tomlrpc/client/src/lib.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
rpc/server/src/module/debug_rpc.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:
rpc/server/src/module/debug_rpc.rschain/src/chain.rsrpc/client/src/lib.rs
📚 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:
sync/Cargo.tomlrpc/server/Cargo.toml
📚 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:
sync/Cargo.tomlchain/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.rsvm2/vm-runtime/src/starcoin_vm.rsrpc/client/src/lib.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:
vm2/account/src/account_manager.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:
rpc/server/src/module/txpool_rpc.rstxpool/src/tx_pool_service_impl.rs
🧬 Code graph analysis (16)
rpc/api/src/txpool/mod.rs (4)
rpc/client/src/lib.rs (2)
submit_transactions2(364-370)next_sequence_number_in_batch2(339-349)rpc/server/src/module/txpool_rpc.rs (2)
submit_transactions2(126-135)next_sequence_number_in_batch2(261-267)cmd/tx-factory/src/mocker.rs (1)
next_sequence_number_in_batch2(681-706)txpool/src/pool/queue.rs (1)
addresses(558-571)
vm2/service/account/src/service.rs (1)
rpc/api/src/types.rs (2)
txns(851-856)txns(866-868)
rpc/client/src/remote_state_reader2/mod.rs (1)
vm2/vm-types/src/token/stc.rs (1)
stc_struct_tag(24-31)
test-helper/src/lib.rs (1)
test-helper/src/node.rs (3)
run_node_by_config(17-22)run_node_with_all_service(24-28)run_test_node(12-15)
vm2/rpc/api/src/account_api.rs (3)
vm2/account/api/src/service.rs (2)
sign_txn_in_batch(48-51)sign_txn_in_batch(194-208)vm2/account/src/account_manager.rs (1)
sign_txn_in_batch(374-396)vm2/rpc/server/src/account_rpc.rs (2)
sign_txn_in_batch(174-185)unlock_in_batch(207-223)
rpc/server/src/module/debug_rpc.rs (2)
vm2/vm-runtime/src/starcoin_vm.rs (2)
set_concurrency_level_once(1487-1492)get_concurrency_level(1496-1501)vm/vm-runtime/src/starcoin_vm.rs (2)
set_concurrency_level_once(1333-1338)get_concurrency_level(1342-1347)
vm2/rpc/server/src/account_rpc.rs (5)
vm2/rpc/api/src/account_api.rs (2)
sign_txn_in_batch(48-51)unlock_in_batch(64-68)vm2/account/api/src/service.rs (2)
sign_txn_in_batch(48-51)sign_txn_in_batch(194-208)vm2/account/src/account_manager.rs (1)
sign_txn_in_batch(374-396)rpc/client/src/lib.rs (1)
map_err(1323-1325)vm2/rpc/server/src/lib.rs (1)
map_err(6-29)
sync/tests/full_build_and_execute_test.rs (4)
chain/src/chain.rs (5)
std(805-805)std(2493-2493)current_header(1166-1168)e(242-242)e(2020-2020)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(16-19)create_account(112-119)node/src/lib.rs (18)
config(128-132)storage2(140-144)registry(120-122)registry(129-130)registry(135-136)registry(141-142)registry(147-148)registry(153-153)registry(157-158)registry(163-163)registry(167-167)registry(171-171)registry(190-191)registry(196-197)registry(203-203)registry(214-214)registry(215-215)txpool(189-193)
cmd/tx-factory/src/txn_generator.rs (5)
cmd/starcoin/src/cli_state.rs (1)
vm2(140-145)cmd/tx-factory/src/mocker.rs (2)
new(38-72)sequence_number(736-754)rpc/client/src/lib.rs (4)
new(136-141)new(167-203)new(1301-1320)chain_id(856-859)types/src/multi_transaction.rs (2)
sequence_number(155-160)sender(180-185)testsuite/tests/steps/transaction.rs (1)
transfer_txn(83-113)
cmd/tx-factory/src/main.rs (3)
cmd/tx-factory/src/pressure_vm1.rs (1)
start_vm1_pressure_test(11-59)cmd/tx-factory/src/pressure_vm2.rs (1)
start_vm2_pressure_test(13-61)rpc/client/src/lib.rs (4)
sleep(831-834)new(136-141)new(167-203)new(1301-1320)
vm2/account/src/account_manager.rs (3)
vm2/account/api/src/service.rs (4)
unlock_account_in_batch(59-63)unlock_account_in_batch(226-239)sign_txn_in_batch(48-51)sign_txn_in_batch(194-208)vm2/rpc/api/src/account_api.rs (1)
sign_txn_in_batch(48-51)vm2/rpc/server/src/account_rpc.rs (1)
sign_txn_in_batch(174-185)
vm2/account/api/src/service.rs (3)
vm2/rpc/api/src/account_api.rs (1)
sign_txn_in_batch(48-51)vm2/account/src/account_manager.rs (3)
sign_txn_in_batch(374-396)unlock_account(87-100)unlock_account_in_batch(102-119)vm2/rpc/server/src/account_rpc.rs (1)
sign_txn_in_batch(174-185)
rpc/server/src/module/txpool_rpc.rs (4)
rpc/server/src/module/mod.rs (1)
convert_to_rpc_error(69-72)types/src/multi_transaction.rs (1)
id(148-153)rpc/api/src/txpool/mod.rs (2)
submit_transactions2(29-32)next_sequence_number_in_batch2(86-89)rpc/client/src/lib.rs (2)
submit_transactions2(364-370)next_sequence_number_in_batch2(339-349)
rpc/client/src/lib.rs (3)
cmd/tx-factory/src/mocker.rs (1)
next_sequence_number_in_batch2(681-706)rpc/api/src/txpool/mod.rs (2)
next_sequence_number_in_batch2(86-89)submit_transactions2(29-32)rpc/server/src/module/txpool_rpc.rs (2)
next_sequence_number_in_batch2(261-267)submit_transactions2(126-135)
cmd/tx-factory/src/mocker.rs (4)
rpc/client/src/lib.rs (9)
sync_status(1185-1188)new(136-141)new(167-203)new(1301-1320)state_reader(606-611)node_info(255-258)account_list(411-414)next_sequence_number_in_batch2(339-349)next_sequence_number_in_batch(331-337)rpc/client/src/remote_state_reader2/mod.rs (1)
new(36-61)vm2/account/api/src/service.rs (2)
unlock_account(53-58)unlock_account(210-224)vm2/account/src/account_manager.rs (1)
unlock_account(87-100)
sync/starcoin-execute-bench/src/main.rs (3)
config/src/genesis_config/mod.rs (11)
genesis_config(176-185)genesis_config(501-503)from_str(105-115)from_str(260-268)from_str(297-302)new(241-246)new(454-466)fmt(90-99)fmt(235-237)fmt(279-285)fmt(442-444)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)node/src/lib.rs (3)
config(128-132)storage2(140-144)txpool(189-193)
⏰ 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 (38)
vm2/vm-runtime/src/starcoin_vm.rs (1)
322-331: Log message text label clarity improvement—LGTM.The warning message at line 324 now uses "max" to label the gas price upper-bound validation scenario, which correctly aligns with the check condition and error code (
GAS_UNIT_PRICE_ABOVE_MAX_BOUND). This improves consistency with the parallel min-bound check above (line 308-319) and makes the log output clearer for operators. No functional behavior is altered.config/src/genesis_config/mod.rs (1)
241-241: LGTM! API exposure looks good.Making the constructor public is a clean way to allow external creation of custom network configurations.
scripts/release.sh (1)
8-8: The code change is correct and no issues were found.The
starcoin_txfactorybinary is properly defined as a[[bin]]target incmd/tx-factory/Cargo.tomlwith the correct name, andcmd/tx-factoryis included as a workspace member in the rootCargo.toml. The added line follows the established pattern of copying release binaries to the artifacts directory and will execute successfully.kube/manifest/starcoin-halley.yaml (1)
14-14: Scaling nodes to support multi-pod benchmark infrastructure.Increasing replicas from 1 to 3 aligns with the PR's benchmarking and testing objectives for VM2 dual-execution. This enables parallel node execution for load testing and performance metrics collection.
config/src/txpool_config.rs (1)
11-11: Fallback memory limit increase looks reasonable.The 8x increase (128MB → 1GB) for the fallback memory limit is appropriate for modern systems and benchmarking scenarios.
sync/src/tasks/block_sync_task.rs (1)
355-357: Correct state positioning before block application with proper error handling.This change correctly positions the chain state at the block's parent hash before applying the block, which aligns with the DAG execution requirement that state must be initialized at the parent before execution. The implementation properly validates that the parent block exists (via
fork's internalhas_dag_blockcheck) and propagates errors appropriately.The edge case of a missing parent block is handled: if
parent_hashdoesn't exist,fork()validates this and returns an error, which is correctly propagated via the?operator.Performance is not a concern in this context:
apply_blockis called selectively (only whenblock_infoisNonein the match statement), not in a tight loop, so fork overhead is minimal and occurs only when the chain state requires repositioning.rpc/server/Cargo.toml (1)
79-79: LGTM!The dependency addition is correctly positioned within the VM2 dependencies section and follows the workspace pattern consistently.
vm2/vm-types/src/token/stc.rs (1)
24-34: LGTM!Good refactoring that extracts the StructTag construction into a reusable function. This enables other components to obtain the STC StructTag directly without wrapping it in a TypeTag, improving code modularity.
Cargo.toml (1)
52-52: LGTM!The new benchmark crate is properly registered in the workspace and included in both
membersanddefault-members, consistent with the project's workspace structure.txpool/src/tx_pool_service_impl.rs (1)
365-367: Verify the API design for exposing Inner methods.Four previously private methods in
Innerare now exposed as public APIs. While the AI summary indicates these support new VM2 batch RPC operations, exposing internal implementation details can break encapsulation and make future refactoring more difficult.Consider whether:
- These methods should remain internal with new public wrapper methods added to
TxPoolServiceinstead- The RPC layer could be restructured to avoid requiring direct access to
Inner- The current approach is acceptable as a pragmatic solution for VM2 integration
node/src/lib.rs (1)
140-144: LGTM!The new
storage2()accessor follows the same pattern as the existingstorage()method and properly integrates with the registry service, providing consistent access to the secondary storage layer.rpc/server/src/module/debug_rpc.rs (1)
16-16: Verify VM2 is the intended target for concurrency control.The import has been switched from
StarcoinVMtoStarcoinVM2, meaning the debug RPC methodsset_concurrency_levelandget_concurrency_levelnow exclusively control VM2's concurrency settings.Confirm whether:
- Controlling only VM2's concurrency level is intentional (VM1 uses separate settings or defaults)
- Both VM1 and VM2 should be controlled together by these debug endpoints
- This change aligns with the broader dual-VM execution strategy
sync/Cargo.toml (1)
61-81: LGTM!The new dev-dependencies appropriately support VM2 testing infrastructure, benchmarking, and RPC client interactions. All additions use the workspace pattern except for
plotters, which is addressed in a separate comment.cmd/tx-factory/src/lib.rs (1)
1-3: LGTM!The three new module declarations expose dual-VM pressure testing and transaction mocking capabilities, properly extending the tx-factory library's public API.
vm2/service/account/src/service.rs (1)
193-200: LGTM! Batch operation handlers are well-integrated.The batch signing and unlocking handlers follow the established pattern and correctly delegate to the manager's batch methods. The response types are properly wrapped in their respective variants.
test-helper/src/node.rs (1)
24-28: LGTM! Useful test helper for running node with all services.This function appropriately mirrors
run_node_by_configbut keeps the pacemaker running, which is useful for integration tests that need the full node operational.sync/starcoin-execute-bench/Cargo.toml (1)
1-37: LGTM! Well-structured benchmark crate manifest.The manifest correctly uses workspace-driven metadata and includes an appropriate external dependency (plotters) for benchmark visualization.
vm2/account/api/src/message.rs (1)
25-27: LGTM! Batch operation message variants are well-defined.The new enum variants for batch signing and unlocking operations follow the existing pattern and use appropriate types for batch processing.
Also applies to: 36-36, 67-67
test-helper/src/lib.rs (1)
24-24: LGTM! Clean public API extension.The re-export appropriately makes the new helper function available through the test-helper crate's public API.
vm2/rpc/api/src/account_api.rs (1)
47-51: LGTM! Batch RPC methods are properly defined.The batch signing and unlocking RPC methods follow the established API pattern with appropriate naming, documentation, and type signatures.
Also applies to: 62-68
rpc/api/generated_rpc_schema/txpool.json (1)
884-1179: LGTM! Generated schema additions for VM2 batch operations.The new RPC method schemas for
txpool.submit_transactions2andtxpool.next_sequence_number_in_batch2are consistent with the existing schema format and properly define the VM2 batch operation interfaces.Note: This appears to be a generated file. Ensure the code generation process is correctly configured to produce these schemas.
Also applies to: 2990-3036
rpc/api/src/txpool/mod.rs (2)
28-32: LGTM - Batch submission API for VM2.The new
submit_transactions2endpoint mirrors the VM1 batch submission pattern and correctly uses VM2 types (SignedUserTransaction2). The signature and return type are consistent with the existing batch submission methods.
83-89: LGTM - Batch sequence number API for VM2.The new
next_sequence_number_in_batch2endpoint follows the established pattern for batch sequence queries and correctly uses VM2 types (AccountAddress2). The nestedOption<Vec<...>>structure is consistent with the VM1 equivalent.cmd/tx-factory/src/pressure_vm1.rs (1)
11-59: LGTM - VM1 pressure test implementation.The function correctly:
- Spawns a background thread for pressure testing
- Uses
Arc<AtomicBool>for graceful shutdown signaling- Implements error recovery by rechecking sequence numbers on transaction rejection
- Sleeps between iterations to control pressure
The use of
expect()for account creation (line 24) is appropriate since account setup is a precondition for the test. The thread-based blocking approach withstd::thread::sleepaligns with the established pattern for test scenarios. Based on learnings.vm2/rpc/server/src/account_rpc.rs (2)
174-185: LGTM - Batch transaction signing.The implementation correctly delegates to the account service's
sign_txn_in_batchmethod and follows the established async pattern: clone service, spawn async block, await the service call, map errors, and box the future.
207-223: LGTM - Batch account unlocking.The implementation correctly delegates to the account service's
unlock_account_in_batchmethod. The duration defaults tou32::MAXseconds (~49 days) whenNoneis provided, which aligns with the preference for long unlock durations in the tx-factory tooling. Based on learnings.cmd/tx-factory/src/pressure_vm2.rs (1)
13-61: LGTM - VM2 pressure test implementation.The function mirrors the VM1 pressure test implementation with appropriate VM2-specific method calls:
get_or_create_accounts2instead ofget_or_create_accountsstress_test2instead ofstress_testgen_and_submit_txn2instead ofgen_and_submit_txnrecheck_sequence_number2instead ofrecheck_sequence_numberThe structure, error handling, and shutdown signaling are consistent with the VM1 version, providing a clean dual-VM testing approach.
vm2/account/src/account_manager.rs (2)
102-119: LGTM - Batch account unlocking.The implementation correctly:
- Iterates over the batch of (address, password) pairs
- Loads each account with the provided password
- Caches the password with the specified TTL
- Returns the list of
AccountInfoThe approach is straightforward and consistent with the single-account
unlock_accountmethod.
374-396: LGTM - Batch transaction signing with fail-fast behavior.The implementation correctly:
- Iterates over raw transactions
- Retrieves cached passwords for each signer
- Short-circuits with
AccountLockederror if any signer's password is unavailable- Signs each transaction and collects results
The fail-fast behavior (line 383) is appropriate for batch signing - if any account is locked, the entire batch fails immediately, ensuring consistency. The TODO comment (line 380) notes a future enhancement to verify signer consistency within the batch.
cmd/tx-factory/src/main.rs (3)
108-138: Clean VM2 account resolution.The
get_account_or_default2function correctly mirrors the VM1 version (get_account_or_default) for VM2 account handling. The polling loop ensures the default account is available before proceeding.
120-129: Commented balance check code.The commented-out balance checking code appears in both
get_account_or_default2(lines 120-129) andget_account_or_default(lines 152-161).Should this commented code be removed entirely, or is it intentionally kept for potential future use? If it's no longer needed, removing it would improve code clarity.
Also applies to: 152-161
187-189: VM version validation uses panic.Using
panic!for invalidvm_versionvalues is appropriate for command-line argument validation, as it represents a user input error that should fail fast before any processing begins.vm2/account/api/src/service.rs (3)
48-51: LGTM - Batch signing trait method.The trait method correctly defines
sign_txn_in_batchto accept a vector ofRawUserTransactionand return a future ofVec<SignedUserTransaction>.
59-63: LGTM - Batch unlocking trait method.The trait method correctly defines
unlock_account_in_batchto accept a batch of (address, password) pairs with a duration, returning a future ofVec<AccountInfo>.
226-239: LGTM - Batch unlocking implementation.The implementation correctly sends
AccountRequest::UnlockAccountInBatchand expects anAccountListresponse. The use ofpanic!for unexpected response types is consistent with other methods in this file.rpc/server/src/module/txpool_rpc.rs (3)
37-68: Well-structured batch submission helper.The
submit_transactions_multimethod correctly:
- Determines
bypass_vm1_limitby checking if all transactions are VM2 (lines 41-43)- Sets
local_peer_idappropriately based on VM version (lines 44-48)- Collects transaction IDs before submission (line 49)
- Calls the underlying
add_txns_multi_signedservice method (lines 50-57)- Maps per-transaction results to their corresponding IDs or RPC errors (lines 59-66)
This approach enables efficient batch processing with proper error handling for individual transactions within the batch.
126-135: LGTM - VM2 batch submission.The
submit_transactions2method correctly converts VM2 transactions to the multi-signed format and delegates to the sharedsubmit_transactions_multihelper. This provides a clean separation of concerns and code reuse.
261-267: LGTM - VM2 batch sequence number query.The
next_sequence_number_in_batch2method correctly delegates to the service'snext_sequence_number2_in_batchmethod and wraps the result in a boxed future. The implementation is straightforward and consistent with the VM1 equivalent.
| pub struct TxnMocker { | ||
| client: RpcClient, | ||
| generator: MockTxnGenerator, | ||
| account_address: AccountAddress, | ||
| account_address2: AccountAddress2, | ||
| account_password: String, | ||
| account_password2: String, | ||
| unlock_duration: Duration, | ||
|
|
||
| next_sequence_number: u64, | ||
| account_unlock_time: Option<Instant>, | ||
| watch_timeout: u32, | ||
| } |
There was a problem hiding this comment.
Track unlock TTLs per account.
account_unlock_time used to be safe when we only had one account. Now that VM1 and VM2 addresses coexist, unlocking account_address2 marks the timer, so the next call to unlock_account() skips the actual unlock for account_address; the subsequent sign fails and the whole flow aborts. We need separate timers per account.
@@
- account_unlock_time: Option<Instant>,
+ account_unlock_time_vm1: Option<Instant>,
+ account_unlock_time_vm2: Option<Instant>,
@@
- Ok(Self {
+ Ok(Self {
client,
generator,
account_address,
account_address2,
account_password,
account_password2,
unlock_duration,
- account_unlock_time: None,
+ account_unlock_time_vm1: None,
+ account_unlock_time_vm2: None,
next_sequence_number,
watch_timeout,
})Apply the same split inside unlock_account2()/unlock_account() so each function reads and updates its own field.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/tx-factory/src/mocker.rs around lines 23 to 35, the single
account_unlock_time field is shared between both accounts causing unlocks for
account_address2 to prevent unlocking account_address; add two separate
Option<Instant> fields (e.g. account1_unlock_time and account2_unlock_time or
account_unlock_time and account2_unlock_time) to the TxnMocker struct,
initialize them appropriately, and then modify unlock_account() and
unlock_account2() so each reads, updates and checks its own timer field (not the
shared one). Ensure any other methods that reference the old account_unlock_time
are updated to use the correct per-account field and preserve the same
unlock-duration logic.
| pub fn gen_and_submit_txn2(&mut self, blocking: bool) -> Result<HashValue> { | ||
| let expiration_timestamp = self.fetch_expiration_time(); | ||
| let raw_txn = self | ||
| .generator | ||
| .generate_mock_txn2(self.next_sequence_number, expiration_timestamp)?; | ||
| info!("prepare to sign txn, sender: {}", raw_txn.sender()); | ||
|
|
||
| self.unlock_account2()?; | ||
|
|
||
| let user_txn = match self.client.account_sign_txn2(raw_txn) { | ||
| Err(e) => { | ||
| // sign txn fail, we should unlock again | ||
| self.account_unlock_time = None; | ||
| return Err(e); | ||
| } | ||
| Ok(txn) => txn, | ||
| }; | ||
| info!( | ||
| "prepare to submit txn, sender:{},seq:{},id:{}", | ||
| user_txn.sender(), | ||
| user_txn.sequence_number(), | ||
| user_txn.id(), | ||
| ); | ||
| let txn_hash = user_txn.id(); | ||
| let result = self.client.submit_transaction2(user_txn); | ||
|
|
||
| // increase sequence number if added in pool. | ||
| if result.is_ok() { | ||
| self.next_sequence_number += 1; | ||
| } | ||
| if blocking { | ||
| self.client.watch_txn( | ||
| txn_hash, | ||
| Some(Duration::from_secs(self.watch_timeout as u64)), | ||
| )?; | ||
| } | ||
| result | ||
| } | ||
|
|
||
| pub fn gen_and_submit_txn(&mut self, blocking: bool) -> Result<HashValue> { | ||
| let expiration_timestamp = self.fetch_expiration_time(); | ||
| let raw_txn = self | ||
| .generator | ||
| .generate_mock_txn(self.next_sequence_number, expiration_timestamp)?; | ||
| info!("prepare to sign txn, sender: {}", raw_txn.sender()); | ||
|
|
||
| self.unlock_account()?; | ||
|
|
||
| let user_txn = match self.client.account_sign_txn(raw_txn) { | ||
| Err(e) => { | ||
| // sign txn fail, we should unlock again | ||
| self.account_unlock_time = None; | ||
| return Err(e); | ||
| } | ||
| Ok(txn) => txn, | ||
| }; | ||
| info!( | ||
| "prepare to submit txn, sender:{},seq:{},id:{}", | ||
| user_txn.sender(), | ||
| user_txn.sequence_number(), | ||
| user_txn.id(), | ||
| ); | ||
| let txn_hash = user_txn.id(); | ||
| let result = self.client.submit_transaction(user_txn); | ||
|
|
||
| // increase sequence number if added in pool. | ||
| if result.is_ok() { | ||
| self.next_sequence_number += 1; | ||
| } | ||
| if blocking { | ||
| self.client.watch_txn( | ||
| txn_hash, | ||
| Some(Duration::from_secs(self.watch_timeout as u64)), | ||
| )?; | ||
| } | ||
| result | ||
| } | ||
|
|
There was a problem hiding this comment.
Re-sync sequence numbers before generating VM1/VM2 txns.
next_sequence_number now starts from the VM2 account and both gen_and_submit_txn2 and gen_and_submit_txn reuse it directly. Once the other VM path runs, the counter points at the wrong account, so the next submission uses an out-of-date sequence and is rejected. Re-check against the chain or txpool right before building each transaction.
pub fn gen_and_submit_txn2(&mut self, blocking: bool) -> Result<HashValue> {
+ self.recheck_sequence_number2()?;
let expiration_timestamp = self.fetch_expiration_time();
@@
pub fn gen_and_submit_txn(&mut self, blocking: bool) -> Result<HashValue> {
+ self.recheck_sequence_number()?;
let expiration_timestamp = self.fetch_expiration_time();That keeps each path consistent even when they interleave.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/tx-factory/src/mocker.rs around lines 128-205, both gen_and_submit_txn2
and gen_and_submit_txn reuse self.next_sequence_number without re-synchronizing
with the chain/txpool, causing wrong sequence numbers when VM1/VM2 interleave;
before calling generate_mock_txn2/generate_mock_txn, query the authoritative
sequence for the sender (from chain or txpool via self.client), update
self.next_sequence_number to that value (or max(local,next) as appropriate),
then proceed to generate and sign the txn so each VM path always uses a fresh,
correct sequence number.
| } | ||
| pub fn max_count(&self) -> u64 { | ||
| self.max_count.unwrap_or(4096) | ||
| self.max_count.unwrap_or(40960000) |
There was a problem hiding this comment.
🧩 Analysis chain
Extreme default increases raise production safety concerns.
The 10,000x increases to transaction pool limits (4096 → 40,960,000 for max_count and 128 → 1,280,000 for max_per_sender) are concerning because they affect production default configuration, not just benchmark environments.
Potential issues:
- Memory consumption: 40M transactions could consume 40GB+ of memory (assuming ~1KB per transaction)
- DoS vulnerability: A single sender can now queue 1.28M transactions, enabling potential flood attacks
- Performance degradation: Operations over 40M transactions may become prohibitively slow
- Production impact: These defaults will apply to all nodes unless explicitly overridden
Given the PR title "Benchmark the block execution", are these increases intended only for benchmarking? If so, consider:
- Using a separate benchmark-specific config file
- Adding a feature flag like
benchmark-mode - Setting these via environment variables in benchmark scripts
- Documenting that operators must override these in production
Can you clarify:
- Are these defaults intended for production use or only for benchmarking?
- What is the expected memory footprint with these limits?
- Have you considered the security implications of allowing 1.28M transactions per sender?
Also applies to: 65-65
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the txpool_config.rs file to see current values and context
cat config/src/txpool_config.rs | head -80 | tail -30
Length of output: 1010
🏁 Script executed:
#!/bin/bash
# Search for any benchmark-specific config or feature flags
fd -e rs | xargs rg -l "benchmark|test_config|bench" --type=rs | head -20Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for max_count and max_per_sender definitions and usage
rg -n "max_count|max_per_sender" --type=rs -B 3 -A 3Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check if there's a separate benchmark config file
fd -e rs -e toml | xargs rg -l "benchmark.*config|config.*benchmark" --type=rs --type=tomlLength of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# Full context of txpool_config.rs to see defaults and comments
cat config/src/txpool_config.rs
Length of output: 4682
🏁 Script executed:
#!/bin/bash
# Search for benchmark configs or test configs without type flag
find . -type f \( -name "*benchmark*" -o -name "*test*config*" \) | grep -E "\.(rs|toml)$" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for where max_count and max_per_sender are referenced
rg "max_count|max_per_sender" -B 2 -A 2Length of output: 6342
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about memory limits
rg "40960000|1280000|transaction.*limit|memory.*limit" -B 2 -A 2Length of output: 241093
🏁 Script executed:
#!/bin/bash
# Check if there are any old or baseline values mentioned
rg "4096|128" config/src/txpool_config.rs -B 2 -A 2Length of output: 638
Critical: Doc comments do not match actual code defaults—values differ by 10,000x.
The doc comments claim "default to 4096" and "default to 128", but the actual code defaults are 40,960,000 and 1,280,000 respectively. This creates a dangerous mismatch where operators relying on documentation will be unaware of the extreme actual limits.
Immediate fixes required:
-
Update doc comments to reflect actual defaults (lines 18, 23): Change "default to 4096" → "default to 40960000" and "default to 128" → "default to 1280000"
-
Justify the 10,000x increase or revert: These extreme defaults will apply to all production nodes. Provide rationale in a comment or PR description explaining:
- Why 40M+ transactions per pool and 1.28M per sender are acceptable defaults
- Memory/performance/security implications considered
- Whether this is benchmarking-specific (if so, should use feature flags or separate config)
Without these fixes, this becomes a hidden breaking change affecting production deployments.
| pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { | ||
| let data = self.collect_executions(); | ||
| if data.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let min_ts = data | ||
| .iter() | ||
| .filter_map(|(t, _)| *t) | ||
| .fold(f64::INFINITY, f64::min); | ||
| let max_ts = data | ||
| .iter() | ||
| .filter_map(|(t, _)| *t) | ||
| .fold(f64::NEG_INFINITY, f64::max); | ||
|
|
||
| let max_latency = data | ||
| .iter() | ||
| .filter(|(_, d)| d.is_finite()) | ||
| .map(|(_, d)| *d) | ||
| .fold(0.0, f64::max) | ||
| .max(1.0); | ||
|
|
||
| let root = SVGBackend::new(file_path, (1600, 800)).into_drawing_area(); | ||
| root.fill(&WHITE)?; | ||
|
|
||
| let mut chart = ChartBuilder::on(&root) | ||
| .caption( | ||
| "Add → Execute Latency Timeline (included unexecuted transactions)", | ||
| ("sans-serif", 28), | ||
| ) | ||
| .margin(20) | ||
| .x_label_area_size(80) | ||
| .y_label_area_size(70) | ||
| .build_cartesian_2d(min_ts..(max_ts + 2000.0), 0f64..max_latency)?; | ||
|
|
||
| chart | ||
| .configure_mesh() | ||
| .x_desc("Execution Time (ms since start)") | ||
| .y_desc("Latency (ms)") | ||
| .axis_desc_style(("sans-serif", 20)) | ||
| .label_style(("sans-serif", 14)) | ||
| .draw()?; | ||
|
|
||
| let bar_width = 200.0; | ||
|
|
||
| chart.draw_series(data.iter().map(|(opt_t, delay)| { | ||
| let color = if delay.is_finite() { | ||
| RGBColor(50, 100, 220).filled() | ||
| } else { | ||
| RED.filled() | ||
| }; | ||
|
|
||
| if let Some(ts) = opt_t { | ||
| Rectangle::new( | ||
| [ | ||
| (ts - bar_width / 2.0, 0.0), | ||
| (ts + bar_width / 2.0, delay.min(max_latency)), | ||
| ], | ||
| color, | ||
| ) | ||
| } else { | ||
| Rectangle::new( | ||
| [(max_ts + 1000.0, 0.0), (max_ts + 1500.0, max_latency * 0.9)], | ||
| color, | ||
| ) | ||
| } | ||
| }))?; | ||
|
|
||
| root.present()?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Short‑circuit SVG export when no executed timestamps exist.
Same issue here: if every transaction stays in “Added/Rejected/Culled” (no execution timestamp), min_ts/max_ts remain ±∞ and Plotters panics while building the chart. Please guard for that case before creating the axes.
- if data.is_empty() {
- return Ok(());
- }
+ if data.is_empty() {
+ return Ok(());
+ }
+ if !data.iter().any(|(t, _)| t.is_some()) {
+ return Ok(());
+ }🤖 Prompt for AI Agents
In sync/starcoin-execute-bench/src/main.rs around lines 624 to 694, after
computing min_ts and max_ts guard the case where there are no executed
timestamps (min_ts is INFINITY or max_ts is NEG_INFINITY) and short‑circuit the
SVG export (return Ok(())) before creating the chart/axes; implement a simple
check like if !min_ts.is_finite() || !max_ts.is_finite() { return Ok(()); } so
Plotters is not given infinite bounds.
| pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { | ||
| let data = self.collect_executions(); | ||
| if data.is_empty() { | ||
| return std::result::Result::Ok(()); | ||
| } | ||
|
|
||
| let min_ts = data | ||
| .iter() | ||
| .filter_map(|(t, _)| *t) | ||
| .fold(f64::INFINITY, f64::min); | ||
| let max_ts = data | ||
| .iter() | ||
| .filter_map(|(t, _)| *t) | ||
| .fold(f64::NEG_INFINITY, f64::max); | ||
|
|
||
| let max_latency = data | ||
| .iter() | ||
| .filter(|(_, d)| d.is_finite()) | ||
| .map(|(_, d)| *d) | ||
| .fold(0.0, f64::max) | ||
| .max(1.0); | ||
|
|
||
| let root = SVGBackend::new(file_path, (1600, 800)).into_drawing_area(); | ||
| root.fill(&WHITE)?; | ||
|
|
||
| let mut chart = ChartBuilder::on(&root) | ||
| .caption( | ||
| "Add → Execute Latency Timeline (included unexecuted transactions)", | ||
| ("sans-serif", 28), | ||
| ) | ||
| .margin(20) | ||
| .x_label_area_size(80) | ||
| .y_label_area_size(70) | ||
| .build_cartesian_2d(min_ts..(max_ts + 2000.0), 0f64..max_latency)?; | ||
|
|
||
| chart | ||
| .configure_mesh() | ||
| .x_desc("Execution Time (ms since start)") | ||
| .y_desc("Latency (ms)") | ||
| .axis_desc_style(("sans-serif", 20)) | ||
| .label_style(("sans-serif", 14)) | ||
| .draw()?; | ||
|
|
||
| let bar_width = 200.0; | ||
|
|
||
| chart.draw_series(data.iter().map(|(opt_t, delay)| { | ||
| let color = if delay.is_finite() { | ||
| RGBColor(50, 100, 220).filled() | ||
| } else { | ||
| RED.filled() | ||
| }; | ||
|
|
||
| if let Some(ts) = opt_t { | ||
| Rectangle::new( | ||
| [ | ||
| (ts - bar_width / 2.0, 0.0), | ||
| (ts + bar_width / 2.0, delay.min(max_latency)), | ||
| ], | ||
| color, | ||
| ) | ||
| } else { | ||
| Rectangle::new( | ||
| [(max_ts + 1000.0, 0.0), (max_ts + 1500.0, max_latency * 0.9)], | ||
| color, | ||
| ) | ||
| } | ||
| }))?; | ||
|
|
||
| root.present()?; | ||
| std::result::Result::Ok(()) | ||
| } |
There was a problem hiding this comment.
Short‑circuit SVG export when no executed timestamps exist.
When every record in collect_executions has None for the timestamp (e.g., nothing ever executes), min_ts stays at f64::INFINITY and max_ts becomes f64::NEG_INFINITY. Passing that span into build_cartesian_2d triggers a panic, so the observer crashes exactly in the scenario we most want diagnostics. Please bail out before plotting when no finite timestamps are present.
- if data.is_empty() {
- return std::result::Result::Ok(());
- }
+ if data.is_empty() {
+ return std::result::Result::Ok(());
+ }
+ if !data.iter().any(|(t, _)| t.is_some()) {
+ return std::result::Result::Ok(());
+ }🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 483 to 553, the code
computes min_ts/max_ts from potentially all-None timestamps which yields
INFINITY/NEG_INFINITY and causes build_cartesian_2d to panic; add an early
return when there are no finite timestamps (e.g. check if min_ts.is_finite() and
max_ts.is_finite() or detect no Some timestamps via filter_map). If none are
present, return Ok(()) before creating the SVG/chart; keep the rest of the
export logic unchanged so the function only proceeds when a valid time span
exists.
942a3de to
6d2ae38
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
txpool/src/tx_pool_service_impl.rs (1)
397-409: Document this write operation carefully.This method imports transactions into the pool and is now publicly accessible. As a write operation with security-relevant parameters (
bypass_vm1_limit,peer_id), it requires comprehensive documentation explaining:
- The purpose and use cases
- The implications of the
bypass_vm1_limitflag- When and how
peer_idshould be provided- Error conditions and return value semantics
Add documentation:
+ /// Imports a batch of transactions into the pool. + /// + /// # Parameters + /// - `txns`: Transactions to import + /// - `bypass_vm1_limit`: If true, bypasses VM1 transaction limits + /// - `peer_id`: Optional peer identifier for tracking transaction source + /// + /// # Returns + /// A vector of results, one per transaction, indicating success or the specific error. pub fn import_txns(chain/src/chain.rs (1)
2200-2215: Fix apply_with_verifier to reposition self to parent before execute call.The state positioning requirement is broken.
verify_with_verifiercreates a temporaryBlockChainatparent_hashfor verification, then discards it. Whenapply_with_verifiercallsself.execute()on line 523,selfremains unpositioned at its original state, not at the parent's state root.
execute_dag_blockthen usesself.statedb.0andself.statedb.1directly (lines 2202, 2209, 2219-2227), assuming they are positioned at the selected parent's state roots—but they are not. Per the design constraint in learnings, this violates: "BlockChain state must be positioned at selected parent's state roots BEFORE execute_dag_block is called."Required fix:
apply_with_verifiermust repositionselfto the parent block before callingexecute. The temporary chain logic inverify_with_verifiershould be mirrored: ifself.head_block() != block.parent_hash(), create a new positionedBlockChainat parent and use that for both verification and execution, or repositionselfin-place before execution.
♻️ Duplicate comments (10)
kube/manifest/starcoin-halley.yaml (2)
26-26: **** Image SHA verification still required.The image reference
starcoin/starcoin:sha-718e01aremains flagged from prior review. A past reviewer could not locate this image in the registry and flagged it as a blocker. Confirm this SHA exists and is stable before merge, or update to a verified tag/digest.
32-33: **** Startup script complexity and robustness issues persist.The inline shell script remains unrefactored from prior review feedback. Previous analysis flagged: redundant cleanup (dead-code deletions), fragile pod-name parsing, unvalidated array indexing, missing error handling, and an unmaintainable command length.
A comprehensive refactor suggestion (moving to a ConfigMap-backed script with proper error handling and bounds checks) was provided in prior review. Please address those concerns before merge.
config/src/txpool_config.rs (2)
18-19: Documentation still does not match implementation.The doc comments claim "default to 4096" (line 18) and "default to 128" (line 23), but the actual code defaults are 40,960,000 (line 62) and 1,280,000 (line 65) — a 10,000x difference. This was flagged in the previous review as a critical issue requiring immediate correction.
Required fix:
- Line 18: Change
default to 4096→default to 40960000- Line 23: Change
default to 128→default to 1280000Also applies to: 23-24
62-62: Extreme defaults for benchmark purposes affect all production nodes.The 10,000x increases (4096 → 40,960,000 for
max_count; 128 → 1,280,000 formax_per_sender) were flagged in the previous review as critical production safety concerns. These defaults will apply to all nodes unless explicitly overridden.This architectural issue remains unresolved. Given the PR title "Benchmark the block execution", consider the previous review's suggestions:
- Separate benchmark-specific config file
- Feature flag like
benchmark-mode- Environment variables set in benchmark scripts only
- Clear documentation that production operators must override these
Can you clarify whether production deployments are expected to use these extreme defaults, or if this PR should implement a benchmark-specific configuration approach?
Also applies to: 65-65
rpc/client/src/remote_state_reader2/mod.rs (1)
67-94: The early return logic and equality check issues remain unresolved.The loop over resources still has the problems identified in the previous review:
Line 79: The exact equality check
if key.0 != stcwill fail to match STC resources becausestchas emptytype_argswhile stored struct tags include type arguments (e.g.,CoinStore<STC>).Lines 87-89: The else branch returns
Ok(None)immediately whentoken_code()isNone, exiting the loop without checking remaining resources. This contradicts the loop structure that suggests multiple resources might be present.These issues can cause the method to return
Noneprematurely even when a valid STC balance resource exists in the list.cmd/tx-factory/src/mocker.rs (3)
23-35: Shared unlock timer between VM1 and VM2 accounts causes unlock failures.The single
account_unlock_timefield (line 33) is shared between both accounts. Whenunlock_account2()unlocksaccount_address2and updates the timer, the subsequent call tounlock_account()sees a valid timestamp and skips unlockingaccount_address, causing the sign operation to fail.You need separate timers per account:
- account_unlock_time: Option<Instant>, + account_unlock_time_vm1: Option<Instant>, + account_unlock_time_vm2: Option<Instant>,And update the constructors and both unlock methods to use their respective fields.
128-204: Missing sequence number resync before transaction generation.Both
gen_and_submit_txn2()andgen_and_submit_txn()useself.next_sequence_numberdirectly without callingrecheck_sequence_number2()orrecheck_sequence_number()first.When VM1 and VM2 paths interleave, the locally cached sequence number becomes stale, causing transactions to use incorrect sequence numbers and be rejected by the txpool.
Add recheck calls at the start of each method:
pub fn gen_and_submit_txn2(&mut self, blocking: bool) -> Result<HashValue> { + self.recheck_sequence_number2()?; let expiration_timestamp = self.fetch_expiration_time();pub fn gen_and_submit_txn(&mut self, blocking: bool) -> Result<HashValue> { + self.recheck_sequence_number()?; let expiration_timestamp = self.fetch_expiration_time();
32-72: VM1 and VM2 paths share a single sequence number initialized from VM2.The single
next_sequence_numberfield (line 32) is initialized fromaccount_address2(VM2 account) in the constructor (lines 48-59), but bothgen_and_submit_txn()andgen_and_submit_txn2()increment and use this same counter.When the VM1 path (
gen_and_submit_txn()) uses this sequence number, it's using the sequence number from the VM2 account, causing transaction rejection.You need separate sequence number tracking per account, or always recheck from chain/txpool before each transaction generation.
sync/starcoin-execute-bench/src/main.rs (1)
624-637: Guard against all-Nonetimestamps before plotting to avoid plotters panics.If every record from
collect_executionshasNonefor the timestamp (e.g., transactions never reach Executed/ExecutedNotInMain),min_tsstays atf64::INFINITYandmax_tsbecomesf64::NEG_INFINITY. Passing that span intobuild_cartesian_2dcan panic inside plotters.Short-circuit the SVG export when there are no finite timestamps:
pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { let data = self.collect_executions(); if data.is_empty() { return Ok(()); } + if !data.iter().any(|(t, _)| t.is_some()) { + return Ok(()); + } let min_ts = data .iter() .filter_map(|(t, _)| *t) .fold(f64::INFINITY, f64::min); let max_ts = datasync/tests/full_build_and_execute_test.rs (1)
483-497: Guard against all-Nonetimestamps before plotting in the test observer as well.Same issue as in the bench binary: when every record has
Nonefor the timestamp,min_tsbecomesf64::INFINITYandmax_tsf64::NEG_INFINITY, which can cause plotters to panic inbuild_cartesian_2d.Add an early return when there are no concrete timestamps:
pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { let data = self.collect_executions(); if data.is_empty() { return std::result::Result::Ok(()); } + if !data.iter().any(|(t, _)| t.is_some()) { + return std::result::Result::Ok(()); + } let min_ts = data .iter() .filter_map(|(t, _)| *t)
🧹 Nitpick comments (10)
test-helper/src/node.rs (1)
24-28: Consider extracting common logic to reduce duplication.This function duplicates the logger initialization and node launch logic from
run_node_by_config. Consider refactoring to a shared helper function to improve maintainability.Apply this diff to reduce duplication:
+fn launch_node_with_logger(config: Arc<NodeConfig>) -> Result<NodeHandle> { + let logger_handle = starcoin_logger::init_for_test(); + NodeService::launch(config, logger_handle) +} + pub fn run_node_by_config(config: Arc<NodeConfig>) -> Result<NodeHandle> { - let logger_handle = starcoin_logger::init_for_test(); - let node_handle = NodeService::launch(config, logger_handle)?; + let node_handle = launch_node_with_logger(config)?; block_on(async { node_handle.node_service().stop_pacemaker().await })?; Ok(node_handle) } pub fn run_node_with_all_service(config: Arc<NodeConfig>) -> Result<NodeHandle> { - let logger_handle = starcoin_logger::init_for_test(); - let node_handle = NodeService::launch(config, logger_handle)?; - Ok(node_handle) + launch_node_with_logger(config) }txpool/src/tx_pool_service_impl.rs (3)
365-367: Add documentation for the newly public method.This method is now part of the public API, providing direct access to the internal transaction queue. Public methods should include documentation describing their purpose, usage patterns, and any important considerations.
Consider adding documentation like:
+ /// Returns a clone of the transaction queue. + /// + /// This provides direct access to the pool's internal queue structure. + /// Use with caution as it bypasses the service layer abstraction. pub fn queue(&self) -> Arc<TxnQueue> { self.queue.clone() }
420-439: Add documentation and consider removing commented code.This method is now public and provides access to pending transactions. The commented-out code (lines 425-433) appears to be outdated and should likely be removed to keep the codebase clean.
Add documentation and clean up:
+ /// Returns pending transactions eligible for block inclusion. + /// + /// # Parameters + /// - `max_len`: Maximum number of transactions to return + /// - `current_timestamp_secs`: Current timestamp for filtering time-locked transactions + /// + /// # Returns + /// Vector of verified pending transactions, up to `max_len`. pub fn get_pending( &self, max_len: u64, current_timestamp_secs: u64, ) -> Result<Vec<Arc<VerifiedTransaction>>> { - // let pending_settings = PendingSettings { - // block_number: u64::MAX, - // current_timestamp: current_timestamp_secs, - // max_len: max_len as usize, - // ordering: PendingOrdering::Priority, - // }; - // self.queue - // .inner_status(self.get_pool_client(), u64::MAX, current_timestamp_secs); - // self.queue.pending(self.get_pool_client(), pending_settings) Ok(self.get_pending_with_pool_client(
561-574: Document the public API for state access.This method creates a
PoolClientthat provides access to blockchain state. Since it's now public, document whatPoolClientis used for and any important considerations.Add documentation:
+ /// Creates a pool client for the current chain state. + /// + /// The returned `PoolClient` provides access to the blockchain state at the current + /// chain header, enabling transaction verification and state queries. + /// + /// # Errors + /// Returns an error if the VM multi-state cannot be retrieved for the current chain header. pub fn get_pool_client(&self) -> Result<PoolClient> {rpc/api/generated_rpc_schema/txpool.json (1)
2990-3036:txpool.next_sequence_number_in_batch2schema matches existing batch formThe VM2 batch sequence endpoint uses the same JSON shape as
next_sequence_number_in_batch(array of(AccountAddress, Option<u64>), wrapped in an outerOption), which is compatible with theVec<(AccountAddress2, Option<u64>)>Rust type alias. Only thename/titlestrings still mentionAccountAddress, which is a minor naming inconsistency but not a wire-format problem; adjust only if you want clearer generated docs.rpc/api/src/txpool/mod.rs (1)
83-99: Tidy up doc comments around new batch VM2 sequence APIsThe new
next_sequence_number_in_batch2method signature looks correct and aligns with server/client code. Two minor doc issues you may want to clean up:
- The comment for
next_sequence_number_in_batch2still says “given sender” (singular) even though it returns a batch for multiple addresses.- The stray doc comment on Line 91 (“or
Noneif there are no pending transactions from that sender in txpool.”) now precedesstate()and no longer matches that method’s behavior.Consider rephrasing the batch method comment (plural) and either moving or updating the comment above
state()so generated OpenRPC docs remain accurate.cmd/tx-factory/src/pressure_vm1.rs (1)
1-59: VM1 pressure loop logic is sound; only minor efficiency nitsThe thread-based pressure loop with
stopping_signaland periodicstd::thread::sleep(interval)is straightforward and safe for this tooling use-case, and the error-handling +recheck_sequence_numberrecovery path looks reasonable. One minor nit: in stress mode,accounts.clone()happens on every iteration; ifTxnMocker::stress_testdoesn’t need ownership, consider taking a slice (&[AccountAddress]) instead to avoid repeated cloning, but this is just an optimization for very large account sets.cmd/tx-factory/src/pressure_vm2.rs (1)
1-61: VM2 pressure-test entry mirrors VM1 correctly
start_vm2_pressure_testfaithfully mirrors the VM1 version, using the VM2-specific mocker methods (get_or_create_accounts2,stress_test2,gen_and_submit_txn2,recheck_sequence_number2) with the same stop and sleep behavior; this symmetry is good for comparing VM1 vs VM2 under load. If duplication becomes a maintenance burden later, you could factor the common loop into a generic helper parametrized over the VM-specific callbacks, but it’s fine as-is given the limited size.sync/starcoin-execute-bench/src/main.rs (1)
251-317: Optional: lengthen unlock duration for benchmark robustness.Each transaction build unlocks the sender for only 100 seconds. For long-running or higher-load benchmarks, this may expire mid-run and introduce noise. Given the preference for multi‑hour unlocks in
tx-factory, you might consider using a longer duration here as well, or unlocking once per account per phase instead of per-tx, to reduce overhead. Based on learningssync/tests/full_build_and_execute_test.rs (1)
42-166: Reduce duplication between test helpers/observer and the execute-bench crate.
create_account,default_account,build_transaction_to_send_token_to_account,get_current_header,get_balance,TransactionExecutionResult,ObserverService, and the SVG export logic are effectively duplicated here and insync/starcoin-execute-bench/src/main.rs.To keep behavior consistent and simplify future changes, consider moving these into a shared module (e.g., a small library in the execute-bench crate) and importing them into the test, rather than maintaining two copies.
Also applies to: 339-553
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.toml(2 hunks)chain/src/chain.rs(5 hunks)cmd/tx-factory/src/lib.rs(1 hunks)cmd/tx-factory/src/main.rs(7 hunks)cmd/tx-factory/src/mocker.rs(1 hunks)cmd/tx-factory/src/pressure_vm1.rs(1 hunks)cmd/tx-factory/src/pressure_vm2.rs(1 hunks)cmd/tx-factory/src/txn_generator.rs(4 hunks)config/src/genesis_config/mod.rs(1 hunks)config/src/lib.rs(1 hunks)config/src/txpool_config.rs(2 hunks)kube/manifest/starcoin-halley.yaml(2 hunks)node/src/lib.rs(2 hunks)rpc/api/generated_rpc_schema/txpool.json(2 hunks)rpc/api/src/txpool/mod.rs(2 hunks)rpc/client/src/lib.rs(7 hunks)rpc/client/src/remote_state_reader2/mod.rs(2 hunks)rpc/server/Cargo.toml(1 hunks)rpc/server/src/module/debug_rpc.rs(2 hunks)rpc/server/src/module/txpool_rpc.rs(3 hunks)scripts/release.sh(1 hunks)sync/Cargo.toml(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)sync/starcoin-execute-bench/Cargo.toml(1 hunks)sync/starcoin-execute-bench/src/main.rs(1 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)test-helper/src/lib.rs(1 hunks)test-helper/src/node.rs(1 hunks)txpool/src/tx_pool_service_impl.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- sync/src/tasks/block_sync_task.rs
- Cargo.toml
- config/src/lib.rs
- rpc/server/src/module/debug_rpc.rs
- config/src/genesis_config/mod.rs
- sync/starcoin-execute-bench/Cargo.toml
- scripts/release.sh
- cmd/tx-factory/src/lib.rs
- node/src/lib.rs
- test-helper/src/lib.rs
🧰 Additional context used
🧠 Learnings (18)
📓 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: 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.
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: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:
rpc/api/generated_rpc_schema/txpool.jsoncmd/tx-factory/src/main.rscmd/tx-factory/src/pressure_vm1.rscmd/tx-factory/src/pressure_vm2.rsrpc/api/src/txpool/mod.rssync/tests/full_build_and_execute_test.rscmd/tx-factory/src/txn_generator.rsrpc/server/src/module/txpool_rpc.rsrpc/client/src/lib.rscmd/tx-factory/src/mocker.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:
cmd/tx-factory/src/main.rssync/tests/full_build_and_execute_test.rschain/src/chain.rsrpc/server/Cargo.tomlrpc/client/src/lib.rssync/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:
cmd/tx-factory/src/main.rscmd/tx-factory/src/pressure_vm1.rscmd/tx-factory/src/pressure_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:
cmd/tx-factory/src/main.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:
cmd/tx-factory/src/pressure_vm1.rscmd/tx-factory/src/pressure_vm2.rssync/tests/full_build_and_execute_test.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:
rpc/client/src/remote_state_reader2/mod.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:
rpc/client/src/remote_state_reader2/mod.rsrpc/client/src/lib.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:
txpool/src/tx_pool_service_impl.rsrpc/server/src/module/txpool_rpc.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:
kube/manifest/starcoin-halley.yaml
📚 Learning: 2025-09-13T14:15:32.756Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/starcoin/src/account/sign_multisig_txn_cmd.rs:61-63
Timestamp: 2025-09-13T14:15:32.756Z
Learning: In Starcoin codebase PR #4655 (Clap v3 to v4 upgrade), maintainer welbon confirmed the preference for minimal configuration changes during the upgrade, specifically declining suggestions to add num_args or value_delimiter attributes to maintain CLI compatibility with existing behavior, even when it might mean less optimal UX compared to new Clap v4 features.
Applied to files:
kube/manifest/starcoin-halley.yamlsync/Cargo.toml
📚 Learning: 2025-09-13T14:13:05.713Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/resource-code-exporter/src/main.rs:42-44
Timestamp: 2025-09-13T14:13:05.713Z
Learning: In the Starcoin codebase Clap v3 to v4 upgrade (PR #4655), the maintainer welbon prefers minimal configuration changes to ensure compatibility, even if it means slight UX differences, rather than adding additional attributes like num_args or value_delimiter.
Applied to files:
kube/manifest/starcoin-halley.yamlsync/Cargo.toml
📚 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-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.rssync/Cargo.toml
📚 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.rsrpc/client/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/chain.rsrpc/client/src/lib.rs
📚 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:
rpc/server/Cargo.tomlsync/Cargo.toml
📚 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:
cmd/tx-factory/src/mocker.rs
🧬 Code graph analysis (9)
cmd/tx-factory/src/main.rs (3)
cmd/tx-factory/src/pressure_vm1.rs (1)
start_vm1_pressure_test(11-59)cmd/tx-factory/src/pressure_vm2.rs (1)
start_vm2_pressure_test(13-61)rpc/client/src/lib.rs (4)
sleep(831-834)new(136-141)new(167-203)new(1301-1320)
rpc/api/src/txpool/mod.rs (4)
rpc/client/src/lib.rs (2)
submit_transactions2(364-370)next_sequence_number_in_batch2(339-349)rpc/server/src/module/txpool_rpc.rs (2)
submit_transactions2(126-135)next_sequence_number_in_batch2(261-267)cmd/tx-factory/src/mocker.rs (1)
next_sequence_number_in_batch2(681-706)txpool/src/pool/queue.rs (1)
addresses(558-571)
rpc/client/src/remote_state_reader2/mod.rs (1)
vm2/vm-types/src/token/stc.rs (1)
stc_struct_tag(24-31)
sync/tests/full_build_and_execute_test.rs (5)
test-helper/src/node.rs (1)
run_node_with_all_service(24-28)node/src/lib.rs (18)
config(128-132)storage2(140-144)registry(120-122)registry(129-130)registry(135-136)registry(141-142)registry(147-148)registry(153-153)registry(157-158)registry(163-163)registry(167-167)registry(171-171)registry(190-191)registry(196-197)registry(203-203)registry(214-214)registry(215-215)txpool(189-193)txpool/src/tx_pool_service_impl.rs (1)
new(41-83)vm2/vm-runtime/src/starcoin_vm.rs (3)
new(87-110)new(113-136)set_concurrency_level_once(1487-1492)vm2/service/account/src/service.rs (6)
registry(222-222)create(94-100)ctx(65-66)ctx(95-95)ctx(96-96)started(46-90)
cmd/tx-factory/src/txn_generator.rs (2)
cmd/starcoin/src/cli_state.rs (1)
vm2(140-145)cmd/tx-factory/src/mocker.rs (2)
new(38-72)sequence_number(736-754)
rpc/server/src/module/txpool_rpc.rs (4)
txpool/src/pool/queue.rs (2)
ready(479-495)addresses(558-571)rpc/server/src/module/mod.rs (1)
convert_to_rpc_error(69-72)types/src/multi_transaction.rs (1)
id(148-153)rpc/api/src/txpool/mod.rs (2)
submit_transactions2(29-32)next_sequence_number_in_batch2(86-89)
rpc/client/src/lib.rs (3)
cmd/tx-factory/src/mocker.rs (1)
next_sequence_number_in_batch2(681-706)rpc/api/src/txpool/mod.rs (2)
next_sequence_number_in_batch2(86-89)submit_transactions2(29-32)rpc/server/src/module/txpool_rpc.rs (2)
next_sequence_number_in_batch2(261-267)submit_transactions2(126-135)
cmd/tx-factory/src/mocker.rs (3)
rpc/client/src/lib.rs (8)
new(136-141)new(167-203)new(1301-1320)state_reader(606-611)node_info(255-258)account_list(411-414)next_sequence_number_in_batch2(339-349)next_sequence_number_in_batch(331-337)rpc/client/src/remote_state_reader2/mod.rs (1)
new(36-61)vm2/account/api/src/service.rs (2)
unlock_account(53-58)unlock_account(210-224)
sync/starcoin-execute-bench/src/main.rs (3)
config/src/genesis_config/mod.rs (11)
genesis_config(176-185)genesis_config(501-503)from_str(105-115)from_str(260-268)from_str(297-302)new(241-246)new(454-466)fmt(90-99)fmt(235-237)fmt(279-285)fmt(442-444)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)node/src/lib.rs (3)
config(128-132)storage2(140-144)txpool(189-193)
🔇 Additional comments (15)
kube/manifest/starcoin-halley.yaml (2)
14-14: Good alignment: replica count matches miner-thread configuration.The increase to 3 replicas coordinates well with the miner-thread selection logic in the startup script, which explicitly assigns thread counts (1, 2, 3) to pods (starcoin-0, starcoin-1, starcoin-2). This shows intentional scaling design.
50-57: Verify rationale for removing readiness probe.The readiness probe has been removed while the liveness probe is retained. In Kubernetes, readiness affects pod availability for traffic routing, while liveness drives restarts. Confirm:
- Is this intentional for a benchmark/testing environment only?
- Should readiness probe be retained to ensure pods are marked available before receiving traffic?
- Does the startup script complete initialization quickly enough that missing readiness checks is acceptable?
rpc/server/Cargo.toml (1)
79-79: VM2 runtime dependency wiring looks consistentAdding
starcoin-vm2-vm-runtimenext to the existing VM1 runtime aligns with the new VM2 txpool RPC paths; no issues from this manifest change alone.sync/Cargo.toml (1)
62-83: New VM2 and tooling dev-dependencies are reasonableThe added dev-dependencies (VM2 account/service/statedb/runtime, rpc client, chrono/plotters/tempfile, etc.) are scoped to tests/bench tooling and shouldn’t affect the sync crate’s runtime behavior; manifest wiring looks fine.
rpc/api/generated_rpc_schema/txpool.json (1)
884-1179:txpool.submit_transactions2schema mirrors existing batch endpoint correctlyThe new
txpool.submit_transactions2method reuses the existingSignedUserTransactionarray schema andVec<HashValue>result shape, matching the TxPoolApi trait and server/client implementations; no schema-level issues spotted.rpc/api/src/txpool/mod.rs (1)
28-32: VM2 batch submit API is consistent with existing TxPool methods
submit_transactions2cleanly parallels the VM1submit_transactionsand the existingsubmit_transaction2single-call variant; signature matches the server/client implementations and return type is consistent (FutureResult<Vec<HashValue>>).rpc/server/src/module/txpool_rpc.rs (3)
37-68: LGTM! Correct batch transaction submission logic.The method properly:
- Determines bypass logic by checking if all transactions are VM2
- Collects transaction IDs upfront
- Maps per-transaction results to their IDs or errors
126-135: LGTM! Proper delegation to batch handler.Correctly wraps VM2 transactions and delegates to the multi-transaction handler.
261-267: LGTM! Correct delegation pattern.Properly delegates to the service layer for VM2 batch sequence number queries.
cmd/tx-factory/src/txn_generator.rs (1)
44-82: Verify the inconsistent gas limits between VM1 and VM2 methods.The gas limits differ significantly between VM1 and VM2 transaction generation methods:
generate_mock_txn(VM1): 40,000,000 (line 77)generate_mock_txn2(VM2): 4,000,000 (line 57)Similarly inconsistent for transfer and batch transactions. Is this intentional due to differences in VM1/VM2 gas consumption, or should they be aligned?
rpc/client/src/lib.rs (2)
315-370: LGTM! VM2 RPC client methods correctly implemented.The new VM2 methods properly delegate to the appropriate client layers and follow the same patterns as their VM1 counterparts.
461-520: LGTM! VM2 batch operations correctly implemented.The batch signing and unlocking methods for VM2 accounts correctly delegate to the VM2 account client.
chain/src/chain.rs (3)
2217-2227: LGTM! In-place write set application is correct.The write set application to
self.statedb.0in-place (with immediate commits after each write set) is acceptable. Based on learnings, committing state at intermediate points is fine because if any subsequent operation in the block fails, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level.Based on learnings.
2229-2241: LGTM! VM2 in-place execution and gas budgeting are correct.The VM2 execution using
&self.statedb.1(line 2237) and gas budgeting fromself.epoch.block_gas_limit() - vm1_gas_used(line 2239) are correct. This maintains consistency with the VM1 in-place execution pattern and properly allocates the remaining gas budget to VM2 after VM1 consumption.
2389-2393: LGTM! In-place state flushing and epoch usage are correct.The in-place flushing of both VM states (lines 2391-2392) correctly replaces the previous forked-statedb flushing pattern. As noted in the comment, this is critical for dual-VM support to ensure both VM1 and VM2 state tree nodes are persisted before the block info is created.
The epoch usage at line 2429 (
self.epoch.max_uncles_per_block()) is consistent with the in-place pattern established throughout the method.Also applies to: 2427-2429
| use std::sync::Arc; | ||
|
|
||
| pub const DEFAULT_MEM_SIZE: u64 = 128 * 1024 * 1024; // 128M | ||
| pub const DEFAULT_MEM_SIZE: u64 = 1024 * 1024 * 1024; // 1G |
There was a problem hiding this comment.
8x memory default increase compounds benchmark-vs-production risk.
DEFAULT_MEM_SIZE increased from 128 MiB to 1 GiB. While the comment is correctly updated, this 8x increase is used as a fallback in max_mem_usage() (lines 74, 77) and compounds the memory concerns from the 10,000x transaction limit increases.
Combined effect: 40M transactions at ~1KB each + 1 GiB fallback memory could require 40+ GiB for the pool, which is excessive for production defaults.
🤖 Prompt for AI Agents
In config/src/txpool_config.rs around line 11 (and referring to max_mem_usage at
lines ~74 and ~77), DEFAULT_MEM_SIZE was raised to 1 GiB causing an unsafe
production fallback that, combined with increased tx limits, can require
excessive RAM; revert DEFAULT_MEM_SIZE to a conservative value (e.g., 128 * 1024
* 1024) or make it configurable via env/config so production defaults remain
low, and update max_mem_usage to use a sane computed limit or explicit
config-provided value instead of the 1 GiB constant; also adjust the inline
comment to reflect the safer default or the fact it’s overridden by config.
| async fn run_benchmark( | ||
| node_config: Arc<NodeConfig>, | ||
| account_count: u32, | ||
| initial_balance: u128, | ||
| initial_gas_fee: u128, |
There was a problem hiding this comment.
Handle odd account_count before splitting receivers in the second phase.
In run_benchmark, the second transfer phase computes mid = receivers.len() / 2 and then calls build_transaction_to_send_token_to_account(&receivers[..mid], &receivers[mid..], ...). If account_count is odd, these slices differ in length and the helper will bail with sender.len() != receiver.len(), causing the benchmark to fail unexpectedly.
Consider enforcing an even account count (or gracefully handling the leftover) before splitting:
@@
- StarcoinVM::set_concurrency_level_once(num_cpus::get());
-
- let mid = receivers.len() / 2;
+ StarcoinVM::set_concurrency_level_once(num_cpus::get());
+
+ if receivers.len() % 2 != 0 {
+ bail!(
+ "account-count must be even for second-phase transfers (got {})",
+ receivers.len()
+ );
+ }
+ let mid = receivers.len() / 2;
let signed_transactions = build_transaction_to_send_token_to_account(
&receivers[..mid],
&receivers[mid..],
1,Also applies to: 437-448
🤖 Prompt for AI Agents
In sync/starcoin-execute-bench/src/main.rs around lines 344-348 (and similarly
437-448), the second transfer phase splits receivers with mid = receivers.len()
/ 2 and passes uneven halves to build_transaction_to_send_token_to_account,
which expects equal-length sender/receiver slices and will panic for odd
account_count; before splitting, ensure receivers length is even — either
enforce account_count to be even (return an error or adjust account_count down
by 1) or remove/skip the leftover element (e.g., pop the last receiver) so both
halves have equal length; implement the chosen approach and add a small comment
and a unit/logging check to make the behavior explicit.
| async fn create_account( | ||
| accoun_number: u32, | ||
| account_service: ServiceRef<AccountService2>, | ||
| ) -> Result<Vec<AccountInfo>> { | ||
| let mut results = vec![]; | ||
| for _ in 0..accoun_number { | ||
| let receiver = match account_service | ||
| .send(AccountRequest::CreateAccount("".to_string())) | ||
| .await?? | ||
| { | ||
| AccountResponse::AccountInfo(account) => *account, | ||
| _ => bail!("Unexpect response type."), | ||
| }; | ||
| results.push(receiver); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix minor spelling issues in parameter names and messages.
There are a few small typos that will show up in error/log output and make the code slightly harder to read:
accoun_number→account_numberincreate_accountand its loop."Unexpect response type."→"Unexpected response type."in the variousbail!calls."reuslts"→"results"in the log inActorService::stopped.
Example fix for create_account:
-async fn create_account(
- accoun_number: u32,
+async fn create_account(
+ account_number: u32,
@@
- for _ in 0..accoun_number {
+ for _ in 0..account_number {And adjust the string literals accordingly.
Also applies to: 69-70, 125-126, 575-577
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 42-57 (and also update
occurrences at 69-70, 125-126, and 575-577), fix small spelling issues: rename
the function parameter and local loop variable accoun_number to account_number
everywhere, change the error string "Unexpect response type." to "Unexpected
response type." in all bail! (or similar) calls, and correct the log/message
"reuslts" to "results" (e.g., in ActorService::stopped). Make sure to update all
corresponding usages and tests so identifiers and string literals remain
consistent.
| pub fn run_node_with_all_service(config: Arc<NodeConfig>) -> Result<NodeHandle> { | ||
| let logger_handle = starcoin_logger::init_for_test(); | ||
| let node_handle = NodeService::launch(config, logger_handle)?; | ||
| Ok(node_handle) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation to explain the purpose and usage.
This new test helper function lacks documentation explaining:
- Its purpose (launches node with pacemaker running)
- How it differs from
run_node_by_config(doesn't stop pacemaker) - When to use this function vs
run_node_by_config
Without documentation, test authors won't understand when to use which function.
Apply this diff to add documentation:
+/// Launches a test node with all services running, including the pacemaker.
+///
+/// Unlike `run_node_by_config`, this function does not stop the pacemaker after launch,
+/// allowing the node to operate with its full service stack. Use this when your test
+/// requires the pacemaker to remain active.
+///
+/// # Arguments
+/// * `config` - The node configuration to use
+///
+/// # Returns
+/// A `NodeHandle` that can be used to interact with the running node
pub fn run_node_with_all_service(config: Arc<NodeConfig>) -> Result<NodeHandle> {
let logger_handle = starcoin_logger::init_for_test();
let node_handle = NodeService::launch(config, logger_handle)?;
Ok(node_handle)
}📝 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.
| pub fn run_node_with_all_service(config: Arc<NodeConfig>) -> Result<NodeHandle> { | |
| let logger_handle = starcoin_logger::init_for_test(); | |
| let node_handle = NodeService::launch(config, logger_handle)?; | |
| Ok(node_handle) | |
| } | |
| /// Launches a test node with all services running, including the pacemaker. | |
| /// | |
| /// Unlike `run_node_by_config`, this function does not stop the pacemaker after launch, | |
| /// allowing the node to operate with its full service stack. Use this when your test | |
| /// requires the pacemaker to remain active. | |
| /// | |
| /// # Arguments | |
| /// * `config` - The node configuration to use | |
| /// | |
| /// # Returns | |
| /// A `NodeHandle` that can be used to interact with the running node | |
| pub fn run_node_with_all_service(config: Arc<NodeConfig>) -> Result<NodeHandle> { | |
| let logger_handle = starcoin_logger::init_for_test(); | |
| let node_handle = NodeService::launch(config, logger_handle)?; | |
| Ok(node_handle) | |
| } |
🤖 Prompt for AI Agents
In test-helper/src/node.rs around lines 24 to 28, the function
run_node_with_all_service lacks doc comments; add a Rust doc comment (///) block
above the function that briefly describes its purpose (launches a NodeService
with the pacemaker running), clarifies how it differs from run_node_by_config
(it leaves the pacemaker running and therefore does not stop it), and states
when to use this helper versus run_node_by_config (use when tests need the
pacemaker and full services running; use run_node_by_config for lightweight node
setups or when you want pacemaker stopped). Keep the doc concise (one short
summary line, one line about the difference, and one line about usage/example).
2, cache the vm2 config from shengguo lu 3, add batch operations for the transaction interface
add batch interface for transaction
2, bench testing will be ignored
2, cache the vm2 config from shengguo lu 3, add batch operations for the transaction interface
add batch interface for transaction
2, bench testing will be ignored
jackzhhuang
force-pushed
the
vm2benchmark-bin
branch
from
6d2ae38 to
47803f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
test-helper/src/node.rs (1)
24-28: Documentation still missing for this function.The function lacks documentation explaining its purpose, how it differs from
run_node_by_config(keeps pacemaker running), and when to use it. This was already flagged in a previous review.sync/tests/full_build_and_execute_test.rs (2)
42-57: Clean up helper parameter name and log/error strings.Minor but user-facing issues:
- Rename
accoun_numbertoaccount_numberincreate_accountand its loop.- Change
"Unexpect response type."to"Unexpected response type."in the variousbail!branches (here and similar matches below, e.g., Lines 69, 125, 135, 151).- Fix the log message
"failed to dump the reuslts"inActorService::stoppedto"failed to dump the results".These improve readability and log quality without changing behavior.
427-553: Guard SVG export when there are no executed timestamps.When every transaction only has Added/Rejected/Culled events,
collect_executionsreturns only(None, f64::INFINITY)entries.min_ts/max_tsare then folded over an empty iterator and remain±∞, which can causebuild_cartesian_2dto panic when constructing the X axis.Short-circuit the export if there are no
Sometimestamps before computingmin_ts/max_ts:pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { let data = self.collect_executions(); if data.is_empty() { return std::result::Result::Ok(()); } + if !data.iter().any(|(t, _)| t.is_some()) { + return std::result::Result::Ok(()); + }This keeps the rest of the plotting logic unchanged while avoiding panics in the “nothing executed” scenario.
sync/starcoin-execute-bench/src/main.rs (2)
344-349: Handle oddaccount_countbefore splitting receivers in phase two.In
run_benchmark, the second transfer phase does:let mid = receivers.len() / 2; build_transaction_to_send_token_to_account(&receivers[..mid], &receivers[mid..], ...)
build_transaction_to_send_token_to_accountrequiressender.len() == receiver.len(). Ifaccount_countis odd, these slices differ in length and the helper will bail with a genericsender.len() != receiver.len()error.Consider enforcing an even receiver count before splitting:
- StarcoinVM::set_concurrency_level(num_cpus::get()); - - let mid = receivers.len() / 2; + StarcoinVM::set_concurrency_level(num_cpus::get()); + + if receivers.len() % 2 != 0 { + bail!( + "account-count must be even for second-phase transfers (got {})", + receivers.len() + ); + } + let mid = receivers.len() / 2;This fails fast with a clear message instead of a later, opaque bail from the helper.
Also applies to: 437-448
624-694: Short‑circuit SVG export when no executions have timestamps.As in the test helper, if every entry from
collect_executionshasNonefor the timestamp (i.e., never executed),min_tsstaysf64::INFINITYandmax_tsstaysf64::NEG_INFINITY, andbuild_cartesian_2dcan panic on that invalid range.Add a guard to return early when there are no
Sometimestamps:pub fn export_latency_timeline_svg(&self, file_path: &str) -> Result<(), Box<dyn Error>> { let data = self.collect_executions(); if data.is_empty() { return Ok(()); } + if !data.iter().any(|(t, _)| t.is_some()) { + return Ok(()); + }This keeps the plotting logic intact while avoiding panics in the “no executed txns” case.
cmd/tx-factory/src/mocker.rs (2)
32-35: next_sequence_number is shared across VM1/VM2 and initialized from VM2 only — leads to wrong sequences.
TxnMocker::newcomputesnext_sequence_numbersolely fromaccount_address2(VM2), but bothgen_and_submit_txn2andgen_and_submit_txnread and mutate this single field without callingrecheck_sequence_number2()/recheck_sequence_number()first. That has two effects:
- A pure VM1 flow that calls
gen_and_submit_txnfirst will use a VM2-derivednext_sequence_number, so the very first VM1 txn can have a bogus sequence.- If VM1 and VM2 paths interleave, whichever
recheck_*ran last overwrites the shared field; subsequent txs for the other VM use out‑of‑date sequences and are likely to be rejected.You already have
recheck_sequence_number*helpers; the simplest fix is to refresh before building each tx and decouple VM1/VM2 sequencing. For example:pub fn gen_and_submit_txn2(&mut self, blocking: bool) -> Result<HashValue> { - let expiration_timestamp = self.fetch_expiration_time(); + self.recheck_sequence_number2()?; + let expiration_timestamp = self.fetch_expiration_time(); @@ pub fn gen_and_submit_txn(&mut self, blocking: bool) -> Result<HashValue> { - let expiration_timestamp = self.fetch_expiration_time(); + self.recheck_sequence_number()?; + let expiration_timestamp = self.fetch_expiration_time();For full isolation, consider separate fields like
next_sequence_number_vm1andnext_sequence_number_vm2, each maintained by its own recheck helper and used only on the corresponding path.Also applies to: 38-72, 89-126, 128-165, 167-204
23-35: Sharedaccount_unlock_timefor VM1 and VM2 causes incorrect unlock behavior.Both
unlock_account2()andunlock_account()read and update the sameaccount_unlock_time. Unlocking one address (e.g.,account_address2) will make the other path think it is still within the unlock TTL and skip the actual unlock, so the next sign on that other account can fail spuriously.Use separate timers per account, e.g.:
pub struct TxnMocker { @@ - next_sequence_number: u64, - account_unlock_time: Option<Instant>, + next_sequence_number: u64, + account_unlock_time_vm1: Option<Instant>, + account_unlock_time_vm2: Option<Instant>,and have
unlock_account()useaccount_unlock_time_vm1,unlock_account2()useaccount_unlock_time_vm2, adjusting the constructor and sign‑failure paths accordingly.Also applies to: 206-248
🧹 Nitpick comments (8)
config/src/lib.rs (1)
509-515: Consider updating the error message for clarity.The error message "Auto generate test config should success." appears to be copied from
random_for_test(). Since Halley is a persistent test network (not an ephemeral test config), consider making the message more specific.Apply this diff for a more descriptive message:
- Self::load_with_opt(&opt).expect("Auto generate test config should success.") + Self::load_with_opt(&opt).expect("Failed to generate Halley network config")commons/parallel-executor/src/executor.rs (1)
318-347: Variable shadowing ofnum_txnsreduces clarity.The variable
num_txnsis shadowed on line 319 after being used for capacity allocation on line 318. This can cause confusion about which value is used where:
- Line 318: Uses original
num_txns(block length) for capacity- Line 319: Shadows to
scheduler.num_txn_to_execute()- Lines 320, 345: Use shadowed value
Additionally, if the loop at lines 320-332 completes without error,
final_results.len()already equalsnum_txns, making theresize_withon line 345 a no-op.Consider renaming to clarify intent:
- let mut final_results = Vec::with_capacity(num_txns); - let num_txns = scheduler.num_txn_to_execute(); - for idx in 0..num_txns { + let num_txns_to_execute = scheduler.num_txn_to_execute(); + let mut final_results = Vec::with_capacity(num_txns_to_execute); + for idx in 0..num_txns_to_execute {And update line 345:
- final_results.resize_with(num_txns, E::Output::skip_output); + final_results.resize_with(num_txns_to_execute, E::Output::skip_output);vm2/rpc/api/src/account_api.rs (1)
47-51: Batch RPC additions are consistent with existing AccountApi.
sign_txn_in_batchandunlock_in_batchmirror the single-item APIs and match the vm2 account service/message shapes, so the extension looks sound. If you touch this again, consider renamingraw_txntoraw_txnsfor clarity, but it’s not blocking.Also applies to: 62-68
rpc/api/generated_rpc_schema/txpool.json (1)
884-1179: Confirm VM2 txpool schema naming consistency for*2methods.The new methods:
txpool.submit_transactions2mirrorssubmit_transactionsand reuses theSignedUserTransactionschema.txpool.next_sequence_number_in_batch2returnsOption < Vec < (AccountAddress2, Option < u64 >) > >by name, but the JSON schema/title/format still referenceAccountAddress.If your OpenRPC tooling/codegen keys only off the
schemasection, this is harmless; if it inspects thenameas well, theAccountAddress2vsAccountAddressmismatch might be confusing. It’s worth double-checking that the generators/clients you care about don’t rely on thenamestring here, or aligning the labels for clarity.Also applies to: 2990-3036
txpool/src/tx_pool_service_impl.rs (1)
365-367: Be deliberate about exposing txpool internals viaInner’s new public methods.Making
Inner::queue,import_txns,get_pending, andget_pool_clientpublic (and reachable viaTxPoolService.inner) gives external crates direct access to the internal queue and verified transaction types. That’s useful for the new benchmarks/tests, but it also:
- Couples external code tightly to
poolinternals (TxnQueue,VerifiedTransaction,PoolClient).- May expose types that weren’t intended as part of the stable public API surface.
If the intent is only to support specific hooks (e.g., adding listeners, importing batches, or fetching pending txns), consider instead:
- Keeping
queue()andget_pending()pub(crate)and adding narrowerTxPoolService-level helpers (e.g.,add_full_listener,pending_verified_txns) that return/accept more stable types; or- Explicitly documenting that these internal types are now part of the public API and expected to remain stable.
Also worth double-checking that the visibility of
TransactionQueue/VerifiedTransactionmatches this new exposure and won’t cause privacy or semver surprises.Also applies to: 397-409, 420-439, 561-574
rpc/server/src/module/txpool_rpc.rs (1)
37-68: Batch VM2 submission wiring looks correct; consider reusing helper for VM1 path.
submit_transactions_multicorrectly setsbypass_vm1_limit, passeslocal_peer_id, and returns per-tx hashes or errors;submit_transactions2is a thin, appropriate wrapper. You may later want to route the VM1submit_transactionsthrough the same helper for more consistent batch semantics across VM1/VM2, but it’s not required for this PR.Also applies to: 126-135
cmd/tx-factory/src/txn_generator.rs (1)
44-63: VM2 generator wiring looks good; double‑check gas budget choices.The new
generate_*2methods correctly use vm2 builders, types, andchain_id.id().into(). Gas limits differ between VM1/VM2 and between mock/account flows (e.g.,generate_mock_txn2at4_000_000vsgenerate_transfer_txn2/generate_account_txn2at40_000_000, and VM1generate_account_txnat4_000_000). If these asymmetries are intentional for specific benchmarks, this is fine; otherwise, consider aligning them or adding comments documenting the rationale.Also applies to: 84-105, 128-148, 150-170
cmd/tx-factory/src/mocker.rs (1)
406-679: Account discovery/creation logic is fine; consider hardening balance checks.The
get_or_create_accounts*andcreate_accounts*flows are structured reasonably for a pressure tool (reuse existing funded accounts, otherwise create and fund new ones). The fewunwrap()s on balances (e.g., when polling the funder’s balance before funding new accounts) will panic if the funder account is missing; if you expect misconfiguration, you might prefer explicit error messages instead, but it’s acceptable for a fail-fast CLI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksync/starcoin-execute-bench/test.logis excluded by!**/*.log
📒 Files selected for processing (38)
Cargo.toml(2 hunks)cmd/tx-factory/src/lib.rs(1 hunks)cmd/tx-factory/src/main.rs(7 hunks)cmd/tx-factory/src/mocker.rs(1 hunks)cmd/tx-factory/src/pressure_vm1.rs(1 hunks)cmd/tx-factory/src/pressure_vm2.rs(1 hunks)cmd/tx-factory/src/txn_generator.rs(4 hunks)commons/parallel-executor/src/executor.rs(11 hunks)config/src/genesis_config/mod.rs(1 hunks)config/src/lib.rs(1 hunks)config/src/txpool_config.rs(2 hunks)kube/manifest/starcoin-halley.yaml(2 hunks)node/src/lib.rs(2 hunks)rpc/api/generated_rpc_schema/txpool.json(2 hunks)rpc/api/src/txpool/mod.rs(2 hunks)rpc/client/src/lib.rs(7 hunks)rpc/client/src/remote_state_reader2/mod.rs(2 hunks)rpc/server/Cargo.toml(0 hunks)rpc/server/src/module/txpool_rpc.rs(3 hunks)scripts/release.sh(1 hunks)sync/Cargo.toml(2 hunks)sync/src/tasks/block_sync_task.rs(1 hunks)sync/starcoin-execute-bench/Cargo.toml(1 hunks)sync/starcoin-execute-bench/src/main.rs(1 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)test-helper/src/lib.rs(1 hunks)test-helper/src/node.rs(1 hunks)txpool/src/tx_pool_service_impl.rs(4 hunks)vm/vm-runtime/src/parallel_executor/mod.rs(0 hunks)vm2/account/api/src/message.rs(3 hunks)vm2/account/api/src/service.rs(3 hunks)vm2/account/src/account_manager.rs(2 hunks)vm2/rpc/api/src/account_api.rs(2 hunks)vm2/rpc/server/src/account_rpc.rs(2 hunks)vm2/service/account/src/service.rs(1 hunks)vm2/vm-runtime/src/parallel_executor/mod.rs(0 hunks)vm2/vm-runtime/src/starcoin_vm.rs(1 hunks)vm2/vm-types/src/token/stc.rs(1 hunks)
💤 Files with no reviewable changes (3)
- rpc/server/Cargo.toml
- vm2/vm-runtime/src/parallel_executor/mod.rs
- vm/vm-runtime/src/parallel_executor/mod.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- Cargo.toml
- vm2/service/account/src/service.rs
- config/src/txpool_config.rs
- rpc/api/src/txpool/mod.rs
- vm2/vm-runtime/src/starcoin_vm.rs
- cmd/tx-factory/src/main.rs
- scripts/release.sh
- vm2/account/api/src/service.rs
- vm2/vm-types/src/token/stc.rs
- vm2/account/src/account_manager.rs
- rpc/client/src/remote_state_reader2/mod.rs
- vm2/rpc/server/src/account_rpc.rs
- cmd/tx-factory/src/pressure_vm1.rs
- kube/manifest/starcoin-halley.yaml
- sync/Cargo.toml
🧰 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: 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.
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: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
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.
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-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-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.rscommons/parallel-executor/src/executor.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.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.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:
rpc/api/generated_rpc_schema/txpool.jsoncmd/tx-factory/src/lib.rscmd/tx-factory/src/pressure_vm2.rstest-helper/src/lib.rssync/tests/full_build_and_execute_test.rsrpc/server/src/module/txpool_rpc.rscmd/tx-factory/src/txn_generator.rsrpc/client/src/lib.rscmd/tx-factory/src/mocker.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:
cmd/tx-factory/src/lib.rssync/starcoin-execute-bench/Cargo.tomlrpc/client/src/lib.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:
txpool/src/tx_pool_service_impl.rsrpc/server/src/module/txpool_rpc.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/lib.rstest-helper/src/lib.rscommons/parallel-executor/src/executor.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:
sync/starcoin-execute-bench/Cargo.tomlcmd/tx-factory/src/pressure_vm2.rssync/starcoin-execute-bench/src/main.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:
sync/starcoin-execute-bench/Cargo.tomlcmd/tx-factory/src/pressure_vm2.rstest-helper/src/lib.rssync/tests/full_build_and_execute_test.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:
commons/parallel-executor/src/executor.rsrpc/client/src/lib.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
commons/parallel-executor/src/executor.rs
📚 Learning: 2025-02-10T10:00:57.422Z
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Applied to files:
commons/parallel-executor/src/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:
rpc/client/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:
rpc/client/src/lib.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:
cmd/tx-factory/src/mocker.rs
🧬 Code graph analysis (6)
config/src/lib.rs (1)
config/src/genesis_config/mod.rs (1)
default(427-429)
test-helper/src/lib.rs (1)
test-helper/src/node.rs (3)
run_node_by_config(17-22)run_node_with_all_service(24-28)run_test_node(12-15)
sync/tests/full_build_and_execute_test.rs (1)
test-helper/src/node.rs (1)
run_node_with_all_service(24-28)
rpc/client/src/lib.rs (3)
cmd/tx-factory/src/mocker.rs (1)
next_sequence_number_in_batch2(681-706)rpc/api/src/txpool/mod.rs (2)
next_sequence_number_in_batch2(86-89)submit_transactions2(29-32)rpc/server/src/module/txpool_rpc.rs (2)
next_sequence_number_in_batch2(261-267)submit_transactions2(126-135)
sync/starcoin-execute-bench/src/main.rs (1)
test-helper/src/node.rs (1)
run_node_with_all_service(24-28)
cmd/tx-factory/src/mocker.rs (5)
rpc/client/src/lib.rs (3)
state_reader(606-611)next_sequence_number_in_batch2(339-349)next_sequence_number_in_batch(331-337)rpc/client/src/remote_state_reader2/mod.rs (2)
new(36-61)resource(83-84)txpool/src/tx_pool_service_impl.rs (5)
new(41-83)next_sequence_number(193-202)next_sequence_number(463-472)next_sequence_number_in_batch(204-237)next_sequence_number_in_batch(474-501)cmd/tx-factory/src/txn_generator.rs (1)
new(28-42)rpc/server/src/module/txpool_rpc.rs (4)
next_sequence_number(248-251)gas_price(181-184)next_sequence_number_in_batch2(261-267)next_sequence_number_in_batch(253-259)
🔇 Additional comments (17)
node/src/lib.rs (2)
20-20: Import ofStorage2looks correct and consistentBringing
Storage2into scope alongsideStorageis aligned with the new accessor below and follows existing import style.
140-144:storage2()accessor correctly mirrors existingstorage()patternThe accessor cleanly exposes
Arc<Storage2>via the registry and uses the same must-existexpectpattern as other NodeHandle helpers; this is consistent and appropriate.test-helper/src/lib.rs (1)
24-24: LGTM! Clean public API expansion.The re-export correctly adds the new
run_node_with_all_servicehelper to the public API, making it available alongside the existing node launch helpers. The placement and naming are consistent with the existing pattern.config/src/genesis_config/mod.rs (1)
241-246: I'll verify the concern about CustomNetworkID::new visibility allowing bypass of validation logic. Let me search for usage patterns and context.
<function_calls>
#!/bin/bashSearch for direct usage of CustomNetworkID::new
rg -n "CustomNetworkID::new" --type rust -C 3
</function_calls>
<function_calls>
#!/bin/bashGet the full context around CustomNetworkID definition and new() method
rg -n "impl CustomNetworkID" --type rust -A 50 -B 5 | head -100
</function_calls>
<function_calls>
#!/bin/bashCheck the ChainNetworkID::new_custom validation logic
rg -n "fn new_custom" --type rust -A 15 -B 2
</function_calls>
<function_calls>
#!/bin/bashSearch for any builtin network validation checks
rg -n "builtin" --type rust -C 3 | head -60
</function_calls>
<function_calls>
#!/bin/bashCheck file size and read the relevant sections
wc -l config/src/genesis_config/mod.rs
echo "---"Read around line 241 to see the visibility change
sed -n '235,250p' config/src/genesis_config/mod.rs
</function_calls>commons/parallel-executor/src/executor.rs (4)
116-126: Constructor simplification looks good.Removing the
gas_limitparameter streamlines the API. The concurrency level assertion correctly enforces bounds.
393-459: Test executor simplification is appropriate.The
TestExecutorand itsexecute_transactionimplementation have been correctly updated to remove gas-related logic. The simplified design appropriately focuses on testing concurrency and write propagation.
461-569: Tests appropriately updated for the simplified API.Both test cases correctly use
.max(2)to ensure the concurrency level meets the minimum requirement. The tests exercise key scenarios:
test_parallel_conflicting_transactions: Validates correct handling of write-write conflicts using condition variables to control execution ordertest_parallel_independent_transactions: Validates independent transactions execute without interference
300-300: No issues found withNonegas_limit handling inScheduler::new().The Scheduler correctly handles the
Nonegas_limit parameter. Thegas_trackerfield is properly typed asOption<GasTracker>, initialized withgas_limit.map(|limit| GasTracker::new(num_txns, limit)), and all usages throughout the code safely handle the Option type using pattern matching (if let Some()) and.as_ref().map()rather than unsafe unwrap operations. This is idiomatic Rust and poses no risk.sync/src/tasks/block_sync_task.rs (1)
355-357: Fork-to-parent is defensive validation, not state positioning requirementThe review comment incorrectly attributes the purpose of this change to state positioning based on learnings about DAG execution. However, verification reveals that
execute_dag_block()explicitly forks state at the parent's state roots (lines 2213-2216) regardless of the chain's current position, making the pre-positioned state unnecessary.The actual benefit of the fork-to-parent check is early validation:
fork()validates parent existence viahas_dag_block()and returns a proper error, whereasexecute_dag_block()usesexpect()which would panic if parent block info is missing. This provides graceful error handling rather than state positioning as claimed.The change is sound for error handling but the original review's reasoning about DAG execution state requirements does not align with the actual implementation of
execute_dag_block().vm2/account/api/src/message.rs (1)
21-37: Batch account request/response extensions look consistent.
SignTxnInBatch/SignedTxnListandUnlockAccountInBatchalign with the existing single-txn variants and reuse existing response shapes appropriately. No issues from an API or type-usage perspective.Also applies to: 61-68
sync/starcoin-execute-bench/Cargo.toml (1)
1-37: Bench crate manifest looks coherent.Workspace-scoped metadata and dependencies plus explicit
plotterspin are consistent with the new benchmark binary’s usage. No blocking issues here.cmd/tx-factory/src/lib.rs (1)
1-3: Public module exports align with new tooling.Exposing
mocker,pressure_vm1, andpressure_vm2from the crate root makes sense given the new pressure-test and mocker entry points. No issues here.cmd/tx-factory/src/pressure_vm2.rs (1)
13-61: start_vm2_pressure_test loop looks correct and mirrors VM1 behavior.Thread spawning, stop flag handling, and error recovery via
recheck_sequence_number2()are consistent and appropriate for the pressure test use case.rpc/server/src/module/txpool_rpc.rs (1)
261-267: next_sequence_number_in_batch2 implementation is consistent with VM1.The VM2 batch sequence RPC simply delegates to
next_sequence_number2_in_batchand wraps it inFutureResult, mirroring the VM1 variant.cmd/tx-factory/src/mocker.rs (1)
250-310: Batch and single submit helpers are coherent and fail‑fast.
submit_transaction_in_batch*andsubmit_txn*correctly unlock the necessary accounts, sign, submit, and optionally block onwatch_txn, short‑circuiting on the first error via?as preferred for tx-factory tools.Also applies to: 312-381
rpc/client/src/lib.rs (2)
63-77: VM2 txpool client APIs verified and correctly implemented.All three methods (
next_sequence_number_in_txpool2,next_sequence_number_in_batch2,submit_transactions2) are properly exposed in theTxPoolApitrait with matching signatures and are correctly delegated by the client wrapper with consistent error handling.
461-467: Perfect! I have completed the verification. All the claims in the review comment are accurate:VM2 batch account sign/unlock methods are consistent with VM1.
account_sign_txn_in_batch2(lines 461-467) mirrorsaccount_sign_txn_in_batch(lines 453-458)account_unlock_in_batch2(lines 509-520) mirrorsaccount_unlock_in_batch(lines 522-532)- Both VM1 and VM2 variants delegate to their respective clients (
account_clientvsaccount_client2)- Duration conversion (
duration.as_secs() as u32) is consistent and appropriate- Error mapping (
map_err) is identical across all methods- Type compatibility verified:
RawUserTransaction2andAccountAddress2correctly map to the underlying types expected byAccountApimethods- Both
sign_txn_in_batchandunlock_in_batchmethods exist in thestarcoin_vm2_rpc_api::account_api::AccountClientwith matching parameter order
| fn next_sequence_number_in_batch2( | ||
| &self, | ||
| addresses: Vec<AccountAddress2>, | ||
| ) -> Result<Vec<(AccountAddress2, Option<u64>)>> { | ||
| let seq_numbers = self | ||
| .client | ||
| .next_sequence_number_in_batch2(addresses)? | ||
| .ok_or_else(|| format_err!("next_sequence_number_in_batch error"))?; | ||
| Ok(seq_numbers | ||
| .into_iter() | ||
| .map(|(address, seq_number)| match seq_number { | ||
| Some(seq_number) => (address, Some(seq_number)), | ||
| None => { | ||
| let state_reader = self | ||
| .client | ||
| .state_reader2(StateRootOption::Latest) | ||
| .expect("state_reader error"); | ||
| let account_resource = state_reader | ||
| .get_account_resource(address) | ||
| .expect("get_account_resource error"); | ||
| let seq = account_resource.sequence_number(); | ||
| (address, Some(seq)) | ||
| } | ||
| }) | ||
| .collect()) | ||
| } | ||
|
|
||
| fn next_sequence_number_in_batch( | ||
| &self, | ||
| addresses: Vec<AccountAddress>, | ||
| ) -> Result<Vec<(AccountAddress, Option<u64>)>> { | ||
| let seq_numbers = self | ||
| .client | ||
| .next_sequence_number_in_batch(addresses)? | ||
| .ok_or_else(|| format_err!("next_sequence_number_in_batch error"))?; | ||
| Ok(seq_numbers | ||
| .into_iter() | ||
| .map(|(address, seq_number)| match seq_number { | ||
| Some(seq_number) => (address, Some(seq_number)), | ||
| None => { | ||
| let state_reader = self | ||
| .client | ||
| .state_reader(StateRootOption::Latest) | ||
| .expect("state_reader error"); | ||
| let account_resource = state_reader | ||
| .get_account_resource(address) | ||
| .expect("get_account_resource error"); | ||
| let seq = account_resource.map(|resource| resource.sequence_number()); | ||
| (address, seq) | ||
| } | ||
| }) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
Stress tests reuse stale sequence numbers between forward and reverse passes.
stress_test2 and stress_test call next_sequence_number_in_batch* once to obtain sequences, then:
- Use
sequences.chunks(...)for the first pass (senders in the first half, receivers in the second). - Use
sequences.rchunks(...)for the second pass, but still with the same originalsequences.
Since send_and_receive* derives per-tx sequence numbers from this snapshot (seq + i), the reverse pass reuses the same base sequences even though the first pass has already enqueued txns for those ranges. The reverse-pass txns are therefore prone to being rejected as stale/duplicate, which distorts the stress workload.
Consider refreshing sequences between passes, e.g.:
- let sequences =
- self.next_sequence_number_in_batch2(accounts.iter().map(|a| a.address).collect())?;
+ let mut sequences =
+ self.next_sequence_number_in_batch2(accounts.iter().map(|a| a.address).collect())?;
@@
for addresses in sequences.chunks(transfer_account_size) {
// forward pass ...
}
@@
- for addresses in sequences.rchunks(transfer_account_size) {
+ // Re-sync sequences before reverse pass
+ sequences =
+ self.next_sequence_number_in_batch2(accounts.iter().map(|a| a.address).collect())?;
+ for addresses in sequences.rchunks(transfer_account_size) {
// reverse pass ...
}and similarly for the VM1 variant using next_sequence_number_in_batch.
Based on learnings, this keeps behavior aligned with the intended “use up-to-date txpool sequences before each batch”.
Also applies to: 756-793, 795-832, 834-944
🤖 Prompt for AI Agents
In cmd/tx-factory/src/mocker.rs around lines 681 to 733,
next_sequence_number_in_batch2/next_sequence_number_in_batch are being called
once and the same snapshot (sequences) is reused for both forward (chunks) and
reverse (rchunks) passes, causing the reverse pass to reuse stale base sequence
numbers and produce duplicate/stale txns; fix by refreshing the sequences before
the reverse pass — call self.client.next_sequence_number_in_batch2(addresses)?
(and the VM1 variant self.client.next_sequence_number_in_batch for VM1) again
and assign to a new variable used for the rchunks/ranges so the reverse pass
computes per-tx sequence numbers from up-to-date txpool state (apply same change
for the other occurrences noted at lines ~756-793, 795-832, 834-944).
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
Release Notes
New Features
Improvements
Optimizations
✏️ Tip: You can customize this high-level summary in your review settings.