-
Notifications
You must be signed in to change notification settings - Fork 17
Refactor: Move difficulty adjustment ownership to lib-consensus #641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Refactor: Move difficulty adjustment ownership to lib-consensus #641
Conversation
PeterGRutherford
commented
Jan 6, 2026
- Create DifficultyManager in lib-consensus
- Move DifficultyConfig to lib-consensus
- Update BlockchainConsensusCoordinator to use DifficultyManager
- Update Blockchain to delegate difficulty adjustment to Coordinator
- Add governance parameters for difficulty adjustment
- Update documentation and architecture diagrams
- Create DifficultyManager in lib-consensus - Move DifficultyConfig to lib-consensus - Update BlockchainConsensusCoordinator to use DifficultyManager - Update Blockchain to delegate difficulty adjustment to Coordinator - Add governance parameters for difficulty adjustment - Update documentation and architecture diagrams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors difficulty adjustment ownership from lib-blockchain to lib-consensus, establishing the consensus layer as the authoritative source for difficulty policy. The changes implement a new DifficultyManager that encapsulates Bitcoin-style difficulty adjustment logic and makes parameters governable via DAO proposals.
Key Changes:
- Created
DifficultyManagerandDifficultyConfigin lib-consensus with Bitcoin-compatible defaults - Updated
BlockchainConsensusCoordinatorto own and manage difficulty calculations - Modified
Blockchain::adjust_difficulty()to delegate to consensus coordinator while maintaining backward compatibility - Extended DAO governance to support blockchain difficulty parameter updates
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib-consensus/src/lib.rs | Adds difficulty module export and re-exports key types |
| lib-consensus/src/difficulty.rs | New module implementing DifficultyManager with Bitcoin-style adjustment algorithm, validation, and DAO governance support |
| lib-consensus/src/dao/dao_types.rs | Adds governance parameters for blockchain difficulty (InitialDifficulty, AdjustmentInterval, TargetTimespan) |
| lib-consensus/src/dao/dao_engine.rs | Adds validation for blockchain difficulty parameters in DAO governance proposals |
| lib-consensus/docs/api-reference.md | Documents DifficultyManager API, configuration options, and DAO integration patterns |
| lib-consensus/README.md | Adds difficulty management section highlighting consensus ownership and DAO governance |
| lib-blockchain/src/lib.rs | Re-exports difficulty types from lib-consensus for convenience |
| lib-blockchain/src/integration/consensus_integration.rs | Integrates DifficultyManager into BlockchainConsensusCoordinator with delegation methods |
| lib-blockchain/src/blockchain.rs | Delegates difficulty adjustment to consensus coordinator with legacy fallback |
| lib-blockchain/docs/architecture.md | Updates architecture to include difficulty management coordination |
| lib-blockchain/docs/api-reference.md | Documents that DIFFICULTY_ADJUSTMENT_INTERVAL is now a fallback constant |
| Cargo.toml | Changes package.metadata to workspace.metadata (appears unrelated to PR scope) |
| Cargo.lock | Adds new dependencies (rand_chacha, turmoil, rand_distr, libm) not mentioned in PR description |
lib-consensus/src/difficulty.rs
Outdated
| height > 0 | ||
| && height >= self.config.adjustment_interval |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition height > 0 && height >= self.config.adjustment_interval is redundant. If height >= adjustment_interval is true and adjustment_interval is guaranteed to be greater than zero (validated in config), then height > 0 is always true. The first condition can be removed.
| height > 0 | |
| && height >= self.config.adjustment_interval | |
| height >= self.config.adjustment_interval |
lib-blockchain/src/blockchain.rs
Outdated
| // Use tokio block_in_place to call async methods from sync context | ||
| // This is safe because we're not holding any locks when calling this | ||
| let interval = tokio::task::block_in_place(|| { | ||
| tokio::runtime::Handle::current().block_on(async { | ||
| let coord = coordinator.read().await; | ||
| coord.get_difficulty_adjustment_interval().await | ||
| }) | ||
| }); | ||
| let timespan = tokio::task::block_in_place(|| { | ||
| tokio::runtime::Handle::current().block_on(async { | ||
| let coord = coordinator.read().await; | ||
| let config = coord.get_difficulty_config().await; | ||
| config.target_timespan | ||
| }) | ||
| }); | ||
| (interval, timespan) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function acquires the coordinator read lock twice in separate block_in_place calls. This is inefficient and could lead to race conditions if the configuration changes between the two calls. Consider acquiring the lock once and getting both values in a single async block.
| // Use tokio block_in_place to call async methods from sync context | |
| // This is safe because we're not holding any locks when calling this | |
| let interval = tokio::task::block_in_place(|| { | |
| tokio::runtime::Handle::current().block_on(async { | |
| let coord = coordinator.read().await; | |
| coord.get_difficulty_adjustment_interval().await | |
| }) | |
| }); | |
| let timespan = tokio::task::block_in_place(|| { | |
| tokio::runtime::Handle::current().block_on(async { | |
| let coord = coordinator.read().await; | |
| let config = coord.get_difficulty_config().await; | |
| config.target_timespan | |
| }) | |
| }); | |
| (interval, timespan) | |
| // Use a single tokio block_in_place to call async methods from sync context | |
| // Acquire the coordinator read lock once to avoid race conditions and redundant locking | |
| tokio::task::block_in_place(|| { | |
| tokio::runtime::Handle::current().block_on(async { | |
| let coord = coordinator.read().await; | |
| let interval = coord.get_difficulty_adjustment_interval().await; | |
| let config = coord.get_difficulty_config().await; | |
| (interval, config.target_timespan) | |
| }) | |
| }) |
lib-blockchain/src/blockchain.rs
Outdated
| let actual_timespan = interval_end_time - interval_start_time; | ||
| // Clamp to prevent extreme adjustments (4x range) | ||
| let actual_timespan = actual_timespan.max(target_timespan / 4).min(target_timespan * 4); | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The legacy calculation does not handle potential division by zero when actual_timespan is zero after clamping. While the clamping should prevent this, if target_timespan is zero (which should be caught by validation), this could still occur. Consider adding a defensive check or at least documenting the assumption that target_timespan is never zero.
| let actual_timespan = interval_end_time - interval_start_time; | |
| // Clamp to prevent extreme adjustments (4x range) | |
| let actual_timespan = actual_timespan.max(target_timespan / 4).min(target_timespan * 4); | |
| // Defensive check: target_timespan should be validated to be non-zero upstream, | |
| // but avoid panicking here if that validation is ever bypassed. | |
| if target_timespan == 0 { | |
| warn!( | |
| "calculate_difficulty_legacy called with target_timespan = 0; \ | |
| returning current difficulty without adjustment" | |
| ); | |
| return self.difficulty.bits(); | |
| } | |
| let actual_timespan = interval_end_time.saturating_sub(interval_start_time); | |
| // Clamp to prevent extreme adjustments (4x range) | |
| let actual_timespan = actual_timespan | |
| .max(target_timespan / 4) | |
| .min(target_timespan * 4); | |
| // Additional defensive check in case clamping still results in zero. | |
| if actual_timespan == 0 { | |
| warn!( | |
| "calculate_difficulty_legacy computed actual_timespan = 0 after clamping; \ | |
| returning current difficulty without adjustment" | |
| ); | |
| return self.difficulty.bits(); | |
| } |
| pub fn set_min_difficulty(&mut self, min_difficulty: u32) -> DifficultyResult<()> { | ||
| if min_difficulty > self.config.max_difficulty { | ||
| return Err(DifficultyError::InvalidConfig( | ||
| "Min difficulty cannot exceed max difficulty".to_string(), | ||
| )); | ||
| } | ||
| self.config.min_difficulty = min_difficulty; | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Set the maximum difficulty bound | ||
| pub fn set_max_difficulty(&mut self, max_difficulty: u32) -> DifficultyResult<()> { | ||
| if max_difficulty < self.config.min_difficulty { | ||
| return Err(DifficultyError::InvalidConfig( | ||
| "Max difficulty cannot be less than min difficulty".to_string(), | ||
| )); | ||
| } | ||
| self.config.max_difficulty = max_difficulty; | ||
| Ok(()) | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_min_difficulty and set_max_difficulty methods are not covered by tests. These methods handle important validation logic (ensuring min <= max) that should be tested to prevent regression.
| pub fn set_max_adjustment_factor(&mut self, factor: u64) -> DifficultyResult<()> { | ||
| if factor == 0 { | ||
| return Err(DifficultyError::InvalidConfig( | ||
| "Max adjustment factor must be greater than zero".to_string(), | ||
| )); | ||
| } | ||
| self.config.max_adjustment_factor = factor; | ||
| Ok(()) | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_max_adjustment_factor method is not covered by tests. This method is critical for preventing extreme difficulty adjustments and should be tested, especially the validation that prevents zero values.
| [package.metadata.dev-tools] | ||
| # Helper metadata for developers: local test runner script path | ||
| run-tests = "scripts/run_tests.sh" | ||
| [workspace.metadata.dev-tools] |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from [package.metadata.dev-tools] to [workspace.metadata.dev-tools] appears unrelated to the difficulty management refactoring. If this is intentional, it should be mentioned in the PR description or committed separately. If unintentional, it should be reverted.
| [workspace.metadata.dev-tools] | |
| [package.metadata.dev-tools] |
| let min_timespan = self.config.target_timespan / self.config.max_adjustment_factor; | ||
| let max_timespan = self.config.target_timespan * self.config.max_adjustment_factor; | ||
| let clamped_timespan = actual_timespan.max(min_timespan).min(max_timespan); | ||
|
|
||
| // Calculate new difficulty: current * target / actual | ||
| // Using u64 intermediate to prevent overflow | ||
| let new_difficulty = (current_difficulty as u64) | ||
| .saturating_mul(self.config.target_timespan) | ||
| .checked_div(clamped_timespan) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm documentation states the formula uses division, but this could result in division by zero if clamped_timespan is 0. Although the timespan is clamped with a minimum value, if target_timespan is very small and max_adjustment_factor is very large, min_timespan could theoretically be 0 due to integer division. Consider adding an explicit check before the division or ensuring min_timespan cannot be zero.
| ## Difficulty Management | ||
|
|
||
| The consensus package owns the blockchain mining difficulty adjustment policy. This ensures that difficulty parameters can be governed via DAO proposals and maintains clear separation of concerns. | ||
|
|
||
| ### DifficultyManager | ||
|
|
||
| Manages blockchain mining difficulty calculations and DAO governance updates. | ||
|
|
||
| ```rust | ||
| pub struct DifficultyManager { | ||
| config: DifficultyConfig, | ||
| } | ||
| ``` | ||
|
|
||
| #### Methods | ||
|
|
||
| **`new(config: DifficultyConfig) -> Self`** | ||
| - Creates a new difficulty manager with the given configuration | ||
| - `config`: Initial difficulty configuration | ||
| - Returns: New DifficultyManager instance | ||
|
|
||
| **`default() -> Self`** | ||
| - Creates a new difficulty manager with Bitcoin-compatible defaults | ||
| - Initial difficulty: `0x1d00ffff` | ||
| - Adjustment interval: `2016` blocks | ||
| - Target timespan: `1209600` seconds (2 weeks) | ||
|
|
||
| **`config() -> &DifficultyConfig`** | ||
| - Gets the current difficulty configuration | ||
| - Returns: Reference to DifficultyConfig | ||
|
|
||
| **`config_mut() -> &mut DifficultyConfig`** | ||
| - Gets a mutable reference to the difficulty configuration (for internal updates) | ||
| - Returns: Mutable reference to DifficultyConfig | ||
|
|
||
| **`initial_difficulty() -> u32`** | ||
| - Gets the initial difficulty value | ||
| - Returns: Initial difficulty in Bitcoin compact format | ||
|
|
||
| **`adjustment_interval() -> u64`** | ||
| - Gets the adjustment interval (blocks between difficulty adjustments) | ||
| - Returns: Number of blocks | ||
|
|
||
| **`target_timespan() -> u64`** | ||
| - Gets the target timespan for adjustment intervals | ||
| - Returns: Target time in seconds | ||
|
|
||
| **`should_adjust(height: u64) -> bool`** | ||
| - Checks if difficulty should be adjusted at the given block height | ||
| - `height`: Current blockchain height | ||
| - Returns: `true` if adjustment should occur | ||
|
|
||
| **`calculate_new_difficulty(current_difficulty: u32, actual_timespan: u64) -> DifficultyResult<u32>`** | ||
| - Calculates new difficulty based on actual vs target timespan | ||
| - `current_difficulty`: Current difficulty in compact format | ||
| - `actual_timespan`: Actual time taken for the last interval (seconds) | ||
| - Returns: New difficulty value, clamped to prevent extreme changes (4x max) | ||
| - Algorithm: `new_difficulty = current * target_timespan / actual_timespan` | ||
|
|
||
| **`adjust_difficulty(height: u64, current_difficulty: u32, interval_start_time: u64, interval_end_time: u64) -> DifficultyResult<Option<u32>>`** | ||
| - Main entry point for difficulty adjustment | ||
| - `height`: Current blockchain height | ||
| - `current_difficulty`: Current difficulty value | ||
| - `interval_start_time`: Timestamp of block at start of interval | ||
| - `interval_end_time`: Timestamp of current block | ||
| - Returns: `Some(new_difficulty)` if adjustment occurred, `None` otherwise | ||
|
|
||
| **`apply_governance_update(initial_difficulty: Option<u32>, adjustment_interval: Option<u64>, target_timespan: Option<u64>) -> DifficultyResult<()>`** | ||
| - Applies DAO governance updates to difficulty parameters | ||
| - All parameters are optional (only specified parameters are updated) | ||
| - Validates configuration before applying (no zero values, no invalid ranges) | ||
| - Returns: `Ok(())` if successful, `Err(...)` if validation fails | ||
|
|
||
| **`set_min_difficulty(min_difficulty: u32) -> DifficultyResult<()>`** | ||
| - Sets the minimum difficulty bound | ||
| - `min_difficulty`: Minimum allowed difficulty | ||
| - Returns: Error if `min_difficulty > max_difficulty` | ||
|
|
||
| **`set_max_difficulty(max_difficulty: u32) -> DifficultyResult<()>`** | ||
| - Sets the maximum difficulty bound | ||
| - `max_difficulty`: Maximum allowed difficulty | ||
| - Returns: Error if `max_difficulty < min_difficulty` | ||
|
|
||
| **`set_max_adjustment_factor(factor: u64) -> DifficultyResult<()>`** | ||
| - Sets the maximum adjustment factor per interval | ||
| - `factor`: Maximum multiplier/divisor for difficulty changes | ||
| - Default: `4` (difficulty can at most quadruple or quarter per interval) | ||
|
|
||
| ### DifficultyConfig | ||
|
|
||
| Configuration for blockchain difficulty adjustment. | ||
|
|
||
| ```rust | ||
| pub struct DifficultyConfig { | ||
| pub initial_difficulty: u32, | ||
| pub adjustment_interval: u64, | ||
| pub target_timespan: u64, | ||
| pub min_difficulty: u32, | ||
| pub max_difficulty: u32, | ||
| pub max_adjustment_factor: u64, | ||
| } | ||
| ``` | ||
|
|
||
| **Fields:** | ||
| - `initial_difficulty`: Initial difficulty for genesis block (Bitcoin compact format) | ||
| - `adjustment_interval`: Number of blocks between difficulty adjustments | ||
| - `target_timespan`: Target time for completing an adjustment interval (seconds) | ||
| - `min_difficulty`: Minimum allowed difficulty (default: `1`) | ||
| - `max_difficulty`: Maximum allowed difficulty (default: `0xFFFFFFFF`) | ||
| - `max_adjustment_factor`: Maximum change per interval (default: `4`) | ||
|
|
||
| **Default Values (Bitcoin-compatible):** | ||
| ```rust | ||
| DifficultyConfig { | ||
| initial_difficulty: 0x1d00ffff, // Bitcoin's initial difficulty | ||
| adjustment_interval: 2016, // 2016 blocks | ||
| target_timespan: 14 * 24 * 60 * 60, // 2 weeks in seconds | ||
| min_difficulty: 1, | ||
| max_difficulty: 0xFFFFFFFF, | ||
| max_adjustment_factor: 4, | ||
| } | ||
| ``` | ||
|
|
||
| #### Methods | ||
|
|
||
| **`new(initial_difficulty: u32, adjustment_interval: u64, target_timespan: u64) -> DifficultyResult<Self>`** | ||
| - Creates a new difficulty configuration with custom values | ||
| - Validates all parameters before creation | ||
| - Returns: Error if any parameter is invalid (e.g., zero values) | ||
|
|
||
| **`validate() -> DifficultyResult<()>`** | ||
| - Validates the configuration | ||
| - Checks: non-zero values, min <= max, valid ranges | ||
| - Returns: `Ok(())` if valid, `Err(...)` with specific validation error | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's duplicate documentation for DifficultyManager and DifficultyConfig. The same content appears at lines 285-367 and again at lines 450-584. This duplication should be removed to keep the documentation maintainable and avoid inconsistencies.
|
|
||
| /// Get the difficulty manager | ||
| pub fn difficulty_manager(&self) -> &Arc<RwLock<DifficultyManager>> { | ||
| &self.difficulty_manager | ||
| } | ||
|
|
||
| /// Get the current difficulty configuration | ||
| pub async fn get_difficulty_config(&self) -> DifficultyConfig { | ||
| let manager = self.difficulty_manager.read().await; | ||
| manager.config().clone() | ||
| } | ||
|
|
||
| /// Calculate new difficulty using the consensus-owned algorithm | ||
| /// | ||
| /// This is the entry point for blockchain difficulty adjustment. | ||
| /// The blockchain calls this method and the consensus engine owns the algorithm. | ||
| pub async fn calculate_difficulty_adjustment( | ||
| &self, | ||
| height: u64, | ||
| current_difficulty: u32, | ||
| interval_start_time: u64, | ||
| interval_end_time: u64, | ||
| ) -> Result<Option<u32>> { | ||
| let manager = self.difficulty_manager.read().await; | ||
| manager | ||
| .adjust_difficulty(height, current_difficulty, interval_start_time, interval_end_time) | ||
| .map_err(|e| anyhow!("Difficulty adjustment failed: {}", e)) | ||
| } | ||
|
|
||
| /// Check if difficulty should be adjusted at the given height | ||
| pub async fn should_adjust_difficulty(&self, height: u64) -> bool { | ||
| let manager = self.difficulty_manager.read().await; | ||
| manager.should_adjust(height) | ||
| } | ||
|
|
||
| /// Get the initial difficulty value from consensus policy | ||
| pub async fn get_initial_difficulty(&self) -> u32 { | ||
| let manager = self.difficulty_manager.read().await; | ||
| manager.initial_difficulty() | ||
| } | ||
|
|
||
| /// Get the difficulty adjustment interval from consensus policy | ||
| pub async fn get_difficulty_adjustment_interval(&self) -> u64 { | ||
| let manager = self.difficulty_manager.read().await; | ||
| manager.adjustment_interval() | ||
| } | ||
|
|
||
| /// Apply DAO governance updates to difficulty parameters | ||
| pub async fn apply_difficulty_governance_update( | ||
| &self, | ||
| initial_difficulty: Option<u32>, | ||
| adjustment_interval: Option<u64>, | ||
| target_timespan: Option<u64>, | ||
| ) -> Result<()> { | ||
| let mut manager = self.difficulty_manager.write().await; | ||
| manager | ||
| .apply_governance_update(initial_difficulty, adjustment_interval, target_timespan) | ||
| .map_err(|e| anyhow!("Failed to apply difficulty governance update: {}", e)) | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new integration between BlockchainConsensusCoordinator and DifficultyManager lacks test coverage. Specifically, the methods calculate_difficulty_adjustment, apply_difficulty_governance_update, and the new constructor new_with_difficulty_config should have integration tests to ensure the delegation from blockchain to consensus works correctly.
lib-consensus/src/difficulty.rs
Outdated
| /// Get a mutable reference to the configuration (for DAO updates) | ||
| pub fn config_mut(&mut self) -> &mut DifficultyConfig { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing config_mut() as a public API could bypass the validation logic in apply_governance_update and the setter methods. Callers could directly mutate the config to invalid states (e.g., setting adjustment_interval to 0). Consider making this method private or removing it in favor of the validated setter methods.
| /// Get a mutable reference to the configuration (for DAO updates) | |
| pub fn config_mut(&mut self) -> &mut DifficultyConfig { | |
| /// Get a mutable reference to the configuration (for internal updates). | |
| /// | |
| /// This is intentionally not public to ensure all external updates go through | |
| /// validated governance or setter methods that enforce configuration invariants. | |
| fn config_mut(&mut self) -> &mut DifficultyConfig { |
- Remove redundant height > 0 condition in should_adjust() - Fix double lock acquisition in blockchain.rs adjust_difficulty() - Add division by zero checks in legacy difficulty calculation - Make config_mut() private to prevent bypassing validation - Add min_timespan safeguard against integer division returning 0 - Document complete governance application flow for difficulty params - Remove duplicate DifficultyManager documentation - Add comprehensive tests for setter methods - Add 4 new integration tests for difficulty manager - Export initialize_consensus_integration_with_difficulty_config() All tests passing. Fixes address security, correctness, and maintainability issues.
…culty-adjustment-ownership-to-lib-consensus Resolved conflict in Cargo.toml by keeping [workspace.metadata.dev-tools]
|


