feat!: vm calc height#7082
Conversation
WalkthroughThe changes update the calculation used to determine the block height for fetching a reference block header, replacing the previous inline modulo 2000 logic with a centralized function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Node
participant ChainDB
Client->>Node: Request new block/template or validate block
Node->>ChainDB: Fetch header at tari_rx_vm_key_height(height)
ChainDB-->>Node: Return reference header
Node-->>Client: Respond with new block/template or validation result
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Test Results (CI) 2 files 60 suites 34m 47s ⏱️ For more details on these failures, see this check. Results for commit 92bf305. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/validation/difficulty_calculator.rs (1)
74-80: Consider adding documentation for the function implementationThe function logic looks sound and correctly implements the height calculation with reorg protection. However, the bitwise operation might not be immediately clear to all developers.
+/// Returns the block height to use for RandomX VM key selection. +/// +/// For heights <= 2112, returns 0 for backward compatibility. +/// For heights > 2112, returns height aligned to the nearest lower multiple of 2048, +/// with a 65-block reorg safety offset to protect against chain reorganizations. +/// +/// The bitwise operation `& !(TARI_RX_VM_KEY_BLOCK_SWAP - 1)` efficiently rounds down +/// to the nearest multiple of 2048 by clearing the lower 11 bits. pub fn tari_rx_vm_key_height(height: u64) -> u64 { if height <= TARI_RX_VM_KEY_BLOCK_SWAP + TARI_RX_VM_KEY_REORG_SAFETY_NUMBER { 0 } else { (height - TARI_RX_VM_KEY_REORG_SAFETY_NUMBER - 1) & !(TARI_RX_VM_KEY_BLOCK_SWAP - 1) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/validation/difficulty_calculator.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (4)
base_layer/core/src/validation/difficulty_calculator.rs (4)
31-32: Well-defined constants improve readabilityThese constants nicely parameterize the VM key calculation, making the code more maintainable and self-documenting. The values align with the PR objective of making the height calculation more resilient to blockchain reorganizations.
59-59: Good refactoring to use centralized functionReplacing the inline calculation with a call to the dedicated function centralizes the logic and ensures consistency across the codebase, which is a good practice.
82-124: Comprehensive test suiteExcellent test coverage with various heights, including boundary cases around multiples of 2048 and the reorg safety threshold. The tests clearly verify the expected behavior of the function.
74-80:Details
✅ Verification successful
Verify consensus impact across the codebase
The change from
height - (height % 2000)to this new algorithm could have consensus implications. Let's verify that all references to the old calculation have been updated consistently.
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of the old calculation pattern echo "Searching for remaining instances of the old calculation pattern..." rg "height -\s*\(height %\s*2000\)" --type rust # Search for all places using the new function echo "Searching for all places using the new function..." rg "tari_rx_vm_key_height" --type rustLength of output: 2669
Consensus Calculation Fully Updated
All instances of the old formula
height - (height % 2000)have been replaced with calls totari_rx_vm_key_height. A repository-wide search confirms:• No remaining matches for the old pattern
• All references now invoketari_rx_vm_key_heightin:
difficulty_calculator.rsheader_full_validator.rsinbound_handlers.rsbase_node_grpc_server.rsNo further action required.
2 participants
Description
Changes the vm key header hash height to be more reorg resilient and keep the same height as monero,
Summary by CodeRabbit