feat: unified metadata system with blockchain-like properties for challenge storage validation #4
Conversation
- Add BlockHeader struct with state roots, challenge roots, and signatures - Add ValidatorSignature struct for attestations - Add Block struct with transitions and computed hash - Add StateTransition enum for state changes - Add BlockchainStorage with full CRUD operations - Support for historical state queries - Implement 2f+1 signature verification - Add comprehensive unit tests (23 tests)
Implements MetadataRegistry for centralized challenge storage validation: - ChallengeMetadata: tracks schema version, merkle root, and config per challenge - GlobalMetadata: tracks all challenges and combined state root - Registry methods: register/unregister challenges, update state roots, validate - Comprehensive unit tests (28 tests)
- Add NetworkMigrationStatus for tracking network-wide schema versions - Add NetworkMigrationCoordinator for coordinating migrations across validators - Add ChallengeMigration and ChallengeMigrationStatus for per-challenge migrations - Add compute_migration_state_hash for migration verification - Add ChallengeMigrationHandler trait for challenge-specific migrations - Add comprehensive unit tests for all new functionality
📝 WalkthroughWalkthroughAdds a new state root consensus module, a persistent blockchain storage layer, a metadata registry, and migration coordination; exposes related public types and re-exports, and adds a small dependency. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Validator as Validator Node
participant Consensus as StateRootConsensus
participant Linker as GlobalStateLinker
participant Storage as BlockchainStorage
end
Validator->>Consensus: propose_state_root(block_num, global_root, challenge_roots)
Consensus->>Consensus: create & sign StateRootProposal
loop other validators
Validator->>Consensus: receive_proposal(proposal)
Consensus->>Consensus: validate proposal
end
Validator->>Consensus: vote_on_proposal(proposal, local_root)
Consensus->>Linker: verify_inclusion(challenge_root)
Linker-->>Consensus: inclusion result
Consensus->>Consensus: create & sign StateRootVote
loop receive votes
Validator->>Consensus: receive_vote(vote)
Consensus->>Consensus: validate & tally
alt conflict detected
Consensus->>Consensus: create FraudProof
else quorum reached (2f+1)
Consensus->>Consensus: produce ConsensusResult
Consensus->>Storage: append_block(state transition with agreed_root)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@crates/distributed-storage/src/state_consensus.rs`:
- Around line 397-403: compute_global_root currently returns a computed root but
never stores it in cached_global_root, so the cache is always empty; update
compute_global_root to populate cached_global_root after computing the root (use
interior mutability like RefCell/Mutex/RwLock around cached_global_root or
change the method signature to &mut self) and then return the cached value;
specifically, in compute_global_root call
compute_global_root_from_challenges(&self.challenge_roots), store that result
into cached_global_root (e.g., via cached_global_root.borrow_mut() or
equivalent) and then return the stored root so subsequent calls use the cache.
- Around line 716-750: The merkle verification is inverted: verify_fraud_proof
currently walks the merkle_path starting at claimed_root which doesn't prove the
actual state; change the logic so the merkle_path is applied to the disputed
leaf (e.g., add/use a field like FraudProof::leaf or FraudProof::actual_leaf)
and compute the root by folding siblings (using the same hash_pair order) and
assert that the computed root == proof.actual_root and != proof.claimed_root;
update verify_fraud_proof to use the leaf (not claimed_root) for the traversal
and, if the FraudProof struct lacks a leaf, add it and ensure callers populate
it.
In `@crates/storage/src/blockchain.rs`:
- Around line 607-625: The quorum calculation in has_quorum is wrong for some n
(e.g., n=3); replace the current arithmetic with the standard BFT formula:
compute f = floor((total_validators - 1) / 3) and set required_signatures =
total_validators - f, keeping the early return for total_validators == 0 and
preserving behavior for small n; update the comment near required_signatures to
reflect "f = floor((n-1)/3), required = n - f" and leave the final check using
block.header.validator_signatures.len() unchanged.
- Around line 544-595: The verify_block function currently only checks signature
count and duplicates; update it to cryptographically verify each signature:
iterate over block.header.validator_signatures, for each signature call the
available verification helpers (e.g. SignedMessage::verify() or
validator.verify_signature()) passing the block hash (or
block.header.block_hash) and the validator's public hotkey (sig.validator) to
ensure the signature is valid; treat signatures that fail verification as
invalid (log with block_number and validator) and do not count them toward
signature_count, then enforce the required threshold (replace the current
signature_count == 0 check with one that compares the number of valid signatures
against the BFT threshold or at least 1 proposer signature) and return Ok(false)
on insufficient valid signatures.
In `@crates/storage/src/metadata.rs`:
- Around line 76-78: The current config() method silently returns
serde_json::Value::Null on parse failures which can hide corrupted config_json;
change config(&self) to return Result<serde_json::Value, serde_json::Error> (or
Option with logging) so callers must handle parse errors, parse config_json with
serde_json::from_str(&self.config_json) and propagate the error instead of using
unwrap_or; update call sites that use config() accordingly (and consider
referencing set_config to ensure invariants or add a warning log there if you
prefer non-fallible behavior).
In `@crates/storage/src/migration.rs`:
- Around line 598-624: The cached_status field is never populated on reads;
change NetworkMigrationCoordinator::cached_status to an interior-mutable
container (e.g., std::cell::RefCell<Option<NetworkMigrationStatus>>), initialize
it to RefCell::new(None) in NetworkMigrationCoordinator::new, then update
get_network_status to check the RefCell first and if empty read from
network_tree and store the deserialized NetworkMigrationStatus into the RefCell
(use borrow and borrow_mut). Also adjust other mutating methods you already have
(report_validator_version, start_network_migration, etc.) to write into the same
RefCell (borrow_mut) so the cache stays consistent. Ensure you add the RefCell
import and handle borrow errors appropriately.
- Around line 865-883: The function compute_migration_state_hash currently
swallows errors from ctx.scan_prefix causing silent incomplete hashes; change
its signature to return Result<[u8; 32], E> (choose the appropriate error type,
e.g., anyhow::Error or the crate's error type), propagate the scan_prefix error
instead of ignoring it (use the ? operator on ctx.scan_prefix(...) or map_err to
convert), and ensure callers of compute_migration_state_hash are updated to
handle the Result (or adjust to log and return an error). Update references to
compute_migration_state_hash and any callers to handle the new Result return so
scan failures are no longer masked.
🧹 Nitpick comments (6)
crates/storage/src/metadata.rs (2)
1-1: Consider removing blanket allow attributes before merging.The
#![allow(dead_code, unused_variables, unused_imports)]suppresses useful compiler warnings. While acceptable during development, these should be removed or narrowed before release to catch actual unused code.
501-512:persist_globaldoes not callflush.While
flush()is exposed as a public method,persist_globalonly inserts data without flushing. This is fine for batching writes, but callers must remember to callflush()for durability guarantees. The current pattern of callingpersist_globalafter every mutation relies on sled's background flushing, which may not be immediate.crates/storage/src/blockchain.rs (1)
1-1: Consider removing blanket allow attributes before merging.Similar to metadata.rs, the
#![allow(dead_code, unused_variables, unused_imports)]should be removed before release.crates/storage/src/lib.rs (1)
52-65: Inconsistent re-export pattern betweenblockchainandmetadatamodules.
metadatais re-exported withpub use metadata::*;whileblockchainis only declared aspub mod blockchainwithout a glob re-export. This means users must writeplatform_storage::blockchain::BlockchainStoragebut can writeplatform_storage::MetadataRegistrydirectly.Consider whether this asymmetry is intentional (blockchain types accessed via submodule) or if
blockchainshould also have a glob re-export for consistency.crates/distributed-storage/src/state_consensus.rs (2)
71-71: Consider removing blanket allow attributes before merging.Same recommendation as other new files.
602-646:receive_voteproperly detects equivocation but doesn't persist fraud evidence.When a validator sends conflicting votes (lines 619-633), the error is returned but the evidence isn't stored in
fraud_proofs. Consider capturing this as a formalFraudProoffor later slashing.Suggested enhancement
if let Some(existing) = self.votes.get(&vote.voter) { if existing.state_root != vote.state_root { - // This is potential fraud - voter sending different roots warn!( voter = vote.voter.to_hex(), first_root = hex::encode(existing.state_root), second_root = hex::encode(vote.state_root), "Detected conflicting votes from same validator" ); + // Create and store fraud proof for equivocation + let fraud_proof = FraudProof { + accuser: self.local_hotkey.clone(), + accused: vote.voter.clone(), + block_number: vote.block_number, + claimed_root: existing.state_root, + actual_root: vote.state_root, + merkle_proof: None, + timestamp: chrono::Utc::now().timestamp_millis(), + signature: Vec::new(), + }; + self.fraud_proofs.push(fraud_proof); return Err(StateRootConsensusError::FraudDetected(format!( "Validator {} sent conflicting votes", vote.voter.to_hex() ))); } }
| pub fn compute_global_root(&self) -> [u8; 32] { | ||
| if let Some(cached) = self.cached_global_root { | ||
| return cached; | ||
| } | ||
|
|
||
| compute_global_root_from_challenges(&self.challenge_roots) | ||
| } |
There was a problem hiding this comment.
compute_global_root doesn't update the cache.
The method returns the computed root but never stores it in cached_global_root. The cache is only invalidated on mutations but never populated, making it always None and ineffective.
Fix requires interior mutability
Since compute_global_root takes &self, updating the cache requires interior mutability:
use std::cell::RefCell;
pub struct GlobalStateLinker {
challenge_roots: HashMap<String, [u8; 32]>,
cached_global_root: RefCell<Option<[u8; 32]>>,
}
pub fn compute_global_root(&self) -> [u8; 32] {
if let Some(cached) = *self.cached_global_root.borrow() {
return cached;
}
let root = compute_global_root_from_challenges(&self.challenge_roots);
*self.cached_global_root.borrow_mut() = Some(root);
root
}Alternatively, remove the cache field if the performance benefit isn't needed.
🤖 Prompt for AI Agents
In `@crates/distributed-storage/src/state_consensus.rs` around lines 397 - 403,
compute_global_root currently returns a computed root but never stores it in
cached_global_root, so the cache is always empty; update compute_global_root to
populate cached_global_root after computing the root (use interior mutability
like RefCell/Mutex/RwLock around cached_global_root or change the method
signature to &mut self) and then return the cached value; specifically, in
compute_global_root call
compute_global_root_from_challenges(&self.challenge_roots), store that result
into cached_global_root (e.g., via cached_global_root.borrow_mut() or
equivalent) and then return the stored root so subsequent calls use the cache.
| /// Verify a fraud proof. | ||
| pub fn verify_fraud_proof(&self, proof: &FraudProof) -> bool { | ||
| // Basic validation: roots must actually differ | ||
| if !proof.roots_differ() { | ||
| debug!("Fraud proof invalid: roots are identical"); | ||
| return false; | ||
| } | ||
|
|
||
| // If merkle proof is provided, verify it | ||
| if let Some(ref merkle_path) = proof.merkle_proof { | ||
| // Verify the merkle path leads to actual_root | ||
| let mut current = proof.claimed_root; | ||
| for sibling in merkle_path { | ||
| current = if current <= *sibling { | ||
| hash_pair(¤t, sibling) | ||
| } else { | ||
| hash_pair(sibling, ¤t) | ||
| }; | ||
| } | ||
|
|
||
| // The merkle path should NOT lead to actual_root if fraud is genuine | ||
| // (the accused claimed a wrong root) | ||
| if current == proof.actual_root { | ||
| debug!("Fraud proof invalid: merkle path verifies to actual root"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| debug!( | ||
| accused = proof.accused.to_hex(), | ||
| "Fraud proof verified" | ||
| ); | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
Fraud proof verification logic is inverted.
In verify_fraud_proof, lines 738-740 check if the merkle path leads to actual_root and return false if it does. The comment says "The merkle path should NOT lead to actual_root if fraud is genuine."
However, this logic seems backwards. If the accused claimed a wrong root, we'd want to prove that the actual data hashes to actual_root (not claimed_root). The current logic verifies the path starting from claimed_root, which doesn't prove anything about actual_root.
Analysis
The merkle proof should demonstrate:
- What the accused claimed (
claimed_root) - What the data actually hashes to (
actual_root) - Evidence connecting the actual data to
actual_root
The current verification starts from claimed_root and walks up, but this doesn't prove the actual state. Consider revising the fraud proof structure and verification to properly demonstrate the discrepancy.
🤖 Prompt for AI Agents
In `@crates/distributed-storage/src/state_consensus.rs` around lines 716 - 750,
The merkle verification is inverted: verify_fraud_proof currently walks the
merkle_path starting at claimed_root which doesn't prove the actual state;
change the logic so the merkle_path is applied to the disputed leaf (e.g.,
add/use a field like FraudProof::leaf or FraudProof::actual_leaf) and compute
the root by folding siblings (using the same hash_pair order) and assert that
the computed root == proof.actual_root and != proof.claimed_root; update
verify_fraud_proof to use the leaf (not claimed_root) for the traversal and, if
the FraudProof struct lacks a leaf, add it and ensure callers populate it.
| pub fn verify_block(&self, block: &Block) -> Result<bool> { | ||
| // First verify the hash is correct | ||
| if !block.verify_hash() { | ||
| warn!( | ||
| block_number = block.header.block_number, | ||
| "Block hash verification failed" | ||
| ); | ||
| return Ok(false); | ||
| } | ||
|
|
||
| // Genesis block doesn't require signatures | ||
| if block.is_genesis() { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| let signature_count = block.header.validator_signatures.len(); | ||
|
|
||
| // Check for duplicate validators in signatures | ||
| let mut seen_validators = std::collections::HashSet::new(); | ||
| for sig in &block.header.validator_signatures { | ||
| if !seen_validators.insert(&sig.validator) { | ||
| warn!( | ||
| block_number = block.header.block_number, | ||
| validator = %sig.validator.to_hex(), | ||
| "Duplicate validator signature detected" | ||
| ); | ||
| return Ok(false); | ||
| } | ||
| } | ||
|
|
||
| // For Byzantine fault tolerance with n validators, we need at least 2f+1 signatures | ||
| // where f = floor((n-1)/3) is the max faulty validators | ||
| // This means we need at least ceiling(2n/3) signatures | ||
| // | ||
| // For a practical minimum, we require at least 1 signature (the proposer) | ||
| // In production, this threshold should be calculated from the active validator set | ||
| if signature_count == 0 { | ||
| warn!( | ||
| block_number = block.header.block_number, | ||
| "Block has no validator signatures" | ||
| ); | ||
| return Ok(false); | ||
| } | ||
|
|
||
| debug!( | ||
| block_number = block.header.block_number, | ||
| signature_count, | ||
| "Block signature verification passed" | ||
| ); | ||
|
|
||
| Ok(true) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's signature verification infrastructure elsewhere in the codebase
rg -n "verify.*signature|ed25519|sign|crypto" --type rust -g '!target/*' -A 3Repository: PlatformNetwork/platform-v2
Length of output: 50384
🏁 Script executed:
cat -n crates/storage/src/blockchain.rs | sed -n '540,600p'Repository: PlatformNetwork/platform-v2
Length of output: 2660
🏁 Script executed:
rg -n "pub struct Block|pub struct.*Signature|fn verify_hash" --type rust crates/storage/src/blockchain.rs -A 5Repository: PlatformNetwork/platform-v2
Length of output: 1296
Signature verification is incomplete - only checks count and uniqueness, not cryptographic validity.
The verify_block method checks for duplicate validators and requires at least one signature, but does not verify that signatures are cryptographically valid. The comment on lines 541-543 acknowledges this limitation. Cryptographic verification infrastructure exists in the codebase (SignedMessage::verify() and validator.verify_signature()), but verify_block() does not use it.
Each validator signature should be cryptographically verified against:
- The validator's public key (hotkey)
- The block hash being signed
This is critical for Byzantine fault tolerance guarantees and must be implemented before production use.
🤖 Prompt for AI Agents
In `@crates/storage/src/blockchain.rs` around lines 544 - 595, The verify_block
function currently only checks signature count and duplicates; update it to
cryptographically verify each signature: iterate over
block.header.validator_signatures, for each signature call the available
verification helpers (e.g. SignedMessage::verify() or
validator.verify_signature()) passing the block hash (or
block.header.block_hash) and the validator's public hotkey (sig.validator) to
ensure the signature is valid; treat signatures that fail verification as
invalid (log with block_number and validator) and do not count them toward
signature_count, then enforce the required threshold (replace the current
signature_count == 0 check with one that compares the number of valid signatures
against the BFT threshold or at least 1 proposer signature) and return Ok(false)
on insufficient valid signatures.
| pub fn has_quorum(&self, block: &Block, total_validators: usize) -> bool { | ||
| if total_validators == 0 { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate required signatures for 2f+1 (Byzantine majority) | ||
| // n = total_validators, f = floor((n-1)/3) | ||
| // Required = n - f = n - floor((n-1)/3) | ||
| // Simplified: ceiling(2n/3) + 1 for n > 1, or n for n <= 1 | ||
| let required_signatures = if total_validators <= 1 { | ||
| total_validators | ||
| } else { | ||
| // ceiling((2 * n + 2) / 3) | ||
| (2 * total_validators + 2) / 3 | ||
| }; | ||
|
|
||
| let signature_count = block.header.validator_signatures.len(); | ||
| signature_count >= required_signatures | ||
| } |
There was a problem hiding this comment.
Quorum calculation formula may be off by one in edge cases.
The comment states ceiling(2n/3) + 1 but the formula (2 * total_validators + 2) / 3 computes floor((2n+2)/3). For n=3: expected 2f+1=3 (f=0), formula gives (6+2)/3=2. For n=4: expected 2f+1=3 (f=1), formula gives (8+2)/3=3.
Let me verify: For BFT with n validators, f = floor((n-1)/3), required = n - f.
- n=3: f=0, required=3. Formula: 8/3=2 ❌
- n=4: f=1, required=3. Formula: 10/3=3 ✓
The formula appears to under-count for n=3.
Suggested fix
pub fn has_quorum(&self, block: &Block, total_validators: usize) -> bool {
if total_validators == 0 {
return false;
}
- // Calculate required signatures for 2f+1 (Byzantine majority)
- // n = total_validators, f = floor((n-1)/3)
- // Required = n - f = n - floor((n-1)/3)
- // Simplified: ceiling(2n/3) + 1 for n > 1, or n for n <= 1
- let required_signatures = if total_validators <= 1 {
- total_validators
- } else {
- // ceiling((2 * n + 2) / 3)
- (2 * total_validators + 2) / 3
- };
+ // For BFT: n validators can tolerate f = floor((n-1)/3) failures
+ // Required signatures = 2f + 1 = 2*floor((n-1)/3) + 1
+ // Or equivalently: n - f = n - floor((n-1)/3)
+ let f = (total_validators - 1) / 3;
+ let required_signatures = total_validators - f;
let signature_count = block.header.validator_signatures.len();
signature_count >= required_signatures
}🤖 Prompt for AI Agents
In `@crates/storage/src/blockchain.rs` around lines 607 - 625, The quorum
calculation in has_quorum is wrong for some n (e.g., n=3); replace the current
arithmetic with the standard BFT formula: compute f = floor((total_validators -
1) / 3) and set required_signatures = total_validators - f, keeping the early
return for total_validators == 0 and preserving behavior for small n; update the
comment near required_signatures to reflect "f = floor((n-1)/3), required = n -
f" and leave the final check using block.header.validator_signatures.len()
unchanged.
| pub fn config(&self) -> serde_json::Value { | ||
| serde_json::from_str(&self.config_json).unwrap_or(serde_json::Value::Null) | ||
| } |
There was a problem hiding this comment.
Silent fallback to Null on JSON parse failure.
If config_json contains invalid JSON (which shouldn't happen if set_config is always used), this silently returns Null rather than surfacing the error. This could mask data corruption issues.
Suggested improvement
Consider logging a warning when deserialization fails, or returning a Result:
pub fn config(&self) -> serde_json::Value {
- serde_json::from_str(&self.config_json).unwrap_or(serde_json::Value::Null)
+ serde_json::from_str(&self.config_json).unwrap_or_else(|e| {
+ tracing::warn!("Failed to parse config_json: {}", e);
+ serde_json::Value::Null
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn config(&self) -> serde_json::Value { | |
| serde_json::from_str(&self.config_json).unwrap_or(serde_json::Value::Null) | |
| } | |
| pub fn config(&self) -> serde_json::Value { | |
| serde_json::from_str(&self.config_json).unwrap_or_else(|e| { | |
| tracing::warn!("Failed to parse config_json: {}", e); | |
| serde_json::Value::Null | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@crates/storage/src/metadata.rs` around lines 76 - 78, The current config()
method silently returns serde_json::Value::Null on parse failures which can hide
corrupted config_json; change config(&self) to return Result<serde_json::Value,
serde_json::Error> (or Option with logging) so callers must handle parse errors,
parse config_json with serde_json::from_str(&self.config_json) and propagate the
error instead of using unwrap_or; update call sites that use config()
accordingly (and consider referencing set_config to ensure invariants or add a
warning log there if you prefer non-fallible behavior).
| pub struct NetworkMigrationCoordinator { | ||
| /// Tree for storing network migration state | ||
| network_tree: Tree, | ||
| /// Cached network status | ||
| cached_status: Option<NetworkMigrationStatus>, | ||
| } | ||
|
|
||
| impl NetworkMigrationCoordinator { | ||
| /// Create a new network migration coordinator | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `db` - The sled database to use for persistence | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A new `NetworkMigrationCoordinator` instance | ||
| pub fn new(db: &sled::Db) -> Result<Self> { | ||
| let network_tree = db.open_tree("network_migrations").map_err(|e| { | ||
| MiniChainError::Storage(format!("Failed to open network_migrations tree: {}", e)) | ||
| })?; | ||
|
|
||
| Ok(Self { | ||
| network_tree, | ||
| cached_status: None, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Cached status is never populated on read path.
The cached_status field is only updated during write operations (report_validator_version, start_network_migration, etc.) but get_network_status() always reads from the database and never populates the cache. This makes the cache ineffective and the field essentially dead code.
Proposed fix to populate cache on read
The cache field would need interior mutability (e.g., RefCell or making get_network_status take &mut self) to be useful. Consider either:
- Removing the cache field if it's not needed
- Using
RefCell<Option<NetworkMigrationStatus>>for interior mutability - Making
get_network_statustake&mut self
🤖 Prompt for AI Agents
In `@crates/storage/src/migration.rs` around lines 598 - 624, The cached_status
field is never populated on reads; change
NetworkMigrationCoordinator::cached_status to an interior-mutable container
(e.g., std::cell::RefCell<Option<NetworkMigrationStatus>>), initialize it to
RefCell::new(None) in NetworkMigrationCoordinator::new, then update
get_network_status to check the RefCell first and if empty read from
network_tree and store the deserialized NetworkMigrationStatus into the RefCell
(use borrow and borrow_mut). Also adjust other mutating methods you already have
(report_validator_version, start_network_migration, etc.) to write into the same
RefCell (borrow_mut) so the cache stays consistent. Ensure you add the RefCell
import and handle borrow errors appropriately.
crates/storage/src/migration.rs
Outdated
| pub fn compute_migration_state_hash(ctx: &MigrationContext, challenge_id: &ChallengeId) -> [u8; 32] { | ||
| let mut hasher = Sha256::new(); | ||
|
|
||
| // Hash the challenge ID | ||
| hasher.update(challenge_id.0.as_bytes()); | ||
|
|
||
| // Scan and hash all keys in the challenge namespace | ||
| let namespace = challenge_id.0.to_string(); | ||
| if let Ok(entries) = ctx.scan_prefix(&namespace) { | ||
| for (key, value) in entries { | ||
| hasher.update(key.to_bytes()); | ||
| if let Ok(data) = bincode::serialize(&value) { | ||
| hasher.update(&data); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| hasher.finalize().into() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate StorageKey implementation and understand namespace formatting
fd -e rs -p "storage" | head -20Repository: PlatformNetwork/platform-v2
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Check the migration.rs file to see the context around the flagged function
cat -n crates/storage/src/migration.rs | sed -n '860,890p'Repository: PlatformNetwork/platform-v2
Length of output: 1301
🏁 Script executed:
#!/bin/bash
# Search for StorageKey implementation and challenge-related methods
rg -n "impl.*StorageKey|fn.*challenge|namespace_prefix" crates/storage/src/ --type rust -A 3Repository: PlatformNetwork/platform-v2
Length of output: 19874
🏁 Script executed:
#!/bin/bash
# Look for scan_prefix implementation and usage patterns
rg -n "fn scan_prefix|scan_prefix\(" crates/storage/src/ --type rust -B 2 -A 5Repository: PlatformNetwork/platform-v2
Length of output: 9630
🏁 Script executed:
#!/bin/bash
# Look for tests of compute_migration_state_hash
rg -n "compute_migration_state_hash" crates/storage/src/migration.rs -A 10 -B 2Repository: PlatformNetwork/platform-v2
Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check other hash computation functions for comparison
rg -n "hasher.finalize" crates/storage/src/ --type rust -B 10 -A 3Repository: PlatformNetwork/platform-v2
Length of output: 7893
Error handling masks scan failures silently.
The compute_migration_state_hash function silently ignores scan_prefix errors (line 873). If the scan fails, the hash is computed from only the challenge ID, making it impossible for callers to detect that the state snapshot is incomplete. This differs from similar code like DistributedStorage::merkle_root() which properly returns Result and propagates errors. Either return Result<[u8; 32], _> to signal failures, or log the error condition.
🤖 Prompt for AI Agents
In `@crates/storage/src/migration.rs` around lines 865 - 883, The function
compute_migration_state_hash currently swallows errors from ctx.scan_prefix
causing silent incomplete hashes; change its signature to return Result<[u8;
32], E> (choose the appropriate error type, e.g., anyhow::Error or the crate's
error type), propagate the scan_prefix error instead of ignoring it (use the ?
operator on ctx.scan_prefix(...) or map_err to convert), and ensure callers of
compute_migration_state_hash are updated to handle the Result (or adjust to log
and return an error). Update references to compute_migration_state_hash and any
callers to handle the new Result return so scan failures are no longer masked.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/distributed-storage/src/state_consensus.rs`:
- Around line 533-646: The code currently stores proposals and votes without
checking their cryptographic signatures; update receive_proposal and
receive_vote to verify signatures with the sender hotkey before accepting: call
the proposer public key (proposal.proposer) to verify the proposal signature and
the voter public key (vote.voter) to verify the vote.signature, and if
verification fails return StateRootConsensusError::InvalidSignature and do not
insert into self.current_proposal or self.votes; keep existing structural checks
(e.g., verify_global_root and block_number checks) but move any storage
(self.current_proposal = Some(...), self.votes.insert(...)) to after successful
signature verification so only signed, valid messages are recorded.
- Around line 614-646: receive_vote currently accepts votes that set
agrees_with_proposal=true even when vote.state_root doesn't match
proposal.state_root; add a check in receive_vote after the block_number check
(and before inserting into self.votes) that if vote.agrees_with_proposal &&
vote.state_root != proposal.state_root then log a warning (include voter and
both roots) and return an error (e.g., StateRootConsensusError::FraudDetected or
a suitable InvalidVote error) so “agree” votes with mismatched roots are
rejected rather than counted.
In `@crates/storage/src/blockchain.rs`:
- Around line 340-366: compute_block_hash currently omits state_transitions and
the Block::new constructor does not validate that state_root matches those
transitions, allowing undetected mutation; update the design by either (A)
adding a state_transitions_digest (e.g., state_transitions_root/digest) to
BlockHeader and include that digest in compute_block_hash (and ensure whatever
code creates headers computes and sets this digest from the provided
state_transitions), or (B) validate inside Block::new that the provided
state_root is derived from the given state_transitions by computing the same
digest and rejecting/returning an error if it differs; also update the
compute_block_hash doc comment to reflect that hashing is done field-by-field
(or that it includes the state_transitions digest) rather than
“bincode-serialized header.” Reference symbols: compute_block_hash, BlockHeader,
Block::new, state_transitions, state_root.
- Around line 751-754: The helper create_test_db currently drops the TempDir
before returning the sled::Db which deletes the directory while sled is still
using it; change create_test_db to return the TempDir along with the sled::Db
(or create a small helper struct holding both) so the TempDir lives for the test
lifetime, e.g., adjust the function signature and callers to receive (TempDir,
sled::Db) or the helper struct to ensure the temp directory is not deleted while
sled::Db is in use.
- Around line 439-525: append_block currently writes blocks_tree,
hash_index_tree, and metadata_tree separately which can leave state
inconsistent; serialize the block first, then perform the three inserts inside a
single sled transaction using the Transactional API (wrap blocks_tree.insert,
hash_index_tree.insert, and metadata_tree.insert(KEY_LATEST_BLOCK, ...) in one
transaction closure), map any sled::transaction::TransactionError into
MiniChainError::Storage, and only log and return Ok(()) after the transaction
commits; use the existing symbols append_block, blocks_tree, hash_index_tree,
metadata_tree, and KEY_LATEST_BLOCK to locate and change the code.
🧹 Nitpick comments (4)
crates/storage/src/migration.rs (3)
533-545: Consider usingBTreeMapfor deterministic serialization.
HashMap<Hotkey, MigrationVersion>has non-deterministic iteration order, which could produce different serialized bytes on different validators even for identical data. IfNetworkMigrationStatusis ever used in consensus hashing or cross-validator verification, this would cause spurious mismatches.Proposed fix
- /// Validators that have reported their version (hotkey -> version) - pub validator_versions: HashMap<Hotkey, MigrationVersion>, + /// Validators that have reported their version (hotkey -> version) + pub validator_versions: BTreeMap<Hotkey, MigrationVersion>,Also update the
Defaultimpl at line 551 and the import at line 30 to useBTreeMap.
773-792: Silent fallback may mask inconsistent state.Line 780 uses
unwrap_or(status.network_version)whentarget_versionisNoneduring an in-progress migration. Whilestart_network_migrationshould always set bothmigration_in_progressandtarget_versiontogether, this silent fallback could mask data corruption or bugs where the invariant is violated.Consider logging a warning or returning an error when this inconsistent state is detected.
Proposed fix
pub fn complete_migration(&mut self, validator: &Hotkey) -> Result<()> { let mut status = self.get_network_status()?; if !status.migration_in_progress { return Ok(()); // No migration in progress } - let target = status.target_version.unwrap_or(status.network_version); + let target = match status.target_version { + Some(v) => v, + None => { + warn!("Migration in progress but target_version is None - inconsistent state"); + status.network_version + } + }; status.validator_versions.insert(validator.clone(), target);
1533-1552: Consider adding test coverage for validator acceptance during active migration.The test covers the normal case where
migration_in_progressis false, but doesn't test lines 716-720 where validators are accepted based ontarget_versionduring an active migration. Consider adding a test case that starts a migration and then verifies validator acceptance logic against the target version.Suggested test case
#[test] fn test_can_accept_validator_during_migration() { let dir = tempdir().unwrap(); let db = sled::open(dir.path()).unwrap(); let mut coordinator = NetworkMigrationCoordinator::new(&db).unwrap(); let validator = Hotkey([1u8; 32]); // Start migration to version 5 coordinator.start_network_migration(5).unwrap(); // During migration, only accept validators at or above target version assert!(!coordinator.can_accept_validator(&validator, 4)); assert!(coordinator.can_accept_validator(&validator, 5)); assert!(coordinator.can_accept_validator(&validator, 6)); }crates/storage/src/blockchain.rs (1)
1-1: Consider narrowing the crate-levelallowlint suppression.
#![allow(dead_code, unused_variables, unused_imports)]can hide real regressions across the whole module. Prefer scoping#[allow]to specific items or gating with#[cfg(test)]once unused items are removed.
| /// Receive and process an incoming proposal. | ||
| /// | ||
| /// Validates the proposal structure and stores it for voting. | ||
| pub fn receive_proposal( | ||
| &mut self, | ||
| proposal: StateRootProposal, | ||
| ) -> Result<(), StateRootConsensusError> { | ||
| // Verify the proposal's internal consistency | ||
| if !proposal.verify_global_root() { | ||
| return Err(StateRootConsensusError::ConflictingRoots { | ||
| expected: hex::encode(compute_global_root_from_challenges( | ||
| &proposal.challenge_roots, | ||
| )), | ||
| got: hex::encode(proposal.global_state_root), | ||
| }); | ||
| } | ||
|
|
||
| debug!( | ||
| block_number = proposal.block_number, | ||
| proposer = proposal.proposer.to_hex(), | ||
| "Received state root proposal" | ||
| ); | ||
|
|
||
| // Clear any previous proposal and votes | ||
| self.current_proposal = Some(proposal); | ||
| self.votes.clear(); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Vote on the current proposal. | ||
| /// | ||
| /// Compares the proposal against the locally computed state root. | ||
| pub fn vote_on_proposal( | ||
| &mut self, | ||
| proposal: &StateRootProposal, | ||
| local_root: [u8; 32], | ||
| ) -> StateRootVote { | ||
| let agrees = local_root == proposal.global_state_root; | ||
| let timestamp = chrono::Utc::now().timestamp_millis(); | ||
|
|
||
| let vote = StateRootVote { | ||
| block_number: proposal.block_number, | ||
| voter: self.local_hotkey.clone(), | ||
| state_root: local_root, | ||
| agrees_with_proposal: agrees, | ||
| timestamp, | ||
| signature: Vec::new(), // Signature would be added by caller with keypair | ||
| }; | ||
|
|
||
| if !agrees { | ||
| warn!( | ||
| block_number = proposal.block_number, | ||
| expected = hex::encode(proposal.global_state_root), | ||
| local = hex::encode(local_root), | ||
| "Local state differs from proposal" | ||
| ); | ||
| } else { | ||
| debug!( | ||
| block_number = proposal.block_number, | ||
| "Voting in agreement with proposal" | ||
| ); | ||
| } | ||
|
|
||
| // Record our own vote | ||
| self.votes.insert(self.local_hotkey.clone(), vote.clone()); | ||
|
|
||
| vote | ||
| } | ||
|
|
||
| /// Receive and process an incoming vote. | ||
| /// | ||
| /// Returns `Some(ConsensusResult)` if consensus is reached with this vote. | ||
| pub fn receive_vote( | ||
| &mut self, | ||
| vote: StateRootVote, | ||
| ) -> Result<Option<ConsensusResult>, StateRootConsensusError> { | ||
| let proposal = self.current_proposal.as_ref().ok_or_else(|| { | ||
| StateRootConsensusError::InternalError("No active proposal".to_string()) | ||
| })?; | ||
|
|
||
| // Verify vote is for current proposal | ||
| if vote.block_number != proposal.block_number { | ||
| return Err(StateRootConsensusError::InternalError(format!( | ||
| "Vote block {} doesn't match proposal block {}", | ||
| vote.block_number, proposal.block_number | ||
| ))); | ||
| } | ||
|
|
||
| // Check for conflicting votes from same voter | ||
| if let Some(existing) = self.votes.get(&vote.voter) { | ||
| if existing.state_root != vote.state_root { | ||
| // This is potential fraud - voter sending different roots | ||
| warn!( | ||
| voter = vote.voter.to_hex(), | ||
| first_root = hex::encode(existing.state_root), | ||
| second_root = hex::encode(vote.state_root), | ||
| "Detected conflicting votes from same validator" | ||
| ); | ||
| return Err(StateRootConsensusError::FraudDetected(format!( | ||
| "Validator {} sent conflicting votes", | ||
| vote.voter.to_hex() | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| debug!( | ||
| voter = vote.voter.to_hex(), | ||
| agrees = vote.agrees_with_proposal, | ||
| "Received vote" | ||
| ); | ||
|
|
||
| self.votes.insert(vote.voter.clone(), vote); | ||
|
|
There was a problem hiding this comment.
Add signature verification before accepting proposals/votes.
receive_proposal and receive_vote accept data without validating the signature, yet StateRootConsensusError::InvalidSignature exists. This allows spoofed proposals/votes and undermines consensus integrity. Please verify signatures using the proposer/voter hotkeys before storing or counting them, and return InvalidSignature on failure.
🤖 Prompt for AI Agents
In `@crates/distributed-storage/src/state_consensus.rs` around lines 533 - 646,
The code currently stores proposals and votes without checking their
cryptographic signatures; update receive_proposal and receive_vote to verify
signatures with the sender hotkey before accepting: call the proposer public key
(proposal.proposer) to verify the proposal signature and the voter public key
(vote.voter) to verify the vote.signature, and if verification fails return
StateRootConsensusError::InvalidSignature and do not insert into
self.current_proposal or self.votes; keep existing structural checks (e.g.,
verify_global_root and block_number checks) but move any storage
(self.current_proposal = Some(...), self.votes.insert(...)) to after successful
signature verification so only signed, valid messages are recorded.
| // Verify vote is for current proposal | ||
| if vote.block_number != proposal.block_number { | ||
| return Err(StateRootConsensusError::InternalError(format!( | ||
| "Vote block {} doesn't match proposal block {}", | ||
| vote.block_number, proposal.block_number | ||
| ))); | ||
| } | ||
|
|
||
| // Check for conflicting votes from same voter | ||
| if let Some(existing) = self.votes.get(&vote.voter) { | ||
| if existing.state_root != vote.state_root { | ||
| // This is potential fraud - voter sending different roots | ||
| warn!( | ||
| voter = vote.voter.to_hex(), | ||
| first_root = hex::encode(existing.state_root), | ||
| second_root = hex::encode(vote.state_root), | ||
| "Detected conflicting votes from same validator" | ||
| ); | ||
| return Err(StateRootConsensusError::FraudDetected(format!( | ||
| "Validator {} sent conflicting votes", | ||
| vote.voter.to_hex() | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| debug!( | ||
| voter = vote.voter.to_hex(), | ||
| agrees = vote.agrees_with_proposal, | ||
| "Received vote" | ||
| ); | ||
|
|
||
| self.votes.insert(vote.voter.clone(), vote); | ||
|
|
There was a problem hiding this comment.
Reject “agree” votes whose root doesn’t match the proposal.
receive_vote trusts agrees_with_proposal even if state_root differs from the proposal’s root, which can inflate consensus with inconsistent votes.
🛠️ Proposed fix
// Verify vote is for current proposal
if vote.block_number != proposal.block_number {
return Err(StateRootConsensusError::InternalError(format!(
"Vote block {} doesn't match proposal block {}",
vote.block_number, proposal.block_number
)));
}
+
+ // Ensure "agree" votes actually match the proposed root
+ if vote.agrees_with_proposal && vote.state_root != proposal.global_state_root {
+ return Err(StateRootConsensusError::ConflictingRoots {
+ expected: hex::encode(proposal.global_state_root),
+ got: hex::encode(vote.state_root),
+ });
+ }🤖 Prompt for AI Agents
In `@crates/distributed-storage/src/state_consensus.rs` around lines 614 - 646,
receive_vote currently accepts votes that set agrees_with_proposal=true even
when vote.state_root doesn't match proposal.state_root; add a check in
receive_vote after the block_number check (and before inserting into self.votes)
that if vote.agrees_with_proposal && vote.state_root != proposal.state_root then
log a warning (include voter and both roots) and return an error (e.g.,
StateRootConsensusError::FraudDetected or a suitable InvalidVote error) so
“agree” votes with mismatched roots are rejected rather than counted.
| /// Compute the hash of a block header | ||
| /// | ||
| /// Uses SHA-256 over the bincode-serialized header. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `header` - The block header to hash | ||
| pub fn compute_block_hash(header: &BlockHeader) -> [u8; 32] { | ||
| let mut hasher = Sha256::new(); | ||
|
|
||
| // Hash the core header fields deterministically | ||
| hasher.update(&header.block_number.to_le_bytes()); | ||
| hasher.update(&header.parent_hash); | ||
| hasher.update(&header.state_root); | ||
| hasher.update(&header.timestamp.to_le_bytes()); | ||
| hasher.update(&header.proposer.0); | ||
|
|
||
| // Hash challenge roots in deterministic order | ||
| let mut sorted_challenges: Vec<_> = header.challenge_roots.iter().collect(); | ||
| sorted_challenges.sort_by(|a, b| a.0 .0.cmp(&b.0 .0)); | ||
| for (challenge_id, root) in sorted_challenges { | ||
| hasher.update(challenge_id.0.as_bytes()); | ||
| hasher.update(root); | ||
| } | ||
|
|
||
| hasher.finalize().into() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the BlockHeader struct definition
rg "struct BlockHeader" crates/storage/src/ -A 20Repository: PlatformNetwork/platform-v2
Length of output: 1525
🏁 Script executed:
# Also search for state_transitions field usage
rg "state_transitions" crates/storage/src/blockchain.rs -B 2 -A 2Repository: PlatformNetwork/platform-v2
Length of output: 1346
🏁 Script executed:
# Check the Block struct definition to see how state_transitions fits
rg "struct Block" crates/storage/src/ -A 20Repository: PlatformNetwork/platform-v2
Length of output: 4226
🏁 Script executed:
# Look at the full context of the compute_block_hash function
sed -n '340,366p' crates/storage/src/blockchain.rsRepository: PlatformNetwork/platform-v2
Length of output: 1066
🏁 Script executed:
# Search for how state_root is computed
rg "state_root" crates/storage/src/blockchain.rs -B 3 -A 3Repository: PlatformNetwork/platform-v2
Length of output: 4833
🏁 Script executed:
# Check if there's a state root computation function
rg "compute.*state.*root|state.*root.*compute" crates/storage/src/ -iRepository: PlatformNetwork/platform-v2
Length of output: 1216
🏁 Script executed:
# Look for how Block::new uses state_root and state_transitions
sed -n '1,100p' crates/storage/src/blockchain.rs | grep -A 50 "impl Block"Repository: PlatformNetwork/platform-v2
Length of output: 646
🏁 Script executed:
# Search for any relationship between state_transitions and state_root
rg "state_transitions.*state_root|state_root.*state_transitions" crates/storage/src/ -iRepository: PlatformNetwork/platform-v2
Length of output: 53
🏁 Script executed:
# Check the entire Block::new implementation
rg "fn new.*header.*state_transitions" crates/storage/src/blockchain.rs -A 10Repository: PlatformNetwork/platform-v2
Length of output: 378
Block hash does not commit to state_transitions, allowing undetected mutation.
compute_block_hash hashes only BlockHeader fields; state_transitions are structurally separate and excluded from the hash. More critically, Block::new() does not validate that state_root is derived from or matches state_transitions, so they can diverge—and state_transitions can be modified after block creation without invalidating block_hash or signatures.
Consider adding a state transitions digest to the header and including it in the block hash, or validating at creation time that state_root is derived from the provided transitions.
Also, the doc comment claims "bincode-serialized header," but the implementation manually hashes individual fields. Align the comment with the actual implementation.
🤖 Prompt for AI Agents
In `@crates/storage/src/blockchain.rs` around lines 340 - 366, compute_block_hash
currently omits state_transitions and the Block::new constructor does not
validate that state_root matches those transitions, allowing undetected
mutation; update the design by either (A) adding a state_transitions_digest
(e.g., state_transitions_root/digest) to BlockHeader and include that digest in
compute_block_hash (and ensure whatever code creates headers computes and sets
this digest from the provided state_transitions), or (B) validate inside
Block::new that the provided state_root is derived from the given
state_transitions by computing the same digest and rejecting/returning an error
if it differs; also update the compute_block_hash doc comment to reflect that
hashing is done field-by-field (or that it includes the state_transitions
digest) rather than “bincode-serialized header.” Reference symbols:
compute_block_hash, BlockHeader, Block::new, state_transitions, state_root.
| /// Append a new block to the chain | ||
| /// | ||
| /// Validates that the block's parent hash matches the current chain tip | ||
| /// before appending. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `block` - The block to append | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if: | ||
| /// - The parent hash doesn't match the previous block's hash | ||
| /// - The block number is not sequential | ||
| /// - The block hash verification fails | ||
| pub fn append_block(&mut self, block: Block) -> Result<()> { | ||
| // Verify the block hash is correctly computed | ||
| if !block.verify_hash() { | ||
| return Err(MiniChainError::Validation( | ||
| "Block hash verification failed".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let latest_number = self.get_latest_block_number()?; | ||
|
|
||
| // Validate block number | ||
| let expected_number = latest_number.map(|n| n + 1).unwrap_or(0); | ||
| if block.header.block_number != expected_number { | ||
| return Err(MiniChainError::Validation(format!( | ||
| "Invalid block number: expected {}, got {}", | ||
| expected_number, block.header.block_number | ||
| ))); | ||
| } | ||
|
|
||
| // Validate parent hash for non-genesis blocks | ||
| if let Some(prev_number) = latest_number { | ||
| let prev_block = self | ||
| .get_block_by_number(prev_number)? | ||
| .ok_or_else(|| MiniChainError::NotFound("Previous block not found".to_string()))?; | ||
|
|
||
| if block.header.parent_hash != prev_block.block_hash { | ||
| return Err(MiniChainError::Validation(format!( | ||
| "Parent hash mismatch: expected {:?}, got {:?}", | ||
| hex::encode(prev_block.block_hash), | ||
| hex::encode(block.header.parent_hash) | ||
| ))); | ||
| } | ||
| } else { | ||
| // Genesis block should have zero parent hash | ||
| if block.header.parent_hash != [0u8; 32] { | ||
| return Err(MiniChainError::Validation( | ||
| "Genesis block must have zero parent hash".to_string(), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| // Serialize and store the block | ||
| let block_bytes = | ||
| bincode::serialize(&block).map_err(|e| MiniChainError::Serialization(e.to_string()))?; | ||
|
|
||
| let block_number_key = block.header.block_number.to_be_bytes(); | ||
|
|
||
| self.blocks_tree | ||
| .insert(&block_number_key, block_bytes) | ||
| .map_err(|e| MiniChainError::Storage(format!("Failed to store block: {}", e)))?; | ||
|
|
||
| // Update hash index | ||
| self.hash_index_tree | ||
| .insert(&block.block_hash, &block_number_key) | ||
| .map_err(|e| MiniChainError::Storage(format!("Failed to update hash index: {}", e)))?; | ||
|
|
||
| // Update latest block number | ||
| self.metadata_tree | ||
| .insert(KEY_LATEST_BLOCK, &block_number_key) | ||
| .map_err(|e| { | ||
| MiniChainError::Storage(format!("Failed to update latest block number: {}", e)) | ||
| })?; | ||
|
|
||
| info!( | ||
| block_number = block.header.block_number, | ||
| hash = hex::encode(block.block_hash), | ||
| transitions = block.state_transitions.len(), | ||
| "Appended block to chain" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the blockchain.rs file and understand the structure
find . -name "blockchain.rs" -type fRepository: PlatformNetwork/platform-v2
Length of output: 105
🏁 Script executed:
# Check the Cargo.toml for sled version and features
find . -name "Cargo.toml" -type f | head -5 | xargs grep -l "sled"Repository: PlatformNetwork/platform-v2
Length of output: 83
🏁 Script executed:
# Examine the blockchain.rs file to understand the BlockChain struct and sled trees
cat -n crates/storage/src/blockchain.rs | head -100Repository: PlatformNetwork/platform-v2
Length of output: 4012
🏁 Script executed:
# Look for the struct definition and field types
rg "struct BlockChain|blocks_tree|hash_index_tree|metadata_tree" crates/storage/src/blockchain.rs -A 2Repository: PlatformNetwork/platform-v2
Length of output: 2252
🏁 Script executed:
# Check sled version in Cargo.toml
cat Cargo.toml | grep -A 5 "sled"Repository: PlatformNetwork/platform-v2
Length of output: 180
🏁 Script executed:
# Look for imports and current transaction usage in blockchain.rs
head -50 crates/storage/src/blockchain.rs | grep -E "^use|^mod"Repository: PlatformNetwork/platform-v2
Length of output: 288
🏁 Script executed:
# Search for any existing transaction patterns in the codebase
rg "transaction|Transactional" --type rust -B 2 -A 3Repository: PlatformNetwork/platform-v2
Length of output: 2279
🏁 Script executed:
# Check the append_block method implementation in full
sed -n '439,525p' crates/storage/src/blockchain.rsRepository: PlatformNetwork/platform-v2
Length of output: 3323
🌐 Web query:
sled 0.34 transaction API multiple trees Transactional
💡 Result:
In sled 0.34.x, multi-key and multi-Tree ACID transactions are done via the Transactional trait: you call .transaction(...) on a single Tree or on a tuple of Tree references.
Single-Tree transaction
use sled::transaction::TransactionResult;
let db = sled::Config::new().temporary(true).open()?;
let t = db.open_tree(b"a")?;
t.transaction(|tx| {
tx.insert(b"k1", b"cats")?;
tx.insert(b"k2", b"dogs")?;
Ok(())
})?;[1]
Multi-Tree transaction (multiple trees in one atomic transaction)
use sled::Transactional; // brings .transaction into scope for tuples
let db = sled::Config::new().temporary(true).open()?;
let t1 = db.open_tree(b"unprocessed")?;
let t2 = db.open_tree(b"processed")?;
(&t1, &t2).transaction(|(unprocessed, processed)| {
let item = unprocessed.remove(b"k3")?.unwrap();
processed.insert(b"k3", item)?;
Ok(())
})?;This runs as a single serializable (ACID) transaction across both Trees. [1]
Note: sled transactions are optimistic and the closure may be retried on conflict, so avoid non-idempotent external side effects inside the closure. [2]
Sources:
[1] sled transaction module docs (multi-Tree tuple transactions) https://docs.rs/sled/latest/sled/transaction/index.html
[2] sled repository notes on transactions (optimistic / retry) https://github.com/spacejam/sled
🏁 Script executed:
# Check sled documentation or examples in the repo
fd "sled" --type f | xargs grep -l "transaction\|Transactional" 2>/dev/null | head -5Repository: PlatformNetwork/platform-v2
Length of output: 53
🏁 Script executed:
# Look for how sled is used elsewhere in the codebase
rg "\.insert\(" crates/storage/ -B 2 -A 2 | head -60Repository: PlatformNetwork/platform-v2
Length of output: 3361
🏁 Script executed:
# Check sled transaction error handling documentation by looking at actual usage
rg "transaction\(" crates/ --type rust -B 1 -A 5 | head -40Repository: PlatformNetwork/platform-v2
Length of output: 53
🏁 Script executed:
# Check if there are any other blockchain storage operations that might use transactions
rg "struct BlockchainStorage|impl BlockchainStorage" crates/storage/src/blockchain.rs -A 30 | head -60Repository: PlatformNetwork/platform-v2
Length of output: 1295
Use sled transactions to atomically persist block to all three trees.
append_block updates three trees separately; a crash between inserts can leave the chain inconsistent (orphaned blocks, stale metadata, or broken hash index). Use sled's Transactional trait to wrap all three inserts in a single ACID transaction.
Transactional append pattern
+use sled::Transactional;
...
- self.blocks_tree
- .insert(&block_number_key, block_bytes)
- .map_err(|e| MiniChainError::Storage(format!("Failed to store block: {}", e)))?;
-
- self.hash_index_tree
- .insert(&block.block_hash, &block_number_key)
- .map_err(|e| MiniChainError::Storage(format!("Failed to update hash index: {}", e)))?;
-
- self.metadata_tree
- .insert(KEY_LATEST_BLOCK, &block_number_key)
- .map_err(|e| {
- MiniChainError::Storage(format!("Failed to update latest block number: {}", e))
- })?;
+ (&self.blocks_tree, &self.hash_index_tree, &self.metadata_tree)
+ .transaction(|(blocks, hash_index, metadata)| {
+ blocks.insert(&block_number_key, block_bytes.as_slice())?;
+ hash_index.insert(&block.block_hash, &block_number_key)?;
+ metadata.insert(KEY_LATEST_BLOCK, &block_number_key)?;
+ Ok(())
+ })
+ .map_err(|e| MiniChainError::Storage(format!("Failed to store block: {e}")))?;🤖 Prompt for AI Agents
In `@crates/storage/src/blockchain.rs` around lines 439 - 525, append_block
currently writes blocks_tree, hash_index_tree, and metadata_tree separately
which can leave state inconsistent; serialize the block first, then perform the
three inserts inside a single sled transaction using the Transactional API (wrap
blocks_tree.insert, hash_index_tree.insert, and
metadata_tree.insert(KEY_LATEST_BLOCK, ...) in one transaction closure), map any
sled::transaction::TransactionError into MiniChainError::Storage, and only log
and return Ok(()) after the transaction commits; use the existing symbols
append_block, blocks_tree, hash_index_tree, metadata_tree, and KEY_LATEST_BLOCK
to locate and change the code.
| fn create_test_db() -> sled::Db { | ||
| let dir = tempdir().expect("Failed to create temp dir"); | ||
| sled::open(dir.path()).expect("Failed to open test database") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the exact code at the specified lines
cat -n crates/storage/src/blockchain.rs | sed -n '745,760p'Repository: PlatformNetwork/platform-v2
Length of output: 604
🏁 Script executed:
# Search for usages of create_test_db()
rg "create_test_db" crates/storage/src/blockchain.rs -A 5 -B 2Repository: PlatformNetwork/platform-v2
Length of output: 5221
🏁 Script executed:
# Let's search for other sled db creation patterns in the codebase
rg "sled::open|tempdir|TempDir" crates/ -B 2 -A 2 | head -100Repository: PlatformNetwork/platform-v2
Length of output: 5899
🏁 Script executed:
# Let's check if there are any sled-specific database patterns in tests
rg "sled::open" crates/ -B 5 -A 5Repository: PlatformNetwork/platform-v2
Length of output: 43074
🏁 Script executed:
# Let's check the pattern in dynamic.rs to see the correct approach
sed -n '1,100p' crates/storage/src/dynamic.rs | grep -A 20 "create_test_storage"Repository: PlatformNetwork/platform-v2
Length of output: 53
🏁 Script executed:
# Let's check the create_test_storage pattern in dynamic.rs
rg "create_test_storage" crates/storage/src/dynamic.rs -A 5Repository: PlatformNetwork/platform-v2
Length of output: 8351
🏁 Script executed:
# Let me check the metadata.rs create_test_db pattern
rg "create_test_db" crates/storage/src/metadata.rs -A 5 -B 2Repository: PlatformNetwork/platform-v2
Length of output: 5441
🏁 Script executed:
# Let's check if there are any issues reported or if tests are failing with this pattern
git log --oneline crates/storage/src/blockchain.rs | head -10Repository: PlatformNetwork/platform-v2
Length of output: 112
Keep TempDir alive for the test lifetime.
tempdir() is dropped at the end of create_test_db, deleting the directory while sled is actively using it. This causes flaky tests, especially on Windows where file deletion has stricter guarantees. Return TempDir alongside Db (or store it in a helper struct) to keep the directory alive for the duration of each test.
🤖 Prompt for AI Agents
In `@crates/storage/src/blockchain.rs` around lines 751 - 754, The helper
create_test_db currently drops the TempDir before returning the sled::Db which
deletes the directory while sled is still using it; change create_test_db to
return the TempDir along with the sled::Db (or create a small helper struct
holding both) so the TempDir lives for the test lifetime, e.g., adjust the
function signature and callers to receive (TempDir, sled::Db) or the helper
struct to ensure the temp directory is not deleted while sled::Db is in use.
1 participant
Summary
This PR implements a comprehensive metadata management system for the Platform storage layer, enabling blockchain-like properties for tracking storage schemas and ensuring state consistency across the validator network.
Changes
Blockchain Storage Module (
crates/storage/src/blockchain.rs)BlockHeaderstruct with state roots, challenge roots, and validator signaturesValidatorSignaturestruct for block attestationsBlockstruct with state transitions and computed hashStateTransitionenum tracking challenge registration, state updates, migrations, and validator changesBlockchainStoragewith full CRUD operations and historical state queriesUnified Metadata Registry (
crates/storage/src/metadata.rs)ChallengeMetadata: tracks schema version, merkle root, and config per challengeGlobalMetadata: tracks all challenges and combined state rootMetadataRegistry: centralized registry with register/unregister, state root updates, and validationState Root Consensus Protocol (
crates/distributed-storage/src/state_consensus.rs)StateRootProposalandStateRootVotefor coordinated state verificationGlobalStateLinkerfor aggregating per-challenge roots into global rootStateRootConsensusmanager for proposals, votes, and consensus achievementFraudProofgeneration for detecting and proving Byzantine behaviorNetwork-Aware Migration Coordination (
crates/storage/src/migration.rs)NetworkMigrationStatusfor tracking network-wide schema versionsNetworkMigrationCoordinatorfor coordinating migrations across validatorsChallengeMigrationandChallengeMigrationStatusfor per-challenge migrationsChallengeMigrationHandlertrait for challenge-specific migrationsArchitecture
Testing
All modules include comprehensive unit tests covering:
Notes
This system provides blockchain-like properties (immutability, verifiability, consensus) for tracking storage schemas without handling payments - purely for validation coordination across the Bittensor subnet validators.
Summary by CodeRabbit
New Features
Tests / Style