Merged
Conversation
Contributor
## Walkthrough
This change introduces the concept of a "vm_key" (a block hash at a computed height) into block header validation and orphan block processing. The validator now tracks, caches, and passes the vm_key as context during validation, updating the cache at regular intervals. Related method signatures and trait interfaces are updated to accommodate this additional parameter.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| base_layer/core/src/base_node/sync/header_sync/validator.rs | Added a `vm_key` cache to the validator state, seeded at genesis. Updated validation logic to compute, retrieve, and cache vm_key hashes, passing them to the header validator, and updating the cache at defined intervals. |
| base_layer/core/src/chain_storage/blockchain_database.rs | Added a helper to compute the vm_key for candidate/orphan blocks. Updated orphan block validation logic to fetch and pass the vm_key hash to the validator. |
| base_layer/core/src/validation/header/header_full_validator.rs | Updated the `validate` method and trait implementation to require a vm_key parameter. Removed internal vm_key lookup logic. Enhanced proof-of-work validation to check algorithm validity against consensus rules. |
| base_layer/core/src/validation/traits.rs<br>base_layer/core/src/validation/mocks.rs | Updated the `HeaderChainLinkedValidator` trait and its mock implementation to include the vm_key parameter in the `validate` method signature. |
| base_layer/core/src/chain_storage/target_difficulties.rs | Added methods to update permitted proof-of-work algorithms and count them, ensuring current consensus rules are respected at a given height. |
| base_layer/core/src/blocks/block_header.rs | Added a new error variant for invalid proof-of-work algorithms. |
| base_layer/core/src/validation/difficulty_calculator.rs | Made the `TARI_RX_VM_KEY_BLOCK_SWAP` constant public. |
| base_layer/core/src/validation/helpers.rs | Removed a redundant blank line; no logic changes. |
| base_layer/core/src/validation/test.rs<br>base_layer/core/tests/tests/block_validation.rs | Updated test calls to `validate` methods to include the new `vm_key` parameter, typically passing `FixedHash::zero()`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Validator
participant State
participant DB
participant HeaderValidator
Validator->>State: Retrieve/calculate vm_key for header.height
State->>DB: Try fetch vm_key hash at vm_key_height
alt Not found in DB
State->>State: Search vm_key cache
end
Validator->>HeaderValidator: validate(header, prev_header, ..., vm_key)
HeaderValidator->>HeaderValidator: Check PoW algorithm validity
HeaderValidator->>HeaderValidator: Validate header with vm_key context
alt header.height % TARI_RX_VM_KEY_BLOCK_SWAP == 0
Validator->>State: Update vm_key cache with (height, hash)
endPossibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
base_layer/core/src/chain_storage/blockchain_database.rs (2)
2199-2199: Fix typo in comment.There's a grammatical error in the comment.
- // Now we now the orphan links back to the main chain here + // Now we know the orphan links back to the main chain here
2373-2376: Consider using consistent approach for vm_key calculation.This code directly calculates the vm_key using
fetch_chain_header_by_height, while the other location uses the helper functionget_vm_key_for_candidate_block. This inconsistency could lead to different behavior in edge cases.Consider either:
- Using the helper function here as well for consistency
- Or documenting why this simpler approach is safe in this specific context
- let vm_key = *db - .fetch_chain_header_by_height(tari_rx_vm_key_height(child.header.height))? - .hash(); + let vm_key = get_vm_key_for_candidate_block(db, Arc::new(child.clone()))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
base_layer/core/src/base_node/sync/header_sync/validator.rs(8 hunks)base_layer/core/src/blocks/block_header.rs(1 hunks)base_layer/core/src/chain_storage/blockchain_database.rs(4 hunks)base_layer/core/src/chain_storage/target_difficulties.rs(2 hunks)base_layer/core/src/validation/difficulty_calculator.rs(1 hunks)base_layer/core/src/validation/header/header_full_validator.rs(3 hunks)base_layer/core/src/validation/helpers.rs(0 hunks)base_layer/core/src/validation/mocks.rs(2 hunks)base_layer/core/src/validation/traits.rs(2 hunks)
💤 Files with no reviewable changes (1)
- base_layer/core/src/validation/helpers.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/core/src/validation/header/header_full_validator.rs (1)
base_layer/core/src/consensus/consensus_manager.rs (1)
consensus_constants(110-119)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (18)
base_layer/core/src/validation/difficulty_calculator.rs (1)
31-31: LGTM! Good visibility change for vm_key integration.Making
TARI_RX_VM_KEY_BLOCK_SWAPpublic enables consistent vm_key height calculations across modules while maintaining the same constant value (2048).base_layer/core/src/blocks/block_header.rs (1)
85-86: LGTM! Well-designed error variant for POW algorithm validation.The new
InvalidPowAlgorithm(String)variant follows the established pattern and provides descriptive error messages for enhanced proof-of-work validation.base_layer/core/src/chain_storage/target_difficulties.rs (2)
49-66: LGTM! Solid implementation for dynamic algorithm updates.The
update_algosmethod correctly:
- Retrieves current permitted algorithms from consensus rules
- Removes outdated algorithms
- Adds new algorithms with proper error handling
- Maintains consistency between difficulty tracking and consensus rules
100-102: LGTM! Simple and useful accessor method.The
algo_countmethod provides a clean way to query the number of tracked algorithms.base_layer/core/src/validation/mocks.rs (2)
28-31: LGTM! Proper import update for trait compatibility.Adding
FixedHashto the imports supports the updated trait interface requirements.
120-120: LGTM! Appropriate mock signature update.The unused
FixedHashparameter maintains compatibility with the updatedHeaderChainLinkedValidatortrait while correctly indicating it's not used in the mock implementation.base_layer/core/src/validation/traits.rs (2)
23-26: LGTM: Import addition supports new vm_key functionality.The addition of
FixedHashto the imports is necessary to support the newvm_keyparameter in the trait method.
70-70:Details
✅ Verification successful
Breaking change: Verify all implementations are updated.
The addition of the
vm_key: FixedHashparameter to thevalidatemethod is a breaking change to theHeaderChainLinkedValidatortrait interface. Ensure all implementations of this trait have been updated to match the new signature.
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of HeaderChainLinkedValidator trait to verify they handle the new vm_key parameter # Expected: All implementations should have been updated with the new parameter ast-grep --pattern $'impl $_ HeaderChainLinkedValidator<$_> for $_ { $$$ fn validate($$$) { $$$ } $$$ }'Length of output: 117
🏁 Script executed:
#!/bin/bash # Locate the trait definition rg --type rust -n "trait HeaderChainLinkedValidator" # Find all implementations of the trait rg --type rust -n "impl.*HeaderChainLinkedValidator"Length of output: 736
🏁 Script executed:
#!/bin/bash # Show the trait definition and its validate method signature rg -n 'trait HeaderChainLinkedValidator' -C5 base_layer/core/src/validation/traits.rs rg -n 'fn validate' -C5 base_layer/core/src/validation/traits.rs # Inspect MockValidator implementation rg -n 'impl.*HeaderChainLinkedValidator' -C2 base_layer/core/src/validation/mocks.rs rg -n 'fn validate' -C3 base_layer/core/src/validation/mocks.rs # Inspect HeaderFullValidator implementation rg -n 'impl.*HeaderChainLinkedValidator' -C2 base_layer/core/src/validation/header/header_full_validator.rs rg -n 'fn validate' -C3 base_layer/core/src/validation/header/header_full_validator.rsLength of output: 4374
🏁 Script executed:
#!/bin/bash # Search for vm_key occurrences in the validation directory to verify implementations rg -n 'vm_key' -C3 base_layer/core/src/validationLength of output: 8659
🏁 Script executed:
#!/bin/bash # Display the full validate method signature in MockValidator implementation sed -n '110,140p' base_layer/core/src/validation/mocks.rsLength of output: 1239
HeaderChainLinkedValidator implementations updated
Both
MockValidatorandHeaderFullValidatornow include the newvm_key: FixedHashparameter in theirvalidatesignatures:• base_layer/core/src/validation/mocks.rs
• base_layer/core/src/validation/header/header_full_validator.rsNo further changes required.
base_layer/core/src/chain_storage/blockchain_database.rs (2)
111-111: LGTM: Import addition supports vm_key functionality.The import of
tari_rx_vm_key_heightis necessary for the new vm_key calculation logic.
2272-2279: LGTM: Validator call updated with vm_key parameter.The validator call has been correctly updated to include the new vm_key parameter, maintaining consistency with the updated trait interface.
base_layer/core/src/validation/header/header_full_validator.rs (3)
69-69: Enhanced validation with vm_key context.The addition of the
vm_keyparameter aligns with the architectural changes to track block hashes at computed heights during validation. This provides the necessary context for proper validation flow.
82-82: Improved PoW validation with consensus constants.The enhanced
check_pow_datacall now receives consensus constants, enabling validation of permitted PoW algorithms. This is a critical security improvement.
190-197: Critical security enhancement: PoW algorithm validation.This validation logic correctly ensures that only consensus-permitted PoW algorithms are accepted in block headers. The implementation properly:
- Retrieves allowed algorithms from consensus constants
- Validates the header's PoW algorithm against the allowed set
- Returns appropriate error for invalid algorithms
This addresses a critical security requirement mentioned in the PR objectives.
base_layer/core/src/base_node/sync/header_sync/validator.rs (5)
64-64: VM key cache implementation.The addition of
vm_key: Vec<(u64, FixedHash)>to cache height-hash pairs is appropriate for tracking block hashes at specific heights during validation.
115-115: Proper genesis initialization for vm_key cache.Correctly initializes the vm_key cache with the genesis block hash at height 0, providing a foundation for the caching mechanism.
127-136: Critical sync fix: Update target difficulties at consensus height changes.This logic correctly addresses the sync issue mentioned in the PR objectives by updating target difficulties when consensus constants change at specific heights (swap heights). This ensures proper synchronization behavior across consensus constant transitions.
150-166: Robust vm_key lookup with database and cache fallback.The implementation correctly:
- Attempts to fetch the vm_key from the database first
- Falls back to searching the cached vm_key vector if not found in database
- Provides appropriate error handling if vm_key cannot be located
The linear search through the cache is acceptable given the cache is only updated at
TARI_RX_VM_KEY_BLOCK_SWAPintervals, keeping the cache size manageable.
234-237: Strategic vm_key caching at swap intervals.The caching logic correctly stores block hashes at heights that are multiples of
TARI_RX_VM_KEY_BLOCK_SWAP. This provides efficient access to required vm_keys for future validations while maintaining a reasonable cache size.
Test Results (CI)1 088 tests 1 084 ✅ 18m 39s ⏱️ For more details on these failures, see this check. Results for commit 60121f4. ♻️ This comment has been updated with latest results. |
stringhandler
approved these changes
May 23, 2025
This was referenced Aug 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a validation of allowed algo type
Fixes sync to ensure node correctly updates the consensus constants when going over swap heights
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements