refactor: non-default sealing (#1189)

CHANGELOG.md

@@ -31,6 +31,10 @@
   `base_path` field
 - feat(cli): for `run` param `--dev` now imply `--tmp`, as it is in substrate
 - test(starknet-rpc-test): run all tests against a single madara node
+- fix(service): confusing message when node starts (output the actual sealing
+  method being used)
+- refactor(sealing): how the sealing mode is passed into runtime
+- feat(sealing): finalization for instant sealing
 
 ## v0.4.0
 

crates/node/src/chain_spec.rs

@@ -1,6 +1,6 @@
 use std::path::PathBuf;
 
-use madara_runtime::{AuraConfig, EnableManualSeal, GenesisConfig, GrandpaConfig, SystemConfig, WASM_BINARY};
+use madara_runtime::{AuraConfig, GenesisConfig, GrandpaConfig, SealingMode, SystemConfig, WASM_BINARY};
 use mp_felt::Felt252Wrapper;
 use pallet_starknet::genesis_loader::{GenesisData, GenesisLoader, HexFelt};
 use sc_service::{BasePath, ChainType};
@@ -27,18 +27,19 @@ pub type DevChainSpec = sc_service::GenericChainSpec<DevGenesisExt>;
 pub struct DevGenesisExt {
     /// Genesis config.
     genesis_config: GenesisConfig,
-    /// The flag that if enable manual-seal mode.
-    enable_manual_seal: Option<bool>,
+    /// The sealing mode being used.
+    sealing: SealingMode,
 }
 
-/// If enable_manual_seal is true, then the runtime storage variable EnableManualSeal will be set to
-/// true. This is just a common way to pass information from the chain spec to the runtime.
+/// The `sealing` from the `DevGenesisExt` is passed to the runtime via the storage. The runtime
+/// can then use this information to adjust accordingly. This is just a common way to pass
+/// information from the chain spec to the runtime.
+///
+/// NOTE: if `sealing` is `None`, then the runtime will use the default sealing mode.
 impl sp_runtime::BuildStorage for DevGenesisExt {
     fn assimilate_storage(&self, storage: &mut Storage) -> Result<(), String> {
         BasicExternalities::execute_with_storage(storage, || {
-            if let Some(enable_manual_seal) = &self.enable_manual_seal {
-                EnableManualSeal::set(enable_manual_seal);
-            }
+            madara_runtime::Sealing::set(&self.sealing);
         });
         self.genesis_config.assimilate_storage(storage)
     }
@@ -54,7 +55,7 @@ pub fn authority_keys_from_seed(s: &str) -> (AuraId, GrandpaId) {
     (get_from_seed::<AuraId>(s), get_from_seed::<GrandpaId>(s))
 }
 
-pub fn development_config(enable_manual_seal: Option<bool>, base_path: BasePath) -> Result<DevChainSpec, String> {
+pub fn development_config(sealing: SealingMode, base_path: BasePath) -> Result<DevChainSpec, String> {
     let wasm_binary = WASM_BINARY.ok_or_else(|| "Development wasm not available".to_string())?;
     let chain_id = DEV_CHAIN_ID;
 
@@ -78,7 +79,7 @@ pub fn development_config(enable_manual_seal: Option<bool>, base_path: BasePath)
                     vec![authority_keys_from_seed("Alice")],
                     true,
                 ),
-                enable_manual_seal,
+                sealing: sealing.clone(),
             }
         },
         // Bootnodes

crates/node/src/cli.rs

@@ -44,7 +44,7 @@ pub enum Subcommand {
     /// Revert the chain to a previous state.
     Revert(sc_cli::RevertCmd),
 
-    // Setup madara node
+    /// Setup madara node
     Setup(SetupCmd),
 
     /// Try some command against runtime state.

crates/node/src/command.rs

@@ -35,12 +35,12 @@ impl SubstrateCli for Cli {
         2017
     }
 
-    fn load_spec(&self, id: &str) -> Result<Box<dyn sc_service::ChainSpec>, String> {
+    fn load_spec(&self, id: &str) -> Result<Box<dyn ChainSpec>, String> {
         Ok(match id {
             DEV_CHAIN_ID => {
-                let enable_manual_seal = self.run.sealing.map(|_| true);
+                let sealing = self.run.sealing.map(Into::into).unwrap_or_default();
                 let base_path = self.run.base_path().map_err(|e| e.to_string())?;
-                Box::new(chain_spec::development_config(enable_manual_seal, base_path)?)
+                Box::new(chain_spec::development_config(sealing, base_path)?)
             }
             #[cfg(feature = "sharingan")]
             SHARINGAN_CHAIN_ID => Box::new(chain_spec::ChainSpec::from_json_bytes(

crates/node/src/commands/run.rs

@@ -1,20 +1,35 @@
 use std::path::PathBuf;
 
+use madara_runtime::SealingMode;
 use mc_data_availability::DaLayer;
 use sc_cli::{Result, RpcMethods, RunCmd, SubstrateCli};
 use sc_service::BasePath;
+use serde::{Deserialize, Serialize};
 
 use crate::cli::Cli;
 use crate::service;
 
 /// Available Sealing methods.
-#[derive(Debug, Copy, Clone, clap::ValueEnum, Default)]
+#[derive(Debug, Copy, Clone, clap::ValueEnum, Default, Serialize, Deserialize)]
 pub enum Sealing {
-    // Seal using rpc method.
+    /// Seal using rpc method.
     #[default]
     Manual,
-    // Seal when transaction is executed.
+    /// Seal when transaction is executed. This mode does not finalize blocks, if you want to
+    /// finalize blocks use `--sealing=instant-finality`.
     Instant,
+    /// Seal when transaction is executed with finalization.
+    InstantFinality,
+}
+
+impl From<Sealing> for SealingMode {
+    fn from(value: Sealing) -> Self {
+        match value {
+            Sealing::Manual => SealingMode::Manual,
+            Sealing::Instant => SealingMode::Instant { finalize: false },
+            Sealing::InstantFinality => SealingMode::Instant { finalize: true },
+        }
+    }
 }
 
 #[derive(Clone, Debug, clap::Args)]
@@ -63,9 +78,9 @@ pub fn run_node(mut cli: Cli) -> Result<()> {
             None
         }
     };
-    let sealing = cli.run.sealing;
 
     runner.run_node_until_exit(|config| async move {
+        let sealing = cli.run.sealing.map(Into::into).unwrap_or_default();
         service::new_full(config, sealing, da_config).map_err(sc_cli::Error::Service)
     })
 }

crates/node/src/service.rs

@@ -8,9 +8,10 @@ use std::time::Duration;
 
 use futures::channel::mpsc;
 use futures::future;
+use futures::future::BoxFuture;
 use futures::prelude::*;
 use madara_runtime::opaque::Block;
-use madara_runtime::{self, Hash, RuntimeApi, StarknetHasher};
+use madara_runtime::{self, Hash, RuntimeApi, SealingMode, StarknetHasher};
 use mc_block_proposer::ProposerFactory;
 use mc_data_availability::avail::config::AvailConfig;
 use mc_data_availability::avail::AvailClient;
@@ -41,7 +42,6 @@ use sp_offchain::STORAGE_PREFIX;
 use sp_runtime::traits::BlakeTwo256;
 use sp_trie::PrefixedMemoryDB;
 
-use crate::commands::Sealing;
 use crate::genesis_block::MadaraGenesisBlockBuilder;
 use crate::rpc::StarknetDeps;
 use crate::starknet::{db_config_dir, MadaraBackend};
@@ -254,11 +254,11 @@ where
 /// Builds a new service for a full client.
 pub fn new_full(
     config: Configuration,
-    sealing: Option<Sealing>,
+    sealing: SealingMode,
     da_layer: Option<(DaLayer, PathBuf)>,
 ) -> Result<TaskManager, ServiceError> {
     let build_import_queue =
-        if sealing.is_some() { build_manual_seal_import_queue } else { build_aura_grandpa_import_queue };
+        if sealing.is_default() { build_aura_grandpa_import_queue } else { build_manual_seal_import_queue };
 
     let sc_service::PartialComponents {
         client,
@@ -278,9 +278,7 @@ pub fn new_full(
         &config.chain_spec,
     );
 
-    let warp_sync_params = if sealing.is_some() {
-        None
-    } else {
+    let warp_sync_params = if sealing.is_default() {
         net_config
             .add_notification_protocol(sc_consensus_grandpa::grandpa_peers_set_config(grandpa_protocol_name.clone()));
         let warp_sync = Arc::new(sc_consensus_grandpa::warp_proof::NetworkProvider::new(
@@ -289,6 +287,8 @@ pub fn new_full(
             Vec::default(),
         ));
         Some(WarpSyncParams::WithProvider(warp_sync))
+    } else {
+        None
     };
 
     let (network, system_rpc_tx, tx_handler_controller, network_starter, sync_service) =
@@ -311,14 +311,18 @@ pub fn new_full(
     let force_authoring = config.force_authoring;
     let backoff_authoring_blocks: Option<()> = None;
     let name = config.network.node_name.clone();
-    let enable_grandpa = !config.disable_grandpa && sealing.is_none();
+    let enable_grandpa = !config.disable_grandpa && sealing.is_default();
     let prometheus_registry = config.prometheus_registry().cloned();
     let starting_block = client.info().best_number;
 
     // Channel for the rpc handler to communicate with the authorship task.
-    // TODO: commands_stream is is currently unused, but should be used to implement the `sealing`
-    // parameter
-    let (command_sink, commands_stream) = mpsc::channel(1000);
+    let (command_sink, commands_stream) = match sealing {
+        SealingMode::Manual => {
+            let (sender, receiver) = mpsc::channel(1000);
+            (Some(sender), Some(receiver))
+        }
+        _ => (None, None),
+    };
 
     let overrides = overrides_handle(client.clone());
     let starknet_rpc_params = StarknetDeps {
@@ -341,7 +345,7 @@ pub fn new_full(
                 graph: graph.clone(),
                 deny_unsafe,
                 starknet: starknet_rpc_params.clone(),
-                command_sink: if sealing.is_some() { Some(command_sink.clone()) } else { None },
+                command_sink: command_sink.clone(),
             };
             crate::rpc::create_full(deps).map_err(Into::into)
         })
@@ -409,7 +413,9 @@ pub fn new_full(
 
     if role.is_authority() {
         // manual-seal authorship
-        if let Some(sealing) = sealing {
+        if !sealing.is_default() {
+            log::info!("{} sealing enabled.", sealing);
+
             run_manual_seal_authorship(
                 sealing,
                 client,
@@ -423,7 +429,6 @@ pub fn new_full(
 
             network_starter.start_network();
 
-            log::info!("Manual Seal Ready");
             return Ok(task_manager);
         }
 
@@ -533,14 +538,14 @@ pub fn new_full(
 
 #[allow(clippy::too_many_arguments)]
 fn run_manual_seal_authorship(
-    sealing: Sealing,
+    sealing: SealingMode,
     client: Arc<FullClient>,
     transaction_pool: Arc<FullPool<Block, FullClient>>,
     select_chain: FullSelectChain,
     block_import: BoxBlockImport<FullClient>,
     task_manager: &TaskManager,
     prometheus_registry: Option<&Registry>,
-    commands_stream: mpsc::Receiver<sc_consensus_manual_seal::rpc::EngineCommand<Hash>>,
+    commands_stream: Option<mpsc::Receiver<sc_consensus_manual_seal::rpc::EngineCommand<Hash>>>,
 ) -> Result<(), ServiceError>
 where
     RuntimeApi: ConstructRuntimeApi<Block, FullClient>,
@@ -586,30 +591,36 @@ where
         Ok(timestamp)
     };
 
-    let manual_seal = match sealing {
-        Sealing::Manual => future::Either::Left(sc_consensus_manual_seal::run_manual_seal(
-            sc_consensus_manual_seal::ManualSealParams {
+    let manual_seal: BoxFuture<_> = match sealing {
+        SealingMode::Manual => {
+            Box::pin(sc_consensus_manual_seal::run_manual_seal(sc_consensus_manual_seal::ManualSealParams {
                 block_import,
                 env: proposer_factory,
                 client,
                 pool: transaction_pool,
-                commands_stream,
+                commands_stream: commands_stream.expect("Manual sealing requires a channel from RPC."),
                 select_chain,
                 consensus_data_provider: None,
                 create_inherent_data_providers,
-            },
-        )),
-        Sealing::Instant => future::Either::Right(sc_consensus_manual_seal::run_instant_seal(
-            sc_consensus_manual_seal::InstantSealParams {
+            }))
+        }
+        SealingMode::Instant { finalize } => {
+            let instant_seal_params = sc_consensus_manual_seal::InstantSealParams {
                 block_import,
                 env: proposer_factory,
                 client,
                 pool: transaction_pool,
                 select_chain,
                 consensus_data_provider: None,
                 create_inherent_data_providers,
-            },
-        )),
+            };
+            if finalize {
+                Box::pin(sc_consensus_manual_seal::run_instant_seal_and_finalize(instant_seal_params))
+            } else {
+                Box::pin(sc_consensus_manual_seal::run_instant_seal(instant_seal_params))
+            }
+        }
+        _ => unreachable!("Other sealing modes are not expected in manual-seal."),
     };
 
     // we spawn the future on a background thread managed by service.

crates/runtime/Cargo.toml

@@ -18,6 +18,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 [dependencies]
 parity-scale-codec = { workspace = true, features = [] }
 scale-info = { workspace = true, features = [] }
+serde = { workspace = true }
 
 sp-api = { workspace = true }
 sp-block-builder = { workspace = true }

crates/runtime/src/config.rs

@@ -6,6 +6,9 @@ pub use frame_support::weights::constants::{
 pub use frame_support::weights::{IdentityFee, Weight};
 pub use frame_support::{construct_runtime, parameter_types, StorageValue};
 pub use frame_system::Call as SystemCall;
+use parity_scale_codec::{Decode, Encode};
+use serde::{Deserialize, Serialize};
+use sp_core::RuntimeDebug;
 use sp_runtime::create_runtime_str;
 #[cfg(any(feature = "std", test))]
 pub use sp_runtime::BuildStorage;
@@ -77,7 +80,36 @@ parameter_types! {
     pub const SS58Prefix: u8 = 42;
 }
 
+/// The current sealing mode being used. This is needed for the runtime to adjust its behavior
+/// accordingly, e.g. suppress Aura validations in `OnTimestampSet` for manual or instant sealing.
+#[derive(Default, Clone, PartialEq, Decode, Encode, RuntimeDebug, Deserialize, Serialize)]
+pub enum SealingMode {
+    #[default]
+    Default,
+    Manual,
+    Instant {
+        finalize: bool,
+    },
+}
+
+impl SealingMode {
+    pub fn is_default(&self) -> bool {
+        matches!(self, SealingMode::Default)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::fmt::Display for SealingMode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            SealingMode::Default => write!(f, "Default"),
+            SealingMode::Manual => write!(f, "Manual"),
+            SealingMode::Instant { finalize } => write!(f, "Instant (finalize: {})", finalize),
+        }
+    }
+}
+
 // This storage item will be used to check if we are in the manual sealing mode
 parameter_types! {
-    pub storage EnableManualSeal: bool = false;
+    pub storage Sealing: SealingMode = SealingMode::default();
 }

crates/runtime/src/pallets.rs

@@ -143,7 +143,7 @@ impl pallet_grandpa::Config for Runtime {
 /// --------------------------------------
 
 /// Timestamp manipulation.
-/// For instance, we need it to set the timestamp of the Starkknet block.
+/// For instance, we need it to set the timestamp of the Starknet block.
 impl pallet_timestamp::Config for Runtime {
     /// A timestamp: milliseconds since the unix epoch.
     type Moment = u64;
@@ -163,11 +163,11 @@ parameter_types! {
 }
 
 /// Implement the OnTimestampSet trait to override the default Aura.
-/// This is needed to support manual sealing.
+/// This is needed to suppress Aura validations in case of non-default sealing.
 pub struct ConsensusOnTimestampSet<T>(PhantomData<T>);
 impl<T: pallet_aura::Config> OnTimestampSet<T::Moment> for ConsensusOnTimestampSet<T> {
     fn on_timestamp_set(moment: T::Moment) {
-        if EnableManualSeal::get() {
+        if Sealing::get() != SealingMode::Default {
             return;
         }
         <pallet_aura::Pallet<T> as OnTimestampSet<T::Moment>>::on_timestamp_set(moment)