Tx verify pool #4769
Tx verify pool #4769
Conversation
WalkthroughThis PR introduces a configurable, thread-safe Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
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: 1
🧹 Nitpick comments (7)
vm2/vm-runtime/src/starcoin_vm.rs (1)
470-470: Consider usingDuration::ZEROfor idiomatic Rust.
Duration::ZEROis the idiomatic constant for a zero duration in modern Rust (stabilized since Rust 1.53).- let mut cfg_dur = Duration::from_millis(0); + let mut cfg_dur = Duration::ZERO;vm2/executor/src/executor.rs (1)
35-51: Minor: RedundantInstant::now()calls.Lines 35-36 create two
Instantvalues (total_startandinit_start) at nearly the same moment. You could reusetotal_startfor the init measurement and computeinit_durasverify_start - total_startif you want to reduce allocations, though the overhead is negligible.- let total_start = Instant::now(); - let init_start = Instant::now(); + let total_start = Instant::now(); let mut vm = StarcoinVM::new(metrics, chain_state); - let init_dur = init_start.elapsed(); + let init_dur = total_start.elapsed(); let verify_start = Instant::now();txpool/src/pool_client.rs (1)
237-265: Redundantvm_type == "vm2"condition.The checks at lines 239 and 254 (
if vm_type == "vm2") are always true since we're inside theMultiSignedUserTransaction::VM2(_)match arm wherevm_typeis set to"vm2". These conditions can be removed.None => { let vm_dur = vm_start.elapsed(); - if vm_type == "vm2" { - debug!( - target: "txpool", - "verify_transaction tx={:?} type={} sig_ms={:.3} vm_ms={:.3} total_ms={:.3}", - tx_hash, - vm_type, - sig_dur.as_secs_f64() * 1000.0, - vm_dur.as_secs_f64() * 1000.0, - total_start.elapsed().as_secs_f64() * 1000.0, - ); - } + debug!( + target: "txpool", + "verify_transaction tx={:?} type=vm2 sig_ms={:.3} vm_ms={:.3} total_ms={:.3}", + tx_hash, + sig_dur.as_secs_f64() * 1000.0, + vm_dur.as_secs_f64() * 1000.0, + total_start.elapsed().as_secs_f64() * 1000.0, + ); Ok(checked_txn) }config/src/txpool_config.rs (1)
7-7: Redundant import statement.The
use num_cpus;import is unnecessary in Rust 2018+. You can callnum_cpus::get()directly without this import since it's already a dependency.-use num_cpus;txpool/benches/verify_transaction.rs (2)
172-184: Consider moving clone outside the parallel iteration.The
client.clone()on line 178 is called insidepar_iter().for_each(), which clones the client for every transaction. Sinceclientis already anArc, you could clone it once before the parallel block.|txns| { + let client = client.clone(); RAYON_EXEC_POOL.install(|| { txns.par_iter().for_each(|tx| { let pool_txn = PoolTransaction::Unverified( UnverifiedUserTransaction::from(tx.clone()), ); - let _ = client.clone().verify_transaction(pool_txn).expect("verify"); + let _ = client.verify_transaction(pool_txn).expect("verify"); }); }); },
197-209: Same clone optimization applies here.Same suggestion as above - clone
clientonce before the RAYON block rather than insidefor_each.txpool/src/verifier_pool.rs (1)
94-102: Unused parameters in constructor.The
_storage2and_vm_metricsparameters are not used inVerifierEntry::new. If these are reserved for future lazy initialization, consider documenting that intent. Otherwise, remove them to avoid confusion.- fn new(_storage2: Arc<dyn Store2>, _vm_metrics: Option<VMMetrics>) -> Self { + fn new() -> Self { Self { state_root1: None, state_root2: None, statedb2: None, vm2: None, } }This would also require updating the call site in
VerifierPool::newat line 33-36.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
commons/utils/src/thread_pool.rs(1 hunks)config/src/txpool_config.rs(4 hunks)flake.nix(2 hunks)txpool/Cargo.toml(3 hunks)txpool/benches/verify_transaction.rs(1 hunks)txpool/src/lib.rs(3 hunks)txpool/src/pool/queue.rs(3 hunks)txpool/src/pool_client.rs(7 hunks)txpool/src/test.rs(2 hunks)txpool/src/tx_pool_service_impl.rs(8 hunks)txpool/src/verifier_pool.rs(1 hunks)vm2/executor/Cargo.toml(1 hunks)vm2/executor/src/executor.rs(2 hunks)vm2/vm-runtime/src/starcoin_vm.rs(9 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
vm2/executor/Cargo.tomltxpool/Cargo.tomlvm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-09-14T01:27:57.535Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: rpc/server/src/module/debug_rpc.rs:87-90
Timestamp: 2025-09-14T01:27:57.535Z
Learning: For debug/dev commands in the Starcoin codebase, the preference is to keep the implementation simple without excessive validation or access control checks, focusing on functionality rather than restrictive safeguards.
Applied to files:
vm2/executor/Cargo.toml
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/pool_client.rstxpool/src/pool/queue.rstxpool/src/verifier_pool.rstxpool/src/test.rstxpool/src/lib.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
txpool/src/pool_client.rstxpool/src/pool/queue.rstxpool/src/verifier_pool.rstxpool/src/test.rstxpool/src/lib.rstxpool/src/tx_pool_service_impl.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
txpool/src/pool_client.rsvm2/executor/src/executor.rsvm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
txpool/src/pool_client.rsvm2/executor/src/executor.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
txpool/src/pool_client.rsvm2/executor/src/executor.rstxpool/src/tx_pool_service_impl.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
txpool/src/pool_client.rsvm2/executor/src/executor.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
txpool/src/pool_client.rsvm2/executor/src/executor.rsvm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
txpool/Cargo.tomltxpool/src/pool/queue.rsvm2/executor/src/executor.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
txpool/Cargo.tomltxpool/src/pool/queue.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-11-13T05:42:00.353Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4755
File: network/api/src/peer_provider.rs:489-501
Timestamp: 2025-11-13T05:42:00.353Z
Learning: In network/api/src/peer_provider.rs, the select_peer_by_index method intentionally sorts the peer list on every invocation rather than caching the sorted list. This is because peer scores are updated concurrently via async operations, and caching would result in stale rankings. The design prioritizes correctness over performance, with the expectation that most calls succeed on the first attempt.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-02-10T10:00:57.422Z
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
txpool/src/tx_pool_service_impl.rs
🧬 Code graph analysis (5)
commons/utils/src/thread_pool.rs (1)
config/src/rpc_config.rs (1)
threads(76-78)
txpool/src/verifier_pool.rs (2)
txpool/src/tx_pool_service_impl.rs (1)
new(42-94)types/src/multi_state.rs (2)
state_root1(35-37)state_root2(39-41)
txpool/src/test.rs (2)
genesis/src/lib.rs (1)
init_storage_for_test(449-460)txpool/src/verifier_pool.rs (1)
state_roots(148-150)
vm2/vm-runtime/src/starcoin_vm.rs (2)
txpool/src/pool_client.rs (1)
verify_transaction(187-271)vm/vm-runtime/src/starcoin_vm.rs (1)
verify_transaction(457-488)
txpool/src/tx_pool_service_impl.rs (1)
txpool/src/pool_client.rs (3)
new(33-38)new(77-83)new(158-177)
🔇 Additional comments (33)
vm2/vm-runtime/src/starcoin_vm.rs (3)
67-67: LGTM!The
Durationimport is correctly added to support the new timing instrumentation.
343-438: Timing instrumentation looks well-structured.The instrumentation captures key verification phases (gas check, argument verification, prologue) with minimal overhead. The
payload_kindtracking provides useful context in logs.One minor observation: the timing variables (
check_gas_dur,verify_args_dur,prologue_dur) follow a consistent naming pattern, which aids readability.
441-524: Good API design for configurable config-loading behavior.The split between
verify_transaction(always reloads) andverify_transaction_cached_config(uses cached if available) provides flexibility for different use cases. VerifierPool correctly usesverify_transaction_cached_configto avoid redundant config loading across multiple verifications, while direct verification paths useverify_transactionfor consistency.The fallback condition at line 471 (
self.vm_config.is_none() || self.gas_schedule.is_none()) is a good safety net ensuring configs are always loaded when missing, regardless of thereload_configsflag.vm2/executor/Cargo.toml (1)
22-22: LGTM!The
logdependency addition is appropriate to support the timing instrumentation added inexecutor.rs. Usingworkspace = truemaintains consistency with the existing dependency pattern.vm2/executor/src/executor.rs (1)
13-16: LGTM!Imports for
debuglogging andInstanttiming are appropriate additions for the instrumentation below.txpool/src/test.rs (2)
5-5: LGTM!Import added for the new
VerifierPooltype to support the test below.
68-90: LGTM! Good test coverage for VerifierPool lifecycle.The test validates the core VerifierPool behavior:
- Initial checkout captures correct state roots
- After
invalidate_all(), a fresh checkout still returns correct rootsThis covers the essential state management contract of the pool.
txpool/Cargo.toml (4)
15-15: LGTM!The
rayondependency is appropriate for enabling parallel transaction verification in the newVerifierPool.
42-43: LGTM!These dependencies support VM2 verification:
starcoin-vm2-vm-runtimeenables direct VM2 verification pathssp-utilsprovides threading utilities for pool management
46-59: LGTM!Dev-dependencies are appropriate for benchmarking:
criterionfor benchmark harnessrayonfor parallel benchmark scenariosnum_cpusfor dynamic thread pool sizing in benchmarks
65-67: LGTM!Benchmark target correctly configured with
harness = falsefor criterion-based benchmarks.txpool/src/lib.rs (3)
14-18: Expanded public API surface noted.These exports make internal pool types publicly accessible. This appears intentional to support external verification flows, benchmarks, and the VerifierPool integration.
33-33: LGTM!Public export of
VerifierPoolenables external callers to construct and inject the pool intoPoolClient.
42-42: LGTM!Module declaration for the new
verifier_poolcomponent.txpool/src/pool_client.rs (4)
2-2: LGTM!Import for
VerifierPoolto support optional pool-based verification.
148-148: LGTM!Adding the optional
verifier_poolfield enables pool-based VM2 verification while maintaining backward compatibility.
165-176: LGTM!Constructor correctly accepts and stores the optional
verifier_poolparameter.
226-235: LGTM! Clean pool integration with fallback.The verification correctly delegates to the
VerifierPoolwhen available and falls back to directstarcoin_vm2_executor::validate_transactionotherwise. This design maintains backward compatibility.config/src/txpool_config.rs (3)
56-65: LGTM!The new configuration fields follow the established pattern in this file with proper serde skip-if-none, clap annotations, and documentation comments.
106-111: LGTM!The accessor methods correctly default to
num_cpus::get()for pool size andtruefor enabled, following the same pattern as other accessors in the file.
141-146: LGTM!The merge logic correctly propagates the new configuration options from
StarcoinOpt, following the same pattern as the other fields.txpool/src/pool/queue.rs (3)
350-364: LGTM - Parallel verification with sensible optimization.The parallel verification using
RAYON_EXEC_POOLfor batches larger than 1 is appropriate, and the sequential fallback for single transactions avoids Rayon overhead. TheArcwrapper aroundverifiercorrectly enables thread-safe sharing across parallel tasks.
373-386: LGTM!The early-exit checks for already-imported and recently-rejected transactions are correctly placed after verification, using
continueto skip to the next transaction while still recording the appropriate error in results.
388-414: LGTM!The VM1 limit handling and blacklist logic are correctly integrated into the per-transaction processing path. The verification result is properly matched, and the pool import is performed within the success arm.
txpool/benches/verify_transaction.rs (1)
25-84: LGTM!The
build_verifierhelper correctly sets up test infrastructure with optional VerifierPool support, and theLOG_INITOnce ensures logger initialization happens only once across benchmark iterations.txpool/src/verifier_pool.rs (3)
85-91:invalidate_allmay skip locked entries, leaving stale state.Using
try_lock()means entries currently in use will not be reset. When those entries are released, they'll retain stale state until the nextcheckoutcall with different roots triggers a refresh. This is likely acceptable sincecheckoutalways validates roots before returning, but consider if blocking invalidation is needed for consistency guarantees.Is it intentional that
invalidate_allcan skip entries that are currently locked? The current behavior means:
- A locked entry keeps its stale state until released
- The next
checkoutwith different roots will refresh it anywayIf strong consistency is required (all entries must be invalidated before proceeding), a blocking approach would be needed.
130-140: Verify thatexpect()panics are unreachable in practice.The
expect()calls inverify_vm2will panic if VM2 or statedb2 is None. This should be safe becausecheckoutalways callsrefreshbefore returning a guard, but consider using a debug assertion or returning an error for defensive coding.The current flow guarantees initialization:
checkoutalways returns afterrefreshis called (or entry already matches)refreshalways setsvm2andstatedb2Please verify this invariant is maintained across all code paths.
153-300: Comprehensive test coverage.The tests cover lazy initialization, blocking checkout reuse, root change refresh, invalidation rebuild, and parallel verification. Good coverage of the VerifierPool behavior.
txpool/src/tx_pool_service_impl.rs (3)
55-63: LGTM!The VerifierPool is correctly instantiated based on configuration, with proper size and metrics passed through. The conditional creation based on
verifier_pool_enabled()provides a clean toggle mechanism.
385-391: LGTM!Invalidating the verifier pool on new chain headers is the correct approach to ensure stale VM state isn't used for verification after chain state changes.
578-591: LGTM!The
get_pool_clientmethod correctly passes theverifier_poolto thePoolClientconstructor, enabling pool-backed verification throughout the service.flake.nix (2)
36-36: No action needed—this is an intentional optimization strategy.The change to
CARGO_INCREMENTAL = "1"is part of a deliberate decision to replace sccache (distributed caching) with Rust's built-in incremental compilation. This is a reasonable trade-off for development builds. The build system is functioning correctly with no evidence of issues related to the build scripts present in the repository.
23-23: No action needed; gcc.cc.lib is safe across platforms.The change from
pkgs.stdenv.cc.cc.libtopkgs.gcc.cc.libdoes not cause portability issues. nixpkgs providesgcc.cc.libon all platforms, including macOS. The explicit GCC dependency is intentional and aligns with the toolchain configuration shown in the flake (which includes both clang and GCC packages). The inclusion inLD_LIBRARY_PATHis harmless on macOS, where this variable is unused in favor ofDYLD_LIBRARY_PATH.Likely an incorrect or invalid review comment.
sanlee42
force-pushed
the
tx-verify-pool
branch
from
766d78c to
88c11c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
txpool/src/pool_client.rs (1)
224-268: Redundantvm_type == "vm2"check in VM2 branch.Lines 239 and 254 check
if vm_type == "vm2", but this code is already inside theMultiSignedUserTransaction::VM2(_)match arm, so the condition is always true.match status { None => { let vm_dur = vm_start.elapsed(); - if vm_type == "vm2" { - debug!( - target: "txpool", - "verify_transaction tx={:?} type={} sig_ms={:.3} vm_ms={:.3} total_ms={:.3}", - tx_hash, - vm_type, - sig_dur.as_secs_f64() * 1000.0, - vm_dur.as_secs_f64() * 1000.0, - total_start.elapsed().as_secs_f64() * 1000.0, - ); - } + debug!( + target: "txpool", + "verify_transaction tx={:?} type=vm2 sig_ms={:.3} vm_ms={:.3} total_ms={:.3}", + tx_hash, + sig_dur.as_secs_f64() * 1000.0, + vm_dur.as_secs_f64() * 1000.0, + total_start.elapsed().as_secs_f64() * 1000.0, + ); Ok(checked_txn) } Some(status) => { let vm_dur = vm_start.elapsed(); - if vm_type == "vm2" { - debug!( - target: "txpool", - "verify_transaction failed tx={:?} type={} sig_ms={:.3} vm_ms={:.3} total_ms={:.3} status={:?}", - tx_hash, - vm_type, - sig_dur.as_secs_f64() * 1000.0, - vm_dur.as_secs_f64() * 1000.0, - total_start.elapsed().as_secs_f64() * 1000.0, - status, - ); - } + debug!( + target: "txpool", + "verify_transaction failed tx={:?} type=vm2 sig_ms={:.3} vm_ms={:.3} total_ms={:.3} status={:?}", + tx_hash, + sig_dur.as_secs_f64() * 1000.0, + vm_dur.as_secs_f64() * 1000.0, + total_start.elapsed().as_secs_f64() * 1000.0, + status, + ); Err(TransactionError2::CallErr(CallError2::ExecutionError(status)).into()) } }txpool/benches/verify_transaction.rs (1)
172-179: Minor inefficiency: double cloning of transactions in parallel benchmark.The
UnverifiedUserTransactionis cloned in the setup closure (line 169) and then cloned again inside the iteration (line 176). Consider cloning only once.|txns| { RAYON_EXEC_POOL.install(|| { - txns.par_iter().for_each(|tx| { - let pool_txn = PoolTransaction::Unverified( - UnverifiedUserTransaction::from(tx.clone()), - ); + txns.into_par_iter().for_each(|tx| { + let pool_txn = PoolTransaction::Unverified(tx); let _ = client.clone().verify_transaction(pool_txn).expect("verify"); }); });Apply the same pattern to lines 197-204.
txpool/src/verifier_pool.rs (1)
94-102: Unused parameters inVerifierEntry::new.The
_storage2and_vm_metricsparameters are unused since initialization is lazy. Consider removing them or documenting the intent.- fn new(_storage2: Arc<dyn Store2>, _vm_metrics: Option<VMMetrics>) -> Self { + fn new() -> Self { Self { state_root1: None, state_root2: None, statedb2: None, vm2: None, } }And update the caller in
VerifierPool::new:- Arc::new(Mutex::new(VerifierEntry::new( - storage2.clone(), - vm_metrics.clone(), - ))) + Arc::new(Mutex::new(VerifierEntry::new()))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
commons/utils/src/thread_pool.rs(1 hunks)config/src/txpool_config.rs(4 hunks)flake.nix(2 hunks)txpool/Cargo.toml(3 hunks)txpool/benches/verify_transaction.rs(1 hunks)txpool/src/lib.rs(3 hunks)txpool/src/pool/queue.rs(3 hunks)txpool/src/pool_client.rs(7 hunks)txpool/src/test.rs(2 hunks)txpool/src/tx_pool_service_impl.rs(8 hunks)txpool/src/verifier_pool.rs(1 hunks)vm2/executor/src/executor.rs(2 hunks)vm2/vm-runtime/src/starcoin_vm.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- commons/utils/src/thread_pool.rs
- txpool/src/lib.rs
- config/src/txpool_config.rs
- vm2/executor/src/executor.rs
- flake.nix
- txpool/Cargo.toml
- txpool/src/test.rs
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/pool/queue.rstxpool/src/pool_client.rstxpool/src/verifier_pool.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
txpool/src/pool/queue.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
txpool/src/pool/queue.rstxpool/src/pool_client.rstxpool/src/tx_pool_service_impl.rstxpool/src/verifier_pool.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
txpool/src/pool/queue.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rstxpool/src/pool_client.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rstxpool/src/pool_client.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
txpool/src/pool_client.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
txpool/src/pool_client.rstxpool/src/tx_pool_service_impl.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
txpool/src/pool_client.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
txpool/src/tx_pool_service_impl.rs
🧬 Code graph analysis (5)
txpool/src/pool/queue.rs (5)
txpool/src/pool_client.rs (3)
new(33-38)new(77-83)new(158-177)txpool/src/pool/local_transactions.rs (1)
new(78-85)txpool/src/pool/replace.rs (1)
new(32-34)txpool/src/pool/verifier.rs (1)
new(42-54)txpool/src/pool.rs (3)
hash(37-39)hash(105-111)hash(196-198)
txpool/benches/verify_transaction.rs (3)
txpool/src/pool_client.rs (3)
new(33-38)new(77-83)new(158-177)txpool/src/tx_pool_service_impl.rs (1)
new(42-94)txpool/src/verifier_pool.rs (2)
new(29-44)new(95-102)
txpool/src/pool_client.rs (3)
types/src/multi_transaction.rs (8)
from(39-41)from(45-47)from(136-138)from(142-144)from(248-253)from(257-262)from(294-296)from(300-302)vm2/executor/src/executor.rs (1)
validate_transaction(30-52)executor/src/executor.rs (1)
validate_transaction(44-51)
txpool/src/tx_pool_service_impl.rs (1)
txpool/src/pool_client.rs (3)
new(33-38)new(77-83)new(158-177)
txpool/src/verifier_pool.rs (2)
txpool/src/tx_pool_service_impl.rs (1)
new(42-94)types/src/multi_state.rs (2)
state_root1(35-37)state_root2(39-41)
🔇 Additional comments (25)
vm2/vm-runtime/src/starcoin_vm.rs (3)
67-67: LGTM: Standard timing imports.The addition of
DurationandInstantis appropriate for the timing instrumentation added throughout the verification flow.
343-438: LGTM: Well-structured timing instrumentation.The timing measurements for
check_gas,verify_args, andprologuephases are correctly placed and provide useful observability for VM2 verification performance. The per-payload-kind logging (package/script/entry_function) will help identify performance characteristics of different transaction types.
441-524: LGTM: Well-designed verification API with flexible config loading.The new public APIs
verify_transactionandverify_transaction_cached_configprovide useful flexibility for different verification contexts (e.g., one-off verification vs. pooled/batched verification). The internal helperverify_transaction_with_optionsproperly handles:
- Signature verification timing (even on failure path)
- Conditional config loading (line 471) that safely loads configs even when
reload_configs=falseif configs are missing- Preservation of existing behavior (e.g.,
SEQUENCE_NUMBER_TOO_NEWhandling at lines 491-496)The comprehensive timing logs will be valuable for monitoring VM2 verification performance in the txpool.
txpool/src/pool_client.rs (3)
2-2: LGTM!The new imports for
VerifierPoolandInstantare appropriate for the added timing instrumentation and verifier pool integration.Also applies to: 22-22
148-148: LGTM!The optional
verifier_poolfield is correctly wired through the struct and constructor, enabling seamless toggling between pooled and direct VM2 verification paths.Also applies to: 165-165, 175-175
191-210: LGTM on timing instrumentation.The signature check timing and failure logging provide useful diagnostics for transaction verification performance.
txpool/benches/verify_transaction.rs (3)
1-84: LGTM!The benchmark setup is well-structured, with proper logger initialization guarding via
Once, and clean separation between pool and non-pool verifier construction.
86-147: LGTM on sequential benchmark structure.The benchmark correctly compares pool vs non-pool verification paths across varying batch sizes.
213-218: LGTM!Criterion group and main entry point are correctly defined.
txpool/src/pool/queue.rs (4)
15-15: LGTM!Rayon import added to support parallel verification.
294-294: LGTM!The
Send + Syncbounds onCare required for the Rayon parallel verification to work correctly, as the client is shared across threads.
350-364: LGTM on parallel verification implementation.The conditional parallelization (only for batches > 1) is a sensible optimization that avoids Rayon overhead for single transactions. Wrapping the verifier in
Arcenables safe sharing across the thread pool.
366-414: LGTM on post-verification processing.The logic correctly:
- Computes
existing_vm1count before the loop- Handles already-imported and recently-rejected early exits
- Applies VM1 transaction limits and peer blacklisting per-transaction
- Increments
existing_vm1only on successful import of VM1 transactionstxpool/src/verifier_pool.rs (7)
28-44: LGTM on pool initialization.Pool size is correctly clamped to at least 1, and entries are lazily initialized.
46-83: Consider the checkout behavior when all entries are locked.The three-phase checkout strategy is reasonable, but note that the final fallback (line 73) uses a blocking
lock()onentries[0]. If the pool size is 1 and all callers converge here, this becomes a bottleneck. This is acceptable given the pool is meant to reduce VM init overhead rather than eliminate contention.
85-91:invalidate_allmay skip locked entries.Using
try_lock()means entries that are currently in use won't be reset. This is likely intentional to avoid blocking, but the caller should be aware that some entries may retain stale state until the nextcheckoutcall refreshes them.Please confirm this behavior is acceptable. If stale entries must be cleared synchronously on chain head change, consider using blocking locks or adding a "dirty" flag that forces refresh on next checkout.
108-121: LGTM on refresh logic.The refresh correctly initializes both
ChainStateDB2andStarcoinVM2with the new state roots.
130-141: LGTM on verify_vm2.The expects are appropriate since
checkoutguarantees initialization before returning a guard.
143-151: LGTM on VerifierGuard.Clean delegation to the underlying entry methods.
153-300: LGTM on test coverage.Tests cover:
- Lazy initialization
- Lock reuse across threads
- State root change triggering refresh
- Invalidation and rebuild
- Parallel verification
Good coverage of the core verifier pool functionality.
txpool/src/tx_pool_service_impl.rs (5)
11-11: LGTM!VerifierPool import added for integration.
55-63: LGTM on conditional VerifierPool initialization.The pool is correctly created only when enabled via configuration, and properly wired into the
Innerstruct.Also applies to: 90-90
385-391: LGTM on invalidation during chain header updates.Calling
invalidate_all()when the chain header changes ensures that pooled VM instances don't operate on stale state roots.
195-196: LGTM on consistent VerifierPool propagation.All
PoolClient::newcall sites correctly pass theverifier_poolparameter, ensuring consistent behavior acrossget_pending_with_state,next_sequence_number_in_batch, andget_pool_client.Also applies to: 514-514, 590-590
365-365: LGTM!The
verifier_poolfield is correctly added to theInnerstruct.
- introduce verifier pool entries, config knobs, and rayon-based parallel verify - wire pool into PoolClient/TxPoolService, refresh on chain head updates - add benches and tests, align rayon pool size with verifier pool defaults
sanlee42
force-pushed
the
tx-verify-pool
branch
from
88c11c5 to
1af5eae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
flake.nix (1)
38-38: LD_LIBRARY_PATH change is consistent with line 23.This change to use
${pkgs.gcc.cc.lib}/libis consistent with the package dependency change on line 23. The same cross-platform compatibility concerns apply here. If the GCC-specific dependency is necessary, both changes are correct; otherwise, both should be reverted to the more portablestdenv.cc.cc.libapproach.
🧹 Nitpick comments (2)
txpool/src/verifier_pool.rs (2)
85-91:invalidate_allmay skip locked entries, leaving stale state.When entries are locked (e.g., during verification),
try_lock()fails and those entries won't be reset. If the state roots change (e.g., after a new block), a concurrent verifier holding an entry will continue using stale state until its nextcheckout().This is likely acceptable since
checkout()always validates roots and refreshes on mismatch, but consider documenting this behavior or using a flag to mark entries as invalidated for deferred reset.
94-102: Unused parameters inVerifierEntry::neware redundant.The
_storage2and_vm_metricsparameters are passed but unused since initialization is lazy. Consider simplifying the signature.🔎 Suggested simplification
- fn new(_storage2: Arc<dyn Store2>, _vm_metrics: Option<VMMetrics>) -> Self { + fn new() -> Self { Self { state_root1: None, state_root2: None, statedb2: None, vm2: None, } }And update the caller at lines 33-36:
- Arc::new(Mutex::new(VerifierEntry::new( - storage2.clone(), - vm_metrics.clone(), - ))) + Arc::new(Mutex::new(VerifierEntry::new()))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
config/src/txpool_config.rs(3 hunks)flake.nix(2 hunks)txpool/Cargo.toml(3 hunks)txpool/benches/verify_transaction.rs(1 hunks)txpool/src/lib.rs(3 hunks)txpool/src/pool/queue.rs(3 hunks)txpool/src/pool_client.rs(7 hunks)txpool/src/test.rs(2 hunks)txpool/src/tx_pool_service_impl.rs(8 hunks)txpool/src/verifier_pool.rs(1 hunks)vm2/executor/src/executor.rs(2 hunks)vm2/vm-runtime/src/starcoin_vm.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- vm2/executor/src/executor.rs
- txpool/Cargo.toml
- txpool/src/test.rs
- config/src/txpool_config.rs
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/lib.rstxpool/src/pool_client.rstxpool/src/pool/queue.rstxpool/src/verifier_pool.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
txpool/src/lib.rstxpool/src/pool_client.rstxpool/src/pool/queue.rstxpool/src/verifier_pool.rstxpool/src/tx_pool_service_impl.rs
📚 Learning: 2025-11-13T05:42:00.353Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4755
File: network/api/src/peer_provider.rs:489-501
Timestamp: 2025-11-13T05:42:00.353Z
Learning: In network/api/src/peer_provider.rs, the select_peer_by_index method intentionally sorts the peer list on every invocation rather than caching the sorted list. This is because peer scores are updated concurrently via async operations, and caching would result in stale rankings. The design prioritizes correctness over performance, with the expectation that most calls succeed on the first attempt.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-02-10T10:00:57.422Z
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Applied to files:
txpool/src/lib.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
txpool/src/pool_client.rsvm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-10-10T01:54:58.184Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: vm2/executor/src/block_executor.rs:73-81
Timestamp: 2025-10-10T01:54:58.184Z
Learning: In the Starcoin blockchain executor (both VM1 and VM2), committing state at intermediate transactions (e.g., first transaction with `index == 0`) is acceptable because if any subsequent transaction in the block is discarded, the entire block execution aborts and the block won't connect to the chain. Block-level atomicity is maintained at a higher level, ensuring partial blocks are never persisted.
Applied to files:
txpool/src/pool_client.rs
📚 Learning: 2025-08-08T10:16:46.394Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:185-187
Timestamp: 2025-08-08T10:16:46.394Z
Learning: In starcoin/chain/src/chain.rs, BlockChain::statedb currently consumes self to move out ChainStateDB during block building to avoid cloning or recreating the state DB for transaction filtering. This is intentional for performance; a rename to into_statedb is acceptable but behavior should remain a consuming getter.
Applied to files:
txpool/src/pool_client.rstxpool/src/tx_pool_service_impl.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
txpool/src/pool_client.rs
📚 Learning: 2025-09-30T01:46:29.197Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4703
File: chain/open-block/src/vm2.rs:44-45
Timestamp: 2025-09-30T01:46:29.197Z
Learning: In the Starcoin blockchain (file: chain/open-block/src/vm2.rs), block metadata transaction gas consumption is intentionally NOT counted toward the block gas limit (self.gas_used is not incremented for metadata transactions in initialize() method). Only user transactions in push_txns2() update self.gas_used.
Applied to files:
txpool/src/pool_client.rsvm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
txpool/src/pool/queue.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
txpool/src/pool/queue.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
vm2/vm-runtime/src/starcoin_vm.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
txpool/src/tx_pool_service_impl.rs
🧬 Code graph analysis (5)
txpool/src/pool/queue.rs (2)
txpool/src/pool/verifier.rs (1)
new(42-54)txpool/src/pool/replace.rs (1)
new(32-34)
txpool/src/verifier_pool.rs (3)
txpool/src/tx_pool_service_impl.rs (1)
new(42-94)vm2/vm-runtime/src/starcoin_vm.rs (2)
new(89-112)new(115-138)types/src/multi_state.rs (2)
state_root1(35-37)state_root2(39-41)
vm2/vm-runtime/src/starcoin_vm.rs (2)
txpool/src/pool_client.rs (1)
verify_transaction(187-271)vm/vm-runtime/src/starcoin_vm.rs (1)
verify_transaction(457-488)
txpool/src/tx_pool_service_impl.rs (2)
txpool/src/lib.rs (1)
new(62-68)txpool/src/pool_client.rs (3)
new(33-38)new(77-83)new(158-177)
txpool/benches/verify_transaction.rs (3)
txpool/src/pool_client.rs (5)
new(33-38)new(77-83)new(158-177)get(41-43)verify_transaction(187-271)txpool/src/tx_pool_service_impl.rs (2)
new(42-94)verify_transaction(107-115)txpool/src/verifier_pool.rs (2)
new(29-44)new(95-102)
🔇 Additional comments (19)
flake.nix (2)
36-36: CARGO_INCREMENTAL="1" is appropriate for development shell context.This setting is in the
devShells.defaultdevelopment environment, where incremental compilation speeds up local development builds. The configuration is unrelated to the transaction verification pool feature—it's a standalone developer experience optimization. No action required.
1-42: This is a new flake.nix file, not modifications to an existing one.The file is being added for the first time in this PR. The original review's references to "removing sccache" or changing compiler libraries don't apply since there's no previous version to compare against.
That said, the underlying scope question is still worth considering: while adding a Nix development environment alongside the transaction verification pool feature is reasonable infrastructure work, it might be clearer in a separate PR focused on development tooling. If this dev environment is necessary for building or testing the verifier pool feature, the PR description should explain that connection.
Likely an incorrect or invalid review comment.
txpool/src/verifier_pool.rs (1)
153-299: Comprehensive test coverage for VerifierPool.Tests cover the critical paths: lazy initialization, blocking checkout with VM reuse, state root refresh on change, invalidation and rebuild, and parallel verification. The parallel test with pool size 4 and 8 concurrent verifications effectively validates the concurrent access pattern.
vm2/vm-runtime/src/starcoin_vm.rs (2)
510-524: Clean API split for cached vs reloading verification.The separation into
verify_transaction(always reloads configs) andverify_transaction_cached_config(uses cached configs when available) enables the VerifierPool optimization while maintaining backward compatibility. The fallback at line 471 ensures configs are loaded if missing even whenreload_configsis false.
429-438: Timing instrumentation provides useful diagnostics.The per-phase timing breakdown (check_gas, verify_args, prologue) with payload_kind context will help identify verification bottlenecks. Debug-level logging ensures minimal production overhead.
txpool/src/lib.rs (1)
14-18: Public API exports properly expose verification infrastructure.The new exports (
SeqNumberAndGasPrice,Verifier,PoolTransaction,UnverifiedUserTransaction,VerifiedTransaction,VerifierOptions,VerifierPool) enable external consumers to construct verifiers and integrate with the pool, supporting the benchmark use case and potential external tooling.Also applies to: 33-33, 42-42
txpool/src/pool_client.rs (2)
224-268: VM2 verification path correctly integrates VerifierPool with fallback.The implementation properly:
- Uses pool-based verification when
verifier_poolis available- Falls back to direct
validate_transactionwhen pool is disabled- Logs timing metrics for both success and failure paths
The conditional
if vm_type == "vm2"checks (lines 239, 254) are technically redundant since this code only executes forMultiSignedUserTransaction::VM2, but they're harmless and make the logging intent explicit.
191-210: Timing instrumentation captures signature verification overhead.The signature verification timing at the pool client level complements the VM-level timing, providing end-to-end visibility into verification performance including the signature check which happens before VM invocation.
txpool/benches/verify_transaction.rs (2)
25-84: Well-structured benchmark setup with pool/no-pool comparison.The
build_verifierhelper cleanly separates infrastructure setup from benchmark execution. UsingArcfor shared state andOncefor logger initialization follows best practices for benchmark setup.
149-207: Parallel benchmark exercises concurrent verification paths.The use of
RAYON_EXEC_POOL.install()withpar_iter()properly exercises the concurrent verification scenario. Note thatclient.clone()inside the parallel loop has some overhead that's included in the measurement, which reflects realistic usage patterns where the client may need to be shared across threads.txpool/src/pool/queue.rs (3)
350-364: Parallel verification with single-transaction optimization.The conditional parallelization (
transactions.len() > 1) avoids Rayon overhead for single transactions while enabling parallel verification for batches. Usingpar_iter()withmap()preserves order, sozip()at line 373 correctly pairs each transaction with its verification result.
373-428: Per-transaction processing loop correctly handles VM1 limits and results.The refactored loop properly:
- Checks for already-imported and recently-rejected transactions (with
continuefor early exit)- Applies VM1 transaction limits with per-peer blacklisting
- Imports verified transactions and tracks
existing_vm1count- Records rejected transactions in the rejection cache
The separation of verification (parallel) from import (sequential with pool lock) is the correct design since pool modification requires exclusive access.
294-294:Send + Syncbounds required for thread-safe parallel verification.The added bounds on
Care necessary because the verifier captures the client and is shared across Rayon threads viaArc<Verifier<C>>. WhenArc<Verifier<C>>is cloned and distributed to parallel threads throughpar_iter(), the type system requires bothSendandSyncto guarantee thread safety. This is a public API signature change that could affect callers if they pass types that don't satisfy these bounds, though PoolClient (the primary implementation) satisfies them through its thread-safe internal structure.txpool/src/tx_pool_service_impl.rs (6)
55-63: LGTM! Clean conditional initialization.The verifier pool is correctly initialized based on configuration, uses the appropriate storage backend (storage2 for VM2), and is properly wrapped for thread-safe sharing.
365-365: LGTM! Appropriate field type.The
Option<Arc<VerifierPool>>type is well-suited for this configurable feature and enables efficient sharing across PoolClient instances.
188-196: LGTM! Correct propagation to PoolClient.The verifier pool is correctly passed to PoolClient. Using a fresh
NonceCache::new(0)is appropriate here since this API queries at a specific state point (explicit state roots) rather than the current chain state.
507-515: LGTM! Consistent verifier pool propagation.The verifier pool is correctly passed to the PoolClient constructor, maintaining consistency with other integration points.
578-592: LGTM! Proper integration in primary factory method.The verifier pool is correctly propagated through the primary
get_pool_clientfactory method, which is used throughout theInnerimplementation. The sharedsequence_number_cacheis appropriately used for the main chain state client.
388-390: Correct invalidation on chain state change.Invalidating the verifier pool when the chain header changes is essential—cached verifications based on the previous state would be stale. The implementation is efficient: it uses non-blocking
try_lock()to avoid blocking the critical path, andreset()is a lightweight O(1) operation. Chain header changes are infrequent (occurring only on new blocks), so no performance concern.
| pkgs.stdenv.cc.cc.lib | ||
| pkgs.mold | ||
| pkgs.sccache | ||
| pkgs.gcc.cc.lib |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd flake.nixRepository: starcoinorg/starcoin
Length of output: 73
🏁 Script executed:
cat -n flake.nix | head -50Repository: starcoinorg/starcoin
Length of output: 1742
🏁 Script executed:
# Check if there are any recent git history or commit messages about this change
git log --oneline -20 -- flake.nix | head -20Repository: starcoinorg/starcoin
Length of output: 356
🏁 Script executed:
git show 1af5eae4:flake.nix > /tmp/new_flake.nix
git show 1af5eae4^:flake.nix > /tmp/old_flake.nix 2>/dev/null || echo "No previous version"
diff -u /tmp/old_flake.nix /tmp/new_flake.nix || trueRepository: starcoinorg/starcoin
Length of output: 1645
🏁 Script executed:
# Check if there are any related issues, PRs, or documentation about mold/gcc requirement
rg -i "mold|gcc|libstdc" --type md README* CONTRIBUTING* || echo "No matches in docs"
# Check if there's any mold configuration in Cargo.toml or build scripts
rg -i "mold" --type toml --type rs | head -20Repository: starcoinorg/starcoin
Length of output: 126
🏁 Script executed:
# Search for mold references in the codebase
rg -i "mold" | head -20
# Also check for any Clang-specific configurations
rg -i "clang" Cargo.toml | head -10Repository: starcoinorg/starcoin
Length of output: 1104
🏁 Script executed:
# Check if there are any platform-specific configurations or CI/CD workflows
find . -name "*.yml" -o -name "*.yaml" 2>/dev/null | xargs grep -l "macos\|darwin" 2>/dev/null | head -10
# Also check README for platform support
head -50 README.md | grep -i "platform\|macos\|linux\|support"Repository: starcoinorg/starcoin
Length of output: 144
🏁 Script executed:
# Check the release workflow to see what platforms are supported
cat .github/workflows/release_asset.yml | grep -A 5 -B 5 "macos\|linux\|runs-on" | head -40Repository: starcoinorg/starcoin
Length of output: 678
🏁 Script executed:
# Check if CI uses nix develop or flake
grep -r "nix develop\|nix flake\|flake.nix" .github/workflows/ || echo "No nix usage in CI"
# Check how builds are configured on different platforms
grep -A 10 "macos-latest" .github/workflows/release_asset.yml | head -20Repository: starcoinorg/starcoin
Length of output: 464
🏁 Script executed:
# Verify if pkgs.gcc.cc.lib exists on macOS (or if it would fail)
# Check Nix documentation or examples for portable library references
rg "stdenv\.cc\.cc\.lib|gcc\.cc\.lib" . --type nix | head -20Repository: starcoinorg/starcoin
Length of output: 210
Use platform-agnostic compiler library reference or add macOS-specific configuration.
The use of pkgs.gcc.cc.lib hardcodes GCC, which breaks development environments on macOS where Clang is the default compiler. Since the project officially supports macOS (see CI matrix in release_asset.yml), the flake should either use pkgs.stdenv.cc.cc.lib for portability or add platform-specific handling. Additionally, LD_LIBRARY_PATH on line 38 is Linux-specific; macOS requires DYLD_LIBRARY_PATH instead.
🤖 Prompt for AI Agents
In flake.nix around line 23, the code references pkgs.gcc.cc.lib which hardcodes
GCC and breaks macOS (Clang) environments; replace it with the platform-agnostic
compiler library reference pkgs.stdenv.cc.cc.lib or add a conditional check on
system to choose pkgs.gcc.cc.lib for linux and pkgs.stdenv.cc.cc.lib for darwin,
and similarly replace the Linux-only LD_LIBRARY_PATH usage at line 38 with a
platform-aware setting that uses LD_LIBRARY_PATH on Linux and DYLD_LIBRARY_PATH
on macOS (or set both appropriately) so the flake works across supported
platforms.
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
Performance
✏️ Tip: You can customize this high-level summary in your review settings.