Address v12 bugs in consensus

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<N: PartialOrd>(
 /// 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<B, C, BE>(client: &C) -> Result<(), ConsensusError>
 where
 	B: BlockT<Hash = H256>,
@@ -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::<B, C>(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::<B, C>(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(())

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);

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)]

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
 		);

pallets/qpow/src/lib.rs

@@ -77,12 +77,12 @@ pub mod pallet {
 	#[pallet::genesis_build]
 	impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
 		fn build(&self) {
-			let initial_difficulty = T::InitialDifficulty::get();
-
-			<CurrentDifficulty<T>>::put(initial_difficulty);
+			// Use the genesis config value, not the runtime constant.
+			// This allows chain-spec overrides of initial difficulty.
+			<CurrentDifficulty<T>>::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<T>) {
-			let current_difficulty = <CurrentDifficulty<T>>::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::<T>::now().saturated_into::<u64>();
 			let last_time = <LastBlockTime<T>>::get();
-			let current_difficulty = <CurrentDifficulty<T>>::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 = <frame_system::Pallet<T>>::block_number();
 
 			// Calculate block time (use target for genesis block)

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 = <Test as Config>::InitialDifficulty::get();
+		let target_time = <Test as Config>::TargetBlockTime::get();
+
+		// Clear the CurrentDifficulty storage to simulate unset state.
+		// This could happen if genesis wasn't properly initialized or storage was corrupted.
+		CurrentDifficulty::<Test>::kill();
+
+		// Verify storage is indeed zero
+		assert_eq!(CurrentDifficulty::<Test>::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::<Test>::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()
+		);
+	});
+}

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