Mint instantly if the uncle rate is below the configured value #4438
Conversation
WalkthroughThe changes introduce a new workspace dependency in the miner project and refactor event handling logic across modules. In the miner component, a time-based field is removed and replaced by a storage reference to support state-based processing of block events. The sync component streamlines block connectivity and error handling while adding an executed block field to synchronization events. Additionally, system event structures have been updated to carry block header information. Changes
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
miner/Cargo.toml(1 hunks)miner/src/generate_block_event_pacemaker.rs(3 hunks)sync/src/block_connector/block_connector_service.rs(2 hunks)sync/src/sync.rs(3 hunks)types/src/system_events.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
types/src/system_events.rs (1)
types/src/block/mod.rs (1)
block(1405-1407)
miner/src/generate_block_event_pacemaker.rs (4)
sync/src/sync.rs (4)
create(660-666)ctx(223-223)new(79-122)new(821-827)chain/src/chain.rs (5)
chain_state(1695-1697)new(76-87)e(178-178)e(684-684)e(1557-1557)chain/api/src/chain.rs (1)
chain_state(134-134)network-rpc/src/rpc.rs (1)
new(42-54)
sync/src/sync.rs (5)
sync/src/tasks/mod.rs (2)
new(575-577)new(598-600)sync/src/tasks/block_sync_task.rs (3)
new(49-55)new(77-103)chain(354-355)sync/src/block_connector/block_connector_service.rs (1)
new(62-71)sync/src/tasks/inner_sync_task.rs (1)
new(50-76)sync/src/tasks/mock.rs (5)
new(53-59)new(104-106)new(142-144)new(189-216)chain(338-340)
🔇 Additional comments (11)
miner/Cargo.toml (1)
25-25: Dependency addition confirmed.The new workspace dependency
starcoin-statedbis properly declared.types/src/system_events.rs (1)
40-42: New field addition appears appropriate.Adding
executed_blockas anArc<BlockHeader>is a reasonable choice and should facilitate concurrent usage with minimal overhead.sync/src/sync.rs (4)
35-35: Import usage check.The newly added imports for
Block,BlockHeader, andBlockIdAndNumberare valid and align with the logic introduced in this file.
731-733: Struct naming is consistent.
SpecificSyncDone's single field for the executed block header is concise. If you plan to add more details, consider grouping them in a dedicated struct.
736-739: Event broadcasting is coherent.Broadcasting
NewDagBlockFromPeerwith the newly addedexecuted_blockis consistent with the changes.
420-422:✅ Verification successful
Verify the correctness of passing only the block header.
It might be worth confirming that referencing only the header with
Arc::new(chain.current_header())provides sufficient data for downstream operations.You can inspect all uses of
SpecificSyncDoneto confirm that no full block data is required:
🏁 Script executed:
#!/bin/bash # Searching for usage of `SpecificSyncDone` to see if a full block is needed anywhere. ast-grep --pattern $'SpecificSyncDone { $$$ }'Length of output: 240
Verification Complete: Passing Only the Block Header is Sufficient
The search result confirms that the only usage of
SpecificSyncDoneis insync/src/sync.rsat lines 420–422, where only the block header (wrapped in anArc) is provided. There’s no indication elsewhere in the codebase that full block data is required for downstream operations.miner/src/generate_block_event_pacemaker.rs (3)
9-11: Imports are suitable.The introduction of
AccountStateReader,ChainStateDB, andStoragealigns with the new state-based logic.
22-22: New struct field is needed for state access.Adding
storageenables on-demandChainStateDBcreation.
30-30: Field injection confirmed.Acquiring
storagefrom the shared context seamlessly provides the necessary state access.sync/src/block_connector/block_connector_service.rs (2)
325-391: Improved error handling with enhanced event broadcasting.The error handling logic has been restructured to use a single match statement for the result of
try_connect, making the code more readable and maintainable. Additionally, the successful case now broadcasts aNewDagBlockFromPeerevent with the executed block header, providing more context about the executed block.The new structure:
- First handles the success case by broadcasting the block header
- Then handles error cases in a more organized way
- Keeps the FutureBlock handling logic intact
This change complements the modification to
NewDagBlockFromPeerstruct which now includes anexecuted_blockfield.
145-145:❓ Verification inconclusive
Significant increase in near block threshold.
The calculation for
config_gaphas been changed fromk.saturating_mul(2)tok.saturating_mul(k), which squares the value instead of doubling it. This change significantly increases the threshold for what's considered a "near" block, potentially allowing blocks that are much further away to be processed via the specific/lightweight sync operation rather than the burden sync operation.
🏁 Script executed:
#!/bin/bash # Search for k value usage in the codebase to understand the impact rg -B 5 -A 5 "ghost_dag_manager.*k\(\)" --type rustLength of output: 1117
Important: Verify the Increased Near Block Threshold Computation
The calculation for
config_gapis now usingk.saturating_mul(k), which squares the value ofkinstead of doubling it (as ink.saturating_mul(2)). Sincekis sourced fromghost_dag_manager().k(), whenk > 2this change substantially raises the threshold for what qualifies as a "near" block. This may result in blocks much further away being processed using the lightweight sync operation instead of the burden sync path.
- Location:
sync/src/block_connector/block_connector_service.rs(around line 145)- Implication: Verify that the squaring of
kis intentional, as it could have broader effects on the sync behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
miner/src/generate_block_event_pacemaker.rs (4)
102-129: The uncle rate-based block generation logic is well implemented but has room for improvement.The implementation correctly:
- Retrieves the state root and sets up the chain state
- Gets epoch information and calculates the uncle rate
- Checks for division by zero (addressing the previous review comment)
- Compares the uncle rate with the configured target
However, there are a few issues to address:
- Using
unwrap_or_elsewith a panic is not ideal for production code- There's a typo in
uncle_rate_in_confing- The log at line 126 is very verbose for INFO level
Consider these improvements:
fn handle_event(&mut self, msg: NewDagBlockFromPeer, ctx: &mut ServiceContext<Self>) { let state_root = msg.executed_block.state_root(); let chain_state = ChainStateDB::new(self.storage.clone().into_super_arc(), Some(state_root)); let account_reader = AccountStateReader::new(&chain_state); - let epoch_info = account_reader - .get_epoch_info() - .unwrap_or_else(|e| panic!("Epoch is none. error: {:?}", e)); + let epoch_info = match account_reader.get_epoch_info() { + Ok(info) => info, + Err(e) => { + error!("[pacemaker] Failed to get epoch info: {:?}", e); + return; + } + }; let total_uncles = epoch_info.epoch_data().uncles(); let blocks = msg .executed_block .number() .saturating_sub(epoch_info.epoch().start_block_number()); if blocks == 0 { debug!("[pacemaker] Zero blocks in the epoch, skipping uncle rate computation."); return; } let uncle_rate = total_uncles * 1000 / blocks; - let uncle_rate_in_confing = self + let uncle_rate_target = self .config .net() .genesis_config() .consensus_config .uncle_rate_target; - info!("NewDagBlockFromPeer, epoch data: {:?}, blocks in this epoch: {:?}, uncle rate for now: {:?}, uncle rate in config: {:?}", epoch_info.epoch_data(), blocks, uncle_rate, uncle_rate_in_confing); - if uncle_rate < uncle_rate_in_confing { + debug!("[pacemaker] NewDagBlockFromPeer - blocks: {}, uncles: {}, uncle_rate: {}, target: {}", + blocks, total_uncles, uncle_rate, uncle_rate_target); + if uncle_rate < uncle_rate_target { + info!("[pacemaker] Uncle rate {} below target {}, triggering block generation", + uncle_rate, uncle_rate_target); self.send_event(false, ctx); } }
120-120: Fix the typo in variable name.There's a typo in the variable name
uncle_rate_in_confing, which should beuncle_rate_in_configor simplyuncle_rate_target.- let uncle_rate_in_confing = self + let uncle_rate_target = self
126-126: Adjust log level and improve log message formatting.The current log message is very verbose for the INFO level and doesn't follow the [pacemaker] prefix convention used elsewhere. Consider moving it to DEBUG level for regular operation and adding a more concise INFO log when actually triggering block generation.
- info!("NewDagBlockFromPeer, epoch data: {:?}, blocks in this epoch: {:?}, uncle rate for now: {:?}, uncle rate in config: {:?}", epoch_info.epoch_data(), blocks, uncle_rate, uncle_rate_in_confing); + debug!("[pacemaker] NewDagBlockFromPeer - blocks: {}, uncles: {}, uncle_rate: {}, target: {}", + blocks, total_uncles, uncle_rate, uncle_rate_target); if uncle_rate < uncle_rate_target { + info!("[pacemaker] Uncle rate {} below target {}, triggering block generation", + uncle_rate, uncle_rate_target); self.send_event(false, ctx); }
120-125: Consider extracting the config access pattern to improve readability.The deeply nested config access to retrieve the uncle rate target makes the code harder to read. Consider extracting this to a helper method or caching the value at service initialization.
Option 1 - Extract to a method:
+ impl GenerateBlockEventPacemaker { + // ... existing methods ... + + fn get_uncle_rate_target(&self) -> u64 { + self.config + .net() + .genesis_config() + .consensus_config + .uncle_rate_target + } + } // In handle_event: - let uncle_rate_in_confing = self - .config - .net() - .genesis_config() - .consensus_config - .uncle_rate_target; + let uncle_rate_target = self.get_uncle_rate_target();Option 2 - Cache at initialization:
pub struct GenerateBlockEventPacemaker { config: Arc<NodeConfig>, sync_status: Option<SyncStatus>, storage: Arc<Storage>, + uncle_rate_target: u64, } impl ServiceFactory<Self> for GenerateBlockEventPacemaker { fn create(ctx: &mut ServiceContext<Self>) -> Result<Self> { + let config = ctx.get_shared::<Arc<NodeConfig>>()?; + let uncle_rate_target = config + .net() + .genesis_config() + .consensus_config + .uncle_rate_target; Ok(Self { - config: ctx.get_shared::<Arc<NodeConfig>>()?, + config, sync_status: None, storage: ctx.get_shared::<Arc<Storage>>()?, + uncle_rate_target, }) } } // In handle_event: - let uncle_rate_in_confing = self - .config - .net() - .genesis_config() - .consensus_config - .uncle_rate_target; + let uncle_rate_target = self.uncle_rate_target;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
miner/src/generate_block_event_pacemaker.rs(3 hunks)sync/src/tasks/continue_execute_absent_block.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
miner/src/generate_block_event_pacemaker.rs (5)
sync/src/sync.rs (13)
create(660-666)ctx(223-223)handle_event(687-696)handle_event(700-727)handle_event(736-740)handle_event(752-807)handle_event(831-836)handle_event(840-848)handle_event(859-895)handle_event(899-906)handle_event(910-915)new(79-122)new(821-827)miner/src/lib.rs (3)
create(109-123)handle_event(273-296)new(79-85)chain/src/chain.rs (5)
chain_state(1695-1697)new(76-87)e(178-178)e(684-684)e(1557-1557)chain/api/src/chain.rs (1)
chain_state(134-134)network-rpc/src/rpc.rs (1)
new(42-54)
🔇 Additional comments (6)
sync/src/tasks/continue_execute_absent_block.rs (1)
30-30: Good use ofSelffor the return type.Changing the return type from
anyhow::Result<ContinueExecuteAbsentBlock<'a>>toanyhow::Result<Self>is a good practice in Rust. This makes the code more maintainable as it:
- Reduces redundancy
- Improves readability
- Ensures the return type automatically updates if the struct name changes in the future
This is a clean, idiomatic Rust change that maintains the same functionality while making the code more robust to future refactoring.
miner/src/generate_block_event_pacemaker.rs (5)
9-11: New imports support state-based decision making.The addition of these imports aligns with the shift from a time-based approach to a state-based approach for block generation decisions. This is a good architectural change that allows the pacemaker to make decisions based on blockchain state rather than just elapsed time.
22-22: Appropriate field change from time-based to state-based.Replacing the
last_time_receivedtimestamp with a storage reference is consistent with the new approach of using uncle rate as the trigger for block generation instead of elapsed time.
30-30: Storage initialization is properly implemented.The service now correctly retrieves the shared Storage instance from the context, which is needed for the state-based block generation decisions.
115-118: Good handling of division by zero case.You've properly addressed the previous review comment regarding division by zero risk by adding this check. This is important for preventing panics in edge cases.
102-129:❓ Verification inconclusive
Verify blockchain state performance impact.
The current implementation accesses blockchain state for every
NewDagBlockFromPeerevent. Consider evaluating the performance impact of this approach, especially the storage clone operation.
🏁 Script executed:
#!/bin/bash # Check if there are other uses of storage.clone() in event handlers to understand common patterns echo "Searching for storage.clone() patterns in event handlers..." rg -A 2 -B 2 "fn handle_event.*\{" | rg "storage\.clone\(\)" -A 5 -B 5Length of output: 201
Assess the Performance Impact of Frequent Blockchain State Access
The current implementation creates a new chain state via
storage.clone()on everyNewDagBlockFromPeerevent. Since the initial search for similar patterns in event handlers returned no results, it's unclear whether this pattern is common or if it incurs a significant performance overhead. Please manually verify that this approach does not degrade performance under high load. Specifically, consider:
- Benchmarking the cost of cloning storage for each event.
- Reviewing whether similar event handlers use alternative, more efficient patterns.
- Evaluating if caching or lazy initialization of the chain state is feasible to reduce overhead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
miner/src/generate_block_event_pacemaker.rs (2)
124-129: Verify configuration parameter naming.There appears to be a typo in the variable name
uncle_rate_in_confing, which might make the code slightly less readable.- let uncle_rate_in_confing = self + let uncle_rate_in_config = selfMake sure to update the reference on line 130 as well.
130-130: Consider log level for production environment.While informative, this log statement produces output on every new block from peers. Consider whether
infois the appropriate log level for this frequent operation, or ifdebugmight be more suitable for production environments.- info!("NewDagBlockFromPeer, epoch data: {:?}, blocks in this epoch: {:?}, uncle rate for now: {:?}, uncle rate in config: {:?}", epoch_info.epoch_data(), blocks, uncle_rate, uncle_rate_in_confing); + debug!("NewDagBlockFromPeer, epoch data: {:?}, blocks in this epoch: {:?}, uncle rate for now: {:?}, uncle rate in config: {:?}", epoch_info.epoch_data(), blocks, uncle_rate, uncle_rate_in_confing);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
miner/src/generate_block_event_pacemaker.rs(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
miner/src/generate_block_event_pacemaker.rs (7)
sync/src/sync.rs (13)
create(660-666)ctx(223-223)handle_event(687-696)handle_event(700-727)handle_event(736-740)handle_event(752-807)handle_event(831-836)handle_event(840-848)handle_event(859-895)handle_event(899-906)handle_event(910-915)new(79-122)new(821-827)miner/src/lib.rs (3)
create(109-123)handle_event(273-296)new(79-85)chain/src/chain.rs (5)
chain_state(1695-1697)new(76-87)e(178-178)e(684-684)e(1557-1557)chain/api/src/chain.rs (1)
chain_state(134-134)sync/src/tasks/continue_execute_absent_block.rs (1)
new(26-36)network-rpc/src/rpc.rs (1)
new(42-54)sync/src/tasks/mock.rs (2)
new(53-59)new(104-106)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run benchmark
🔇 Additional comments (4)
miner/src/generate_block_event_pacemaker.rs (4)
9-11: Appropriate imports added for the state-based approach.The new imports for
AccountStateReader,ChainStateDB, andStorageproperly support the refactoring from time-based to state-based block event handling.
22-22: Good structural change to support the new uncle-rate based approach.The replacement of
last_time_receivedwithstoragefield properly shifts from time-based tracking to state-based evaluation.
30-30: Proper initialization of the storage field.The storage is correctly retrieved from the shared context, maintaining consistency with other service components.
102-133: Well-implemented state-based evaluation logic for instant minting.The refactoring from time-based to state-based block event handling is well-structured. The implementation:
- Uses blockchain state to assess uncle rates
- Properly handles potential division-by-zero issues
- Uses appropriate error handling instead of unwrap_or_else with panic
- Includes informative logging
The approach of minting instantly when the uncle rate is below the configured target is sound, as it indicates the network can safely process more blocks without risking excessive uncle generation.
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
Refactor
Chores