feat(lib-blockchain): Add DifficultyConfig struct for adaptive diffic… #652
Conversation
…ulty adjustment - Add DifficultyConfig struct with governance-controlled parameters - Fields: target_timespan, adjustment_interval, min/max_adjustment_factor, last_updated_at_height - Default values: 14 days, 2016 blocks, 4x max adjustment (Bitcoin-style) - Add difficulty_config field to Blockchain struct - Initialize with defaults in Blockchain::new() - Implement serialization/deserialization for persistence - Add get_difficulty_config() getter method - Add comprehensive unit tests for validation and serialization - Ensure backward compatibility with existing genesis blocks - Document in lib-blockchain/docs/architecture.md Resolves #605
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a DifficultyConfig struct to enable governance-controlled adaptive difficulty adjustment for the blockchain's Proof of Useful Work (PoUW) consensus. The implementation follows Bitcoin-style parameters with configurable timespan, adjustment intervals, and clamping factors that can be modified through DAO proposals.
Key Changes:
- New
DifficultyConfigstruct with validation and helper methods for difficulty adjustment - Integration of
difficulty_configfield into theBlockchainstruct with default initialization - Comprehensive unit tests for configuration validation, serialization, and parameter clamping
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib-blockchain/src/types/difficulty.rs | Adds DifficultyConfig struct with fields for governance-controlled difficulty parameters, validation logic, helper methods, and comprehensive unit tests |
| lib-blockchain/src/types/mod.rs | Updates difficulty module exports to include DifficultyConfig and related functions explicitly |
| lib-blockchain/src/blockchain.rs | Adds difficulty_config field to Blockchain struct, initializes with defaults in constructor, and provides getter method |
| lib-blockchain/src/lib.rs | Re-exports DifficultyConfig for external use (redundant with wildcard export) |
| lib-blockchain/docs/architecture.md | Documents the adaptive difficulty adjustment architecture with configuration details and key methods |
|
|
||
| // Re-export core types for convenience | ||
| pub use types::*; | ||
| pub use types::DifficultyConfig; |
There was a problem hiding this comment.
The re-export on line 29 is redundant because DifficultyConfig is already exported via the wildcard re-export from types on line 28. The types module already exports DifficultyConfig in its mod.rs file. This creates duplicate exports which could lead to ambiguity.
| pub use types::DifficultyConfig; |
| blocks: vec![genesis_block.clone()], | ||
| height: 0, | ||
| difficulty: Difficulty::from_bits(crate::INITIAL_DIFFICULTY), | ||
| difficulty_config: DifficultyConfig::default(), |
There was a problem hiding this comment.
The existing blockchain serialization test (test_blockchain_serialization) does not verify that the difficulty_config field is properly serialized and deserialized. Adding a blockchain field requires updating the test to verify backward compatibility and proper serialization of the new field, especially since this field is critical for governance and consensus operations.
| /// Get the current difficulty configuration | ||
| pub fn get_difficulty_config(&self) -> &DifficultyConfig { | ||
| &self.difficulty_config | ||
| } |
There was a problem hiding this comment.
The getter method should return a reference to avoid unnecessary cloning of the DifficultyConfig. Consider also adding a mutable getter or update method for future governance operations that need to modify the configuration.
| #[test] | ||
| fn test_difficulty_config_validation() { | ||
| // Valid config should pass | ||
| let valid_config = DifficultyConfig::default(); | ||
| assert!(valid_config.validate().is_ok()); | ||
|
|
||
| // Invalid target_timespan | ||
| let mut invalid = DifficultyConfig::default(); | ||
| invalid.target_timespan = 0; | ||
| assert!(invalid.validate().is_err()); | ||
|
|
||
| // Invalid adjustment_interval | ||
| let mut invalid = DifficultyConfig::default(); | ||
| invalid.adjustment_interval = 0; | ||
| assert!(invalid.validate().is_err()); | ||
|
|
||
| // Invalid min_adjustment_factor | ||
| let mut invalid = DifficultyConfig::default(); | ||
| invalid.min_adjustment_factor = 0; | ||
| assert!(invalid.validate().is_err()); | ||
|
|
||
| // Invalid max_adjustment_factor | ||
| let mut invalid = DifficultyConfig::default(); | ||
| invalid.max_adjustment_factor = 0; | ||
| assert!(invalid.validate().is_err()); | ||
| } |
There was a problem hiding this comment.
The validation test only checks for zero values but doesn't test the upper bound validations defined in the validate method. Specifically, it should test cases where target_timespan exceeds 1 year, adjustment_interval exceeds 1,000,000, or adjustment factors exceed 100 to ensure those validation rules work correctly.
| pub fn with_params( | ||
| target_timespan: u64, | ||
| adjustment_interval: u64, | ||
| min_adjustment_factor: u64, | ||
| max_adjustment_factor: u64, | ||
| last_updated_at_height: u64, | ||
| ) -> Self { | ||
| Self { | ||
| target_timespan, | ||
| adjustment_interval, | ||
| min_adjustment_factor, | ||
| max_adjustment_factor, | ||
| last_updated_at_height, | ||
| } | ||
| } |
There was a problem hiding this comment.
The with_params constructor does not validate the parameters before creating the DifficultyConfig instance. This could allow creation of invalid configurations that would later fail validation. Consider either validating parameters in this method or documenting that callers must call validate() after construction.
| /// Calculated as target_timespan / adjustment_interval | ||
| pub fn target_block_time(&self) -> u64 { | ||
| if self.adjustment_interval == 0 { | ||
| return 600; // Default to 10 minutes if misconfigured |
There was a problem hiding this comment.
The target_block_time method returns a hardcoded 600 seconds when adjustment_interval is 0, but this creates an inconsistency - the method should either return an error or handle the zero case in a way that's consistent with validation. Since validate() already prevents adjustment_interval from being 0, this fallback may never be reached in validated configs. Consider documenting this defensive check or returning Result to handle invalid states explicitly.
| /// Calculated as target_timespan / adjustment_interval | |
| pub fn target_block_time(&self) -> u64 { | |
| if self.adjustment_interval == 0 { | |
| return 600; // Default to 10 minutes if misconfigured | |
| /// | |
| /// Calculated as `target_timespan / adjustment_interval`. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `adjustment_interval` is zero. Call [`validate`] on this | |
| /// configuration before using it to ensure parameters are well-formed. | |
| pub fn target_block_time(&self) -> u64 { | |
| if self.adjustment_interval == 0 { | |
| panic!("DifficultyConfig is invalid: adjustment_interval must be greater than 0"); |
| /// Calculate the clamped timespan for difficulty adjustment | ||
| /// | ||
| /// This prevents extreme difficulty changes by clamping the actual timespan | ||
| /// to be within [target_timespan / min_factor, target_timespan * max_factor] | ||
| pub fn clamp_timespan(&self, actual_timespan: u64) -> u64 { | ||
| let max_timespan = self.target_timespan.saturating_mul(self.max_adjustment_factor); | ||
| let min_timespan = self.target_timespan / self.min_adjustment_factor.max(1); | ||
|
|
||
| actual_timespan.clamp(min_timespan, max_timespan) | ||
| } |
There was a problem hiding this comment.
The documentation for clamp_timespan states it clamps to [target_timespan / min_factor, target_timespan * max_factor], but the implementation uses max_adjustment_factor for the upper bound, not min_adjustment_factor. The naming is semantically confusing: min_adjustment_factor controls maximum difficulty decrease (minimum timespan), while max_adjustment_factor controls maximum difficulty increase (maximum timespan). Consider renaming these fields to be clearer, such as max_difficulty_decrease_factor and max_difficulty_increase_factor.
| pub use transaction_type::*; | ||
| pub use hash::*; | ||
| pub use difficulty::*; | ||
| pub use difficulty::{Difficulty, DifficultyConfig, calculate_target, meets_difficulty, target_to_difficulty, max_target, min_target, adjust_difficulty, difficulty_to_work}; |
There was a problem hiding this comment.
The explicit export list on line 32 includes all individual items from the difficulty module. While this provides fine-grained control over exports, it could become a maintenance burden if more items are added to the difficulty module in the future. Consider using a wildcard export with selective re-exports, or ensure this explicit list is kept in sync with module additions.
| pub use difficulty::{Difficulty, DifficultyConfig, calculate_target, meets_difficulty, target_to_difficulty, max_target, min_target, adjust_difficulty, difficulty_to_work}; | |
| pub use difficulty::*; |
…ulty adjustment
Resolves #605