Culling at import and every second - #4781
Conversation
📝 WalkthroughWalkthroughThis PR propagates an explicit genesis hash through BlockDAG constructors and testing helpers, updates call sites and initialization ordering to compute/pass/set genesis, refactors TxPoolActorService into its own module with propagation/culling logic, adds Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/src/txpool_config.rs (1)
106-122: Remove duplicatedmax_mem_usagemerge; keep single source of truth.
max_mem_usageis merged twice (Line 108-110 and Line 114-116). It’s redundant and makes future edits error-prone.Proposed fix
if let Some(m) = txpool_opt.max_mem_usage.as_ref() { self.max_mem_usage = Some(*m); } if let Some(m) = txpool_opt.max_count.as_ref() { self.max_count = Some(*m); } - if let Some(m) = txpool_opt.max_mem_usage.as_ref() { - self.max_mem_usage = Some(*m); - } if let Some(m) = txpool_opt.tx_propagate_interval.as_ref() { self.tx_propagate_interval = Some(*m); } if let Some(m) = txpool_opt.cull_interval.as_ref() { self.cull_interval = Some(*m); }
🤖 Fix all issues with AI agents
In @config/src/txpool_config.rs:
- Around line 88-90: The cull_interval() accessor currently returns whatever
value was parsed (or 1 by default), so passing --txpool-cull-interval 0 yields
0; change cull_interval() to clamp the result to at least 1 by taking the
parsed/unwrap_or value and applying a max(1) before returning (i.e., ensure the
return is >= 1), updating the cull_interval() function accordingly.
- Around line 36-40: The clap options for cull_interval and
tx_propagate_interval currently allow zero which can cause a tight scheduler
loop; update their attributes to use clap 4.5 range validation (use value_parser
= clap::value_parser!(u64).range(1..)) so values <1 are rejected, e.g., modify
the attributes on the cull_interval and tx_propagate_interval fields
accordingly; also remove the duplicate merge block for max_mem_usage (the
repeated block around the existing max_mem_usage declaration) leaving only a
single definition to avoid duplication.
In @txpool/src/tx_pool_actor_service.rs:
- Around line 24-56: The TxPoolActorService type is re-exported publicly but its
constructor fn new is declared pub(crate), preventing downstream crates from
instantiating it; change the visibility of the constructor from pub(crate) fn
new to pub fn new in the TxPoolActorService impl so external users can construct
the re-exported TxPoolActorService (keep the rest of the fields and
initialization the same), and run tests/build to ensure no crate-internal
invariants relied on the restricted visibility are broken.
🧹 Nitpick comments (7)
flexidag/src/blockdag.rs (3)
68-68: Consider making the genesis field private for consistency.The
genesisfield is public, but accessor methodsgenesis()andset_genesis()are also provided (lines 561-567). This dual-access pattern can lead to inconsistent usage across the codebase.Standard Rust practice is either:
- Private field with public accessors (for encapsulation), or
- Public field without accessors (for direct access)
Since you've provided accessors, making the field private would clarify the intended API.
♻️ Suggested refactor
- genesis: Hash, + pub(crate) genesis: Hash,or make it fully private:
- genesis: Hash, + genesis: Hash,
565-567: Consider restricting genesis mutability after initialization.The
set_genesismethod allows the genesis hash to be changed after construction. If genesis is modified after BlockDAG operations have been performed, this could lead to inconsistencies:
- Prior operations that depended on the old genesis may have stored state
- The
get_dag_staterouting logic (lines 549-553) would now routeHash::zero()to the new genesis- Cached or stored data might reference the old genesis
If the setter is only needed for deferred initialization (construct with
Hash::zero(), then set genesis later), consider one of these alternatives:
- Builder pattern: Require genesis at construction time across all code paths
- One-time initialization: Track whether genesis has been set and prevent subsequent mutations
♻️ Example: One-time initialization guard
pub fn set_genesis(&mut self, genesis: Hash) { + if self.genesis != Hash::zero() { + warn!("Attempted to change genesis from {:?} to {:?}", self.genesis, genesis); + return; + } self.genesis = genesis; }Or use a dedicated initialization check if this is critical:
pub fn set_genesis(&mut self, genesis: Hash) -> anyhow::Result<()> { + ensure!( + self.genesis == Hash::zero(), + "Genesis already initialized to {:?}, cannot change to {:?}", + self.genesis, + genesis + ); self.genesis = genesis; + Ok(()) }
548-559: Document and add defensive check for the Hash::zero() routing in get_dag_state.The routing logic substitutes
Hash::zero()withself.genesisbefore querying state. While the current two-phase initialization pattern (create with potential zero, then immediately callset_genesisif needed) ensures genesis is valid before the dag is used, the routing's behavior should be explicit.
- Add a comment explaining why
Hash::zero()is routed to genesis (backward compatibility or edge case handling)- Consider adding a debug assertion to catch cases where genesis might not be initialized when this routing is triggered
This prevents future maintenance issues and clarifies intent without changing current behavior.
txpool/src/tx_pool_service_impl.rs (1)
397-413: Pre-import cull + singlePoolClientreuse looks good; please verify the “now_seconds” time source.Right now
import_txnsusesself.chain_header.read().timestamp() / 1000(Line 403). IfTransactionQueue::cull(..)is intended to expire by wall-clock, this can stall expiry when no new blocks advance the header timestamp.Proposed diff (if cull should use wall-clock seconds)
pub(crate) fn import_txns( &self, txns: Vec<MultiSignedUserTransaction>, bypass_vm1_limit: bool, peer_id: Option<String>, ) -> Result<Vec<Result<(), MultiTransactionError>>> { - let now_seconds = self.chain_header.read().timestamp() / 1000; + let now_seconds = self.node_config.net().time_service().now_secs(); let pool_client = self.get_pool_client()?; self.queue.cull(pool_client.clone(), now_seconds); let txns = txns .into_iter() .map(|t| PoolTransaction::Unverified(UnverifiedUserTransaction::from(t))); Ok(self .queue .import(pool_client, txns, bypass_vm1_limit, peer_id)) }txpool/src/tx_pool_actor_service.rs (3)
57-107: Consider gating transaction propagation onis_synced()(behavioral intent check).
try_propagate_txnscurrently runs whenevernew_txs_receivedis set, even if the node isn’t synced yet, while peer imports are explicitly dropped when not synced (Line 219-234). If this asymmetry isn’t intentional, add a sync check before broadcasting.Proposed diff (if propagation should wait for sync)
fn try_propagate_txns(&self, ctx: &mut ServiceContext<Self>) { + if !self.is_synced() { + return; + } // only propagate when new txns enter pool. if self.new_txs_received.load(Ordering::Relaxed) { match self.transactions_to_propagate() { Err(e) => { log::error!("txpool: fail to get txn to propagate, err: {}", &e) }
174-215: Avoid per-event allocation for metrics label values in the hot path.
with_label_values(&[format!("{}", s).as_str()])allocates a newStringper status entry (Line 201-204). Prefer mappingTxStatusto a small set of&'static str.One possible direction
+fn tx_status_label(s: &TxStatus) -> &'static str { + match s { + TxStatus::Added => "Added", + TxStatus::Rejected => "Rejected", + TxStatus::Dropped => "Dropped", + TxStatus::Canceled => "Canceled", + // adjust to actual enum variants + _ => "Other", + } +} ... metrics .txpool_txn_event_total - .with_label_values(&[format!("{}", s).as_str()]) + .with_label_values(&[tx_status_label(s)]) .inc();
217-236: Peer txn import drops all results/errors; add minimal logging for visibility.
let _ = self.inner.import_txns(...)(Line 226-230) will hide both top-level failures and per-txn rejections, which makes diagnosing propagation/sync issues hard.Proposed diff (log top-level + per-txn failures at debug)
if self.is_synced() { // JUST need to keep at most once delivery. let bypass_vm1_limit = msg .message .txns .iter() .all(|txn| matches!(txn, MultiSignedUserTransaction::VM2(_))); - let _ = self.inner.import_txns( + match self.inner.import_txns( msg.message.txns, bypass_vm1_limit, Some(msg.peer_id.to_string()), - ); + ) { + Err(e) => debug!("[txpool] peer import failed: {}", e), + Ok(results) => { + for r in results { + if let Err(e) = r { + debug!("[txpool] peer txn rejected: {}", e); + } + } + } + } } else {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
chain/mock/src/mock_chain.rschain/tests/block_test_utils.rscmd/generator/src/lib.rsconfig/src/txpool_config.rsflexidag/src/blockdag.rsflexidag/tests/test_commit_atomicity.rsflexidag/tests/tests.rsgenesis/src/lib.rsnode/src/node.rssimnet/src/scene/mod.rssync/src/tasks/test_tools.rstest-helper/src/chain.rstxpool/src/lib.rstxpool/src/tx_pool_actor_service.rstxpool/src/tx_pool_service_impl.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
test-helper/src/chain.rssync/src/tasks/test_tools.rschain/tests/block_test_utils.rschain/mock/src/mock_chain.rsflexidag/tests/test_commit_atomicity.rscmd/generator/src/lib.rsflexidag/tests/tests.rsnode/src/node.rssimnet/src/scene/mod.rsgenesis/src/lib.rsflexidag/src/blockdag.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:
test-helper/src/chain.rssync/src/tasks/test_tools.rschain/tests/block_test_utils.rschain/mock/src/mock_chain.rscmd/generator/src/lib.rsflexidag/tests/tests.rsnode/src/node.rsgenesis/src/lib.rsflexidag/src/blockdag.rs
📚 Learning: 2024-09-30T09:31:42.793Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4194
File: flexidag/src/blockdag.rs:444-446
Timestamp: 2024-09-30T09:31:42.793Z
Learning: In the service, `get_dag_state` is used to get the current state of the chain and it passes the main header ID to `BlockDAG`.
Applied to files:
test-helper/src/chain.rsflexidag/tests/tests.rs
📚 Learning: 2025-05-28T10:21:10.718Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4540
File: chain/src/chain.rs:1816-1824
Timestamp: 2025-05-28T10:21:10.718Z
Learning: In chain/src/chain.rs, the total_blocks calculation for epoch statistics always results in at least 1 block because total_selectd_chain_blocks = (current_block_number - epoch_start_block_number) + 1, which is always >= 1, making division by zero impossible in the avg_total_difficulty calculation.
Applied to files:
test-helper/src/chain.rsflexidag/tests/tests.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:
test-helper/src/chain.rssync/src/tasks/test_tools.rschain/tests/block_test_utils.rschain/mock/src/mock_chain.rscmd/generator/src/lib.rsnode/src/node.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:
sync/src/tasks/test_tools.rschain/tests/block_test_utils.rsflexidag/tests/test_commit_atomicity.rsgenesis/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:
cmd/generator/src/lib.rsnode/src/node.rstxpool/src/lib.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/tx_pool_service_impl.rstxpool/src/lib.rstxpool/src/tx_pool_actor_service.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
txpool/src/tx_pool_service_impl.rstxpool/src/lib.rstxpool/src/tx_pool_actor_service.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/lib.rstxpool/src/tx_pool_actor_service.rs
🧬 Code graph analysis (8)
sync/src/tasks/test_tools.rs (1)
flexidag/src/blockdag.rs (2)
genesis(561-563)create_blockdag(72-75)
chain/tests/block_test_utils.rs (1)
flexidag/src/blockdag.rs (2)
genesis(561-563)create_for_testing(124-131)
chain/mock/src/mock_chain.rs (1)
flexidag/src/blockdag.rs (2)
genesis(561-563)create_for_testing_with_parameters(134-138)
flexidag/tests/test_commit_atomicity.rs (1)
flexidag/src/blockdag.rs (2)
new(77-121)genesis(561-563)
node/src/node.rs (2)
types/src/startup_info.rs (7)
genesis_hash(37-39)new(25-31)new(100-102)new(176-178)new(220-222)new(262-264)new(299-301)flexidag/src/blockdag.rs (1)
new(77-121)
simnet/src/scene/mod.rs (1)
flexidag/src/blockdag.rs (2)
genesis(561-563)new(77-121)
genesis/src/lib.rs (1)
flexidag/src/blockdag.rs (3)
genesis(561-563)create_for_testing(124-131)create_for_testing_with_parameters(134-138)
txpool/src/tx_pool_actor_service.rs (3)
txpool/src/tx_pool_service_impl.rs (2)
new(41-83)status(272-274)config/src/txpool_config.rs (1)
cull_interval(88-90)txpool/src/pool.rs (1)
mem_usage(200-202)
🔇 Additional comments (18)
cmd/generator/src/lib.rs (2)
9-9: LGTM: Necessary imports added.The imports support the genesis hash retrieval and propagation logic introduced in this PR.
Also applies to: 15-15
53-69: LGTM: Genesis initialization flow correctly handles fresh and existing databases.The implementation properly handles both scenarios:
- Existing database: genesis_hash is retrieved and passed to BlockDAG
- Fresh initialization: genesis_hash defaults to zero, then is set after genesis execution
The conditional
set_genesisat lines 67-69 ensures the DAG is properly initialized in both cases.test-helper/src/chain.rs (1)
56-59: LGTM: Genesis hash correctly propagated to DAG constructor.The test helper now derives genesis_hash from the genesis block and passes it to
BlockDAG::create_for_testing, aligning with the PR's pattern of explicit genesis initialization.chain/mock/src/mock_chain.rs (1)
92-93: LGTM: Genesis hash correctly propagated to mock chain DAG initialization.The mock chain now derives genesis_hash from the genesis block and passes it to
BlockDAG::create_for_testing_with_parameters, consistent with the PR's genesis initialization pattern.chain/tests/block_test_utils.rs (1)
42-44: LGTM: Genesis hash correctly integrated into test utilities.The test utility now derives genesis_hash before DAG creation and passes it to
BlockDAG::create_for_testing, maintaining consistency with the PR's genesis initialization pattern.sync/src/tasks/test_tools.rs (1)
58-65: LGTM: Genesis hash correctly propagated to sync test DAG initialization.The sync test tool now derives genesis_hash from the genesis block and passes it to
BlockDAG::create_blockdag, aligning with the PR's pattern of explicit genesis hash propagation at DAG creation time.flexidag/tests/test_commit_atomicity.rs (1)
19-19: LGTM! Genesis hash properly threaded through BlockDAG construction.Both tests correctly compute the genesis ID before passing it to
BlockDAG::new, aligning with the updated constructor signature. The initialization flow (create genesis → create DAG with genesis ID → initialize DAG with full genesis) is clean and consistent.Also applies to: 77-77
simnet/src/scene/mod.rs (1)
43-48: LGTM! Clean genesis initialization.The genesis ID is computed upfront and passed to
BlockDAG::new, avoiding any temporary inconsistencies. This is the cleaner pattern compared to the deferred initialization approach.flexidag/tests/tests.rs (2)
41-41: LGTM! Tests properly updated with genesis hash parameter.Tests that work with actual genesis blocks correctly pass
genesis.id()to the BlockDAG constructor and follow up withinit_with_genesis. The initialization sequence is consistent across all tests.Also applies to: 97-97, 150-150, 207-207, 280-281, 376-376
351-351: Acceptable use of Hash::random() for low-level tests.These tests use
Hash::random()as a genesis placeholder since they're testing low-level functionality (DAG state storage, reachability algorithms) that doesn't require actual genesis initialization. This is appropriate and avoids unnecessary setup overhead for tests that don't need full genesis semantics.Also applies to: 407-407, 464-464, 590-590, 657-657, 713-713
genesis/src/lib.rs (2)
455-456: LGTM! Test helpers properly updated with genesis hash.All test initialization functions follow the correct pattern:
- Load or build the genesis
- Extract
genesis_hashfromgenesis.block().id()- Pass
genesis_hashto BlockDAG constructorThis ensures the DAG is always created with the correct genesis hash, avoiding the temporary placeholder approach.
Also applies to: 470-471, 488-489
564-572: LGTM! Test creates both DAG instances with the same genesis hash.The test correctly creates the genesis hash once and reuses it for both DAG instances, ensuring consistency. Each DAG instance is properly initialized via
init_and_check_storagewith the same genesis reference.node/src/node.rs (1)
303-322: The genesis initialization pattern shown is intentional and safe. After verification,execute_genesis_blockandinit_and_check_storagedo not calldag.genesis()or depend on the DAG instance containing the correct genesis value. The genesis information is passed through other channels (the genesis block parameter and return values), so the temporary state where the cloned DAG has a zero genesis while the original gets updated presents no actual issue.Likely an incorrect or invalid review comment.
flexidag/src/blockdag.rs (3)
72-75: LGTM: Genesis parameter properly added.The function signature correctly extends
create_blockdagto accept a genesis hash and forwards it to the underlying constructor.
77-121: LGTM: Constructor properly extended with genesis parameter.The
newconstructor cleanly integrates the genesis parameter and stores it in the struct. The existing validation and initialization logic remains intact.
124-149: LGTM: Testing constructors consistently updated.All three testing helper functions (
create_for_testing,create_for_testing_with_parameters,create_for_testing_with_k_and_merge_depth) have been properly extended to accept and forward the genesis parameter.txpool/src/lib.rs (1)
11-23: Module split + re-export is clean.This is a nice mechanical refactor that keeps
txpool::TxPoolActorServicestable at the import level.txpool/src/tx_pool_actor_service.rs (1)
109-167: Intervals wiring for propagate + cull is straightforward.Using
tx_propagate_interval()andcull_interval()for periodic work keeps behavior configurable and matches the PR goal (“every second” cull by default).
009f450 to
7860086
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/src/txpool_config.rs (1)
36-40: Clap-level range validation remains optional for better UX.The runtime safety is now ensured via
.max(1)in the accessor. The previously suggested clap range validation (value_parser = clap::value_parser!(u64).range(1..)) would provide immediate CLI feedback when a user passes0, but this is now a nice-to-have since invalid values are silently clamped rather than causing issues.txpool/src/tx_pool_actor_service.rs (1)
41-48: Constructor visibility restricts external instantiation.The
pub(crate)visibility onnewmeans downstream crates can referenceTxPoolActorService(via the re-export) but cannot construct it directly. This may be intentional if construction should only happen throughServiceFactory::create. If external instantiation is needed, change topub fn new.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
chain/mock/src/mock_chain.rschain/tests/block_test_utils.rscmd/generator/src/lib.rsconfig/src/txpool_config.rsflexidag/src/blockdag.rsflexidag/tests/test_commit_atomicity.rsflexidag/tests/tests.rsgenesis/src/lib.rsnode/src/node.rssimnet/src/scene/mod.rssync/src/tasks/test_tools.rstest-helper/src/chain.rstxpool/src/lib.rstxpool/src/tx_pool_actor_service.rstxpool/src/tx_pool_service_impl.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- sync/src/tasks/test_tools.rs
- test-helper/src/chain.rs
- flexidag/tests/tests.rs
- node/src/node.rs
- chain/mock/src/mock_chain.rs
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-09-13T14:15:32.756Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/starcoin/src/account/sign_multisig_txn_cmd.rs:61-63
Timestamp: 2025-09-13T14:15:32.756Z
Learning: In Starcoin codebase PR `#4655` (Clap v3 to v4 upgrade), maintainer welbon confirmed the preference for minimal configuration changes during the upgrade, specifically declining suggestions to add num_args or value_delimiter attributes to maintain CLI compatibility with existing behavior, even when it might mean less optimal UX compared to new Clap v4 features.
Applied to files:
config/src/txpool_config.rs
📚 Learning: 2025-09-13T14:13:05.713Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/resource-code-exporter/src/main.rs:42-44
Timestamp: 2025-09-13T14:13:05.713Z
Learning: In the Starcoin codebase Clap v3 to v4 upgrade (PR `#4655`), the maintainer welbon prefers minimal configuration changes to ensure compatibility, even if it means slight UX differences, rather than adding additional attributes like num_args or value_delimiter.
Applied to files:
config/src/txpool_config.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/tx_pool_service_impl.rstxpool/src/tx_pool_actor_service.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/tx_pool_service_impl.rstxpool/src/tx_pool_actor_service.rstxpool/src/lib.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
cmd/generator/src/lib.rsgenesis/src/lib.rsflexidag/tests/test_commit_atomicity.rschain/tests/block_test_utils.rsflexidag/src/blockdag.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:
cmd/generator/src/lib.rsgenesis/src/lib.rsflexidag/tests/test_commit_atomicity.rschain/tests/block_test_utils.rssimnet/src/scene/mod.rsflexidag/src/blockdag.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:
cmd/generator/src/lib.rschain/tests/block_test_utils.rs
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
cmd/generator/src/lib.rsgenesis/src/lib.rsflexidag/tests/test_commit_atomicity.rschain/tests/block_test_utils.rs
📚 Learning: 2025-07-03T03:21:32.104Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: vm/types/src/block_metadata/mod.rs:47-48
Timestamp: 2025-07-03T03:21:32.104Z
Learning: In the starcoin repository, the BlockMetadata structure changes are part of a clean slate implementation with no legacy data that needs to be deserialized, so backward compatibility concerns for field type changes are not applicable.
Applied to files:
cmd/generator/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:
cmd/generator/src/lib.rstxpool/src/lib.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
txpool/src/tx_pool_actor_service.rstxpool/src/lib.rs
🧬 Code graph analysis (3)
txpool/src/tx_pool_service_impl.rs (1)
commons/stream-task/src/event.rs (1)
now_seconds(13-18)
cmd/generator/src/lib.rs (3)
types/src/startup_info.rs (1)
genesis_hash(37-39)flexidag/src/blockdag.rs (1)
new(77-121)genesis/src/lib.rs (1)
init_and_check_storage(406-447)
genesis/src/lib.rs (1)
flexidag/src/blockdag.rs (3)
genesis(561-563)create_for_testing(124-131)create_for_testing_with_parameters(134-138)
🔇 Additional comments (22)
config/src/txpool_config.rs (2)
88-90: LGTM!The
.max(1)clamping ensurescull_intervalcan never return0, preventing tight scheduler loops regardless of user input.
120-122: LGTM!The merge logic follows the established pattern and correctly propagates the CLI option to the config.
txpool/src/tx_pool_service_impl.rs (1)
403-412: LGTM! Pre-import culling with reused pool client.The change to cull stale transactions before importing new ones is a reasonable approach. Reusing the
pool_clientfor both culling and importing avoids redundant state lookups.The timestamp conversion (
timestamp() / 1000) is consistent with the existingcull()method at line 392, converting milliseconds to seconds.genesis/src/lib.rs (4)
455-456: LGTM! Genesis hash propagation to BlockDAG.The pattern of computing
genesis_hashfromgenesis.block().id()and passing it toBlockDAG::create_for_testingis correct and consistent with the updated API.
470-471: LGTM! Consistent genesis hash propagation.Same pattern applied correctly for the parameterized test initialization.
488-489: LGTM! Consistent genesis hash propagation.Same pattern applied correctly for cache storage test initialization.
564-571: LGTM! Genesis hash reused correctly for both DAG instances.The test correctly computes
genesis_hashonce from the loaded genesis and reuses it for bothdag1(line 566) anddag2(line 571), ensuring both DAG instances are initialized with the same genesis.chain/tests/block_test_utils.rs (1)
42-44: LGTM! Genesis hash propagation in test utilities.The change correctly computes
genesis_hashfromgenesis.block().id()and passes it toBlockDAG::create_for_testing, consistent with the API changes across the codebase.flexidag/tests/test_commit_atomicity.rs (2)
19-22: LGTM! Updated BlockDAG constructor with genesis hash.The test correctly creates the genesis header first, passes
genesis.id()toBlockDAG::new, and then callsinit_with_genesiswith the full genesis header. This aligns with the new constructor signature.
77-80: LGTM! Consistent BlockDAG initialization pattern.Same correct pattern applied in the second test function.
cmd/generator/src/lib.rs (1)
53-69: The two-phase initialization pattern here is safe.BlockDAG::newstores the genesis parameter but does not perform any genesis-dependent operations during initialization. The genesis field is only used in getter methods (genesis(),get_dag_state()) and the setter, all called after initialization completes. PassingHashValue::zero()initially and updating it withdag.set_genesis()afterGenesis::init_and_check_storageworks correctly.Likely an incorrect or invalid review comment.
simnet/src/scene/mod.rs (1)
43-53: LGTM! Clean genesis propagation.The refactor correctly computes
genesis_idonce before constructingBlockDAGand reuses it for both the constructor parameter and thepruning_pointinitialization. This aligns with the updatedBlockDAG::newsignature requiring an explicit genesis hash.txpool/src/lib.rs (1)
13-22: LGTM! Good module organization.Extracting
TxPoolActorServiceinto its own module improves code organization and separation of concerns while maintaining the same public API surface via the re-export.flexidag/src/blockdag.rs (3)
68-68: LGTM! Genesis field addition.Adding the
genesisfield toBlockDAGenables consistent genesis hash propagation throughout the DAG operations.
72-149: LGTM! Constructor and factory method updates.All constructors and testing helpers now consistently accept and propagate the
genesisparameter, ensuring the DAG always knows its genesis hash from construction time.
548-559: The zero-hash to genesis mapping is defensive code with no actual callers relying on it.All uses of
get_dag_statein the codebase pass explicit hash values (genesis_id,pruning_point,bucket, etc.) and never passHash::zero(). The mapping exists as a fallback but is not part of the public contract that callers depend on. No action needed.txpool/src/tx_pool_actor_service.rs (6)
80-100: Relaxed ordering is acceptable for this flag pattern.The
compare_exchangewithRelaxedordering on both success and failure paths is appropriate here since the flag only controls whether to broadcast, and duplicate or missed broadcasts are tolerable. No strict synchronization is required.
109-138: LGTM! ServiceFactory implementation.The factory correctly retrieves shared dependencies from the service context and initializes
TxPoolServicewith proper error handling for missing startup info or blocks.
140-166: LGTM! Actor lifecycle with periodic culling.The
startedmethod correctly sets up event subscriptions and two periodic intervals:
- Transaction propagation at configurable interval
- Transaction culling at configurable
cull_interval(the new feature from this PR)This aligns with the PR objective of "culling at every second."
168-236: LGTM! Event handlers.All three event handlers are correctly implemented:
SyncStatusChangeEvent: Updates internal sync statusTxnStatusFullEvent: Updates metrics and signals new transactionsPeerTransactionsMessage: Imports transactions only when synced, with appropriate VM1/VM2 bypass logic
238-252: LGTM! Trait bounds tests.Good practice to ensure
TxPoolActorServicesatisfiesSend + Sync + 'staticrequirements for use with the actor system.
233-233: No action needed. Thedebug!macro is available in this module through the#[macro_use] extern crate log;declaration at the crate root inlib.rs(line 5-6). All macros from the log crate are automatically in scope for all modules, includingtx_pool_actor_service, without requiring explicit imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@txpool/src/tx_pool_actor_service.rs`:
- Around line 1-23: The code calls the debug! macro but doesn't import it; add
the appropriate logging macro import (for example, add use log::debug; at the
top of tx_pool_actor_service.rs) so the debug!(...) invocation compiles; ensure
you import the same logging crate used elsewhere in the project if it uses
tracing (e.g., use tracing::debug;) to match existing logging conventions.
♻️ Duplicate comments (1)
txpool/src/tx_pool_actor_service.rs (1)
38-45: Constructor visibility may conflict with public type re-export.The
TxPoolActorServicetype is re-exported publicly fromtxpool/src/lib.rs, but this constructor ispub(crate). External crates can name but not construct this type directly.
🧹 Nitpick comments (2)
txpool/src/tx_pool_actor_service.rs (2)
54-75: Consider makingmax_lenconfigurable.The hardcoded
max_len = 100could be extracted toTxPoolConfigalongside other pool configuration values likecull_intervalandtx_propagate_interval, making it tunable without code changes.
214-233: TODO: Consider buffering transactions during sync.The TODO on line 229 notes that transactions received while not synced are discarded. Buffering these for later import could improve transaction propagation latency after sync completes.
Would you like me to open a new issue to track implementing transaction buffering during sync, or help draft an implementation approach?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
txpool/src/tx_pool_actor_service.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
txpool/src/tx_pool_actor_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
txpool/src/tx_pool_actor_service.rs
🧬 Code graph analysis (1)
txpool/src/tx_pool_actor_service.rs (3)
txpool/src/tx_pool_service_impl.rs (2)
new(41-83)status(272-274)config/src/txpool_config.rs (1)
cull_interval(88-90)txpool/src/pool.rs (1)
mem_usage(200-202)
🔇 Additional comments (7)
txpool/src/tx_pool_actor_service.rs (7)
24-36: LGTM!The struct design is appropriate for an actor service. Using
Arc<AtomicBool>fornew_txs_receivedenables safe sharing across cloned instances used in interval closures.
77-97: LGTM!The
compare_exchangepattern correctly handles the atomic flag—resetting it only if it wastrue, preventing duplicate broadcasts. TheRelaxedordering is appropriate for this simple signaling flag.
106-135: LGTM!The
ServiceFactoryimplementation correctly retrieves shared dependencies and lazily creates theTxPoolServicesingleton. Error messages are descriptive for missing startup info or blocks.
137-163: LGTM!The lifecycle implementation correctly sets up event subscriptions and periodic tasks. Cloning
selffor interval closures works correctly becauseTxPoolActorServiceisClonewithArc-wrapped shared state.
165-169: LGTM!Simple and correct handling of sync status updates.
172-212: LGTM!The metrics update logic is comprehensive, tracking pool status (mem_usage, senders, count) and per-transaction events. The
new_txs_receivedflag is correctly set only whenTxStatus::Addedevents occur.
235-249: LGTM!The compile-time trait bound tests ensure
TxPoolActorServicesatisfiesSend + Sync + 'static, which is essential for safe use in the async actor framework.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
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
Improvements
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.