VM2 benchmark #4722
VM2 benchmark #4722
Conversation
2, cache the vm2 config from shengguo lu 3, add batch operations for the transaction interface
WalkthroughReplace forked VM state handling with direct in-memory statedb use for dual‑VM execution and add broad VM2 support: CLI txn factory, RPC batch APIs, account batch services, client plumbing, txpool visibility, tests, node helpers, configs, and Kubernetes manifest updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as TxFactory CLI
participant Mocker as TxnMocker
participant AccountVM2 as AccountService (VM2)
participant RPC as RpcClient
participant TxPool as TxPool Service
rect #f0f9ff
CLI->>Mocker: construct(vm1_account, vm2_account, generator)
CLI->>Mocker: start pressure (vm_version)
end
alt vm_version == 1
Mocker->>RPC: submit_transactions(Vec<SignedUserTransaction>)
RPC->>TxPool: import_txns(MultiSignedUserTransaction::VM1)
else vm_version == 2
Mocker->>AccountVM2: unlock_account_in_batch(batch)
AccountVM2-->>Mocker: Vec<AccountInfo2>
Mocker->>AccountVM2: sign_txn_in_batch(raw_txns)
AccountVM2-->>Mocker: Vec<SignedUserTransaction2>
Mocker->>RPC: submit_transactions2(Vec<SignedUserTransaction2>)
RPC->>TxPool: import_txns(MultiSignedUserTransaction::VM2)
end
Note over TxPool: Pool enqueues and schedules execution (block metadata executed first)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vm2/vm-runtime/src/move_vm_ext/warm_vm_cache.rs (1)
85-102: Fix type mismatch: PartialVMError cannot propagate to VMError in line 99.The
load_modulecall returnsVMResult<...>(i.e.,Result<..., PartialVMError>), butwarm_vm_upreturnsResult<(), VMError>. The?operator on line 99 cannot automatically convertPartialVMErrortoVMErrorbecause noFromimplementation exists between these types.Given the comment states this is a "best effort approach and won't hurt if that fails," consider ignoring the error instead of propagating it:
let _ = vm.load_module( &ModuleId::new(CORE_CODE_ADDRESS, ident_str!("account").to_owned()), resolver, );Alternatively, map the error:
.map_err(|_| VMError::...)?or change the return type toVMResult<()>.chain/src/chain.rs (1)
2168-2174: Wrong “no-VM1-txns” state_root sourceWhen VM1 is offline or there are no VM1 txns, state_root must be the parent VM1 root, not self.statedb.0 (current tip). Use the forked statedb or multi_state root.
- BlockExecutedData { - state_root: self.statedb.0.state_root(), + BlockExecutedData { + state_root: statedb.state_root(), txn_infos: vec![], txn_events: vec![], txn_table_infos: BTreeMap::new(), write_sets: vec![], }
🧹 Nitpick comments (26)
rpc/client/src/remote_state_reader2/mod.rs (1)
63-73: Method works correctly but could be more idiomatic.The implementation is functionally correct and properly handles all cases (error, no state, and successful balance retrieval). However, consider these improvements:
Add documentation: This public method needs a doc comment explaining its purpose, parameters, return value, and potential errors.
Refactor for readability: The
map_orwith a fallible closure (containing?) is unusual. Consider using.transpose()for a more idiomatic conversion fromOption<Result<T>>toResult<Option<T>>:pub fn get_balance2(&self, address: AccountAddress) -> Result<Option<u128>> { let state_key = StateKey::resource_typed::<BalanceResource>(&address)?; - self.client - .state_get_with_proof2(state_key)? - .state - .map_or(Ok(None), |state| { - Ok(Some( - bcs_ext::from_bytes::<BalanceResource>(state.0.as_slice())?.token(), - )) - }) + self.client + .state_get_with_proof2(state_key)? + .state + .map(|state| { + bcs_ext::from_bytes::<BalanceResource>(state.0.as_slice()) + .map(|balance_resource| balance_resource.token()) + }) + .transpose() }Alternatively, a simple
matchexpression would also improve clarity.kube/manifest/starcoin-halley.yaml (2)
26-26: Use a more descriptive image tag instead of a commit SHA.The image tag
sha-6a5e4e8(a Git commit short hash) lacks semantic meaning and traceability. For a benchmark release, consider using a descriptive tag such asvm2-benchmark-v1or a semver version that clearly indicates the purpose and version of the image.Commit SHAs make it difficult to understand what changes are deployed, complicate rollbacks, and reduce reproducibility in documentation and incident response.
50-57: Consider the implications of removing the readiness probe.Per the AI summary, the readiness probe has been removed from this manifest. While the
livenessProberemains, there is noreadinessProbeto verify that the starcoin service is fully initialized and ready to handle traffic (e.g., synced with peers, accepting RPC requests).For a multi-replica (3-node) benchmark environment, a readiness probe can be valuable to ensure nodes are operational before the benchmark applies load. If the readiness probe removal was intentional, please document the rationale. If it was accidental, consider restoring it or adding a custom readiness check (e.g., an HTTP health endpoint).
rpc/server/src/module/debug_rpc.rs (1)
87-94: Document VM2-specific concurrency control.These debug RPC methods now exclusively control VM2's concurrency level without indicating this in the API or documentation. Users calling
set_concurrency_levelandget_concurrency_levelmay not realize they're only affecting VM2.Consider:
- Adding documentation/comments indicating these methods control VM2 specifically
- Adding a parameter to specify which VM to control (if dual-VM support is intended)
- Ensuring VM1's concurrency is controllable through another mechanism (if needed)
Based on learnings: The
set_concurrency_level_once()call is safe due to OnceCell semantics, which prevents race conditions.vm2/vm-runtime/src/move_vm_ext/warm_vm_cache.rs (1)
78-80: Consider logging warm-up failures for observability.The VM is returned to the caller even if
warm_vm_upfails, which is reasonable for a "best effort" approach. However, silently skipping cache insertion could hide systematic issues (e.g., resolver problems, module loading failures) and cause performance degradation as new VMs are repeatedly created.Consider adding a log statement to track warm-up failures:
if Self::warm_vm_up(&vm, resolver).is_ok() { cache_locked.insert(id, vm.clone()); +} else { + log::debug!("Failed to warm up VM, skipping cache insertion"); }Alternatively, use a metrics counter to track cache insertion vs. skip rates for monitoring.
node/src/lib.rs (1)
140-144: Implementation verified and correct; optional documentation recommended.Storage2 is properly registered in the registry during node initialization (node/src/node.rs lines 287-289), so the
expect()call is safe and won't panic at runtime.The implementation is sound. Consider adding a brief doc comment to clarify when to use
storage2()versusstorage():+ /// Returns the VM2-backed storage instance for transaction execution and state access. pub fn storage2(&self) -> Arc<Storage2> { self.registry .get_shared_sync::<Arc<Storage2>>() .expect("Storage2 must exist.") }chain/src/chain.rs (1)
2386-2389: k update should use the parent-derived epochself.epoch can be stale during execution on non-head parents. Use the epoch read from statedb2 above.
- self.dag() - .ghost_dag_manager() - .update_k(self.epoch.max_uncles_per_block().try_into().unwrap()); + self.dag() + .ghost_dag_manager() + .update_k(epoch.max_uncles_per_block().try_into().unwrap());Would you confirm no other callers rely on self.epoch here?
sync/tests/full_build_and_execute_test.rs (4)
218-225: Use TxPoolService APIs to preserve metrics/invariants instead ofinner.*.Calling
inner.import_txns/inner.get_pendingbypasses timers and any service-level guards. Useadd_txns_multi_signedandget_pending_txns.- txpool.inner.import_txns( - signed_transactions - .into_iter() - .map(MultiSignedUserTransaction::VM2) - .collect(), - false, - None, - )?; + txpool.add_txns_multi_signed( + signed_transactions + .into_iter() + .map(MultiSignedUserTransaction::VM2) + .collect(), + false, + None, + )?; @@ - let transactions = txpool - .inner - .get_pending(100, config.net().time_service().now_secs())?; + let transactions = txpool.get_pending_txns( + Some(100), + Some(config.net().time_service().now_secs()), + )?;Also applies to: 228-234
29-35: Nit: fix parameter name typo.Rename
accoun_number→account_numberfor clarity.-async fn create_account( - accoun_number: u32, +async fn create_account( + account_number: u32, @@ - for _ in 0..accoun_number { + for _ in 0..account_number {
86-95: Reduce per-tx unlock overhead and remove index/unwrap panics.Unlock the sender once per unique address and iterate with
zipto avoidget(index).unwrap()panics; also keep address access consistent (.address()or field).- let mut signed_transactions = vec![]; - for index in 0..receiver.len() { - let sender_address = sender.get(index).unwrap().address; + use std::collections::HashSet; + let mut signed_transactions = vec![]; + let mut unlocked: HashSet<_> = HashSet::new(); + for (s, r) in sender.iter().zip(receiver.iter()) { + let sender_address = *s.address(); @@ - let transaction = build_batch_transfer_txn2( - sender.get(index).unwrap().address, - vec![receiver.get(index).unwrap().address], + let transaction = build_batch_transfer_txn2( + *s.address(), + vec![*r.address()], @@ - match account_service - .send(AccountRequest::UnlockAccount( - sender_address, - "".to_string(), - std::time::Duration::from_secs(100), - )) - .await?? - { - AccountResponse::AccountInfo(_) => (), - _ => bail!("Unexpect response type."), - } + if unlocked.insert(sender_address) { + match account_service + .send(AccountRequest::UnlockAccount( + sender_address, + "".to_string(), + std::time::Duration::from_secs(100), + )) + .await?? + { + AccountResponse::AccountInfo(_) => (), + _ => bail!("Unexpected response type."), + } + }Also applies to: 103-114, 114-125
39-41: Minor: error message wording.“Unexpect response type.” → “Unexpected response type.” for clarity.
Also applies to: 56-57, 112-113, 122-123, 138-139
txpool/src/tx_pool_service_impl.rs (1)
397-409: ExposingInnermethods publicly bypasses service-level metrics and guards.Making these
puballows callers to skipTxPoolServiceinstrumentation (timers, error mapping) and invariants. Prefer keeping thempub(crate)and exposing wrapper methods onTxPoolServiceif external access is required.Option A — revert visibility:
- pub fn import_txns( + pub(crate) fn import_txns( @@ - pub fn get_pending( + pub(crate) fn get_pending( @@ - pub fn get_pool_client(&self) -> Result<PoolClient> { + pub(crate) fn get_pool_client(&self) -> Result<PoolClient> {Option B — keep public but add doc comments stating they are for testing/bench and consider moving timers to
Inneror addingcfg(test)gates.Please confirm which external crates need these public methods so we can add proper
TxPoolServiceforwards without exposing internals.Also applies to: 420-439, 561-574
cmd/tx-factory/src/pressure_vm1.rs (1)
11-20: Consider consolidating parameters into a configuration struct.The function takes 8 parameters, which can make call sites harder to read and maintain. Consider introducing a
PressureTestConfigstruct to group related parameters.Example refactor:
pub struct PressureTestConfig { pub round_num: u32, pub account_num: u32, pub batch_size: u32, pub interval: Duration, pub transfer_account_size: usize, pub is_stress: bool, } pub fn start_vm1_pressure_test( mut tx_mocker: TxnMocker, config: PressureTestConfig, stopping_signal: Arc<AtomicBool>, ) -> std::thread::JoinHandle<()>cmd/tx-factory/src/pressure_vm2.rs (1)
13-22: Consider consolidating parameters into a configuration struct.The function takes 8 parameters, mirroring the VM1 implementation. Consider introducing a shared
PressureTestConfigstruct to improve maintainability and reduce duplication.Example:
pub struct PressureTestConfig { pub round_num: u32, pub account_num: u32, pub batch_size: u32, pub interval: Duration, pub transfer_account_size: usize, pub is_stress: bool, } pub fn start_vm2_pressure_test( mut tx_mocker: TxnMocker, config: PressureTestConfig, stopping_signal: Arc<AtomicBool>, ) -> std::thread::JoinHandle<()>vm2/account/src/account_manager.rs (1)
374-396: Address the TODO comment and document fail-fast behavior.Line 380 contains a TODO about verifying the signer. Additionally, the method fails fast on the first locked account (line 383), returning an error without processing remaining transactions. While this is reasonable, it should be documented in the API to set clear expectations.
Do you want me to:
- Generate a verification script to check if there are existing validations for transaction signers elsewhere in the codebase?
- Help document the fail-fast behavior in the API documentation?
- Open an issue to track the TODO for signer validation?
cmd/tx-factory/src/main.rs (2)
186-190: Prefer CLI-level validation for vm_versionUse clap ValueEnum or possible_values to reject invalid vm_version at parse time instead of panicking at runtime.
108-138: Bound the wait loop and add contextCap retries (e.g., max attempts or deadline), add backoff, and include why we’re waiting (ipc path, net, vm_version) to aid ops. Consider honoring a startup timeout flag.
cmd/tx-factory/src/txn_generator.rs (2)
44-62: Unify gas limits and avoid magic numbersGas caps vary (4,000,000 vs 40,000,000) across VM1/VM2 and mock/transfer/batch. If not intentional, align them via named constants and use the same defaults across generators.
Suggested pattern:
- Define DEFAULT_MAX_GAS and DEFAULT_BATCH_MAX_GAS (per-VM if needed).
- Replace numeric literals in all builders.
- 4000000, + DEFAULT_MAX_GAS_AMOUNT, ... - 40_000_000, + DEFAULT_BATCH_MAX_GAS_AMOUNT,Also applies to: 84-104, 128-148, 165-165
128-148: Consider clearer naming for batch buildergenerate_account_txn2 builds a batch transfer; consider rename to generate_batch_transfer_txn2 for clarity.
cmd/tx-factory/src/mocker.rs (7)
48-72: Initialize and re-check sequence per path before generating txns.new() seeds from VM2 only; VM1 gen may start from a wrong seq. Also sequences can drift due to concurrent submissions. Recheck at the start of gen methods.
Apply:
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(); let raw_txn = self .generator .generate_mock_txn2(self.next_sequence_number, expiration_timestamp)?;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(); let raw_txn = self .generator .generate_mock_txn(self.next_sequence_number, expiration_timestamp)?;Also applies to: 128-133, 167-172
263-270: Avoid cloning raw txns in batch signing; consume the vectors.flat_map over iter() clones all RawUserTransaction(2). Use into_iter to move them.
Apply:
- let signed_transactions = self.client.account_sign_txn_in_batch2( - txns.iter() - .flat_map(|(_, raw_txns)| raw_txns.clone()) - .collect(), - )?; + let signed_transactions = self.client.account_sign_txn_in_batch2( + txns.into_iter().flat_map(|(_, raw_txns)| raw_txns).collect(), + )?;And VM1:
- let signed_transactions = self.client.account_sign_txn_in_batch( - txns.iter() - .flat_map(|(_, raw_txns)| raw_txns.clone()) - .collect(), - )?; + let signed_transactions = self.client.account_sign_txn_in_batch( + txns.into_iter().flat_map(|(_, raw_txns)| raw_txns).collect(), + )?;Also applies to: 294-301
767-790: Avoid per-sender Vec clone; use std::mem::take for zero‑copy move.Cloning transactions per sender is wasteful. Move the Vec and reuse it.
Apply (pattern shown for VM2; mirror for VM1):
- for (index, (sender_address, sequence_op)) in senders.iter().enumerate() { + for (index, (sender_address, sequence_op)) in senders.iter().enumerate() { ... - sender_transactions.push((*sender_address, transactions.clone())); - transactions.clear(); + sender_transactions.push((*sender_address, std::mem::take(&mut transactions))); }Also applies to: 805-829
83-87: Don’t unwrap RPC; return Result from get_factory_status.Unwrap panics on transport/handler errors. Prefer propagating.
Apply:
- pub fn get_factory_status(&self) -> bool { - self.client - .debug_txfactory_status(FactoryAction::Status) - .unwrap() - } + pub fn get_factory_status(&self) -> Result<bool> { + Ok(self.client.debug_txfactory_status(FactoryAction::Status)?) + }
76-82: Avoid expect() in fetch_expiration_time; propagate error.A node_info failure will panic. Return Result and handle upstream.
Example:
- fn fetch_expiration_time(&self) -> u64 { - let node_info = self - .client - .node_info() - .expect("node_info() should not failed"); - node_info.now_seconds + DEFAULT_EXPIRATION_TIME - } + fn fetch_expiration_time(&self) -> Result<u64> { + let node_info = self.client.node_info()?; + Ok(node_info.now_seconds + DEFAULT_EXPIRATION_TIME) + }Then adjust call sites to use ? and pass u64.
681-706: Graceful fallback on missing seq_numbers; avoid expect()s.When seq from pool is None, you fetch from state and use expect(...). Prefer propagating with context to avoid panics during outages.
Example VM2 mapping:
- 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)) - } + None => { + let state_reader = self.client.state_reader2(StateRootOption::Latest) + .context("state_reader2 failed")?; + let seq = state_reader.get_account_resource(address) + .context("get_account_resource (vm2) failed")? + .sequence_number(); + (address, Some(seq)) + }Apply similar handling for VM1 path.
Also applies to: 708-733
607-609: Minor: prefer account_list.len() and clean log text.Style/clarity nits.
Apply:
- info!("{:?} accounts are created.", Vec::len(&account_list)); + info!("{} accounts are created.", account_list.len());Apply in both VM2 and VM1 create_accounts paths.
Also applies to: 677-679
📜 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)
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)commons/parallel-executor/src/executor.rs(3 hunks)commons/parallel-executor/src/task.rs(1 hunks)config/src/genesis_config/mod.rs(1 hunks)config/src/lib.rs(1 hunks)kube/manifest/starcoin-halley.yaml(2 hunks)node/src/lib.rs(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/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(3 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/executor/Cargo.toml(1 hunks)vm2/executor/src/block_executor.rs(1 hunks)vm2/framework/gas-meter/src/gas_meter.rs(0 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/move_vm_ext/warm_vm_cache.rs(3 hunks)vm2/vm-runtime/src/parallel_executor/mod.rs(1 hunks)vm2/vm-runtime/src/starcoin_vm.rs(1 hunks)
💤 Files with no reviewable changes (1)
- vm2/framework/gas-meter/src/gas_meter.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
PR: starcoinorg/starcoin#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-10-10T01:54:58.184Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
vm2/executor/src/block_executor.rschain/src/chain.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
PR: starcoinorg/starcoin#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/Cargo.tomlsync/Cargo.toml
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#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/mocker.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
chain/src/chain.rs
🧬 Code graph analysis (20)
commons/parallel-executor/src/task.rs (1)
vm2/vm-runtime/src/parallel_executor/mod.rs (1)
is_block_meta_data(31-36)
vm2/vm-runtime/src/parallel_executor/mod.rs (1)
commons/parallel-executor/src/task.rs (1)
is_block_meta_data(28-30)
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)
rpc/server/src/module/debug_rpc.rs (3)
vm2/vm-runtime/src/starcoin_vm.rs (2)
set_concurrency_level_once(1402-1407)get_concurrency_level(1411-1416)rpc/client/src/lib.rs (1)
get_concurrency_level(841-844)rpc/api/src/debug/mod.rs (1)
get_concurrency_level(38-38)
vm2/rpc/api/src/account_api.rs (3)
vm2/account/api/src/service.rs (2)
sign_txn_in_batch(38-41)sign_txn_in_batch(175-189)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)
vm2/rpc/server/src/account_rpc.rs (5)
vm2/account/api/src/service.rs (2)
sign_txn_in_batch(38-41)sign_txn_in_batch(175-189)vm2/account/src/account_manager.rs (1)
sign_txn_in_batch(374-396)vm2/rpc/api/src/account_api.rs (2)
sign_txn_in_batch(48-51)unlock_in_batch(64-68)rpc/client/src/lib.rs (1)
map_err(1323-1325)vm2/rpc/server/src/lib.rs (1)
map_err(6-29)
test-helper/src/node.rs (2)
node/src/lib.rs (1)
config(128-132)cmd/starcoin/src/cli_state.rs (1)
node_handle(130-132)
commons/parallel-executor/src/executor.rs (2)
commons/parallel-executor/src/task.rs (1)
init(56-56)commons/parallel-executor/src/proptest_types/types.rs (1)
init(163-165)
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/account/src/account_manager.rs (3)
vm2/account/api/src/service.rs (4)
unlock_account_in_batch(49-53)unlock_account_in_batch(207-220)sign_txn_in_batch(38-41)sign_txn_in_batch(175-189)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/account/src/account_manager.rs (3)
sign_txn_in_batch(374-396)unlock_account(87-100)unlock_account_in_batch(102-119)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)
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/client/src/remote_state_reader2/mod.rs (1)
vm2/vm-types/src/state_store/state_key/mod.rs (1)
resource_typed(143-145)
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/txn_generator.rs (2)
cmd/tx-factory/src/mocker.rs (2)
new(38-72)sequence_number(736-754)testsuite/tests/steps/transaction.rs (1)
transfer_txn(83-113)
cmd/tx-factory/src/mocker.rs (5)
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)cmd/tx-factory/src/txn_generator.rs (1)
new(28-42)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)vm2/account/src/account_manager.rs (2)
new(67-74)unlock_account(87-100)txpool/src/pool/queue.rs (1)
addresses(558-571)
sync/tests/full_build_and_execute_test.rs (4)
config/src/lib.rs (6)
temp_dir(110-113)default(153-155)default(281-283)path(263-265)load_with_opt(294-319)load_with_opt(549-552)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(17-17)create_account(93-100)node/src/lib.rs (19)
config(128-132)storage2(140-144)new(78-92)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)
chain/src/chain.rs (2)
vm2/executor/src/block_executor.rs (1)
block_execute(37-99)executor/src/block_executor.rs (1)
block_execute(65-147)
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(89-94)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)
vm2/service/account/src/service.rs (3)
block-relayer/src/block_relayer.rs (1)
txns(164-164)network-rpc/src/rpc.rs (1)
txns(101-101)rpc/api/src/types.rs (2)
txns(846-851)txns(861-863)
🔇 Additional comments (32)
vm2/vm-runtime/src/starcoin_vm.rs (1)
321-332: Correct log message for max gas unit price bound violation.The log message on line 324 now correctly references "max" to match the upper bound violation being checked. This aligns with the
GAS_UNIT_PRICE_ABOVE_MAX_BOUNDstatus code and improves consistency with the analogous minimum bound check (lines 308–318).rpc/server/Cargo.toml (1)
79-79: VM2 runtime dependency correctly added.The new
starcoin-vm2-vm-runtimedependency aligns with the PR objectives to add VM2 benchmark and batch operation support. The configuration follows the established pattern (usingworkspace = true) and is properly placed under the Move 2.0 dependencies section.Verify that this dependency is properly utilized in the RPC server implementation (specifically in batch transaction handling and VM2 integration code that is outside the scope of this Cargo.toml file).
vm2/executor/src/block_executor.rs (1)
6-6: Logging import added consistently with dependency.The import of
infofromstarcoin_logger::preludealigns with the newstarcoin-loggerdependency added inCargo.toml. While the macro is not invoked in the visible code, this is consistent with the PR's goal of adding logging enhancements across VM2 components.vm2/executor/Cargo.toml (1)
22-22: Dependency addition follows established patterns.The
starcoin-loggerdependency is added withworkspace = true, maintaining consistency with all other dependencies in this section. This enables the logging import inblock_executor.rs.config/src/genesis_config/mod.rs (1)
241-246: LGTM! Public constructor improves API flexibility.Making
CustomNetworkID::newpublic is a reasonable API expansion that provides a direct construction method alongside the existingFromStrimplementation, improving usability for programmatic construction.config/src/lib.rs (1)
509-516: LGTM! Consistent with existing test config constructors.The
new_for_halley()constructor follows the same pattern asrandom_for_test()and provides a convenient way to create a NodeConfig for the Halley test network.test-helper/src/lib.rs (1)
24-24: LGTM! Export correctly adds new test helper to public API.The addition of
run_node_with_all_serviceto the public exports is necessary and follows the established pattern for the test-helper module's facade.test-helper/src/node.rs (1)
24-28: LGTM! Provides useful alternative node startup for tests.This function complements
run_node_by_configby launching the node with all services active (including the pacemaker), whereasrun_node_by_configstops the pacemaker after launch. This distinction is valuable for tests that need the full node running versus those requiring controlled block production.rpc/client/src/remote_state_reader2/mod.rs (1)
15-15: LGTM: Import added for VM2 balance retrieval.The
BalanceResourceimport is necessary for the newget_balance2method and is correctly placed within the existingstarcoin_vm2_typesimports.kube/manifest/starcoin-halley.yaml (2)
1-33: Clarify the relationship between this K8s manifest and the VM2 benchmark PR objectives.This Kubernetes StatefulSet manifest appears to be infrastructure configuration rather than code implementing VM2 benchmarking, transaction factory infrastructure, or batch operations. The PR objectives emphasize VM2 transaction execution benchmarks and batch transaction operations, but this file modifies only deployment and scaling parameters.
Please clarify:
- Is this manifest update intended to support the new VM2 benchmark code (e.g., as a test deployment)?
- Should this infrastructure change be included in the same PR as the code changes, or should it be a separate operational PR?
- Is there a specific dependency or ordering requirement?
14-14: Justify the replica scaling change from 1 to 3.Increasing replicas from 1 to 3 is a significant operational change that affects network topology, consensus, peer discovery, and resource allocation. The PR description does not explain this change or its relationship to the VM2 benchmark objectives.
Clarify whether this scaling is necessary for pressure testing, whether 3 is the intended target, and whether this should remain in the same PR as the code changes or be managed separately.
rpc/server/src/module/debug_rpc.rs (1)
16-16: Verify whether VM1 concurrency control removal is intentional.The import change removes access to VM1's concurrency level while VM1 remains active in the system:
- VM1 capability exists:
vm/vm-runtime/src/starcoin_vm.rscontainsset_concurrency_level_once()andget_concurrency_level()methods- VM1 is still used: Txpool and state management handle both
MultiSignedUserTransaction::VM1and::VM2separately- VM2 is now exclusive: Debug RPC calls only
StarcoinVM2::set_concurrency_level_once()and::get_concurrency_level()- No alternative mechanism: No other RPC method or code path provides runtime control of VM1 concurrency
- No documentation: No comments or deprecation notices explain the change
Confirm with the PR author:
- Is this intentional (VM1 being phased out)?
- Should both VMs have independent runtime concurrency control?
- If VM1 control is no longer needed, document the architectural decision
node/src/lib.rs (1)
20-20: LGTM! Import addition supports VM2 infrastructure.The addition of
Storage2to the public imports is necessary for the newstorage2()accessor and aligns with the VM2 support being introduced in this PR.vm2/vm-runtime/src/parallel_executor/mod.rs (1)
31-36: Good hook implementation for BlockMetadata detectionEnum match is correct and aligns with the new Transaction::is_block_meta_data API.
Please ensure unit tests cover a block with BlockMetadata first and none-second to assert scheduling behavior.
commons/parallel-executor/src/task.rs (1)
26-30: Sane default for new API; preserves back-compatThe default false keeps existing implementors compiling. No issues.
commons/parallel-executor/src/executor.rs (2)
357-366: Nice: pre-running BlockMetadata reduces contentionGood placement before thread pool spawn; retains parallelism for remaining txns.
If scheduler invariants ever change, the guard code above will keep this safe; consider adding a unit test asserting (0,0) ordering for a block with metadata first.
14-14: Import is fineerror! macro import is used in new guard paths.
sync/Cargo.toml (1)
62-62: VM2 dev-deps addition looks fine; verify they remain dev-only.Confirm these crates are needed only for tests/benchmarks and won’t be pulled into release via workspace default-members. Also ensure required feature flags for VM2 crates are aligned across vm1/vm2 to avoid feature-gating build breaks (e.g., mirroring empty feature placeholders when needed). Based on learnings.
Also applies to: 75-79
cmd/tx-factory/src/lib.rs (1)
1-3: LGTM — modules exported cleanly.No issues spotted with the added module declarations.
scripts/release.sh (1)
8-8: No changes needed—binary name is explicitly defined in Cargo.toml.The tx-factory package explicitly specifies
[[bin]] name = "starcoin_txfactory"in its Cargo.toml. Cargo respects explicit binary names exactly as written; it does not automatically convert underscores to hyphens. The original script correctly referencesstarcoin_txfactoryand will work as intended. No guard or fallback is required.Likely an incorrect or invalid review comment.
vm2/rpc/server/src/account_rpc.rs (2)
174-185: LGTM: Batch signing implementation follows established patterns.The implementation correctly delegates to the account service and maintains consistency with other RPC methods in this file.
207-223: LGTM: Batch unlock implementation is consistent.The duration conversion and async handling follow the same pattern as the single-account unlock method.
vm2/account/api/src/message.rs (1)
25-27: LGTM: Batch operation message variants are well-structured.The new enum variants follow established patterns and use appropriate types for batch operations.
Also applies to: 36-36, 67-67
cmd/tx-factory/src/pressure_vm1.rs (1)
21-59: LGTM: Thread-based pressure test implementation is sound.The loop correctly checks the stopping signal, handles errors appropriately, and provides clear logging for debugging.
vm2/rpc/api/src/account_api.rs (1)
47-51: LGTM: Batch signing API is correctly defined.The method signature and RPC name follow the established VM2 API conventions.
rpc/api/src/txpool/mod.rs (1)
28-32: LGTM: Batch transaction pool APIs are well-defined.The new batch methods follow established patterns and use appropriate VM2 types.
Also applies to: 83-89
vm2/service/account/src/service.rs (1)
193-200: LGTM: Batch request handling is correctly implemented.The service properly delegates to the account manager and returns the appropriate response types.
vm2/account/src/account_manager.rs (1)
102-119: LGTM: Batch unlock implementation is consistent and correct.The method properly handles each account in the batch, maintaining consistency with the single-account unlock flow.
rpc/server/src/module/txpool_rpc.rs (2)
261-267: Thin pass-through looks correctnext_sequence_number_in_batch2 forwards to service.next_sequence_number2_in_batch as expected.
126-135: Good reuse via MultiSignedUserTransaction mappingDelegating VM2 batch submit to submit_transactions_multi keeps logic unified. Client wrapper parity confirmed: submit_transactions2 is properly exposed at rpc/client/src/lib.rs (lines 364–368).
vm2/account/api/src/service.rs (1)
38-42: API additions look goodNew batch methods on the async service trait are coherent with existing single-item APIs.
Also applies to: 49-54
cmd/tx-factory/src/mocker.rs (1)
637-643: Unify account funding amounts between VM1 and VM2 batch calls; parameter order is correct, but amounts differ inconsistently.The parameter order in both
generate_account_txnandgenerate_account_txn2is consistent across all calls: (sequence_number, sender, receivers, amount, gas_price, expiration_timestamp). However, funding amounts are inconsistent:
generate_account_txn(line 636, main batch): amount=1_000_000_000 (INITIAL_BALANCE), gas_price=1generate_account_txn(line 660, tail batch): amount=1, gas_price=10_000generate_account_txn2(line 566, batch): amount=1, gas_price=1 ← should match VM1's main batchgenerate_account_txn2(line 590, tail batch): amount=1, gas_price=10_000VM2's main batch (line 566) should use
INITIAL_BALANCE(1_000_000_000) instead of 1 to align with VM1's batch funding strategy. Verify this is intentional or unify both to use INITIAL_BALANCE.
| fn get_account_or_default2( | ||
| client: &RpcClient, | ||
| account_address: Option<AccountAddress2>, | ||
| ) -> Result<AccountInfo2> { | ||
| let account = match account_address { | ||
| None => { | ||
| let mut default_account = client.account_default2()?; | ||
| while default_account.is_none() { | ||
| std::thread::sleep(Duration::from_millis(1000)); | ||
| default_account = client.account_default2()?; | ||
| } | ||
|
|
||
| // let addr = default_account.clone().unwrap().address; | ||
| // let state_reader = client.state_reader2(StateRootOption::Latest)?; | ||
| // let mut balance = state_reader.get_balance2(addr)?; | ||
| // // balance resource has not been created | ||
| // while balance.is_none() { | ||
| // std::thread::sleep(Duration::from_millis(1000)); | ||
| // balance = state_reader.get_balance2(addr)?; | ||
| // info!("account balance is null."); | ||
| // } | ||
| default_account.unwrap() | ||
| } | ||
| Some(a) => match client.account_get2(a)? { | ||
| None => bail!("the specified account does not exists in the starcoin node"), | ||
| Some(w) => w, | ||
| }, | ||
| }; | ||
| info!("get_account_or_default for vm2: {}", account.address); | ||
| Ok(account) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unconditional VM2 dependency can hang VM1 runs; gate by vm_version and decouple TxnMocker
- get_account_or_default2 busy-waits before the ctrlc handler is installed; with vm_version=1 this can loop forever if VM2 has no default account.
- TxnMocker::new requires VM2 args even for VM1 path, forcing the above dependency.
Action:
- Make VM2 account fetch/existence conditional on vm_version==2, and adjust TxnMocker to accept Option<…> for VM2 (or provide separate constructors).
- Add bounded retries or a timeout to get_account_or_default2 to avoid indefinite startup hangs.
Also applies to: 205-226, 244-268
🤖 Prompt for AI Agents
In cmd/tx-factory/src/main.rs around lines 108-138 (also apply same changes at
205-226 and 244-268): currently get_account_or_default2 unconditionally
busy-waits for a VM2 default account and TxnMocker::new requires VM2 args even
when vm_version==1; change code so VM2 account fetching only runs when
vm_version == 2, refactor TxnMocker to accept an Option for the VM2 account (or
add a separate Vm1 constructor) so VM1 paths do not construct or block on VM2
resources, and modify get_account_or_default2 to use bounded retries or a
configurable timeout (returning an error if no account found after N attempts)
instead of looping indefinitely so startup cannot hang.
| let chain_reader_service = registry.service_ref::<ChainReaderService>().await?; | ||
| loop { | ||
| // generate_block.notify(DeterminedDagBlock)?; | ||
| let current_header = get_current_header(chain_reader_service.clone()).await?; | ||
| info!("jacktest: current_header: {}", current_header.number()); | ||
| if current_header.number() > 300 { | ||
| break; | ||
| } | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid unbounded busy-wait loops; add a timeout and backoff.
Both loops can spin indefinitely. Add a deadline and small sleep/backoff in the second loop to reduce CPU burn and avoid hangs.
Apply this diff minimally:
@@
- loop {
+ let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120);
+ loop {
// generate_block.notify(DeterminedDagBlock)?;
let current_header = get_current_header(chain_reader_service.clone()).await?;
info!("jacktest: current_header: {}", current_header.number());
if current_header.number() > 300 {
break;
}
+ if std::time::Instant::now() >= deadline {
+ bail!("timed out waiting for block height > 300");
+ }
tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await;
}
@@
- loop {
+ loop {
let transactions = txpool
.inner
.get_pending(100, config.net().time_service().now_secs())?;
if transactions.is_empty() {
break;
}
+ tokio::time::sleep(tokio::time::Duration::from_millis(200)).await;
}Also applies to: 227-234
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 187 to 196 (and
similarly for lines 227 to 234), the loop is an unbounded busy-wait that can
spin forever and burn CPU; add a deadline and a small backoff to avoid hangs.
Implement a timeout/deadline (e.g., record Instant::now() and bail with a test
failure or return Err after a configurable duration like 30s) and add a short
sleep inside the loop (e.g., 100–500ms) or exponential backoff between
iterations; use await-friendly tokio::time::sleep for the backoff and check the
elapsed time each iteration to break with a clear error when the deadline is
exceeded. Ensure both loops get the same treatment and return/propagate the
failure so tests fail fast instead of hanging.
| 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 { | ||
| return Err(anyhow::anyhow!( | ||
| "unexpected response type: expected SignedTxnList" | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid panics; return typed errors consistently in batch methods
- sign_txn_in_batch correctly returns Err on unexpected responses.
- unlock_account_in_batch still panics; align with the above and other non-panicking paths.
Apply this change:
- } else {
- panic!("Unexpect response type.")
- }
+ } else {
+ return Err(anyhow::anyhow!("unexpected response type: expected AccountList"));
+ }Additionally, prefer anyhow::Context on the .send(...).await?? chain for richer diagnostics.
Also applies to: 207-220
| /// unlock accounts for duration in seconds, default to u32::max. | ||
| #[rpc(name = "account.unlock_in_batch2")] | ||
| fn unlock_in_batch( | ||
| &self, | ||
| batch: Vec<(AccountAddress, String)>, | ||
| duration: Option<u32>, | ||
| ) -> FutureResult<Vec<AccountInfo>>; |
There was a problem hiding this comment.
Fix RPC naming inconsistency.
The RPC name uses account.unlock_in_batch2 while other methods in this file use the account2.* prefix pattern (e.g., account2.sign_txn_in_batch on line 47). This inconsistency could cause confusion for API consumers.
Apply this diff to fix the naming:
- #[rpc(name = "account.unlock_in_batch2")]
+ #[rpc(name = "account2.unlock_in_batch")]
fn unlock_in_batch(📝 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.
| /// unlock accounts for duration in seconds, default to u32::max. | |
| #[rpc(name = "account.unlock_in_batch2")] | |
| fn unlock_in_batch( | |
| &self, | |
| batch: Vec<(AccountAddress, String)>, | |
| duration: Option<u32>, | |
| ) -> FutureResult<Vec<AccountInfo>>; | |
| /// unlock accounts for duration in seconds, default to u32::max. | |
| #[rpc(name = "account2.unlock_in_batch")] | |
| fn unlock_in_batch( | |
| &self, | |
| batch: Vec<(AccountAddress, String)>, | |
| duration: Option<u32>, | |
| ) -> FutureResult<Vec<AccountInfo>>; |
🤖 Prompt for AI Agents
In vm2/rpc/api/src/account_api.rs around lines 62 to 68, the #[rpc(name =
"...")] attribute incorrectly uses "account.unlock_in_batch2" while other RPCs
use the "account2.*" prefix; change the RPC name to "account2.unlock_in_batch2"
so it matches the file's naming convention (update the attribute value only).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vm2/vm-runtime/src/starcoin_vm.rs (1)
752-770: Remove temporary debug logs before merge.These
info!logs with "jacktest" prefixes are temporary debugging scaffolding used to trace prologue execution flow. They should be removed before merging to a production branch.Apply this diff to remove the temporary logs:
- info!("StarcoinVM::run_prologue | jacktest: 0"); let (payload_type, script_or_package_hash, package_address) = match txn_data.payload() { TransactionPayloadMetadata::Script(hash) => { (TransactionPayloadType::Script, *hash, AccountAddress::ZERO) } TransactionPayloadMetadata::Package(hash, package_address) => { (TransactionPayloadType::Package, *hash, *package_address) } TransactionPayloadMetadata::ScriptFunction => ( TransactionPayloadType::ScriptFunction, HashValue::zero(), AccountAddress::ZERO, ), }; - info!("StarcoinVM::run_prologue | jacktest: 1"); let traversal_storage = TraversalStorage::new(); // Run prologue by genesis account - info!("StarcoinVM::run_prologue | jacktest: 2, txn_gas_price: {}, txn_max_gas_amount: {}", txn_gas_price, txn_max_gas_amount);
♻️ Duplicate comments (3)
sync/tests/full_build_and_execute_test.rs (3)
3-3: Fix invalid import:anyhow::Okdoes not exist.
RemoveOkfrom theuselist.-use anyhow::{bail, format_err, Ok, Result}; +use anyhow::{bail, format_err, Result};
169-180: Use the intendedgenesis_configwhen loadingNodeConfig.
You setopt.genesis_configbut callNodeConfig::load_with_opt(&global_opt), which ignores it.Minimal fix (reuse
opt):- let node_config = Arc::new(NodeConfig::load_with_opt(&global_opt)?); + let node_config = Arc::new(NodeConfig::load_with_opt(&opt)?);
201-216: Add deadlines to loops to avoid indefinite hangs.
All three loops can spin forever; add a timeout and keep the backoff.@@ - loop { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120); + loop { let current_header = get_current_header(chain_reader_service.clone()).await?; let default_account_balance = match get_balance(default_account.address, storage1.clone(), storage2.clone(), current_header.id()).await { std::result::Result::Ok(balance) => balance, Err(e) => { info!("get balance error: {} and waiting for the token initialization", e); tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; continue; } }; if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for initial balance"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; } @@ - loop { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120); + loop { let transactions = txpool .inner .get_pending(100, config.net().time_service().now_secs())?; if transactions.is_empty() { break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for txpool to drain (phase 1)"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; } @@ - loop { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120); + loop { let transactions = txpool .inner .get_pending(100, config.net().time_service().now_secs())?; if transactions.is_empty() { break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for txpool to drain (phase 2)"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; }Also applies to: 247-255, 282-290
🧹 Nitpick comments (4)
sync/tests/full_build_and_execute_test.rs (4)
88-127: Safer iteration: avoid indexing/unwrap; zip senders/receivers.
Reduces panic risk and clarifies flow.- let mut signed_transactions = vec![]; - for index in 0..receiver.len() { - let sender_address = sender.get(index).unwrap().address; - let next_seq = *next_seq_map.get(&sender_address).unwrap(); - next_seq_map - .entry(sender_address) - .and_modify(|next_seq| *next_seq += 1); - let transaction = build_batch_transfer_txn2( - sender.get(index).unwrap().address, - vec![receiver.get(index).unwrap().address], - next_seq, - amount, - 2, - 40_000_00, - expire_time, - header_block.chain_id().id().into(), - ); + let mut signed_transactions = Vec::with_capacity(receiver.len()); + for (s, r) in sender.iter().zip(receiver.iter()) { + let sender_address = s.address; + let next_seq = *next_seq_map + .get(&sender_address) + .expect("next sequence must be initialized"); + next_seq_map.entry(sender_address).and_modify(|seq| *seq += 1); + let transaction = build_batch_transfer_txn2( + sender_address, + vec![r.address], + next_seq, + amount, + 2, + 40_000_00, + expire_time, + header_block.chain_id().id().into(), + ); match account_service .send(AccountRequest::UnlockAccount( sender_address, "".to_string(), std::time::Duration::from_secs(100), )) .await?? { - AccountResponse::AccountInfo(_) => (), - _ => bail!("Unexpect response type."), + AccountResponse::AccountInfo(_) => (), + _ => bail!("Unexpected response type."), } let signed_transaction = match account_service .send(AccountRequest::SignTxn { txn: Box::new(transaction), signer: sender_address, }) .await?? { - AccountResponse::SignedTxn(signed_transction) => *signed_transction, - _ => bail!("Unexpect response type."), + AccountResponse::SignedTxn(s) => *s, + _ => bail!("Unexpected response type."), }; signed_transactions.push(signed_transaction); }Optional: unlock each unique sender once before the loop to avoid repeated unlocks.
152-154: Mark long-running benchmark as ignored.
Prevents defaultcargo testfrom running this heavy test.-// #[ignore = "This is a benchmark test, not a unit test"] -#[test] +#[ignore = "This is a benchmark test, not a unit test"] +#[test] fn test_full_build_and_execute_in_custom_network() -> Result<()> {
183-184: Set VM concurrency level once, early.
Call it before submitting work; remove the later call.- // StarcoinVM::set_concurrency_level_once(num_cpus::get()); + StarcoinVM::set_concurrency_level_once(num_cpus::get()); @@ - StarcoinVM::set_concurrency_level_once(num_cpus::get()); + // already set aboveAlso applies to: 257-257
31-48: Minor nits: typos and message text.
- Rename
accoun_number→account_number.- “Unexpect response type.” → “Unexpected response type.”
- Optional: rename
signed_transctionbinding →signed_txn.Also applies to: 50-62, 113-115, 124-125, 140-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/src/txpool_config.rs(1 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)vm2/aggregator/src/aggregator_v1_extension.rs(2 hunks)vm2/vm-runtime/src/errors.rs(1 hunks)vm2/vm-runtime/src/starcoin_vm.rs(3 hunks)vm2/vm-runtime/src/verifier/transaction_arg_validation.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#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/tests/full_build_and_execute_test.rs
🧬 Code graph analysis (1)
sync/tests/full_build_and_execute_test.rs (6)
config/src/lib.rs (6)
temp_dir(110-113)default(153-155)default(281-283)path(263-265)load_with_opt(294-319)load_with_opt(549-552)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(17-17)create_account(93-100)node/src/lib.rs (18)
config(128-132)storage2(140-144)new(78-92)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)vm2/vm-runtime/src/starcoin_vm.rs (3)
new(87-110)new(113-136)set_concurrency_level_once(1405-1410)vm2/service/account/src/service.rs (1)
registry(222-222)
🔇 Additional comments (3)
vm2/aggregator/src/aggregator_v1_extension.rs (1)
13-13: Verify the logging import is intentional.The import is valid, but ensure this is part of the intended logging strategy for VM2 and not just supporting the debug statement at line 372.
vm2/vm-runtime/src/verifier/transaction_arg_validation.rs (1)
20-20: LGTM: Logging import added.The import is correctly added to support runtime visibility.
vm2/vm-runtime/src/starcoin_vm.rs (1)
322-332: LGTM! Fixed error message for gas price validation.The warning message now correctly references "max" instead of "min" when the gas unit price exceeds the maximum bound, making the error message consistent with the validation logic.
| 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
Verify the 10,000x increase in default max_count and update outdated documentation.
The default transaction pool size has been increased from 4,096 to 40,960,000 (approximately 40 million), which is a 10,000x increase. This raises several concerns:
- Outdated documentation: Line 18 still states "default to 4096"
- Memory exhaustion risk: 40 million transactions could consume tens of gigabytes of memory, potentially exceeding available system resources
- Performance degradation: Operations on a pool this large (lookups, iteration, eviction, transaction selection) will be significantly slower
- Inconsistency with memory limits: The
max_mem_usagelimit may be exceeded before reaching 40 million transactions, making this count limit ineffective
If this change is intended for VM2 benchmark testing, consider:
- Using a test-specific configuration instead of modifying production defaults
- Adding a separate benchmark config profile
- Documenting why such a large value is necessary
- Choosing a more reasonable production default (e.g., 40,960 for a 10x increase instead)
Please confirm whether this massive increase is intentional for production use, and if so, update the documentation on line 18 and add comments explaining the rationale.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the file content around the relevant lines
echo "=== File content: config/src/txpool_config.rs ==="
cat config/src/txpool_config.rs | head -80
Length of output: 3057
🏁 Script executed:
#!/bin/bash
# Search for related test or benchmark configurations
echo "=== Searching for test or benchmark txpool configs ==="
fd -t f "txpool" -x grep -l "config\|benchmark\|test" {} \;Length of output: 120
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about transaction pool sizing
echo "=== Searching for txpool documentation ==="
find . -type f \( -name "*.md" -o -name "*.rs" \) -exec grep -l "max_count\|transaction pool" {} \; | head -20Length of output: 233
🏁 Script executed:
#!/bin/bash
# Search for references to the 40960000 value to understand context
echo "=== Searching for references to 40960000 ==="
rg -n "40960000" --type rsLength of output: 71
🏁 Script executed:
#!/bin/bash
# Search for max_mem_usage to understand memory limits
echo "=== Checking max_mem_usage configuration ==="
rg -n -A5 "max_mem_usage" config/src/txpool_config.rsLength of output: 1162
🏁 Script executed:
#!/bin/bash
# Check test-helper txpool configuration
echo "=== test-helper txpool config ==="
cat test-helper/src/txpool.rs | head -100Length of output: 3228
🏁 Script executed:
#!/bin/bash
# Search for transaction size or pool documentation
echo "=== Searching for transaction pool implementation details ==="
rg -n -C3 "struct.*Transaction" txpool/src/ -t rs | head -50Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for any size or memory-related calculations
echo "=== Searching for transaction sizing ==="
rg -n "size_of\|len\(\)\|sizeof" txpool/src/ -t rs | head -30Length of output: 67
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about 40960000 or large numbers
echo "=== Searching for large number references in txpool ==="
rg -n "[0-9]{7,}" txpool/ config/ -t rs | grep -v "test\|mock"Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check if there are separate config files for different environments
echo "=== Looking for environment-specific configs ==="
fd -t f "config" config/ | head -20Length of output: 725
🏁 Script executed:
#!/bin/bash
# Check the queue implementation to understand pool behavior
echo "=== Checking pool queue implementation ==="
cat txpool/src/pool/queue.rs | head -100Length of output: 3481
Update line 18 documentation to reflect the actual default value of 40,960,000.
Line 18 comment states "default to 4096" but the actual default on line 62 is 40,960,000. This documentation must be corrected.
The review comment's concerns about memory exhaustion and performance are somewhat overstated—the pool has a secondary memory-based limit that defaults to half of free system memory, which provides a safeguard. However, the documentation discrepancy is a definite issue requiring correction.
Update line 18 to:
/// Maximal number of transactions in the pool. default to 40960000
If the 10,000x increase is intentional, consider adding a comment explaining the rationale, especially if this supports specific use cases like VM2 benchmarking.
🤖 Prompt for AI Agents
In config/src/txpool_config.rs around lines 18 and 61-63, the doc comment on
line 18 incorrectly states the default is 4096 while the code returns
40,960,000; update the doc comment to read that the maximal number of
transactions defaults to 40960000 and, if the larger default is intentional,
append a brief rationale (e.g., VM2 benchmarking or specific workload support)
so readers understand why the value is 10,000x higher than the previous doc.
| let next_seq = statedb2 | ||
| .get_account_resource(*s.address())? | ||
| .sequence_number(); | ||
| next_seq_map.entry(*s.address()).or_insert(next_seq); |
There was a problem hiding this comment.
Compile fix: AccountInfo likely has field address, not method address().
Dereferencing s.address() will not compile; use the field.
- .get_account_resource(*s.address())?
+ .get_account_resource(s.address)?
.sequence_number();
- next_seq_map.entry(*s.address()).or_insert(next_seq);
+ next_seq_map.entry(s.address).or_insert(next_seq);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let next_seq = statedb2 | |
| .get_account_resource(*s.address())? | |
| .sequence_number(); | |
| next_seq_map.entry(*s.address()).or_insert(next_seq); | |
| let next_seq = statedb2 | |
| .get_account_resource(s.address)? | |
| .sequence_number(); | |
| next_seq_map.entry(s.address).or_insert(next_seq); |
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 82 to 85, replace the
incorrect method call and dereference (*s.address()) with the struct field
access (s.address); if Address isn't Copy, use s.address.clone() or otherwise
supply the correct owned/borrowed form to next_seq_map.entry, so the code
compiles with the AccountInfo.address field.
| /// Error for delta application. Can be used by delta partial functions | ||
| /// to return descriptive error messages and an appropriate error code. | ||
| fn abort_error(message: impl ToString, code: u64) -> PartialVMError { | ||
| info!("jacktest: abort error: {:?}", message.to_string()); |
There was a problem hiding this comment.
Remove debug prefix from production code.
The "jacktest:" prefix indicates this is temporary debugging code that should not be merged to production. This will clutter logs with test-prefixed messages.
If abort error logging is intended, apply this diff to clean it up:
- info!("jacktest: abort error: {:?}", message.to_string());
+ warn!("Aggregator abort error: {}", message.to_string());Otherwise, remove the log statement entirely:
- info!("jacktest: abort error: {:?}", message.to_string());📝 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.
| info!("jacktest: abort error: {:?}", message.to_string()); | |
| warn!("Aggregator abort error: {}", message.to_string()); |
| info!("jacktest: abort error: {:?}", message.to_string()); |
🤖 Prompt for AI Agents
In vm2/aggregator/src/aggregator_v1_extension.rs around line 372, the log call
uses a debug/test prefix "jacktest:" that must be removed for production; either
delete the entire info! call if the abort error should not be logged, or keep
the info! call but remove the "jacktest:" prefix and reword it to something like
"abort error:" (preserving the {:?} message formatting) so production logs are
clean and descriptive.
vm2/vm-runtime/src/errors.rs
Outdated
|
|
||
| pub fn convert_prologue_runtime_error(error: VMError) -> Result<(), VMStatus> { | ||
| let status = error.into_vm_status(); | ||
| info!("jacktest: convert_prologue_runtime_error status: {}", status); |
There was a problem hiding this comment.
Remove temporary debug log before merge.
This info! log with the "jacktest" prefix appears to be temporary debugging scaffolding. Debug logs should be removed or converted to debug! level before merging to a production branch.
Apply this diff to remove the temporary log:
- info!("jacktest: convert_prologue_runtime_error status: {}", status);📝 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.
| info!("jacktest: convert_prologue_runtime_error status: {}", status); |
🤖 Prompt for AI Agents
In vm2/vm-runtime/src/errors.rs around line 87, remove the temporary info! log
that reads "jacktest: convert_prologue_runtime_error status: {}" (or
alternatively convert it to debug! if you want to keep the message for local
debugging); update the file by deleting that info! line (or replacing info! with
debug!) so no temporary info-level debug log remains in the production branch.
| // function calls. We know the feature is active when more structs then just strings are | ||
| // allowed. | ||
| let are_struct_constructors_enabled = allowed_structs.len() > 1; | ||
| info!("jacktest: are_struct_constructors_enabled: {}", are_struct_constructors_enabled); |
There was a problem hiding this comment.
Remove or clean up the test/debug log statement.
The log statement contains a "jacktest:" prefix, indicating this is debug or test code. Test artifacts should be removed before merging to production.
Apply this diff to remove the debug log:
- info!("jacktest: are_struct_constructors_enabled: {}", are_struct_constructors_enabled);Alternatively, if this logging is intentional for production diagnostics, remove the "jacktest:" prefix and provide a more descriptive message:
- info!("jacktest: are_struct_constructors_enabled: {}", are_struct_constructors_enabled);
+ info!("Struct constructors enabled: {}", are_struct_constructors_enabled);📝 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.
| info!("jacktest: are_struct_constructors_enabled: {}", are_struct_constructors_enabled); |
| info!("jacktest: are_struct_constructors_enabled: {}", are_struct_constructors_enabled); | |
| info!("Struct constructors enabled: {}", are_struct_constructors_enabled); |
🤖 Prompt for AI Agents
In vm2/vm-runtime/src/verifier/transaction_arg_validation.rs around line 379,
remove the debug/test log statement or replace it with a production-safe
message; either delete the line containing info!("jacktest:
are_struct_constructors_enabled: {}", are_struct_constructors_enabled); or
change it to a clear, descriptive log without the "jacktest:" prefix (e.g.,
info!("are_struct_constructors_enabled = {}", are_struct_constructors_enabled);)
so no test artifact remains in production code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
sync/tests/full_build_and_execute_test.rs (4)
3-3: Compile fix: remove non-existentanyhow::Ok.
anyhow::Okisn't a thing;Okis from the std prelude.-use anyhow::{bail, format_err, Ok, Result}; +use anyhow::{bail, format_err, Result};
82-86: Compile fix: useAccountInfo.addressfield, notaddress().The struct exposes a field (and later lines use it already).
- .get_account_resource(*s.address())? + .get_account_resource(s.address)? .sequence_number(); - next_seq_map.entry(*s.address()).or_insert(next_seq); + next_seq_map.entry(s.address).or_insert(next_seq);
169-177: Ensure the intended genesis_config is used when loading NodeConfig.You're setting
opt.genesis_configbut loading withglobal_opt, which ignores it.Option B — simplest (preferred here):
- let node_config = Arc::new(NodeConfig::load_with_opt(&global_opt)?); + let node_config = Arc::new(NodeConfig::load_with_opt(&opt)?);(Alternatively, make
global_optmutable and set itsgenesis_configbefore load.)Also applies to: 179-179
247-255: Bound txpool-drain waits with deadlines to prevent hangs.Both loops can hang indefinitely; add a deadline check.
- loop { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(180); + loop { let transactions = txpool .inner .get_pending(100, config.net().time_service().now_secs())?; if transactions.is_empty() { break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for txpool to empty (first batch)"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; } @@ - loop { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(180); + loop { let transactions = txpool .inner .get_pending(100, config.net().time_service().now_secs())?; if transactions.is_empty() { break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for txpool to empty (second batch)"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; }Also applies to: 282-290
🧹 Nitpick comments (6)
sync/tests/full_build_and_execute_test.rs (6)
181-183: Prefer?overunwrap()in tests returningResult.Surface errors instead of panicking.
- let node = run_node_with_all_service(node_config.clone()).unwrap(); + let node = run_node_with_all_service(node_config.clone())?;
28-28: Remove unused importPeerId.Avoid warnings; keep imports minimal.
-use starcoin_vm2_vm_types::{state_view::StateReaderExt, PeerId, account_address::AccountAddress}; +use starcoin_vm2_vm_types::{state_view::StateReaderExt, account_address::AccountAddress};
31-37: Nit: parameter name typo.Use
account_numberfor clarity.-async fn create_account( - accoun_number: u32, +async fn create_account( + account_number: u32, @@ - for _ in 0..accoun_number { + for _ in 0..account_number {
132-139: Nit: fixserivicetypo in param.Keeps code tidy and searchable.
-async fn get_current_header( - chain_reader_serivice: ServiceRef<ChainReaderService>, +async fn get_current_header( + chain_reader_service: ServiceRef<ChainReaderService>, ) -> Result<BlockHeader> { - let current_header = match chain_reader_serivice + let current_header = match chain_reader_service
41-43: Nit: fix message typos (“Unexpected”).Minor text polish.
- _ => bail!("Unexpect response type."), + _ => bail!("Unexpected response type."), @@ - _ => bail!("Unexpect response type."), + _ => bail!("Unexpected response type."), @@ - AccountResponse::AccountInfo(_) => (), - _ => bail!("Unexpect response type."), + AccountResponse::AccountInfo(_) => (), + _ => bail!("Unexpected response type."), @@ - AccountResponse::SignedTxn(signed_transction) => *signed_transction, - _ => bail!("Unexpect response type."), + AccountResponse::SignedTxn(signed_transction) => *signed_transction, + _ => bail!("Unexpected response type."), @@ - _ => bail!("Unexpect response type."), + _ => bail!("Unexpected response type."),Also applies to: 57-59, 113-115, 123-125, 139-141
152-154: Gate the benchmark-style test in CI.Mark as ignored to avoid long runs by default; run with
cargo test -- --ignoredwhen needed.-// #[ignore = "This is a benchmark test, not a unit test"] +#[ignore] #[test] fn test_full_build_and_execute_in_custom_network() -> Result<()> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sync/tests/full_build_and_execute_test.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#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/tests/full_build_and_execute_test.rs
🧬 Code graph analysis (1)
sync/tests/full_build_and_execute_test.rs (6)
config/src/lib.rs (6)
temp_dir(110-113)default(153-155)default(281-283)path(263-265)load_with_opt(294-319)load_with_opt(549-552)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(17-17)create_account(93-100)node/src/lib.rs (19)
config(128-132)storage2(140-144)new(78-92)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)vm2/vm-runtime/src/starcoin_vm.rs (3)
new(87-110)new(113-136)set_concurrency_level_once(1405-1410)txpool/src/tx_pool_service_impl.rs (1)
new(41-83)
🔇 Additional comments (1)
sync/tests/full_build_and_execute_test.rs (1)
105-115: Code is correct; original review comment is inaccurate.The
AccountRequest::UnlockAccounthandler inaccount/service/src/service.rs(lines 149–154) definitively returnsAccountResponse::AccountInfo(Box::new(account_info)). The test code's match onAccountResponse::AccountInfo(_)is correct and will properly handle the response. WhileUnlockAccountResponseexists as an enum variant, it is not whatUnlockAccountreturns.Likely an incorrect or invalid review comment.
| let path = temp_dir(); | ||
| opt.base_data_dir = Some(path.clone()); | ||
| opt.genesis_config = Some("halley".to_string()); | ||
|
|
There was a problem hiding this comment.
Use a unique temp subdirectory and clean it up to avoid cross-test pollution.
Writing directly to the OS temp root risks collisions and leaves artifacts.
- let path = temp_dir();
- opt.base_data_dir = Some(path.clone());
+ let path = temp_dir().join(format!("starcoin_vm2_bench_{}", std::process::id()));
+ std::fs::create_dir_all(&path)?;
+ opt.base_data_dir = Some(path.clone());
@@
- // std::fs::remove_dir_all(path)?;
+ std::fs::remove_dir_all(path)?;Also applies to: 296-298
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 162-165 (and similarly
at 296-298) the test writes directly into the OS temp root which can cause
collisions and leftover artifacts; change it to create a unique temporary
subdirectory (e.g., use tempfile::tempdir() or create a uniquely named subdir
under std::env::temp_dir()), set opt.base_data_dir to that tempdir's path, and
ensure the tempdir is cleaned up when the test finishes (keep the TempDir value
in scope for the test or explicitly remove the directory at the end) so each
test gets an isolated, auto-deleted workspace.
| loop { | ||
| // generate_block.notify(DeterminedDagBlock)?; | ||
| let current_header = get_current_header(chain_reader_service.clone()).await?; | ||
| let default_account_balance = match get_balance(default_account.address, storage1.clone(), storage2.clone(), current_header.id()).await { | ||
| std::result::Result::Ok(balance) => balance, | ||
| Err(e) => { | ||
| info!("get balance error: {} and waiting for the token initialization", e); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | ||
| continue; | ||
| } | ||
| }; | ||
| if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas | ||
| break; | ||
| } | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | ||
| } |
There was a problem hiding this comment.
Add a timeout to avoid hanging while waiting for initial balance.
This loop can run forever if initialization fails; bail after a deadline.
- loop {
+ let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300);
+ loop {
// generate_block.notify(DeterminedDagBlock)?;
let current_header = get_current_header(chain_reader_service.clone()).await?;
let default_account_balance = match get_balance(default_account.address, storage1.clone(), storage2.clone(), current_header.id()).await {
std::result::Result::Ok(balance) => balance,
Err(e) => {
info!("get balance error: {} and waiting for the token initialization", e);
tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await;
continue;
}
};
if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas
break;
}
+ if std::time::Instant::now() >= deadline {
+ bail!("timed out waiting for default account balance initialization");
+ }
tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await;
}📝 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.
| loop { | |
| // generate_block.notify(DeterminedDagBlock)?; | |
| let current_header = get_current_header(chain_reader_service.clone()).await?; | |
| let default_account_balance = match get_balance(default_account.address, storage1.clone(), storage2.clone(), current_header.id()).await { | |
| std::result::Result::Ok(balance) => balance, | |
| Err(e) => { | |
| info!("get balance error: {} and waiting for the token initialization", e); | |
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | |
| continue; | |
| } | |
| }; | |
| if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas | |
| break; | |
| } | |
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | |
| } | |
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300); | |
| loop { | |
| // generate_block.notify(DeterminedDagBlock)?; | |
| let current_header = get_current_header(chain_reader_service.clone()).await?; | |
| let default_account_balance = match get_balance(default_account.address, storage1.clone(), storage2.clone(), current_header.id()).await { | |
| std::result::Result::Ok(balance) => balance, | |
| Err(e) => { | |
| info!("get balance error: {} and waiting for the token initialization", e); | |
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | |
| continue; | |
| } | |
| }; | |
| if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas | |
| break; | |
| } | |
| if std::time::Instant::now() >= deadline { | |
| bail!("timed out waiting for default account balance initialization"); | |
| } | |
| tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; | |
| } |
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 201 to 216, the loop
that polls get_balance can hang indefinitely; add a timeout/deadline check
(e.g., record start time and compare elapsed or use tokio::time::timeout) and
abort the wait when the deadline is exceeded, returning an Err (or failing the
test) with a descriptive message. Keep the existing retry/sleep behavior but
bail out after the configured duration (e.g., 30s or test-configurable) so the
test does not hang forever when initialization fails.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
txpool/src/tx_pool_service_impl.rs (1)
397-409: Remove or restrict thebypass_vm1_limitparameter from public API surfaces.The visibility change exposes a security issue: external callers can now bypass VM1 abuse prevention limits by setting
bypass_vm1_limit=true. The parameter should not be user-controllable—it should be computed automatically based on transaction type (as theEventHandlercorrectly does intxpool/src/lib.rsline 222-226).The bypass is accessible through two paths:
- Direct calls to the now-public
import_txns()methodTxPoolSyncService::add_txns_multi_signed()which passes the parameter throughRecommended fixes:
- Keep
import_txns()aspub(crate), OR- Remove
bypass_vm1_limitfromTxPoolSyncService::add_txns_multi_signed()and compute it internally from transaction type, OR- Add authorization checks to restrict who can set
bypass_vm1_limit=true(e.g., admin-only)
🧹 Nitpick comments (1)
txpool/src/tx_pool_service_impl.rs (1)
420-439: Exposing pending transaction retrieval looks reasonable.This read-only method supports the new batch RPC endpoints. The visibility change appears appropriate for the intended use case.
Consider documenting the performance implications of calling this with large
max_lenvalues, especially for external callers who might not be aware of the pool size.
📜 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 (3)
sync/Cargo.toml(2 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)txpool/src/tx_pool_service_impl.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
PR: starcoinorg/starcoin#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:
sync/Cargo.toml
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#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/tests/full_build_and_execute_test.rs
🧬 Code graph analysis (1)
sync/tests/full_build_and_execute_test.rs (5)
config/src/lib.rs (6)
temp_dir(110-113)default(153-155)default(281-283)path(263-265)load_with_opt(294-319)load_with_opt(549-552)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(17-17)create_account(93-100)node/src/lib.rs (19)
config(128-132)storage2(140-144)new(78-92)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)genesis(146-150)vm2/vm-runtime/src/starcoin_vm.rs (3)
new(87-110)new(113-136)set_concurrency_level_once(1405-1410)
🔇 Additional comments (6)
sync/tests/full_build_and_execute_test.rs (5)
140-151: LGTM!The helper function correctly retrieves the current chain header using the service request/response pattern.
153-158: LGTM!The balance retrieval logic correctly constructs the state view and queries the account balance.
273-273: Verify intentional placement of concurrency level setting.Setting the VM concurrency level between the two transfer batches appears intentional for benchmarking (measuring with/without parallelism). If this is deliberate, consider adding a comment explaining the benchmark design.
319-339: LGTM!The
TransactionExecutionResultenum cleanly models transaction states with timestamps and provides a clear Debug implementation.
411-441: LGTM!The event handlers correctly track transaction state transitions across DAG block events and txpool status updates, with appropriate error logging.
txpool/src/tx_pool_service_impl.rs (1)
561-574: No evidence of misuse; review comment addresses speculative future risks.All current usages of
get_pool_client()follow the recommended pattern: each operation creates a freshPoolClientbound to the current chain state. The method is called within public wrapper methods (verify_transaction,get_pending), but no direct external calls toget_pool_client()were found in the codebase. External callers invoking these public methods will receive a fresh client, maintaining state consistency as intended. The concerns raised in the review comment are speculative about potential future misuse rather than addressing observable issues in the current codebase. Verify whether this public method exposure is new to this PR and, if so, whether documentation should be added to clarify the intended external API contract.
| hex = { workspace = true } | ||
| starcoin-miner = { workspace = true } | ||
| starcoin-account-api = { workspace = true } | ||
| starcoin-vm2-account-api = { workspace = true } |
There was a problem hiding this comment.
🧩 Analysis chain
Approve additions of VM2-related dev-dependencies.
The new dependencies in lines 62 and 75-80 support the PR's VM2 benchmarking and batch operation objectives. Placement in [dev-dependencies] is appropriate, and the use of { workspace = true } maintains consistency with other dependencies.
However, please verify the following:
-
Actual usage: Confirm that all added dependencies (starcoin-vm2-account-api, starcoin-account-service, starcoin-vm2-account-service, starcoin-vm2-statedb, starcoin-vm2-vm-runtime, starcoin-rpc-client, chrono) are actually referenced in sync's test or benchmark code. Adding unused dependencies increases build time and can mask dependency resolution issues.
-
Circular dependencies: Verify that none of these dependencies depend on
starcoin-sync. If any do, this introduces a circular dependency, which can cause build failures or versioning conflicts. -
Runtime vs. dev classification: Confirm that these belong in
[dev-dependencies]and not in[dependencies]. For example, ifstarcoin-syncusesstarcoin-account-serviceorstarcoin-rpc-clientat runtime (not just for tests), they should be promoted to main dependencies.
Run the following script to help verify:
Also applies to: 75-80
🏁 Script executed:
#!/bin/bash
# Description: Verify that added dev-dependencies are actually used in sync tests/benchmarks
# and check for circular dependency patterns.
# Find all test/benchmark files in sync crate
echo "=== Test and benchmark files in sync crate ==="
find . -path ./target -prune -o -name "*.rs" -print | xargs grep -l "mod test\|#\[test\]\|#\[bench\]" | grep "sync"
# Check if any of the added dependencies mention starcoin-sync in their Cargo.toml
echo ""
echo "=== Checking for circular dependencies (added deps that depend on starcoin-sync) ==="
for dep in "starcoin-vm2-account-api" "starcoin-account-service" "starcoin-vm2-account-service" "starcoin-vm2-statedb" "starcoin-vm2-vm-runtime" "starcoin-rpc-client"; do
echo "Checking $dep..."
fd -e "Cargo.toml" | xargs grep -l "^name = \"$dep\"" | xargs grep "starcoin-sync" && echo " Found: $dep depends on starcoin-sync" || echo " OK: No circular dependency detected"
done
# Show which added dependencies are actually used in sync source
echo ""
echo "=== Usage of added dependencies in sync/src ==="
for dep in "vm2_account_api" "account_service" "vm2_account_service" "vm2_statedb" "vm2_vm_runtime" "rpc_client"; do
count=$(find . -path ./target -prune -o -path "*/sync/src/*.rs" -print | xargs grep -c "$dep" 2>/dev/null || echo "0")
echo "$dep: $count occurrences"
doneLength of output: 8241
🏁 Script executed:
#!/bin/bash
# Check if unused dependencies are actually used in sync tests or inline test code
echo "=== Checking sync/tests directory for unused dependencies ==="
for dep in "vm2_account_api" "account_service" "vm2_account_service" "vm2_statedb" "vm2_vm_runtime"; do
echo ""
echo "Searching for $dep in sync/tests..."
find ./sync/tests -name "*.rs" -exec grep -l "$dep" {} \; 2>/dev/null || echo " Not found in sync/tests"
done
echo ""
echo ""
echo "=== Checking inline test modules for unused dependencies ==="
for dep in "vm2_account_api" "account_service" "vm2_account_service" "vm2_statedb" "vm2_vm_runtime"; do
echo ""
echo "Searching for $dep in #[cfg(test)] blocks..."
rg -n "#\[cfg\(test\)\]" -A 200 --type rs --path-separator / | grep -B5 -A5 "$dep" 2>/dev/null || true
done
echo ""
echo ""
echo "=== Verify rpc_client is used at runtime (not just in tests) ==="
echo "Checking rpc_client in sync.rs (8 occurrences found)..."
grep -n "rpc_client" ./sync/src/sync.rs | head -10
echo ""
echo "Checking rpc_client in announcement/mod.rs (3 occurrences found)..."
grep -n "rpc_client" ./sync/src/announcement/mod.rs | head -5
echo ""
echo "Checking if these are in test blocks or runtime code..."
grep -B5 -A5 "rpc_client" ./sync/src/verified_rpc_client.rs | head -30Length of output: 1344
🏁 Script executed:
#!/bin/bash
# Check if unused dependencies are used in sync tests or inline test code
echo "=== Checking for five unused dependencies in sync/tests ==="
for dep in "vm2_account_api" "account_service" "vm2_account_service" "vm2_statedb" "vm2_vm_runtime"; do
echo "Searching for $dep in sync/tests..."
count=$(find ./sync/tests -name "*.rs" 2>/dev/null | xargs grep -c "$dep" 2>/dev/null || echo "0")
echo " Found in tests: $count occurrences"
done
echo ""
echo "=== Checking for inline test usage ==="
for dep in "vm2_account_api" "account_service" "vm2_account_service" "vm2_statedb" "vm2_vm_runtime"; do
echo "Searching for $dep in #[cfg(test)] blocks..."
count=$(find ./sync/src -name "*.rs" -exec grep -l "#\[cfg(test)\]" {} \; 2>/dev/null | xargs grep -c "$dep" 2>/dev/null || echo "0")
echo " Found in test blocks: $count occurrences"
done
echo ""
echo "=== Verify rpc_client context (runtime import vs test-only) ==="
echo "File: sync/src/sync.rs"
grep -n "rpc_client" ./sync/src/sync.rs 2>/dev/null | head -5
echo ""
echo "File: sync/src/verified_rpc_client.rs"
head -30 ./sync/src/verified_rpc_client.rs 2>/dev/null | grep -n "rpc_client" || echo "Checking context..."
head -50 ./sync/src/verified_rpc_client.rs 2>/dev/null
echo ""
echo "=== Check chrono usage (line 80 addition) ==="
count=$(find ./sync -name "*.rs" | xargs grep -c "chrono" 2>/dev/null || echo "0")
echo "chrono occurrences in sync: $count"
find ./sync -name "*.rs" | xargs grep -n "chrono" 2>/dev/null | head -5Length of output: 7033
Move starcoin-rpc-client from [dev-dependencies] to [dependencies].
The verification reveals that starcoin-rpc-client is used at runtime in sync/src/sync.rs (imports and calls like fetch_blocks() at lines 241, 271, 318, 497), but is currently placed in [dev-dependencies] at line 79. This is a critical misclassification—runtime dependencies must be in [dependencies].
The five other new dev-dependencies (starcoin-vm2-account-api, starcoin-account-service, starcoin-vm2-account-service, starcoin-vm2-statedb, starcoin-vm2-vm-runtime) are correctly placed in [dev-dependencies] as they are used only in sync/tests/full_build_and_execute_test.rs. Chrono is also appropriately dev-only. No circular dependencies detected.
🤖 Prompt for AI Agents
In sync/Cargo.toml around lines 62-62: the crate starcoin-rpc-client is
currently listed under [dev-dependencies] (around line 79) but is used at
runtime in sync/src/sync.rs; move the starcoin-rpc-client entry out of
[dev-dependencies] and add the same dependency specification under
[dependencies] (preserve any version or workspace = true attribute it currently
has), removing the dev-dependencies entry so the crate is available for normal
builds.
| use std::io::{self, Write}; | ||
|
|
||
| async fn create_account( | ||
| accoun_number: u32, |
There was a problem hiding this comment.
Fix typo in parameter name.
The parameter name accoun_number is missing a 't'.
Apply this diff:
- accoun_number: u32,
+ account_number: u32,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around line 40, the
struct/parameter name `accoun_number` has a typo and should be `account_number`;
rename the field to `account_number` and update all references in this file (and
any related test helpers or pattern matches) to use the corrected identifier so
the code compiles and tests refer to the consistent name.
| fn dump_results(&mut self) -> Result<()> { | ||
| let mut file = OpenOptions::new().write(true).create(true).truncate(true).open("/Users/jack/Documents/code/rust/flexidag/starcoin-jack/main/starcoin/sync/transaction_results.txt")?; | ||
| for (transaction, results) in &self.transaction_data { | ||
| writeln!(file, "transaction id: {}, results: {:?}", *transaction, results)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Replace hardcoded absolute path with portable alternative.
The hardcoded path /Users/jack/Documents/... will fail on other machines, CI, and non-macOS systems.
Apply this diff to use a temporary file:
fn dump_results(&mut self) -> Result<()> {
- let mut file = OpenOptions::new().write(true).create(true).truncate(true).open("/Users/jack/Documents/code/rust/flexidag/starcoin-jack/main/starcoin/sync/transaction_results.txt")?;
+ let output_path = std::env::temp_dir().join("starcoin_transaction_results.txt");
+ let mut file = OpenOptions::new().write(true).create(true).truncate(true).open(output_path.clone())?;
+ info!("Transaction results written to: {:?}", output_path);
for (transaction, results) in &self.transaction_data {🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 367 to 373, the test
currently writes to a hardcoded absolute path which is non-portable; replace
that with a temporary file created at runtime (e.g., use std::env::temp_dir with
a unique filename or use the tempfile crate's NamedTempFile) and write the
results to that temp file instead; ensure the function opens/creates the temp
file via the portable API, writes the same content, and returns Ok(()) (or
returns the temp file path if callers need it) so the test no longer depends on
a machine-specific path.
| fn stopped(&mut self, ctx: &mut ServiceContext<Self>) -> Result<()> { | ||
| // ctx.unsubscribe::<NewHeadBlock>(); | ||
| ctx.unsubscribe::<NewDagBlock>(); | ||
| ctx.unsubscribe::<NewDagBlockFromPeer>(); | ||
|
|
||
| if let Err(e) = self.dump_results() { | ||
| error!("failed to dump the reuslts: {:?}", e); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Fix typo in error message.
Line 400 has a typo: "reuslts" should be "results".
Apply this diff:
- error!("failed to dump the reuslts: {:?}", e);
+ error!("failed to dump the results: {:?}", e);🤖 Prompt for AI Agents
In sync/tests/full_build_and_execute_test.rs around lines 394 to 403, the error
log message in stopped() contains a typo "reuslts"; change the string to
"results" so the processLogger/error call reads "failed to dump the results:
{:?}", keeping the existing error formatting and semantics intact.
| pub fn queue(&self) -> Arc<TxnQueue> { | ||
| self.queue.clone() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify access control at call sites and document usage constraints.
Exposing direct access to the internal transaction queue widens the API surface and could allow unintended manipulation if not carefully controlled. Ensure that all external callers have appropriate authorization checks and cannot bypass the intended transaction flow.
Run the following script to identify all external callers and verify proper access control:
Consider adding documentation to clarify:
- Intended usage patterns for external callers
- Any constraints or invariants that must be maintained
- Whether direct queue manipulation is allowed or if this is read-only access
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of the newly public queue() method to verify access control
# Search for calls to queue() method on Inner or TxPoolService
rg -nP --type=rust -C3 '\b(inner|tx_pool_service|txpool_service)\.queue\s*\(' -g '!**/test/**'Length of output: 580
🏁 Script executed:
#!/bin/bash
# Check for any usages without the test exclusion to ensure nothing was missed
# Also look for direct queue() calls regardless of object name
rg -nP --type=rust '\.queue\s*\(' -C2
echo "=== Checking other visibility changes ==="
# Search for calls to the other three changed methods
rg -nP --type=rust '\.(import_txns|get_pending|get_pool_client)\s*\(' -C2 | head -50Length of output: 3659
🏁 Script executed:
#!/bin/bash
# Verify the TxnQueue type and add_full_listener method to understand what's being exposed
# Also check if there are guard rails on queue access
cd txpool
fd -e rs -x grep -l "struct TxnQueue\|type TxnQueue\|trait.*TxnQueue" {} \;Length of output: 29
🏁 Script executed:
#!/bin/bash
# Find TxnQueue type definition and understand what operations are publicly available
rg -nP --type=rust 'type\s+TxnQueue|struct\s+TxnQueue' -A 5 txpool/src/
echo "=== Check TxnQueue/TransactionQueue definition ==="
# Find the actual type alias or struct definition
rg -nP --type=rust 'TxnQueue\s*=' txpool/src/
echo "=== Look for add_full_listener method to understand what queue exposes ==="
rg -nP --type=rust 'fn add_full_listener' -B2 -A8
echo "=== Check if wrapper methods add validation before calling inner methods ==="
# Look at batch_import_transactions wrapper in lib.rs vs inner.import_txns
grep -nP -A10 'pub fn batch_import_transactions|pub fn handle_batch_import_transactions' txpool/src/lib.rs | head -40Length of output: 2208
🏁 Script executed:
#!/bin/bash
# Get the complete implementation of wrapper methods to check for validation
echo "=== TxPoolService.batch_import_transactions (wrapper) ==="
sed -n '210,240p' txpool/src/lib.rs
echo ""
echo "=== TxPoolService.batch_get_pending (wrapper) ==="
sed -n '80,95p' txpool/src/lib.rs
echo ""
echo "=== Inner.import_txns (inner method being exposed) ==="
sed -n '397,415p' txpool/src/tx_pool_service_impl.rs
echo ""
echo "=== Inner.get_pending (inner method being exposed) ==="
sed -n '420,430p' txpool/src/tx_pool_service_impl.rs
echo ""
echo "=== Check what bypass_vm1_limit does ==="
rg -nP --type=rust 'bypass_vm1_limit' txpool/src/ -B2 -A2 | head -40Length of output: 5126
Restrict public access to import_txns() or add guard logic—bypass_vm1_limit parameter must not be controllable without validation.
The visibility change exposes a security concern: import_txns() takes a bypass_vm1_limit parameter that allows bypassing VM1 transaction limits. The wrapper method in lib.rs:222–230 carefully guards this parameter, setting it to true only when all transactions are VM2. Making import_txns() public allows external callers to invoke it directly with bypass_vm1_limit=true for non-VM2 transactions, circumventing the intended validation logic.
The queue() method has minimal production usage (test-only), so its exposure is lower risk, but it still exposes implementation details without documented constraints.
Action required:
- Either keep
import_txns()aspub(crate)and route all external calls through the wrapper inlib.rs, or - Add guard logic directly in
import_txns()to validate thatbypass_vm1_limitis only set for VM2 transactions - Document any usage constraints for the newly public methods
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (12)
cmd/tx-factory/src/main.rs (3)
108-138: Infinite busy-wait remains unresolved from previous review.The indefinite loop at lines 115-118 will hang the process if VM2 never provides a default account. This was flagged in the previous review and remains unaddressed.
Apply bounded retries with a configurable timeout:
-fn get_account_or_default2( +fn get_account_or_default2( client: &RpcClient, account_address: Option<AccountAddress2>, + max_retries: u32, ) -> Result<AccountInfo2> { let account = match account_address { None => { - let mut default_account = client.account_default2()?; - while default_account.is_none() { - std::thread::sleep(Duration::from_millis(1000)); - default_account = client.account_default2()?; - } + let mut default_account = None; + for attempt in 0..max_retries { + default_account = client.account_default2()?; + if default_account.is_some() { + break; + } + if attempt < max_retries - 1 { + std::thread::sleep(Duration::from_millis(1000)); + } + } + if default_account.is_none() { + bail!("VM2 default account not found after {} retries", max_retries); + } // let addr = default_account.clone().unwrap().address;
205-209: Unconditional VM2 account fetch still blocks VM1 runs.Line 206 fetches the VM2 account regardless of
vm_version, causing VM1-only runs to hang if no VM2 account exists. This was flagged in the previous review and remains unfixed.Make VM2 account fetching conditional:
let account = get_account_or_default(&client, account_address).unwrap(); - let account2 = get_account_or_default2(&client, account_address2).unwrap(); + let account2 = if vm_version == 2 { + Some(get_account_or_default2(&client, account_address2, 30).unwrap()) + } else { + None + }; let receiver_address = opts.receiver_address.unwrap_or_else(association_address); - let receiver_address2 = opts.receiver_address2.unwrap_or_else(association_address2); + let receiver_address2 = if vm_version == 2 { + Some(opts.receiver_address2.unwrap_or_else(association_address2)) + } else { + None + };
212-228: TxnMocker still requires VM2 args for VM1 paths.Both
MockTxnGeneratorandTxnMockerconstructors require VM2 accounts and addresses even whenvm_version == 1. This tight coupling was flagged in the previous review.Refactor to accept
Optiontypes or provide separate constructors:// In txn_generator.rs let txn_generator = if vm_version == 2 { MockTxnGenerator::new_dual( net.chain_id(), account.clone(), receiver_address, account2.unwrap().clone(), receiver_address2.unwrap(), ) } else { MockTxnGenerator::new_vm1( net.chain_id(), account.clone(), receiver_address, ) }; // Similar adjustment for TxnMocker::newsync/tests/full_build_and_execute_test.rs (9)
3-3: Fix invalid import:anyhow::Okdoes not exist.The import statement includes
Okfromanyhow, butOkis provided by the Rust prelude and cannot be imported fromanyhow. This will cause a compilation error.Apply this diff:
-use anyhow::{bail, format_err, Ok, Result}; +use anyhow::{bail, format_err, Result};
47-47: Fix typo in parameter name.The parameter
accoun_numberis missing a 't' and should beaccount_number.Apply this diff:
- accoun_number: u32, + account_number: u32,
97-100: Compile fix:AccountInfohas fieldaddress, not methodaddress().Lines 97-100 attempt to call
s.address()as a method and dereference the result, butAccountInfohas anaddressfield (as evidenced by line 105 which uses field access). This will cause a compilation error.Apply this diff:
- let next_seq = statedb2 - .get_account_resource(*s.address())? - .sequence_number(); - next_seq_map.entry(*s.address()).or_insert(next_seq); + let next_seq = statedb2 + .get_account_resource(s.address)? + .sequence_number(); + next_seq_map.entry(s.address).or_insert(next_seq);
182-183: Use a unique temp subdirectory to avoid test collisions.Writing directly to the OS temp root (
temp_dir()) can cause collisions with other tests and leaves artifacts. Create a unique subdirectory instead.Apply this diff:
- let path = temp_dir(); + let path = temp_dir().join(format!("starcoin_vm2_bench_{}", std::process::id())); + std::fs::create_dir_all(&path)?; opt.base_data_dir = Some(path.clone());Also applies to: 337-337
184-184: Use the intended genesis_config when loading NodeConfig.You set
opt.genesis_configon line 184, but then callNodeConfig::load_with_opt(&global_opt)on line 199, which ignores that configuration sinceglobal_optdoesn't havegenesis_configset.Apply this diff to set
genesis_configonglobal_opt:- let global_opt = StarcoinOpt { + let mut global_opt = StarcoinOpt { net: Some(ChainNetworkID::Custom(CustomNetworkID::new( "my_chain".to_string(), ChainId::new(121), ))), base_data_dir: Some(path.clone()), ..Default::default() }; + global_opt.genesis_config = opt.genesis_config.clone();Also applies to: 189-199
223-249: Add a timeout to avoid hanging while waiting for initial balance.This loop can run indefinitely if token initialization fails. Add a deadline and bail with a clear error message after a reasonable timeout.
Apply this diff:
let chain_reader_service = registry.service_ref::<ChainReaderService>().await?; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300); loop { // generate_block.notify(DeterminedDagBlock)?; let current_header = get_current_header(chain_reader_service.clone()).await?; let default_account_balance = match get_balance( default_account.address, storage1.clone(), storage2.clone(), current_header.id(), ) .await { std::result::Result::Ok(balance) => balance, Err(e) => { info!( "get balance error: {} and waiting for the token initialization", e ); tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; continue; } }; if default_account_balance > account_count as u128 * initial_balance + initial_gas_fee { // get enough token to pay for gas break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for default account balance initialization"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; }
286-294: Add timeouts to txpool drain loops to prevent hanging.Both loops that wait for the txpool to drain can spin indefinitely if transactions remain pending. Add a deadline to each loop.
Apply this diff for the first loop:
+ let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120); loop { let transactions = txpool .inner .get_pending(100, config.net().time_service().now_secs())?; if transactions.is_empty() { break; } + if std::time::Instant::now() >= deadline { + bail!("timed out waiting for txpool to drain (first round)"); + } tokio::time::sleep(tokio::time::Duration::from_millis(3000)).await; }Apply the same pattern to the second loop at lines 321-329.
Also applies to: 321-329
410-420: Replace hardcoded absolute path with portable alternative.The hardcoded path
/Users/jack/Documents/...will fail on other machines, CI systems, and non-macOS platforms.Apply this diff:
fn dump_results(&mut self) -> Result<()> { - let mut file = OpenOptions::new().write(true).create(true).truncate(true).open("/Users/jack/Documents/code/rust/flexidag/starcoin-jack/main/starcoin/sync/transaction_results.txt")?; + let output_path = std::env::temp_dir().join("starcoin_transaction_results.txt"); + let mut file = OpenOptions::new().write(true).create(true).truncate(true).open(&output_path)?; + info!("Transaction results written to: {:?}", output_path); for (transaction, results) in &self.transaction_data {
446-446: Fix typo in error message.The error message contains "reuslts" which should be "results".
Apply this diff:
- error!("failed to dump the reuslts: {:?}", e); + error!("failed to dump the results: {:?}", e);
🧹 Nitpick comments (3)
kube/manifest/starcoin-halley.yaml (1)
32-33: The startup script uses block scalar syntax but remains a single unbroken bash line; add error handling and fix redundant cleanup.The YAML
|-block scalar is an improvement (addressing the prior review suggestion), but the script content is still a single continuous line without line breaks. Additionally, there are two issues:
- Redundant
rmcommands:rm -rf /sc-data/halleyalready removes the directory, making subsequentrm -rf /sc-data/halley/...calls unnecessary.- Missing error handling: No
set -eor error traps. If array indexing fails (e.g.,$idout of bounds), or if NODE_KEYS parsing fails, the starcoin process starts with uninitialized/incorrect variables, potentially corrupting state.Refactor to add line breaks and error handling:
args: - - | - rm -rf /sc-data/halley; rm -rf /sc-data/halley/starcoin.ipc /sc-data/halley/starcoindb/db/starcoindb/LOCK /sc-data/halley/genesis_config.json; id=$(echo -e $POD_NAME | awk -F'-' '{print $2}'); IFS='; ' read -r -a node_keys <<< "$NODE_KEYS"; node_key=${node_keys[$id]}; node_key_flag=""; if [ -n "$node_key" ]; then node_key_flag="--node-key ${node_key}"; fi; miner_thread_flag=""; { case "$POD_NAME" in starcoin-0) miner_thread_flag="--miner-thread=1" ;; starcoin-1) miner_thread_flag="--miner-thread=2" ;; starcoin-2) miner_thread_flag="--miner-thread=3" ;; *) miner_thread_flag="" ;; esac; }; /starcoin/starcoin -n halley -d /sc-data --p2prpc-default-global-api-quota 9000/s --p2prpc-custom-user-api-quota get_header_by_hash=9000/s --p2prpc-custom-user-api-quota get_headers_by_hash=9000/s --p2prpc-custom-user-api-quota info=9000/s --p2prpc-custom-user-api-quota get_block_by_hash=9000/s --p2prpc-custom-user-api-quota get_block_ids=9000/s --p2prpc-custom-user-api-quota get_blocks=9000/s --jsonrpc-default-global-api-quota 9000/s --jsonrpc-custom-user-api-quota chain.get_headers_by_hash=9000/s --jsonrpc-custom-user-api-quota chain.get_header_by_hash=9000/s --jsonrpc-custom-user-api-quota chain.info=9000/s --jsonrpc-custom-user-api-quota chain.get_block_by_hash=9000/s --jsonrpc-custom-user-api-quota chain.get_block_ids=9000/s --jsonrpc-custom-user-api-quota chain.get_blocks=9000/s --min-peers-to-propagate 512 --max-peers-to-propagate 1024 --max-outgoing-peers 512 --max-incoming-peers 512 --txpool-tx-propagate-interval 1 --discover-local=true $node_key_flag $miner_thread_flag + - |- + set -e + + # Clean up data directory + rm -rf /sc-data/halley + + # Extract pod index and select node key + pod_id=$(echo "$POD_NAME" | awk -F'-' '{print $NF}') + IFS='; ' read -r -a node_keys <<< "$NODE_KEYS" + node_key="${node_keys[$pod_id]:-}" + + # Build node key flag + node_key_flag="" + [ -n "$node_key" ] && node_key_flag="--node-key $node_key" + + # Determine miner thread count from pod name + miner_thread_flag="" + case "$POD_NAME" in + starcoin-0) miner_thread_flag="--miner-thread=1" ;; + starcoin-1) miner_thread_flag="--miner-thread=2" ;; + starcoin-2) miner_thread_flag="--miner-thread=3" ;; + esac + + # Launch starcoin with VM2 benchmark flags + exec /starcoin/starcoin \ + -n halley \ + -d /sc-data \ + --p2prpc-default-global-api-quota 9000/s \ + --p2prpc-custom-user-api-quota get_header_by_hash=9000/s \ + --p2prpc-custom-user-api-quota get_headers_by_hash=9000/s \ + --p2prpc-custom-user-api-quota info=9000/s \ + --p2prpc-custom-user-api-quota get_block_by_hash=9000/s \ + --p2prpc-custom-user-api-quota get_block_ids=9000/s \ + --p2prpc-custom-user-api-quota get_blocks=9000/s \ + --jsonrpc-default-global-api-quota 9000/s \ + --jsonrpc-custom-user-api-quota chain.get_headers_by_hash=9000/s \ + --jsonrpc-custom-user-api-quota chain.get_header_by_hash=9000/s \ + --jsonrpc-custom-user-api-quota chain.info=9000/s \ + --jsonrpc-custom-user-api-quota chain.get_block_by_hash=9000/s \ + --jsonrpc-custom-user-api-quota chain.get_block_ids=9000/s \ + --jsonrpc-custom-user-api-quota chain.get_blocks=9000/s \ + --min-peers-to-propagate 512 \ + --max-peers-to-propagate 1024 \ + --max-outgoing-peers 512 \ + --max-incoming-peers 512 \ + --txpool-tx-propagate-interval 1 \ + --discover-local=true \ + $node_key_flag \ + $miner_thread_flagKey improvements:
set -eensures the script exits on errors- Removed redundant second
rmcall- Eliminated unnecessary
echo -eflag- Use
${node_keys[$pod_id]:-}to safely handle array access- Split into logical sections with comments for maintenance
- Line breaks improve readability significantly
execreplaces the shell process with starcoin, improving signal handlingcmd/tx-factory/src/main.rs (2)
120-128: Remove commented-out dead code.The commented balance-checking logic has been disabled but left in place. Consider removing it to improve readability.
Also applies to: 152-160
187-189: Dispatch logic is sound; redundant panic can be simplified.The VM version validation at lines 187-189 ensures
vm_versionis 1 or 2, making the panic at line 267 unreachable. You can simplify to anelsewithout the condition check.Apply this diff:
let handle = if 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 { + } else { 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"); };Otherwise, the conditional dispatch correctly routes to VM1 or VM2 pressure tests.
Also applies to: 244-268
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/tx-factory/src/main.rs(7 hunks)config/src/txpool_config.rs(2 hunks)kube/manifest/starcoin-halley.yaml(2 hunks)sync/tests/full_build_and_execute_test.rs(1 hunks)vm2/vm-runtime/src/errors.rs(1 hunks)vm2/vm-runtime/src/starcoin_vm.rs(3 hunks)vm2/vm-runtime/src/verifier/transaction_arg_validation.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- vm2/vm-runtime/src/errors.rs
- config/src/txpool_config.rs
- vm2/vm-runtime/src/verifier/transaction_arg_validation.rs
- vm2/vm-runtime/src/starcoin_vm.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
PR: starcoinorg/starcoin#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/tests/full_build_and_execute_test.rs
🧬 Code graph analysis (2)
cmd/tx-factory/src/main.rs (2)
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)
sync/tests/full_build_and_execute_test.rs (9)
chain/src/chain.rs (7)
info(1146-1152)new(93-105)current_header(1166-1168)e(247-247)e(1975-1975)dag(259-261)dag(1653-1655)config/src/lib.rs (6)
temp_dir(110-113)default(153-155)default(281-283)path(263-265)load_with_opt(294-319)load_with_opt(549-552)test-helper/src/node.rs (1)
run_node_with_all_service(24-28)vm2/account/api/src/service.rs (2)
create_account(17-17)create_account(93-100)node/src/lib.rs (20)
config(128-132)storage2(140-144)new(78-92)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)genesis(146-150)vm2/vm-runtime/src/starcoin_vm.rs (3)
new(87-110)new(113-136)set_concurrency_level_once(1408-1413)txpool/src/tx_pool_service_impl.rs (1)
new(41-83)vm2/rpc/server/src/account_rpc.rs (3)
get(120-128)default(80-88)create(100-108)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)
🔇 Additional comments (2)
kube/manifest/starcoin-halley.yaml (1)
50-57: Verify the readiness probe removal does not break service traffic routing.The readiness probe has been removed while liveness probe is retained. This is a significant operational change:
- Readiness probes gate traffic routing; removing it may cause requests to route to nodes that aren't fully initialized.
- Liveness probes only restart failed containers; they don't prevent traffic routing.
Please confirm this change is intentional and that downstream consumers of this StatefulSet are aware traffic may arrive before the node is fully ready.
cmd/tx-factory/src/main.rs (1)
4-21: LGTM: VM2 configuration fields and imports are appropriate.The addition of VM2-specific imports and configuration fields properly extends the CLI to support dual-VM scenarios.
Also applies to: 42-106
1, add benchmark for testing vm2 transaction execution
2, cache the vm2 config from shengguo lu
3, add batch operations for the transaction interface
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
New Features
API
Infrastructure
Improvements
Tests