Fix/shielded pool hardening

frame/shielded-pool/CHANGELOG.md

@@ -4,6 +4,19 @@ All notable changes to `pallet-shielded-pool` will be documented in this file.
 
 ## [Unreleased]
 
+## [0.25.2] - 2026-10-21
+
+### Fixed
+
+- A root replaced by a leaf insert stays spendable for `RootRetentionBlocks`
+  from its replacement, not from its creation. The active root never expires,
+  so a wallet may prove against one older than the window; the next insert
+  used to refuse that spend at once with `UnknownMerkleRoot`. Costs at most one
+  extra historic-root slot per block.
+- Pool admission tags each note a spend creates (transfer outputs, unshield
+  change) as well as each note it spends: two spends creating the same note
+  are mutually exclusive in the pool, since only one can execute.
+
 ## [0.25.1] - 2026-10-21
 
 ### Changed

frame/shielded-pool/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-shielded-pool"
-version = "0.25.1"
+version = "0.25.2"
 description = "Shielded pool pallet for private transactions using ZK proofs"
 authors = { workspace = true }
 license = "GPL-3.0-or-later"

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

@@ -1018,6 +1018,51 @@ mod tests {
 		});
 	}
 
+	/// An active root older than the window, replaced by an insert, stays
+	/// spendable for a full window from its replacement: a wallet that proved
+	/// against it is not refused by the next insert.
+	#[test]
+	fn a_replaced_root_counts_its_window_from_its_replacement() {
+		new_test_ext().execute_with(|| {
+			let start = System::block_number();
+			MerkleTreeService::insert_leaf::<Test>(Commitment::new([0x11u8; 32])).unwrap();
+			let old = MerkleRepository::get_poseidon_root::<Test>();
+
+			// Active for longer than a window, then replaced.
+			let replaced_at = start + retention() * 3;
+			System::set_block_number(replaced_at);
+			MerkleTreeService::insert_leaf::<Test>(Commitment::new([0x22u8; 32])).unwrap();
+			assert_ne!(MerkleRepository::get_poseidon_root::<Test>(), old);
+			assert!(
+				MerkleTreeService::is_known_root::<Test>(&old),
+				"replaced root refused right after the insert"
+			);
+
+			System::set_block_number(replaced_at + retention());
+			assert!(MerkleTreeService::is_known_root::<Test>(&old));
+			System::set_block_number(replaced_at + retention() + 1);
+			assert!(!MerkleTreeService::is_known_root::<Test>(&old));
+		});
+	}
+
+	/// Inserts in the same block refresh the outgoing root once: the roots made
+	/// within the block already carry a full window.
+	#[test]
+	fn replacing_a_root_from_the_same_block_queues_nothing_extra() {
+		new_test_ext().execute_with(|| {
+			System::set_block_number(System::block_number() + 1);
+			let before = MerkleRepository::historic_roots_queued::<Test>();
+			for i in 1..=5u8 {
+				MerkleTreeService::insert_leaf::<Test>(Commitment::new([i; 32])).unwrap();
+			}
+			// The genesis root, refreshed once, plus one slot per new root.
+			assert_eq!(
+				MerkleRepository::historic_roots_queued::<Test>(),
+				before + 1 + 5
+			);
+		});
+	}
+
 	/// A root stays spendable for the whole retention window and stops being
 	/// accepted once it has elapsed.
 	#[test]

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

@@ -83,6 +83,7 @@ impl MerkleTreeService {
 		MerkleRepository::set_tree_size::<T>(index.saturating_add(1));
 		PoolStatsRepository::increment_commitments_inserted::<T>();
 		MerkleRepository::set_frontier::<T>(frontier);
+		Self::retire_active_root::<T>(old_poseidon_root);
 		MerkleRepository::set_poseidon_root::<T>(new_poseidon_root);
 		Self::add_poseidon_historic_root::<T>(new_poseidon_root);
 
@@ -124,6 +125,22 @@ impl MerkleTreeService {
 
 	// ── Historic roots ──────────────────────────────────────────────────────
 
+	/// Keep the root being replaced spendable for a full window from now.
+	///
+	/// The active root never expires, so a wallet may prove against one created
+	/// long ago; its window must count from when it stops being active, or the
+	/// next insert would refuse that spend at once. Only a root created in an
+	/// earlier block needs this, so it costs at most one slot per block.
+	fn retire_active_root<T: Config>(root: Hash) {
+		let expires_at =
+			frame_system::Pallet::<T>::block_number().saturating_add(T::RootRetentionBlocks::get());
+		let fresh = MerkleRepository::get_historic_root_expiry::<T>(&root)
+			.is_some_and(|expiry| expiry >= expires_at);
+		if !fresh {
+			Self::add_poseidon_historic_root::<T>(root);
+		}
+	}
+
 	/// Record a new root, dropping the ones whose retention window has passed.
 	///
 	/// The window is measured in blocks (`RootRetentionBlocks`); the queue only

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

@@ -5,7 +5,9 @@
 //! would share the original's nullifier tag, outrank it on priority, replace it
 //! and then fail in the block — free, repeatable censorship — and junk spends
 //! would fill blocks for nothing. The price is node CPU per admission and
-//! revalidation; a peer that sends invalid proofs loses reputation for it.
+//! revalidation, paid by every node a spend reaches. An invalid proof costs the
+//! sending peer too little reputation to get it banned, so validators should
+//! take transactions only from their sentries (`--reserved-only`).
 //! Inside a block (`pre_dispatch`) only the cheap checks run: the dispatchable
 //! verifies the proof, and its weight covers one pairing.
 //!
@@ -42,5 +44,12 @@ pub(crate) const TX_LONGEVITY: u64 = 64;
 /// chain: one note, one entry.
 pub(crate) const SPEND_TAG_PREFIX: &str = "ShieldedPoolSpend";
 
+/// Marks a tag as a note a spend creates, apart from the nullifiers it spends.
+///
+/// A commitment is inserted once, so two spends creating the same note are as
+/// exclusive as two spending the same one: only one can execute. Tagged as
+/// `(OUTPUT_TAG, commitment)`, which never encodes like a bare nullifier.
+pub(crate) const OUTPUT_TAG: &[u8] = b"output";
+
 #[cfg(test)]
 mod tests;

frame/shielded-pool/src/validate_unsigned/tests.rs

@@ -19,7 +19,8 @@ fn full_memo() -> EncryptedMemo {
 
 /// A well-formed transfer with the given public values: two canonical
 /// commitments with a full memo each, and a dummy second input when only one
-/// nullifier is given.
+/// nullifier is given. The outputs follow from the input notes, in any order,
+/// so unrelated spends never create the same note.
 fn transfer_request(
 	merkle_root: &Hash,
 	nullifiers: &BoundedVec<Nullifier, ConstU32<2>>,
@@ -30,10 +31,12 @@ fn transfer_request(
 	while nullifiers.len() < 2 {
 		nullifiers.try_push(Nullifier::new([0u8; 32])).unwrap();
 	}
+	let inputs = nullifiers.iter().fold(0u8, |x, n| x ^ n.0[0]);
 	let commitment = |i: u8| {
 		let mut bytes = [0u8; 32];
 		bytes[0] = i + 1;
 		bytes[1] = 0xC0;
+		bytes[2] = inputs;
 		Commitment::new(bytes)
 	};
 	TransferRequest {
@@ -676,15 +679,16 @@ fn attack_each_nullifier_contributes_an_independent_tag() {
 	new_test_ext().execute_with(|| {
 		MerkleRepository::add_historic_poseidon_root::<Test>(KNOWN_ROOT);
 
+		// Plus one tag per output note, two in every transfer.
 		let one =
 			validate_private_transfer(&KNOWN_ROOT, &nullifiers_of(&[0x91]), &0u128, 1).unwrap();
-		assert_eq!(one.provides.len(), 1, "one real input → one tag");
+		assert_eq!(one.provides.len(), 1 + 2, "one real input → one tag");
 
 		let two = validate_private_transfer(&KNOWN_ROOT, &nullifiers_of(&[0x92, 0x93]), &0u128, 1)
 			.unwrap();
 		assert_eq!(
 			two.provides.len(),
-			2,
+			2 + 2,
 			"two real inputs → two independent tags"
 		);
 
@@ -693,7 +697,11 @@ fn attack_each_nullifier_contributes_an_independent_tag() {
 		padded.try_push(nullifier(0x94)).unwrap();
 		padded.try_push(Nullifier::new([0u8; 32])).unwrap();
 		let p = validate_private_transfer(&KNOWN_ROOT, &padded, &0u128, 1).unwrap();
-		assert_eq!(p.provides.len(), 1, "the dummy must not contribute a tag");
+		assert_eq!(
+			p.provides.len(),
+			1 + 2,
+			"the dummy must not contribute a tag"
+		);
 	});
 }
 
@@ -715,13 +723,91 @@ fn attack_transfer_and_unshield_of_the_same_note_are_mutually_exclusive() {
 			validate_private_transfer(&KNOWN_ROOT, &nullifiers_of(&[0x77]), &10u128, 1).unwrap();
 		let unshield = validate_unshield(&KNOWN_ROOT, &n, &0u32, &100u128, &10u128, 1).unwrap();
 
-		assert_eq!(
-			transfer.provides, unshield.provides,
+		assert!(
+			unshield
+				.provides
+				.iter()
+				.all(|t| transfer.provides.contains(t)),
 			"one note must back one pool entry, whichever operation spends it"
 		);
 	});
 }
 
+// ── one note created, one pool entry ─────────────────────────────────────────
+//
+// A commitment is inserted once, so two spends creating the same note cannot
+// both execute. Without a tag per output both would sit in the pool, propagate
+// and be revalidated, and the loser would fail in the block for free.
+
+/// Two transfers spending different notes into the same output note are
+/// mutually exclusive in the pool.
+#[test]
+fn transfers_creating_the_same_note_are_mutually_exclusive() {
+	new_test_ext().execute_with(|| {
+		MerkleRepository::add_historic_poseidon_root::<Test>(KNOWN_ROOT);
+		let first = transfer_request(&KNOWN_ROOT, &nullifiers_of(&[0xA1]), 0, 1);
+		let mut second = transfer_request(&KNOWN_ROOT, &nullifiers_of(&[0xA2]), 0, 1);
+		second.commitments[1] = first.commitments[0];
+
+		let a = super::validate_private_transfer(Some(&proof()), &first).unwrap();
+		let b = super::validate_private_transfer(Some(&proof()), &second).unwrap();
+		let shared: Vec<_> = a
+			.provides
+			.iter()
+			.filter(|t| b.provides.contains(t))
+			.collect();
+		assert_eq!(shared.len(), 1, "exactly the shared output note's tag");
+	});
+}
+
+/// A partial unshield's change note collides with a transfer creating the same
+/// note; a total unshield creates none and tags its nullifier alone.
+#[test]
+fn an_unshield_change_note_is_tagged_like_a_transfer_output() {
+	new_test_ext().execute_with(|| {
+		MerkleRepository::add_historic_poseidon_root::<Test>(KNOWN_ROOT);
+		PoolBalanceRepository::set_asset_balance::<Test>(0, 1_000);
+		let transfer = transfer_request(&KNOWN_ROOT, &nullifiers_of(&[0xB1]), 0, 1);
+		let total = unshield_request(&KNOWN_ROOT, &nullifier(0xB2), 0, 100, 0, 1);
+		let partial = UnshieldRequest {
+			change_commitment: transfer.commitments[0].0,
+			change_memo: full_memo(),
+			..total.clone()
+		};
+
+		let t = super::validate_private_transfer(Some(&proof()), &transfer).unwrap();
+		let total = super::validate_unshield(Some(&proof()), &total).unwrap();
+		let partial = super::validate_unshield(Some(&proof()), &partial).unwrap();
+		assert_eq!(total.provides.len(), 1);
+		assert_eq!(partial.provides.len(), 2);
+		assert!(partial.provides.iter().any(|tag| t.provides.contains(tag)));
+		assert!(!total.provides.iter().any(|tag| t.provides.contains(tag)));
+	});
+}
+
+/// An output tag never equals a nullifier tag, even for the same 32 bytes: a
+/// note created by one spend must not block an unrelated spend of a note.
+#[test]
+fn an_output_tag_never_collides_with_a_nullifier_tag() {
+	new_test_ext().execute_with(|| {
+		MerkleRepository::add_historic_poseidon_root::<Test>(KNOWN_ROOT);
+		PoolBalanceRepository::set_asset_balance::<Test>(0, 1_000);
+		let transfer = transfer_request(&KNOWN_ROOT, &nullifiers_of(&[0xC1]), 0, 1);
+		let unshield = unshield_request(
+			&KNOWN_ROOT,
+			&Nullifier::new(transfer.commitments[0].0),
+			0,
+			100,
+			0,
+			1,
+		);
+
+		let t = super::validate_private_transfer(Some(&proof()), &transfer).unwrap();
+		let u = super::validate_unshield(Some(&proof()), &unshield).unwrap();
+		assert!(!u.provides.iter().any(|tag| t.provides.contains(tag)));
+	});
+}
+
 // ── Solvency arithmetic ──────────────────────────────────────────────────────
 //
 // `amount + fee` is attacker-chosen and summed before the balance compare.

frame/shielded-pool/src/validate_unsigned/transfer.rs

@@ -52,8 +52,7 @@ pub fn validate_private_transfer<T: Config>(
 	// ── 3. Proof ─────────────────────────────────────────────────────────────
 	// Without it, a copy of a pending spend with swapped memos or a higher fee
 	// would share the nullifier tags, outrank the original on priority, replace
-	// it, and then fail in the block — free, repeatable censorship. An invalid
-	// proof from a peer also costs that peer reputation.
+	// it, and then fail in the block — free, repeatable censorship.
 	if let Some(proof) = proof {
 		let valid = ensure_valid_proof::<T>(|| {
 			T::ZkVerifier::verify_transfer_proof(proof, &req.statement(), Some(req.circuit_version))
@@ -91,6 +90,11 @@ pub fn validate_private_transfer<T: Config>(
 	for nullifier in req.real_nullifiers() {
 		builder = builder.and_provides(nullifier);
 	}
+	// Likewise one per output note: two spends creating the same note cannot
+	// both execute.
+	for commitment in req.commitments.iter() {
+		builder = builder.and_provides((super::OUTPUT_TAG, commitment));
+	}
 
 	builder.build()
 }

frame/shielded-pool/src/validate_unsigned/unshield.rs

@@ -51,8 +51,7 @@ pub fn validate_unshield<T: Config>(
 	// ── 3. Proof ─────────────────────────────────────────────────────────────
 	// Without it, a copy of a pending spend with swapped memos or a higher fee
 	// would share the nullifier tag, outrank the original on priority, replace
-	// it, and then fail in the block — free, repeatable censorship. An invalid
-	// proof from a peer also costs that peer reputation.
+	// it, and then fail in the block — free, repeatable censorship.
 	if let Some(proof) = proof {
 		let valid = req.statement().is_ok_and(|statement| {
 			ensure_valid_proof::<T>(|| {
@@ -65,17 +64,20 @@ pub fn validate_unshield<T: Config>(
 		}
 	}
 
-	// ── 4. Pool tag: the nullifier alone (one note, one pool entry) ──────────
+	// ── 4. Pool tags: the nullifier, and the change note if any ──────────────
 	//
 	// Every copy of a spend is mutually exclusive in the pool, whatever else it
 	// changes; with the proof checked, only a valid one gets in. Mirrors
 	// `transfer.rs`.
-	ValidTransaction::with_tag_prefix(super::SPEND_TAG_PREFIX)
+	let mut builder = ValidTransaction::with_tag_prefix(super::SPEND_TAG_PREFIX)
 		.priority(req.fee.saturated_into())
 		.longevity(TX_LONGEVITY)
 		.and_provides(req.nullifier)
-		.propagate(true)
-		.build()
+		.propagate(true);
+	if req.has_change() {
+		builder = builder.and_provides((super::OUTPUT_TAG, req.change_commitment));
+	}
+	builder.build()
 }
 
 /// The pool rejection for a failed unshield check.

template/runtime/RUNTIME_VERSIONS.md

@@ -31,8 +31,11 @@ raw `Ed25519` (`0x00`) never verifies, and Solana wallets and Substrate extensio
 sign through the new `Ed25519Text` (`0x03`) and `Ed25519WrappedText` (`0x04`).
 Circuit keys: unsound ones are recorded on chain and never verify again, a
 removed version keeps its key, and a key without phase 2 is refused.
+Shielded pool: a replaced root stays spendable for a full window, and spends
+creating the same note are exclusive in the pool.
 `transaction_version` is unchanged. Ships with `pallet-evm-precompile-balances`
-0.2.0, `pallet-zk-verifier` 0.19.0 and `orbinum-zk-verifier` 3.4.0.
+0.2.0, `pallet-zk-verifier` 0.19.0, `orbinum-zk-verifier` 3.4.0 and
+`pallet-shielded-pool` 0.25.2.
 
 - **Balances precompile (`0x…0802`)** refuses static calls, delegated calls
   (DELEGATECALL, CALLCODE) and calls carrying value. Under DELEGATECALL the
@@ -127,6 +130,16 @@ removed version keeps its key, and a key without phase 2 is refused.
 - **Weights.** Every proof verification adds one read (the denied-key check);
   `deny_version` is estimated from `retire_version`, and registration and
   removal add the `RemovedVkHashes` read/write, until re-benchmarked.
+- **Replaced roots keep a full window.** A leaf insert records the root it
+  replaces with `RootRetentionBlocks` from now, not from when that root was
+  created. The active root never expires, so a wallet may prove against one
+  older than the window, and the next insert refused that spend at once
+  (`UnknownMerkleRoot`, pool code 1). At most one extra historic-root slot per
+  block, not yet in the benchmarked weights.
+- **Created notes tagged in the pool.** A transfer's outputs and an unshield's
+  change note each `provide` a pool tag, besides the spent nullifiers: two
+  spends creating the same note can no longer both sit in the pool and
+  propagate when only one can execute.
 
 ### spec 19 — tx 7 — 2026-10-13 (`v0.5.0`)