feat(node): Make sure pallet_gear::run() pseudo-inherent only runs …

…once per block (#3135)

gclient/src/api/listener/iterator.rs

@@ -90,15 +90,15 @@ impl EventProcessor for EventListener {
 
 impl EventListener {
     /// Look through finalized blocks to find the
-    /// [`QueueProcessingReverted`](https://docs.gear.rs/pallet_gear/pallet/enum.Event.html#variant.QueueProcessingReverted)
+    /// [`QueueNotProcessed`](https://docs.gear.rs/pallet_gear/pallet/enum.Event.html#variant.QueueNotProcessed)
     /// event.
     pub async fn queue_processing_reverted(&mut self) -> Result<H256> {
         while let Some(events) = self.0.next_events().await {
             let events = events?;
             let events_bh = events.block_hash();
 
             if let Some(res) = self.proc_events_inner(events, |e| {
-                matches!(e, Event::Gear(GearEvent::QueueProcessingReverted)).then_some(events_bh)
+                matches!(e, Event::Gear(GearEvent::QueueNotProcessed)).then_some(events_bh)
             }) {
                 return Ok(res);
             }

gsdk/src/metadata/generated.rs

@@ -2711,6 +2711,9 @@ pub mod runtime_types {
                     #[codec(index = 14)]
                     #[doc = "Voucher can't be redemmed"]
                     FailureRedeemingVoucher,
+                    #[codec(index = 15)]
+                    #[doc = "Gear::run() already included in current block."]
+                    GearRunAlreadyInBlock,
                 }
                 #[derive(Debug, crate::gp::Decode, crate::gp::DecodeAsType, crate::gp::Encode)]
                 #[doc = "\n\t\t\tThe [event](https://docs.substrate.io/main-docs/build/events-errors/) emitted\n\t\t\tby this pallet.\n\t\t\t"]
@@ -2796,7 +2799,7 @@ pub mod runtime_types {
                     },
                     #[codec(index = 8)]
                     #[doc = "The pseudo-inherent extrinsic that runs queue processing rolled back or not executed."]
-                    QueueProcessingReverted,
+                    QueueNotProcessed,
                     #[codec(index = 9)]
                     #[doc = "Program resume session has been started."]
                     ProgramResumeSessionStarted {
@@ -10167,15 +10170,15 @@ pub mod storage {
     pub enum GearStorage {
         ExecuteInherent,
         BlockNumber,
-        LastGearBlockNumber,
+        GearRunInBlock,
     }
     impl StorageInfo for GearStorage {
         const PALLET: &'static str = "Gear";
         fn storage_name(&self) -> &'static str {
             match self {
                 Self::ExecuteInherent => "ExecuteInherent",
                 Self::BlockNumber => "BlockNumber",
-                Self::LastGearBlockNumber => "LastGearBlockNumber",
+                Self::GearRunInBlock => "GearRunInBlock",
             }
         }
     }

node/authorship/Cargo.toml

@@ -55,7 +55,7 @@ pallet-timestamp = { workspace = true, features = ["std"] }
 pallet-balances  = { workspace = true, features = ["std"] }
 pallet-gear = { workspace = true, features = ["std"] }
 pallet-gear-messenger  = { workspace = true, features = ["std"] }
-testing.workspace = true
+testing = {workspace = true, features = ["vara-native"] }
 vara-runtime  = { workspace = true, features = ["std"] }
 demo-mul-by-const.workspace = true
 env_logger.workspace = true

node/authorship/src/tests.rs

@@ -50,7 +50,7 @@ use testing::{
     client::{ClientBlockImportExt, TestClientBuilder, TestClientBuilderExt},
     keyring::{alice, bob, sign, signed_extra, CheckedExtrinsic},
 };
-use vara_runtime::{AccountId, Runtime, RuntimeCall, SLOT_DURATION, VERSION};
+use vara_runtime::{AccountId, Runtime, RuntimeCall, UncheckedExtrinsic, SLOT_DURATION, VERSION};
 
 const SOURCE: TransactionSource = TransactionSource::External;
 
@@ -631,3 +631,107 @@ fn block_max_gas_works() {
     // 2 out of 5 messages have been processed, 3 remain in the queue
     assert_eq!(queue_len, 3);
 }
+
+#[test]
+fn terminal_extrinsic_discarded_from_txpool() {
+    init_logger();
+
+    let client_builder = TestClientBuilder::new()
+        .set_execution_strategy(sc_client_api::ExecutionStrategy::NativeWhenPossible);
+    let mut client = Arc::new(client_builder.build());
+    let spawner = sp_core::testing::TaskExecutor::new();
+    let txpool = BasicPool::new_full(
+        Default::default(),
+        true.into(),
+        None,
+        spawner.clone(),
+        client.clone(),
+    );
+
+    let genesis_hash =
+        <[u8; 32]>::try_from(&client.info().best_hash[..]).expect("H256 is a 32 byte type");
+
+    // Create Gear::run() extrinsic - both unsigned and signed
+    let unsigned_gear_run_xt =
+        UncheckedExtrinsic::new_unsigned(RuntimeCall::Gear(pallet_gear::Call::run {
+            max_gas: None,
+        }));
+    let signed_gear_run_xt = sign(
+        CheckedExtrinsic {
+            signed: Some((bob(), signed_extra(0))),
+            function: RuntimeCall::Gear(pallet_gear::Call::run { max_gas: None }),
+        },
+        VERSION.spec_version,
+        VERSION.transaction_version,
+        genesis_hash,
+    );
+    // A `DispatchClass::Normal` exrinsic - supposed to end up in the txpool
+    let legit_xt = sign(
+        CheckedExtrinsic {
+            signed: Some((alice(), signed_extra(0))),
+            function: pre_fund_bank_account_call(),
+        },
+        VERSION.spec_version,
+        VERSION.transaction_version,
+        genesis_hash,
+    );
+
+    let extrinsics = vec![
+        unsigned_gear_run_xt.into(),
+        signed_gear_run_xt.into(),
+        legit_xt.into(),
+    ];
+
+    // Attempt to submit extrinsics to txpool; expecting only one of the three to be validated
+    block_on(txpool.submit_at(&BlockId::number(0), SOURCE, extrinsics)).unwrap();
+
+    let new_header = client
+        .block_hash_from_id(&BlockId::Number(0_u32))
+        .unwrap()
+        .unwrap();
+    block_on(
+        txpool.maintain(chain_event(
+            client
+                .header(new_header)
+                .expect("header get error")
+                .expect("there should be header"),
+        )),
+    );
+
+    let mut proposer_factory =
+        ProposerFactory::new(spawner, client.clone(), txpool, None, None, None);
+
+    let timestamp_provider = sp_timestamp::InherentDataProvider::from_system_time();
+
+    let proposer = block_on(
+        proposer_factory.init(
+            &client
+                .header(new_header)
+                .expect("Database error querying block #0")
+                .expect("Block #0 should exist"),
+        ),
+    )
+    .expect("Proposer initialization failed");
+
+    let inherent_data =
+        block_on(timestamp_provider.create_inherent_data()).expect("Create inherent data failed");
+    let time_slot = sp_timestamp::Timestamp::current().as_millis() / SLOT_DURATION;
+
+    let proposal = block_on(proposer.propose(
+        inherent_data,
+        pre_digest(time_slot, 0),
+        time::Duration::from_secs(600),
+        None,
+    ))
+    .unwrap();
+
+    // Both mandatory extrinsics should have been discarded, therefore there are only 3 txs
+    // in the block: 1 timestamp inherent + 1 normal extrinsic + 1 terminal
+    assert_eq!(proposal.block.extrinsics().len(), 3);
+
+    // Importing block #1
+    block_on(client.import(BlockOrigin::Own, proposal.block.clone())).unwrap();
+
+    let best_hash = client.info().best_hash;
+    assert_eq!(best_hash, proposal.block.hash());
+}

pallets/gear-debug/src/mock.rs

@@ -263,7 +263,7 @@ pub fn run_to_block(n: u64, remaining_weight: Option<u64>) {
         assert!(!System::events().iter().any(|e| {
             matches!(
                 e.event,
-                RuntimeEvent::Gear(pallet_gear::Event::QueueProcessingReverted)
+                RuntimeEvent::Gear(pallet_gear::Event::QueueNotProcessed)
             )
         }))
     }

pallets/gear-scheduler/src/mock.rs

@@ -263,7 +263,7 @@ pub fn run_to_block(n: u64, remaining_weight: Option<u64>) {
         assert!(!System::events().iter().any(|e| {
             matches!(
                 e.event,
-                RuntimeEvent::Gear(pallet_gear::Event::QueueProcessingReverted)
+                RuntimeEvent::Gear(pallet_gear::Event::QueueNotProcessed)
             )
         }))
     }

pallets/gear/src/lib.rs

@@ -406,7 +406,7 @@ pub mod pallet {
         },
 
         /// The pseudo-inherent extrinsic that runs queue processing rolled back or not executed.
-        QueueProcessingReverted,
+        QueueNotProcessed,
 
         /// Program resume session has been started.
         ProgramResumeSessionStarted {
@@ -469,6 +469,8 @@ pub mod pallet {
         ProgramNotFound,
         /// Voucher can't be redemmed
         FailureRedeemingVoucher,
+        /// Gear::run() already included in current block.
+        GearRunAlreadyInBlock,
     }
 
     #[cfg(feature = "runtime-benchmarks")]
@@ -497,13 +499,13 @@ pub mod pallet {
         }
     }
 
-    /// The Gear block number before processing messages.
+    /// A guard to prohibit all but the first execution of `pallet_gear::run()` call in a block.
     ///
-    /// A helper variable that mirrors the `BlockNumber` at the beginning of a block.
-    /// Allows to gauge the actual `BlockNumber` progress.
+    /// Set to `Some(())` if the extrinsic is executed for the first time in a block.
+    /// All subsequent attempts would fail with `Error::<T>::GearRunAlreadyInBlock` error.
+    /// Set back to `None` in the `on_finalize()` hook at the end of the block.
     #[pallet::storage]
-    #[pallet::getter(fn last_gear_block_number)]
-    pub(crate) type LastGearBlockNumber<T: Config> = StorageValue<_, T::BlockNumber>;
+    pub(crate) type GearRunInBlock<T> = StorageValue<_, ()>;
 
     #[pallet::hooks]
     impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T>
@@ -521,30 +523,21 @@ pub mod pallet {
             // Incrementing Gear block number
             BlockNumber::<T>::mutate(|bn| *bn = bn.saturating_add(One::one()));
 
-            // Align the last gear block number before inherent execution with current value
-            <LastGearBlockNumber<T>>::put(Self::block_number());
-
             log::debug!(target: "gear::runtime", "⚙️  Initialization of block #{bn:?} (gear #{:?})", Self::block_number());
 
-            // The `LastGearBlockNumber` is killed at the end of a block therefore its value
-            // will only exist in overlay and never be committed to storage.
-            // The respective write weight can be omitted and not accounted for.
             T::DbWeight::get().writes(1)
         }
 
         /// Finalization
         fn on_finalize(bn: BlockNumberFor<T>) {
-            // Check if the queue has been processed, that is gear block number bumped again.
-            // If not (while the queue processing enabled), fire an event.
-            if let Some(last_gear_block_number) = <LastGearBlockNumber<T>>::take() {
-                if Self::execute_inherent() && Self::block_number() <= last_gear_block_number {
-                    Self::deposit_event(Event::QueueProcessingReverted);
-                }
+            // Check if the queue has been processed.
+            // If not (while the queue processing enabled), fire an event and revert
+            // the Gear internal block number increment made in `on_initialize()`.
+            if GearRunInBlock::<T>::take().is_none() && Self::execute_inherent() {
+                Self::deposit_event(Event::QueueNotProcessed);
+                BlockNumber::<T>::mutate(|bn| *bn = bn.saturating_sub(One::one()));
             }
 
-            // Undoing Gear block number increment that was done upfront in `on_initialize`
-            BlockNumber::<T>::mutate(|bn| *bn = bn.saturating_sub(One::one()));
-
             log::debug!(target: "gear::runtime", "⚙️  Finalization of block #{bn:?} (gear #{:?})", Self::block_number());
         }
     }
@@ -1753,6 +1746,15 @@ pub mod pallet {
                 Error::<T>::MessageQueueProcessingDisabled
             );
 
+            ensure!(
+                !GearRunInBlock::<T>::exists(),
+                Error::<T>::GearRunAlreadyInBlock
+            );
+            // The below doesn't create an extra db write, because the value will be "taken"
+            // (set to `None`) at the end of the block, therefore, will only exist in the
+            // overlay and never be committed to storage.
+            GearRunInBlock::<T>::set(Some(()));
+
             let weight_used = <frame_system::Pallet<T>>::block_weight();
             let max_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
             let remaining_weight = max_weight.saturating_sub(weight_used.total());
@@ -1779,12 +1781,6 @@ pub mod pallet {
                 Self::block_number(),
             );
 
-            // Incrementing Gear block number one more time to indicate that the queue processing
-            // has been successfully completed in the current block.
-            // Caveat: this increment must be done strictly after all extrinsics in the block have
-            // been handled to ensure correct block number math.
-            BlockNumber::<T>::mutate(|bn| *bn = bn.saturating_add(One::one()));
-
             Ok(PostDispatchInfo {
                 actual_weight: Some(
                     Weight::from_parts(actual_weight, 0)

pallets/gear/src/mock.rs

@@ -388,7 +388,7 @@ pub fn run_to_block_maybe_with_queue(
             assert!(!System::events().iter().any(|e| {
                 matches!(
                     e.event,
-                    RuntimeEvent::Gear(pallet_gear::Event::QueueProcessingReverted)
+                    RuntimeEvent::Gear(pallet_gear::Event::QueueNotProcessed)
                 )
             }))
         }

pallets/gear/src/tests.rs

@@ -13761,6 +13761,57 @@ fn wasm_ref_types_doesnt_work() {
     });
 }
 
+/// Test that the `Gear::run()` extrinsic can only run once per block,
+/// even if somehow included in a block multiple times.
+#[test]
+fn gear_run_only_runs_once_per_block() {
+    use frame_support::{
+        dispatch::RawOrigin,
+        traits::{OnFinalize, OnInitialize},
+    };
+
+    fn init_block(bn: u32) {
+        System::set_block_number(bn);
+        GasAllowanceOf::<Test>::put(1_000_000_000);
+        Gear::on_initialize(bn);
+    }
+
+    init_logger();
+    new_test_ext().execute_with(|| {
+        init_block(2);
+        assert_ok!(Gear::run(RawOrigin::None.into(), None,));
+        // Second run in a block is not allowed
+        assert_noop!(
+            Gear::run(RawOrigin::None.into(), None,),
+            Error::<Test>::GearRunAlreadyInBlock
+        );
+        Gear::on_finalize(2);
+
+        // Everything goes back to normal in the next block
+        init_block(3);
+        assert_ok!(Gear::run(RawOrigin::None.into(), None,));
+    })
+}
+
+/// Test that the Gear internal block numbering is consistent.
+#[test]
+fn gear_block_number_math_adds_up() {
+    init_logger();
+    new_test_ext().execute_with(|| {
+        run_to_block(100, None);
+        assert_eq!(Gear::block_number(), 100);
+
+        run_to_block_maybe_with_queue(120, None, None);
+        assert_eq!(System::block_number(), 120);
+        assert_eq!(Gear::block_number(), 100);
+
+        System::reset_events();
+        run_to_block(150, None);
+        assert_eq!(System::block_number(), 150);
+        assert_eq!(Gear::block_number(), 130);
+    })
+}
+
 mod utils {
     #![allow(unused)]
 

utils/node-loader/src/batch_pool.rs

@@ -358,7 +358,7 @@ async fn inspect_crash_events(mut rx: EventsReceiver) -> Result<()> {
         let bh = events.block_hash();
         for event in events.iter() {
             let event = event?.as_root_event::<gsdk::metadata::Event>()?;
-            if matches!(event, Event::Gear(GearEvent::QueueProcessingReverted)) {
+            if matches!(event, Event::Gear(GearEvent::QueueNotProcessed)) {
                 let crash_err = CrashAlert::MsgProcessingStopped;
                 tracing::info!("{crash_err} at block hash {bh:?}");
                 return Err(crash_err.into());