fix: randomX seed management #6910
Conversation
WalkthroughThis pull request enhances the Monero seed management by introducing a new LMDB index for mapping block heights to RandomX seeds. It updates the LMDB database creation, deletion, and insertion operations accordingly. Additionally, a new method is added to convert a fixed byte array into a vector, and the block body validator now validates Monero seed height against consensus rules. Separately, the header validator has been streamlined by removing Monero-specific proof-of-work checks. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)base_layer/core/tests/tests/block_validation.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(8 hunks)base_layer/core/src/proof_of_work/monero_rx/fixed_array.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb.rs (4)
lmdb_get(233-256)lmdb_delete(169-182)lmdb_replace(132-166)lmdb_insert(55-103)
🔇 Additional comments (5)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (5)
166-166: New database index for RandomX seed mapping looks good.Adding this new constant for mapping block heights to RandomX seeds is a good approach for solving the reorg issue mentioned in the PR description.
281-282: Well-documented field addition.The field is properly documented with a clear comment explaining its purpose of mapping block heights to randomx seeds.
343-343: Proper database initialization.The new index database is correctly initialized in the constructor.
562-589: Complete all_dbs update.The method signature and array contents are properly updated to include the new database index, ensuring it's included in all database operations.
963-977: Good implementation of seed cleanup during header deletion.This change is crucial for fixing the issue mentioned in the PR. When a header is deleted (e.g., during a reorg), the code now checks for and removes any associated seed from both the height-to-seed index and the seed-to-height mapping. This prevents seeds from orphaned blocks lingering in the database.
Test Results (CI) 2 files 73 suites 27m 10s ⏱️ For more details on these failures, see this check. Results for commit 0db015e. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)0 tests 0 ✅ 0s ⏱️ Results for commit 3c671e5. |
SWvheerden
force-pushed
the
sw_manage_monero_seeds
branch
from
3c671e5 to
05ed8b7
Compare
SWvheerden
force-pushed
the
sw_manage_monero_seeds
branch
from
dfb91f1 to
0db015e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/validation/block_body/block_body_full_validator.rs (1)
110-122: Consider adding debug logging for RandomX seed validationWhile the validation logic is correct, adding debug logging would help diagnose issues when blocks are rejected due to outdated seeds, especially during the resynchronization process that this PR requires.
fn check_monero_seed_height<B: BlockchainBackend>( header: &BlockHeader, rules: &ConsensusManager, backend: &B, ) -> Result<(), ValidationError> { if header.pow.pow_algo == PowAlgorithm::RandomX { let monero_data = MoneroPowData::from_header(header, rules)?; let seed_height = backend.fetch_monero_seed_first_seen_height(&monero_data.randomx_key)?; + log::debug!( + target: LOG_TARGET, + "Block at height {} using RandomX seed first seen at height {}", + header.height, + seed_height + ); if seed_height != 0 { // Saturating sub: subtraction can underflow in reorgs / rewind-blockchain command let seed_used_height = header.height.saturating_sub(seed_height); + log::debug!( + target: LOG_TARGET, + "Seed age (blocks): {}, maximum allowed: {}", + seed_used_height, + rules.consensus_constants(header.height).max_randomx_seed_height() + ); if seed_used_height > rules.consensus_constants(header.height).max_randomx_seed_height() { + log::warn!( + target: LOG_TARGET, + "Block at height {} rejected: RandomX seed too old (age: {}, max: {})", + header.height, + seed_used_height, + rules.consensus_constants(header.height).max_randomx_seed_height() + ); return Err(ValidationError::BlockHeaderError( BlockHeaderValidationError::OldSeedHash, )); } } } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(11 hunks)base_layer/core/src/validation/block_body/block_body_full_validator.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
🔇 Additional comments (3)
base_layer/core/src/validation/block_body/block_body_full_validator.rs (3)
31-34: Appropriate import additions for RandomX seed validationThe added imports for
BlockHeader,BlockHeaderValidationError, andMoneroPowDataare well-aligned with the new functionality for validating Monero seed heights.
100-101: Good integration of the new Monero seed height validationCalling the new validation method here is appropriately placed after the other validation checks. This ensures that the Monero seed height is verified as part of the block validation process.
105-124:❓ Verification inconclusive
Well-implemented validation for RandomX seed age
The implementation correctly validates that RandomX seeds aren't too old according to consensus rules. The method appropriately:
- Checks if the block uses RandomX algorithm
- Retrieves the seed's first seen height
- Calculates the seed age using saturating subtraction (to handle reorgs safely)
- Validates against the maximum allowed seed age
The saturating subtraction is a good approach to handle potential underflows during reorgs or blockchain rewinds.
Since this is a breaking change that requires resynchronization, we should verify the implications. Run this script to check the current implementation of
fetch_monero_seed_first_seen_height:
🏁 Script executed:
#!/bin/bash # Find implementation of fetch_monero_seed_first_seen_height to understand how it integrates with the new LMDB index rg --type rust "fn fetch_monero_seed_first_seen_height" -A 5 -B 2Length of output: 2410
Verified: RandomX Seed Age Validation and LMDB Integration
The validation logic for ensuring that RandomX seeds aren’t too old is implemented correctly. The function correctly:
- Checks if the block uses the RandomX algorithm.
- Constructs the Monero proof-of-work data from the header.
- Retrieves the seed’s first seen height via
backend.fetch_monero_seed_first_seen_height. The LMDB integration (verified inlmdb_db/lmdb_db.rs) confirms that the function returns0when the seed is unknown.- Uses saturating subtraction to safely calculate the seed age while guarding against underflows in reorgs or chain rewinds.
- Compares the seed age against the maximum allowed age from consensus constants.
Note: Since this change modifies how seed heights are fetched (and thus represents a breaking change that requires resynchronization of the LMDB index), please ensure that the resynchronization procedures and database state updates are thoroughly verified in your deployment.
* development: chore: new release v1.13.3-pre.0 (tari-project#6919) fix: fix migration 5 (tari-project#6915) chore: update change logs (tari-project#6913) chore: new release v1.13.2-pre.0 (tari-project#6912) fix: randomX seed management (tari-project#6910) fix(comms/yamux): dont poll the substream after closing/error (tari-project#6911) fix: dont poll yamux substream after an error (tari-project#6909) feat: remove memory allocation for max_size_vec (tari-project#6903) feat: add display info to yamux (tari-project#6904)
Description
Atm we dont remove old seeds if we reorg the chain.
If we mine a block while syncing it might be that we insert the first seen height of the monero seed way too early and thus cause the node to think the seed is too old when verifying the chain. This PR will remove seeds for blocks that have been reorged out of the chain.
Summary by CodeRabbit
New Features
Refactor