diff --git a/client/consensus/qpow/src/chain_management.rs b/client/consensus/qpow/src/chain_management.rs index e6a412905..92da188cf 100644 --- a/client/consensus/qpow/src/chain_management.rs +++ b/client/consensus/qpow/src/chain_management.rs @@ -5,7 +5,7 @@ use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; use sp_consensus::Error as ConsensusError; use sp_consensus_qpow::QPoWApi; -use sp_runtime::traits::{Block as BlockT, Header, Zero}; +use sp_runtime::traits::{Block as BlockT, Header, One, Zero}; use std::fmt; const ACHIEVED_WORK_PREFIX: &[u8] = b"QPow:AchievedWork:"; @@ -163,8 +163,10 @@ pub fn is_heavier( /// This should be called synchronously after each block import to ensure finalization /// happens before the next block is imported. /// -/// Also cleans up achieved work entries for blocks that are now deep enough in the -/// finalized chain that they can never be involved in fork choice again. +/// Cleans up achieved work entries for canonical blocks that are now finalized. +/// Note: Non-canonical fork blocks are not cleaned up since we cannot enumerate them +/// by height. This is acceptable because fork blocks require valid PoW to create, +/// making accumulation attacks expensive, and the entries are small (~96 bytes each). pub fn finalize_canonical_at_depth(client: &C) -> Result<(), ConsensusError> where B: BlockT, @@ -267,21 +269,27 @@ where log::debug!("✓ Finalized block #{:?} ({:?})", finalize_number, finalize_hash); - // Clean up achieved work for the previously finalized block. - // Once a block is finalized and we've moved past it, its achieved work - // is no longer needed for fork choice decisions. - if last_finalized_before > Zero::zero() { - if let Ok(Some(old_finalized_hash)) = client.hash(last_finalized_before) { - if let Err(e) = delete_cumulative_achieved_work::(client, old_finalized_hash) { + // Clean up achieved work entries for blocks that are now below the finalized tip. + // Delete entries from last_finalized_before up to (not including) last_finalized_after. + // The finalized tip's entry must be preserved as it's the parent work source for children. + // + // The loop naturally handles edge cases: + // - If finalization didn't advance (after <= before): zero iterations + // - If multi-step jump (e.g., bursty sync): cleans all intermediate entries + let mut height_to_clean = last_finalized_before; + while height_to_clean < last_finalized_after { + if let Ok(Some(hash_to_clean)) = client.hash(height_to_clean) { + if let Err(e) = delete_cumulative_achieved_work::(client, hash_to_clean) { // Non-fatal: log warning but don't fail the finalization log::warn!( target: "qpow", - "Failed to clean up achieved work for old finalized block #{:?}: {:?}", - last_finalized_before, + "Failed to clean up achieved work for block #{:?}: {:?}", + height_to_clean, e ); } } + height_to_clean = height_to_clean + One::one(); } Ok(()) diff --git a/client/consensus/qpow/src/worker.rs b/client/consensus/qpow/src/worker.rs index eb768d8d4..803e15e57 100644 --- a/client/consensus/qpow/src/worker.rs +++ b/client/consensus/qpow/src/worker.rs @@ -134,77 +134,63 @@ where self.build.lock().as_ref().map(|b| b.metadata.clone()) } - /// Verify a seal without consuming the build. - /// - /// Returns `true` if the seal is valid for the current block, `false` otherwise. - /// Returns `false` if there's no current build. - /// Logs detailed information on failure for debugging. - pub fn verify_seal(&self, seal: &Seal) -> bool { - let build = self.build.lock(); - let build = match build.as_ref() { - Some(b) => b, - None => { - warn!(target: LOG_TARGET, "verify_seal: No current build available"); - return false; - }, - }; - - // Convert seal to nonce [u8; 64] - let nonce: [u8; 64] = match seal.as_slice().try_into() { - Ok(arr) => arr, - Err(_) => { - warn!(target: LOG_TARGET, "Seal does not have exactly 64 bytes, got {}", seal.len()); - return false; - }, - }; + /// Submit a mined seal. The seal will be validated before consuming the build. + /// Returns true if the submission is successful. + pub async fn submit(&self, seal: Seal) -> bool { + // Atomically verify and take the build in a single lock acquisition. + // This prevents TOCTOU issues where a rebuild could land between verify and consume. + let build = { + let mut build_guard = self.build.lock(); + + // Extract metadata for verification while keeping the build in place + let (pre_hash, best_hash) = match build_guard.as_ref() { + Some(b) => (b.metadata.pre_hash.0, b.metadata.best_hash), + None => { + warn!(target: LOG_TARGET, "Unable to import mined block: build does not exist"); + return false; + }, + }; - let pre_hash = build.metadata.pre_hash.0; - let best_hash = build.metadata.best_hash; - let difficulty = build.metadata.difficulty; - let extrinsic_count = build.proposal.block.extrinsics().len(); - - // Verify using runtime API - match self.client.runtime_api().verify_nonce_local_mining(best_hash, pre_hash, nonce) { - Ok(true) => true, - Ok(false) => { - log::error!( - target: LOG_TARGET, - "verify_seal FAILED:\n\ - pre_hash (block template): {}\n\ - best_hash (parent block): {}\n\ - difficulty: {}\n\ - nonce (seal): {}\n\ - extrinsics in block: {}", - hex::encode(pre_hash), - best_hash, - difficulty, - hex::encode(nonce), - extrinsic_count, - ); - false - }, - Err(e) => { - warn!(target: LOG_TARGET, "Runtime API error verifying seal: {:?}", e); - false - }, - } - } + // Verify seal before consuming the build + let nonce: [u8; 64] = match seal.as_slice().try_into() { + Ok(arr) => arr, + Err(_) => { + warn!(target: LOG_TARGET, "Seal does not have exactly 64 bytes, got {}", seal.len()); + return false; + }, + }; - /// Submit a mined seal. The seal will be validated again. Returns true if the submission is - /// successful. - pub async fn submit(&self, seal: Seal) -> bool { - let build = if let Some(build) = { - let mut build = self.build.lock(); - let value = build.take(); - if value.is_some() { - self.increment_version(); + match self.client.runtime_api().verify_nonce_local_mining(best_hash, pre_hash, nonce) { + Ok(true) => { + // Seal is valid, take the build. This cannot be None because: + // - We hold the lock continuously since checking as_ref() above + // - No other code path modifies build_guard between check and take + let build = build_guard.take(); + self.increment_version(); + match build { + Some(b) => b, + None => { + // This branch is unreachable given the lock invariants, but we handle + // it explicitly rather than using unwrap() to satisfy safety + // guidelines. + warn!(target: LOG_TARGET, "Build disappeared while holding lock (should be unreachable)"); + return false; + }, + } + }, + Ok(false) => { + warn!( + target: LOG_TARGET, + "Seal verification failed: pre_hash={:?}, best_hash={:?}", + pre_hash, best_hash + ); + return false; + }, + Err(e) => { + warn!(target: LOG_TARGET, "Runtime API error verifying seal: {:?}", e); + return false; + }, } - value - } { - build - } else { - warn!(target: LOG_TARGET, "Unable to import mined block: build does not exist",); - return false; }; let seal = DigestItem::Seal(POW_ENGINE_ID, seal); diff --git a/miner-api/src/lib.rs b/miner-api/src/lib.rs index 5e5409916..028313d2b 100644 --- a/miner-api/src/lib.rs +++ b/miner-api/src/lib.rs @@ -1,8 +1,11 @@ use serde::{Deserialize, Serialize}; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; -/// Maximum message size (16 MB) to prevent memory exhaustion attacks. -pub const MAX_MESSAGE_SIZE: u32 = 16 * 1024 * 1024; +/// Maximum message size (1 KB) to prevent memory exhaustion attacks. +/// +/// Real MinerMessage payloads are only a few hundred bytes (Ready, NewJob, JobResult). +/// 1 KB provides sufficient headroom while minimizing the amplification attack surface. +pub const MAX_MESSAGE_SIZE: u32 = 1024; /// Status codes returned in API responses. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] diff --git a/node/src/service.rs b/node/src/service.rs index 13965cadd..34c50139f 100644 --- a/node/src/service.rs +++ b/node/src/service.rs @@ -212,12 +212,7 @@ async fn handle_external_mining( }, }; - // Verify the seal before attempting to submit (submit consumes the build) - if !worker_handle.verify_seal(&seal) { - continue; - } - - // Seal is valid, submit it + // Submit the seal (submit verifies atomically before consuming the build) if worker_handle.submit(seal.clone()).await { let mining_time = mining_start_time.elapsed().as_secs(); log::info!( @@ -229,9 +224,9 @@ async fn handle_external_mining( return ExternalMiningOutcome::Success; } - // Submit failed for some other reason (should be rare after verify_seal passed) + // Submit failed (seal invalid or import error) log::warn!( - "⛏️ Failed to submit verified seal from miner {}, continuing to wait (job {})", + "⛏️ Failed to submit seal from miner {}, continuing to wait (job {})", miner_id, job_id ); diff --git a/pallets/qpow/src/lib.rs b/pallets/qpow/src/lib.rs index 3337a417d..1c5148a04 100644 --- a/pallets/qpow/src/lib.rs +++ b/pallets/qpow/src/lib.rs @@ -77,12 +77,12 @@ pub mod pallet { #[pallet::genesis_build] impl BuildGenesisConfig for GenesisConfig { fn build(&self) { - let initial_difficulty = T::InitialDifficulty::get(); - - >::put(initial_difficulty); + // Use the genesis config value, not the runtime constant. + // This allows chain-spec overrides of initial difficulty. + >::put(self.initial_difficulty); log::info!(target: "qpow", "Genesis: Set initial difficulty to {:x}", - initial_difficulty.low_u64()); + self.initial_difficulty.low_u64()); } } @@ -109,7 +109,7 @@ pub mod pallet { /// Called at the end of each block to adjust mining difficulty. fn on_finalize(block_number: BlockNumberFor) { - let current_difficulty = >::get(); + let current_difficulty = Self::get_difficulty(); log::debug!(target: "qpow", "📢 QPoW: before submit at block {:?}, current_difficulty={:?}", block_number, @@ -137,7 +137,10 @@ pub mod pallet { fn adjust_difficulty() { let now = pallet_timestamp::Pallet::::now().saturated_into::(); let last_time = >::get(); - let current_difficulty = >::get(); + // Use get_difficulty() to handle zero/missing storage consistently with verification. + // This ensures we use InitialDifficulty as the base when storage is unset, + // rather than computing from zero which would clamp to min_difficulty. + let current_difficulty = Self::get_difficulty(); let current_block_number = >::block_number(); // Calculate block time (use target for genesis block) diff --git a/pallets/qpow/src/tests.rs b/pallets/qpow/src/tests.rs index 34dd81edf..21c5b3ac5 100644 --- a/pallets/qpow/src/tests.rs +++ b/pallets/qpow/src/tests.rs @@ -1,4 +1,4 @@ -use crate::{mock::*, Config}; +use crate::{mock::*, Config, CurrentDifficulty}; use frame_support::{pallet_prelude::TypedGet, traits::Hooks}; use primitive_types::U512; use qpow_math::{get_nonce_hash, is_valid_nonce}; @@ -392,3 +392,58 @@ fn test_difficulty_below_min_clips_up() { assert_eq!(result_slow, min_diff); }); } + +/// Regression test for V12 audit fix #2: adjust_difficulty must use get_difficulty() +/// (which falls back to InitialDifficulty) rather than reading raw storage (which +/// would return zero if unset, causing difficulty to collapse to min_difficulty). +#[test] +fn test_adjust_difficulty_with_zero_storage_uses_initial_difficulty() { + new_test_ext().execute_with(|| { + let initial_difficulty = ::InitialDifficulty::get(); + let target_time = ::TargetBlockTime::get(); + + // Clear the CurrentDifficulty storage to simulate unset state. + // This could happen if genesis wasn't properly initialized or storage was corrupted. + CurrentDifficulty::::kill(); + + // Verify storage is indeed zero + assert_eq!(CurrentDifficulty::::get(), U512::zero()); + + // But get_difficulty() should return InitialDifficulty, not zero + assert_eq!(QPow::get_difficulty(), initial_difficulty); + + // Set up timestamp for block 1 + pallet_timestamp::Pallet::::set_timestamp(target_time); + System::set_block_number(1); + + // Run on_finalize which calls adjust_difficulty + QPow::on_finalize(1); + + // The new difficulty should be based on InitialDifficulty, not zero. + // With target_time == observed_time, difficulty should remain close to initial. + let new_difficulty = QPow::get_difficulty(); + + // Key assertion: difficulty should NOT have collapsed to min_difficulty. + // If the bug existed (using raw storage zero), we'd get min_difficulty. + let min_difficulty = QPow::get_min_difficulty(); + assert!( + new_difficulty > min_difficulty, + "Difficulty collapsed to min! Bug: adjust_difficulty used raw zero storage. \ + Expected near {}, got {} (min={})", + initial_difficulty.low_u64(), + new_difficulty.low_u64(), + min_difficulty.low_u64() + ); + + // Difficulty should be close to initial (within adjustment bounds) + // Ethereum-style adjustment is at most ±1/2048 per block + let max_change = initial_difficulty / 2048; + assert!( + new_difficulty >= initial_difficulty.saturating_sub(max_change) && + new_difficulty <= initial_difficulty.saturating_add(max_change), + "Difficulty {} not within ±1/2048 of initial {}", + new_difficulty.low_u64(), + initial_difficulty.low_u64() + ); + }); +} diff --git a/qpow-math/src/lib.rs b/qpow-math/src/lib.rs index 35f7996d9..303997565 100644 --- a/qpow-math/src/lib.rs +++ b/qpow-math/src/lib.rs @@ -83,7 +83,7 @@ pub fn mine_range( return Some((nonce_bytes, hash)); } - nonce = nonce.saturating_add(U512::from(1u64)); + nonce = nonce.overflowing_add(U512::from(1u64)).0; } None