fix: prevent duplicate inherent execution within same block

changes/changed/fix-prevent-duplicate-inherent-execution.md

@@ -0,0 +1,9 @@
+#node
+# Prevent duplicate inherent execution within same block
+
+Add InherentExecutedThisBlock guard to cnight-observation and federated-authority-observation
+pallets to ensure inherents can only execute once per block. This prevents potential issues
+from multiple inherent calls being processed in a single block.
+
+PR: https://github.com/midnightntwrk/midnight-node/pull/575
+JIRA: https://shielded.atlassian.net/browse/PM-21649

pallets/cnight-observation/src/lib.rs

@@ -150,6 +150,8 @@ pub mod pallet {
 		MaxCardanoAddrLengthExceeded,
 		MaxRegistrationsExceeded,
 		LedgerApiError(LedgerApiError),
+		/// Only one inherent is allowed per block
+		InherentAlreadyExecuted,
 	}
 
 	impl<T: Config> From<LedgerApiError> for Error<T> {
@@ -213,6 +215,9 @@ pub mod pallet {
 	pub type CardanoTxCapacityPerBlock<T: Config> =
 		StorageValue<_, u32, ValueQuery, DefaultCardanoTxCapacityPerBlock>;
 
+	#[pallet::storage]
+	pub type InherentExecutedThisBlock<T: Config> = StorageValue<_, bool, ValueQuery>;
+
 	#[pallet::genesis_config]
 	#[derive(frame_support::DefaultNoBound)]
 	pub struct GenesisConfig<T: Config> {
@@ -502,6 +507,18 @@ pub mod pallet {
 		}
 	}
 
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
+			// Pre-account for on_finalize weight (storage write to reset inherent flag)
+			T::DbWeight::get().writes(1)
+		}
+
+		fn on_finalize(_n: BlockNumberFor<T>) {
+			InherentExecutedThisBlock::<T>::kill();
+		}
+	}
+
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		#[pallet::call_index(0)]
@@ -512,6 +529,8 @@ pub mod pallet {
 			next_cardano_position: CardanoPosition,
 		) -> DispatchResult {
 			ensure_none(origin)?;
+			ensure!(!InherentExecutedThisBlock::<T>::get(), Error::<T>::InherentAlreadyExecuted);
+			InherentExecutedThisBlock::<T>::put(true);
 
 			let mut events: Vec<CNightGeneratesDustEventSerialized> = Vec::new();
 

pallets/cnight-observation/tests/tests.rs

@@ -11,8 +11,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 use frame_support::{
-	assert_ok, inherent::InherentData, pallet_prelude::*, sp_runtime::traits::Dispatchable,
+	assert_noop, assert_ok, inherent::InherentData, pallet_prelude::*,
+	sp_runtime::traits::Dispatchable, traits::Hooks,
 };
+use frame_system::RawOrigin;
 use midnight_node_ledger::latest::types::BlockContext;
 use midnight_node_ledger_helpers::{
 	CNightGeneratesDustActionType, CNightGeneratesDustEvent, DefaultDB, DustPublicKey,
@@ -147,6 +149,13 @@ fn any_event<F: Fn(&RuntimeEvent) -> bool>(f: F) -> bool {
 	System::events().iter().any(|r| f(&r.event))
 }
 
+fn advance_block_and_reset_events() {
+	CNightObservation::on_finalize(System::block_number());
+	System::set_block_number(System::block_number() + 1);
+	frame_system::Pallet::<Test>::reset_events();
+	CNightObservation::on_initialize(System::block_number());
+}
+
 #[test]
 fn asset_create_should_emit_valid_event_if_registered() {
 	new_test_ext().execute_with(|| {
@@ -376,9 +385,7 @@ fn removing_duplicate_registration_results_in_valid_registration() {
 		let call = RuntimeCall::CNightObservation(call);
 		assert_ok!(call.dispatch(frame_system::RawOrigin::None.into()));
 
-		// Advance block and clear events
-		System::set_block_number(System::block_number() + 1);
-		frame_system::Pallet::<Test>::reset_events();
+		advance_block_and_reset_events();
 
 		let reg_header = test_header(4, 2, 0, None);
 
@@ -409,9 +416,7 @@ fn removing_duplicate_registration_results_in_valid_registration() {
 		// Registration is not emitted when a duplicate is received
 		assert!(!registration_found);
 
-		// Advance block and clear events
-		System::set_block_number(System::block_number() + 1);
-		frame_system::Pallet::<Test>::reset_events();
+		advance_block_and_reset_events();
 
 		let dereg_header = test_header(5, 0, 0, Some(tx_hash(4, 2)));
 
@@ -608,8 +613,7 @@ fn emits_deregistration_and_mapping_removed_on_last_mapping_removed() {
 		let call = RuntimeCall::CNightObservation(call);
 		assert_ok!(call.dispatch(frame_system::RawOrigin::None.into()));
 
-		System::set_block_number(System::block_number() + 1);
-		frame_system::Pallet::<Test>::reset_events();
+		advance_block_and_reset_events();
 
 		// make the removal UTXO reference the registration UTXO so the MappingEntry matches
 		let dereg_header = test_header(21, 0, 0, Some(reg_header.utxo_tx_hash));
@@ -1212,3 +1216,48 @@ fn emits_deregistration_and_mapping_removed_on_last_mapping_removed() {
 // 		assert_eq!(len_after_removal, None, "Key removed entirely from storage");
 // 	});
 // }
+
+#[test]
+fn duplicate_inherent_protection_works() {
+	new_test_ext().execute_with(|| {
+		init_ledger_state();
+		let (cardano_reward_address, dust_public_key) = test_wallet_pairing();
+
+		let utxos = vec![ObservedUtxo {
+			header: test_header(1, 2, 0, None),
+			data: ObservedUtxoData::Registration(RegistrationData {
+				cardano_reward_address,
+				dust_public_key: dust_public_key.clone(),
+			}),
+		}];
+
+		// First call succeeds
+		let inherent_data = create_inherent(utxos.clone(), test_position(3, 0));
+		let call = CNightObservation::create_inherent(&inherent_data).unwrap();
+		assert_ok!(RuntimeCall::CNightObservation(call).dispatch(RawOrigin::None.into()));
+
+		// Second call in same block fails
+		let call2 = Call::process_tokens {
+			utxos: utxos.clone(),
+			next_cardano_position: test_position(3, 0),
+		};
+		assert_noop!(
+			RuntimeCall::CNightObservation(call2).dispatch(RawOrigin::None.into()),
+			Error::<Test>::InherentAlreadyExecuted
+		);
+
+		advance_block_and_reset_events();
+
+		// Third call in new block succeeds
+		let utxos2 = vec![ObservedUtxo {
+			header: test_header(4, 0, 0, None),
+			data: ObservedUtxoData::Registration(RegistrationData {
+				cardano_reward_address,
+				dust_public_key,
+			}),
+		}];
+		let inherent_data2 = create_inherent(utxos2, test_position(5, 0));
+		let call3 = CNightObservation::create_inherent(&inherent_data2).unwrap();
+		assert_ok!(RuntimeCall::CNightObservation(call3).dispatch(RawOrigin::None.into()));
+	});
+}

pallets/federated-authority-observation/src/lib.rs

@@ -79,6 +79,9 @@ pub mod pallet {
 	pub type TechnicalCommitteeMainchainMembers<T: Config> =
 		StorageValue<_, BoundedVec<MainchainMember, T::TechnicalCommitteeMaxMembers>, ValueQuery>;
 
+	#[pallet::storage]
+	pub type InherentExecutedThisBlock<T: Config> = StorageValue<_, bool, ValueQuery>;
+
 	#[pallet::config]
 	pub trait Config: frame_system::Config {
 		/// The MAX number of members for the Council
@@ -163,10 +166,21 @@ pub mod pallet {
 		EmptyMembers,
 		/// Duplicate Members
 		DuplicatedMembers,
+		/// Only one inherent is allowed per block
+		InherentAlreadyExecuted,
 	}
 
 	#[pallet::hooks]
-	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
+			// Pre-account for on_finalize weight (storage write to reset inherent flag)
+			T::DbWeight::get().writes(1)
+		}
+
+		fn on_finalize(_n: BlockNumberFor<T>) {
+			InherentExecutedThisBlock::<T>::kill();
+		}
+	}
 
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
@@ -186,6 +200,8 @@ pub mod pallet {
 			>,
 		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
+			ensure!(!InherentExecutedThisBlock::<T>::get(), Error::<T>::InherentAlreadyExecuted);
+			InherentExecutedThisBlock::<T>::put(true);
 
 			let (mut council_members, council_mainchain_members): (Vec<_>, Vec<_>) =
 				council_authorities.clone().into_iter().unzip();