Fix/prune batch consensus halt

frame/shielded-pool/CHANGELOG.md

@@ -2,6 +2,32 @@
 
 All notable changes to `pallet-shielded-pool` will be documented in this file.
 
+## [0.17.2] - 2026-08-13
+
+### Fixed
+
+- **The sealed-node sweep no longer sizes its batch from the block's leftover
+  weight — this halted the public testnet at block 406997.** `on_idle` received
+  `remaining` and divided it by the benchmarked per-node cost to pick how many
+  nodes to prune. Leftover weight is not consensus: once post-dispatch refunds
+  are in play an author and an importer measure the same block slightly
+  differently, so each pruned a different number of nodes and wrote a different
+  state. Frontier folds that state into the Ethereum block header it builds in
+  `on_finalize`, so the divergence surfaced as a mismatched `"fron"` digest and
+  `Executive::final_checks` panicked with *"Digest item must match that
+  calculated."* — every node rejecting every other node's block.
+
+  Three validators, identical state at 406997 and byte-identical extrinsics in
+  406998, produced three mutually unimportable blocks and the chain stopped for
+  4 hours. It survived five days only because empty blocks left the same
+  leftover weight everywhere; the first block carrying real EVM traffic split
+  the network.
+
+  The sweep now runs in `on_initialize` with a constant batch
+  (`PRUNED_NODES_PER_BLOCK`, unchanged at 512), which costs a fixed ~6.5 ms of a
+  2 s block. Regression test:
+  `prune_batch_is_independent_of_block_fullness`.
+
 ## [0.17.1] - 2026-08-11
 
 ### Security

frame/shielded-pool/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-shielded-pool"
-version = "0.17.1"
+version = "0.17.2"
 description = "Shielded pool pallet for private transactions using ZK proofs"
 authors = ["Orbinum Team"]
 license = "GPL-3.0-or-later"

frame/shielded-pool/src/benchmarking.rs

@@ -281,11 +281,11 @@ mod benchmarks {
 		);
 	}
 
-	/// Cost of one `on_idle` sweep that removes `n` sealed-tree nodes.
+	/// Cost of one sweep that probes `n` sealed-tree nodes.
 	///
-	/// Not an extrinsic: the sweep runs in `on_idle` with whatever weight the
-	/// block has left. It still needs measuring, because the hook must return the
-	/// weight it actually consumed — declaring less would let a block overrun.
+	/// Not an extrinsic: the sweep runs in `on_initialize` over a fixed batch. It
+	/// still needs measuring, because the hook must return the weight it actually
+	/// consumed — declaring less would let a block overrun.
 	///
 	/// The setup seals a tree and populates its prunable levels directly rather
 	/// than inserting 2^20 leaves, which no benchmark could run. What matters for

frame/shielded-pool/src/lib.rs

@@ -111,12 +111,13 @@ pub mod pallet {
 	///   (`migrations::v3::MigrateToV3`); both historic-root items carry an expiry.
 	pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(3);
 
-	/// Ceiling on how many sealed-tree nodes one `on_idle` pass may drop.
+	/// How many sealed-tree nodes each block sweeps.
 	///
-	/// The weight budget already bounds the sweep; this bounds the trie churn on
-	/// an idle chain, where the leftover weight would otherwise allow tens of
-	/// thousands of removals in a single block.
-	pub(crate) const MAX_PRUNED_NODES_PER_BLOCK: u32 = 512;
+	/// Fixed, never derived from the block's leftover weight — see `on_initialize`
+	/// for why that distinction is a consensus matter and not a tuning knob. At
+	/// ~12.7 µs of ref_time per node this costs ~6.5 ms of a 2 s block, and clears
+	/// a sealed 1 M-node tree in roughly 2 048 blocks (~3.4 hours at 6 s).
+	pub(crate) const PRUNED_NODES_PER_BLOCK: u32 = 512;
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -253,7 +254,7 @@ pub mod pallet {
 	/// Resume point for the sealed-tree node sweep: `(tree_id, level, index)`.
 	///
 	/// Pruning a sealed tree touches ~1M keys, far more than one block can absorb,
-	/// so `on_idle` walks it in bounded batches and parks the cursor here. `None`
+	/// so `on_initialize` walks it in bounded batches and parks the cursor here. `None`
 	/// means the sweep is idle — either nothing has sealed yet, or every sealed
 	/// tree is already pruned.
 	#[pallet::storage]
@@ -407,35 +408,25 @@ pub mod pallet {
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-		/// Reclaim internal Merkle nodes from sealed trees with whatever weight
-		/// the block has left over.
+		/// Reclaim internal Merkle nodes from sealed trees, a fixed batch per block.
 		///
 		/// A sealed tree holds ~1M prunable nodes — orders of magnitude past one
 		/// block — so the sweep runs in bounded batches and parks its position in
-		/// `SealedPruneCursor`. Doing this in `on_idle` rather than `on_initialize`
-		/// keeps it off the critical path: a busy block simply skips it, and the
-		/// work resumes when the chain has room.
-		fn on_idle(_now: BlockNumberFor<T>, remaining: Weight) -> Weight {
-			// Size the batch from the benchmarked per-node cost, so a nearly-full
-			// block prunes little or nothing and an idle one prunes up to the cap.
-			// Deriving it from the same `WeightInfo` the hook reports with keeps the
-			// budget and the charge from drifting apart.
-			let base = T::WeightInfo::prune_sealed_nodes(0);
-			let per_node = T::WeightInfo::prune_sealed_nodes(1).saturating_sub(base);
-
-			let Some(available) = remaining.checked_sub(&base) else {
-				return Weight::zero(); // not even the cursor read fits
-			};
-			if per_node.ref_time() == 0 || per_node.proof_size() == 0 {
-				return Weight::zero();
-			}
-			let budget = (available.ref_time() / per_node.ref_time())
-				.min(available.proof_size() / per_node.proof_size())
-				.min(MAX_PRUNED_NODES_PER_BLOCK as u64) as u32;
-
-			let removed = crate::merkle::MerkleTreeService::prune_sealed_nodes::<T>(budget);
-			// Charged even when nothing was removed: the cursor read happened.
-			T::WeightInfo::prune_sealed_nodes(removed)
+		/// `SealedPruneCursor`.
+		///
+		/// The batch size is a constant and must stay one. Sizing it from the
+		/// block's leftover weight looks free, but leftover weight is not
+		/// consensus: an author and an importer measure the same block slightly
+		/// differently once post-dispatch refunds are in play, so each would prune
+		/// a different number of nodes and their state roots would diverge. That
+		/// halted the testnet at block 406997.
+		fn on_initialize(_now: BlockNumberFor<T>) -> Weight {
+			crate::merkle::MerkleTreeService::prune_sealed_nodes::<T>(PRUNED_NODES_PER_BLOCK);
+			// The full batch is charged, not the removals: the sweep probes
+			// `PRUNED_NODES_PER_BLOCK` keys either way, and a miss costs the same
+			// read as a hit. Charging removals would under-declare an all-miss
+			// pass and let the block admit extrinsics it cannot pay for.
+			T::WeightInfo::prune_sealed_nodes(PRUNED_NODES_PER_BLOCK)
 		}
 
 		fn integrity_test() {

frame/shielded-pool/src/merkle/mod.rs

@@ -1217,7 +1217,7 @@ mod prune_tests {
 	use crate::{
 		Config,
 		mock::{Test, new_test_ext},
-		pallet::{LastPrunedTree, MAX_PRUNED_NODES_PER_BLOCK, SealedPruneCursor},
+		pallet::{LastPrunedTree, PRUNED_NODES_PER_BLOCK, SealedPruneCursor},
 		storage::MerkleRepository,
 		types::Commitment,
 	};
@@ -1402,14 +1402,159 @@ mod prune_tests {
 		});
 	}
 
-	/// The per-block ceiling has to be a real bound, not a placeholder.
+	/// Two nodes running the same block must end at the same state root.
+	///
+	/// The old `on_idle` hook received the block's leftover weight, which is NOT consensus: an
+	/// author that filled the block differently from what the importer measures
+	/// hands the hook a different leftover weight, the sweep prunes a different
+	/// number of nodes, and the two states diverge. That halted the testnet at
+	/// block 406997.
+	///
+	/// Regression: the sweep must consume a constant batch, so two nodes running
+	/// the same block land on the same state regardless of how full it was.
+	#[test]
+	fn prune_batch_is_independent_of_block_fullness() {
+		// The mock's 8-leaf tree is swept dry in one call, which would hide the
+		// divergence. Production seals 1_048_576-leaf trees, where the batch
+		// genuinely bounds the sweep — seal several mock trees to match.
+		fn sweep_once() -> (u32, Option<(u32, u8, u32)>) {
+			new_test_ext().execute_with(|| {
+				for _ in 0..40 {
+					seal_one_tree();
+				}
+				let removed = MerkleTreeService::prune_sealed_nodes::<Test>(PRUNED_NODES_PER_BLOCK);
+				(removed, SealedPruneCursor::<Test>::get())
+			})
+		}
+
+		// Same block, two nodes. Nothing about how full the block was may reach
+		// the sweep, so both must land identically.
+		assert_eq!(
+			sweep_once(),
+			sweep_once(),
+			"the sealed-node sweep is not deterministic: author and importer would \
+			 disagree on state and the chain would fork"
+		);
+	}
+
+	/// How many internal nodes the forest still holds — the sweep's whole effect.
+	fn surviving_nodes() -> usize {
+		crate::pallet::MerkleNodes::<Test>::iter().count()
+	}
+
+	// ── adversarial: can anything reintroduce the divergence? ─────────────────
+	//
+	// The fix is only worth what it survives. Each of these attacks the sweep
+	// from a different angle, trying to make two nodes running the same block
+	// prune differently.
+
+	/// Attack: run the sweep from a cursor parked anywhere, repeatedly.
+	///
+	/// The full sweep of a forest must reach the same end state no matter how
+	/// the batches were carved up — otherwise a node that restarted mid-sweep
+	/// would land somewhere its peers never do.
+	#[test]
+	fn sweep_converges_regardless_of_how_the_batches_are_carved() {
+		fn sweep_to_exhaustion(batch: u32) -> (u32, Option<(u32, u8, u32)>, usize) {
+			new_test_ext().execute_with(|| {
+				for _ in 0..20 {
+					seal_one_tree();
+				}
+				let mut total = 0;
+				// Bounded: a stuck sweep must fail the test, not hang it.
+				for _ in 0..10_000 {
+					let removed = MerkleTreeService::prune_sealed_nodes::<Test>(batch);
+					total += removed;
+					if SealedPruneCursor::<Test>::get().is_none() && removed == 0 {
+						break;
+					}
+				}
+				(total, SealedPruneCursor::<Test>::get(), surviving_nodes())
+			})
+		}
+
+		// One node sweeps in tiny batches, another in large ones. Same forest,
+		// so the same nodes must end up gone.
+		let fine = sweep_to_exhaustion(1);
+		let coarse = sweep_to_exhaustion(PRUNED_NODES_PER_BLOCK);
+		assert_eq!(
+			fine, coarse,
+			"batch size changed the end state: nodes that swept at different \
+			 rates would hold different tries"
+		);
+	}
+
+	/// Attack: seal more trees mid-sweep, the way a live chain does.
+	///
+	/// The cursor walks tree-by-tree while the forest grows underneath it. Two
+	/// nodes that saw the same insertions must still agree.
+	#[test]
+	fn sweep_is_stable_while_the_forest_grows() {
+		fn interleaved(batch: u32) -> usize {
+			new_test_ext().execute_with(|| {
+				for _ in 0..20 {
+					seal_one_tree();
+					// A block's worth of sweeping between each sealing.
+					MerkleTreeService::prune_sealed_nodes::<Test>(batch);
+				}
+				for _ in 0..2_000 {
+					let removed = MerkleTreeService::prune_sealed_nodes::<Test>(batch);
+					if SealedPruneCursor::<Test>::get().is_none() && removed == 0 {
+						break;
+					}
+				}
+				surviving_nodes()
+			})
+		}
+
+		assert_eq!(
+			interleaved(PRUNED_NODES_PER_BLOCK),
+			interleaved(PRUNED_NODES_PER_BLOCK),
+			"interleaving sealing with sweeping is not reproducible"
+		);
+	}
+
+	/// Attack: the sweep must never touch the tree still being written to.
+	///
+	/// This is the property that keeps today's notes spendable at O(depth); the
+	/// batch change moved the hook, so re-prove it rather than assume it held.
+	#[test]
+	fn active_tree_survives_an_exhaustive_sweep() {
+		new_test_ext().execute_with(|| {
+			let cap = seal_one_tree();
+			// Start a second tree and leave it active with one leaf.
+			let mut c = [0u8; 32];
+			c[..4].copy_from_slice(&cap.to_le_bytes());
+			MerkleTreeService::insert_leaf::<Test>(Commitment(c)).expect("insert");
+
+			for _ in 0..1_000 {
+				if MerkleTreeService::prune_sealed_nodes::<Test>(PRUNED_NODES_PER_BLOCK) == 0
+					&& SealedPruneCursor::<Test>::get().is_none()
+				{
+					break;
+				}
+			}
+
+			let active = MerkleRepository::get_tree_size::<Test>() / cap;
+			let cut = <Test as Config>::SealedTreePrunedBelowLevel::get();
+			for level in 1..cut {
+				assert!(
+					MerkleRepository::get_node::<Test>(active, level, 0).is_some(),
+					"active tree lost node at level {level}: paths would no longer \
+					 be O(depth) and the sweep is eating live state"
+				);
+			}
+		});
+	}
+
+	/// The per-block batch has to be a real bound, not a placeholder.
 	///
 	/// A `const` block rather than a runtime assert: both operands are constants,
 	/// so the compiler would fold an `assert!` away and the check would never run.
 	/// This one fails the build instead.
 	const _: () = assert!(
-		MAX_PRUNED_NODES_PER_BLOCK > 0 && MAX_PRUNED_NODES_PER_BLOCK <= 4096,
-		"MAX_PRUNED_NODES_PER_BLOCK must bound the sweep: zero disables pruning, \
+		PRUNED_NODES_PER_BLOCK > 0 && PRUNED_NODES_PER_BLOCK <= 4096,
+		"PRUNED_NODES_PER_BLOCK must bound the sweep: zero disables pruning, \
 		 and a value this far above the benchmarked batch would let one block \
 		 absorb work it cannot pay for"
 	);