Address some issues around tx mortality (#2025)

* Clarify mortality config

* Fix EncodeAsType / DecodeAsType impls for Era to be proper ones

* Fix test now that txs are mortal by default

* clippy

* missing imports

* allow Era decoding from CheckMortality

* tweak a comment

* fmt

* Add more explicit tests for mortality

core/src/config/default_extrinsic_params.rs

@@ -2,7 +2,9 @@
 // This file is dual-licensed as Apache-2.0 or GPL-3.0.
 // see LICENSE for license details.
 
-use super::Config;
+use crate::config::transaction_extensions::CheckMortalityParams;
+
+use super::{Config, HashFor};
 use super::{ExtrinsicParams, transaction_extensions};
 
 /// The default [`super::ExtrinsicParams`] implementation understands common signed extensions
@@ -26,8 +28,8 @@ pub type DefaultExtrinsicParams<T> = transaction_extensions::AnyOf<
 /// [`DefaultExtrinsicParams`]. This may expose methods that aren't applicable to the current
 /// chain; such values will simply be ignored if so.
 pub struct DefaultExtrinsicParamsBuilder<T: Config> {
-    /// `None` means the tx will be immortal, else it's mortal for N blocks (if possible).
-    mortality: Option<u64>,
+    /// `None` means the tx will be immortal, else it's mortality is described.
+    mortality: transaction_extensions::CheckMortalityParams<T>,
     /// `None` means the nonce will be automatically set.
     nonce: Option<u64>,
     /// `None` means we'll use the native token.
@@ -39,7 +41,7 @@ pub struct DefaultExtrinsicParamsBuilder<T: Config> {
 impl<T: Config> Default for DefaultExtrinsicParamsBuilder<T> {
     fn default() -> Self {
         Self {
-            mortality: None,
+            mortality: CheckMortalityParams::default(),
             tip: 0,
             tip_of: 0,
             tip_of_asset_id: None,
@@ -55,10 +57,43 @@ impl<T: Config> DefaultExtrinsicParamsBuilder<T> {
         Default::default()
     }
 
+    /// Make the transaction immortal, meaning it will never expire. This means that it could, in
+    /// theory, be pending for a long time and only be included many blocks into the future.
+    pub fn immortal(mut self) -> Self {
+        self.mortality = transaction_extensions::CheckMortalityParams::immortal();
+        self
+    }
+
     /// Make the transaction mortal, given a number of blocks it will be mortal for from
     /// the current block at the time of submission.
+    ///
+    /// # Warning
+    ///
+    /// This will ultimately return an error if used for creating extrinsic offline, because we need
+    /// additional information in order to set the mortality properly.
+    ///
+    /// When creating offline transactions, you must use [`Self::mortal_from_unchecked`] instead to set
+    /// the mortality. This provides all of the necessary information which we must otherwise be online
+    /// in order to obtain.
     pub fn mortal(mut self, for_n_blocks: u64) -> Self {
-        self.mortality = Some(for_n_blocks);
+        self.mortality = transaction_extensions::CheckMortalityParams::mortal(for_n_blocks);
+        self
+    }
+
+    /// Configure a transaction that will be mortal for the number of blocks given, and from the
+    /// block details provided. Prefer to use [`Self::mortal()`] where possible, which prevents
+    /// the block number and hash from being misaligned.
+    pub fn mortal_from_unchecked(
+        mut self,
+        for_n_blocks: u64,
+        from_block_n: u64,
+        from_block_hash: HashFor<T>,
+    ) -> Self {
+        self.mortality = transaction_extensions::CheckMortalityParams::mortal_from_unchecked(
+            for_n_blocks,
+            from_block_n,
+            from_block_hash,
+        );
         self
     }
 
@@ -88,11 +123,7 @@ impl<T: Config> DefaultExtrinsicParamsBuilder<T> {
 
     /// Build the extrinsic parameters.
     pub fn build(self) -> <DefaultExtrinsicParams<T> as ExtrinsicParams<T>>::Params {
-        let check_mortality_params = if let Some(for_n_blocks) = self.mortality {
-            transaction_extensions::CheckMortalityParams::mortal(for_n_blocks)
-        } else {
-            transaction_extensions::CheckMortalityParams::immortal()
-        };
+        let check_mortality_params = self.mortality;
 
         let charge_asset_tx_params = if let Some(asset_id) = self.tip_of_asset_id {
             transaction_extensions::ChargeAssetTxPaymentParams::tip_of(self.tip, asset_id)

core/src/config/transaction_extensions.rs

@@ -295,6 +295,15 @@ impl<T: Config> ExtrinsicParams<T> for CheckMortality<T> {
     type Params = CheckMortalityParams<T>;
 
     fn new(client: &ClientState<T>, params: Self::Params) -> Result<Self, ExtrinsicParamsError> {
+        // If a user has explicitly configured the transaction to be mortal for n blocks, but we get
+        // to this stage and no injected information was able to turn this into MortalFromBlock{..},
+        // then we hit an error as we are unable to construct a mortal transaction here.
+        if matches!(&params.0, CheckMortalityParamsInner::MortalForBlocks(_)) {
+            return Err(ExtrinsicParamsError::custom(
+                "CheckMortality: We cannot construct an offline extrinsic with only the number of blocks it is mortal for. Use mortal_from_unchecked instead.",
+            ));
+        }
+
         Ok(CheckMortality {
             // if nothing has been explicitly configured, we will have a mortal transaction
             // valid for 32 blocks if block info is available.
@@ -347,8 +356,15 @@ impl<T: Config> TransactionExtension<T> for CheckMortality<T> {
 pub struct CheckMortalityParams<T: Config>(CheckMortalityParamsInner<T>);
 
 enum CheckMortalityParamsInner<T: Config> {
+    /// The transaction will be immortal.
     Immortal,
+    /// The transaction is mortal for N blocks. This must be "upgraded" into
+    /// [`CheckMortalityParamsInner::MortalFromBlock`] to ultimately work.
     MortalForBlocks(u64),
+    /// The transaction is mortal for N blocks, but if it cannot be "upgraded",
+    /// then it will be set to immortal instead. This is the default if unset.
+    MortalForBlocksOrImmortalIfNotPossible(u64),
+    /// The transaction is mortal and all of the relevant information is provided.
     MortalFromBlock {
         for_n_blocks: u64,
         from_block_n: u64,
@@ -358,8 +374,8 @@ enum CheckMortalityParamsInner<T: Config> {
 
 impl<T: Config> Default for CheckMortalityParams<T> {
     fn default() -> Self {
-        // default to being mortal for 32 blocks if possible:
-        CheckMortalityParams(CheckMortalityParamsInner::MortalForBlocks(32))
+        // default to being mortal for 32 blocks if possible, else immortal:
+        CheckMortalityParams(CheckMortalityParamsInner::MortalForBlocksOrImmortalIfNotPossible(32))
     }
 }
 
@@ -392,7 +408,8 @@ impl<T: Config> CheckMortalityParams<T> {
 impl<T: Config> Params<T> for CheckMortalityParams<T> {
     fn inject_block(&mut self, from_block_n: u64, from_block_hash: HashFor<T>) {
         match &self.0 {
-            CheckMortalityParamsInner::MortalForBlocks(n) => {
+            CheckMortalityParamsInner::MortalForBlocks(n)
+            | CheckMortalityParamsInner::MortalForBlocksOrImmortalIfNotPossible(n) => {
                 self.0 = CheckMortalityParamsInner::MortalFromBlock {
                     for_n_blocks: *n,
                     from_block_n,

core/src/error.rs

@@ -208,29 +208,20 @@ pub enum ExtrinsicParamsError {
     UnknownTransactionExtension(String),
     /// Some custom error.
     #[error("Error constructing extrinsic parameters: {0}")]
-    Custom(Box<dyn CustomError>),
+    Custom(Box<dyn core::error::Error + Send + Sync + 'static>),
 }
 
-/// Anything implementing this trait can be used in [`ExtrinsicParamsError::Custom`].
-#[cfg(feature = "std")]
-pub trait CustomError: std::error::Error + Send + Sync + 'static {}
-#[cfg(feature = "std")]
-impl<T: std::error::Error + Send + Sync + 'static> CustomError for T {}
-
-/// Anything implementing this trait can be used in [`ExtrinsicParamsError::Custom`].
-#[cfg(not(feature = "std"))]
-pub trait CustomError: core::fmt::Debug + core::fmt::Display + Send + Sync + 'static {}
-#[cfg(not(feature = "std"))]
-impl<T: core::fmt::Debug + core::fmt::Display + Send + Sync + 'static> CustomError for T {}
+impl ExtrinsicParamsError {
+    /// Create a custom [`ExtrinsicParamsError`] from a string.
+    pub fn custom<S: Into<String>>(error: S) -> Self {
+        let error: String = error.into();
+        let error: Box<dyn core::error::Error + Send + Sync + 'static> = Box::from(error);
+        ExtrinsicParamsError::Custom(error)
+    }
+}
 
 impl From<core::convert::Infallible> for ExtrinsicParamsError {
     fn from(value: core::convert::Infallible) -> Self {
         match value {}
     }
 }
-
-impl From<Box<dyn CustomError>> for ExtrinsicParamsError {
-    fn from(value: Box<dyn CustomError>) -> Self {
-        ExtrinsicParamsError::Custom(value)
-    }
-}

core/src/utils/era.rs

@@ -2,7 +2,13 @@
 // This file is dual-licensed as Apache-2.0 or GPL-3.0.
 // see LICENSE for license details.
 
-use scale_decode::DecodeAsType;
+use alloc::{format, vec::Vec};
+use codec::{Decode, Encode};
+use scale_decode::{
+    IntoVisitor, TypeResolver, Visitor,
+    ext::scale_type_resolver,
+    visitor::{TypeIdFor, types::Composite, types::Variant},
+};
 use scale_encode::EncodeAsType;
 
 // Dev note: This and related bits taken from `sp_runtime::generic::Era`
@@ -16,8 +22,6 @@ use scale_encode::EncodeAsType;
     Debug,
     serde::Serialize,
     serde::Deserialize,
-    DecodeAsType,
-    EncodeAsType,
     scale_info::TypeInfo,
 )]
 pub enum Era {
@@ -105,3 +109,126 @@ impl codec::Decode for Era {
         }
     }
 }
+
+/// Define manually how to encode an Era given some type information. Here we
+/// basically check that the type we're targeting is called "Era" and then codec::Encode.
+impl EncodeAsType for Era {
+    fn encode_as_type_to<R: TypeResolver>(
+        &self,
+        type_id: R::TypeId,
+        types: &R,
+        out: &mut Vec<u8>,
+    ) -> Result<(), scale_encode::Error> {
+        // Visit the type to check that it is an Era. This is only a rough check.
+        let visitor = scale_type_resolver::visitor::new((), |_, _| false)
+            .visit_variant(|_, path, _variants| path.last() == Some("Era"));
+
+        let is_era = types
+            .resolve_type(type_id.clone(), visitor)
+            .unwrap_or_default();
+        if !is_era {
+            return Err(scale_encode::Error::custom_string(format!(
+                "Type {type_id:?} is not a valid Era type; expecting either Immortal or MortalX variant"
+            )));
+        }
+
+        // if the type looks valid then just scale encode our Era.
+        self.encode_to(out);
+        Ok(())
+    }
+}
+
+/// Define manually how to decode an Era given some type information. Here we check that the
+/// variant we're decoding is one of the expected Era variants, and that the field is correct if so,
+/// ensuring that this will fail if trying to decode something that isn't an Era.
+pub struct EraVisitor<R>(core::marker::PhantomData<R>);
+
+impl IntoVisitor for Era {
+    type AnyVisitor<R: TypeResolver> = EraVisitor<R>;
+    fn into_visitor<R: TypeResolver>() -> Self::AnyVisitor<R> {
+        EraVisitor(core::marker::PhantomData)
+    }
+}
+
+impl<R: TypeResolver> Visitor for EraVisitor<R> {
+    type Value<'scale, 'resolver> = Era;
+    type Error = scale_decode::Error;
+    type TypeResolver = R;
+
+    // Unwrap any newtype wrappers around the era, eg the CheckMortality extension (which actually
+    // has 2 fields, but scale_info seems to autoamtically ignore the PhantomData field). This
+    // allows us to decode directly from CheckMortality into Era.
+    fn visit_composite<'scale, 'resolver>(
+        self,
+        value: &mut Composite<'scale, 'resolver, Self::TypeResolver>,
+        _type_id: TypeIdFor<Self>,
+    ) -> Result<Self::Value<'scale, 'resolver>, Self::Error> {
+        if value.remaining() != 1 {
+            return Err(scale_decode::Error::custom_string(format!(
+                "Expected any wrapper around Era to have exactly one field, but got {} fields",
+                value.remaining()
+            )));
+        }
+
+        value
+            .decode_item(self)
+            .expect("1 field expected; checked above.")
+    }
+
+    fn visit_variant<'scale, 'resolver>(
+        self,
+        value: &mut Variant<'scale, 'resolver, Self::TypeResolver>,
+        _type_id: TypeIdFor<Self>,
+    ) -> Result<Self::Value<'scale, 'resolver>, Self::Error> {
+        let variant = value.name();
+
+        // If the variant is immortal, we know the outcome.
+        if variant == "Immortal" {
+            return Ok(Era::Immortal);
+        }
+
+        // Otherwise, we expect a variant Mortal1..Mortal255 where the number
+        // here is the first byte, and the second byte is conceptually a field of this variant.
+        // This weird encoding is because the Era is compressed to just 1 byte if immortal and
+        // just 2 bytes if mortal.
+        //
+        // Note: We _could_ just assume we'll have 2 bytes to work with and decode the era directly,
+        // but checking the variant names ensures that the thing we think is an Era actually _is_
+        // one, based on the type info for it.
+        let first_byte = variant
+            .strip_prefix("Mortal")
+            .and_then(|s| s.parse::<u8>().ok())
+            .ok_or_else(|| {
+                scale_decode::Error::custom_string(format!(
+                    "Expected MortalX variant, but got {variant}"
+                ))
+            })?;
+
+        // We need 1 field in the MortalN variant containing the second byte.
+        let mortal_fields = value.fields();
+        if mortal_fields.remaining() != 1 {
+            return Err(scale_decode::Error::custom_string(format!(
+                "Expected Mortal{} to have one u8 field, but got {} fields",
+                first_byte,
+                mortal_fields.remaining()
+            )));
+        }
+
+        let second_byte = mortal_fields
+            .decode_item(u8::into_visitor())
+            .expect("At least one field should exist; checked above.")
+            .map_err(|e| {
+                scale_decode::Error::custom_string(format!(
+                    "Expected mortal variant field to be u8, but: {e}"
+                ))
+            })?;
+
+        // Now that we have both bytes we can decode them into the era using
+        // the same logic as the codec::Decode impl does.
+        Era::decode(&mut &[first_byte, second_byte][..]).map_err(|e| {
+            scale_decode::Error::custom_string(format!(
+                "Failed to codec::Decode Era from Mortal bytes: {e}"
+            ))
+        })
+    }
+}

testing/integration-tests/src/full_client/blocks.rs

@@ -261,47 +261,49 @@ async fn fetch_block_and_decode_extrinsic_details() {
     }
 }
 
+/// A helper function to submit a transaction with some params and then get it back in a block,
+/// so that we can test the decoding of it.
+async fn submit_extrinsic_and_get_it_back(
+    api: &subxt::OnlineClient<SubstrateConfig>,
+    params: subxt::config::DefaultExtrinsicParamsBuilder<SubstrateConfig>,
+) -> subxt::blocks::ExtrinsicDetails<SubstrateConfig, subxt::OnlineClient<SubstrateConfig>> {
+    let alice = dev::alice();
+    let bob = dev::bob();
+
+    let tx = node_runtime::tx()
+        .balances()
+        .transfer_allow_death(bob.public_key().into(), 10_000);
+
+    let signed_extrinsic = api
+        .tx()
+        .create_signed(&tx, &alice, params.build())
+        .await
+        .unwrap();
+
+    let in_block = signed_extrinsic
+        .submit_and_watch()
+        .await
+        .unwrap()
+        .wait_for_finalized()
+        .await
+        .unwrap();
+
+    let block_hash = in_block.block_hash();
+    let block = api.blocks().at(block_hash).await.unwrap();
+    let extrinsics = block.extrinsics().await.unwrap();
+    let extrinsic_details = extrinsics.iter().find(|e| e.is_signed()).unwrap();
+    extrinsic_details
+}
+
 #[cfg(fullclient)]
 #[subxt_test]
 async fn decode_transaction_extensions_from_blocks() {
     let ctx = test_context().await;
     let api = ctx.client();
-    let alice = dev::alice();
-    let bob = dev::bob();
 
-    macro_rules! submit_transfer_extrinsic_and_get_it_back {
-        ($tip:expr) => {{
-            let tx = node_runtime::tx()
-                .balances()
-                .transfer_allow_death(bob.public_key().into(), 10_000);
-
-            let signed_extrinsic = api
-                .tx()
-                .create_signed(
-                    &tx,
-                    &alice,
-                    DefaultExtrinsicParamsBuilder::new().tip($tip).build(),
-                )
-                .await
-                .unwrap();
-
-            let in_block = signed_extrinsic
-                .submit_and_watch()
-                .await
-                .unwrap()
-                .wait_for_finalized()
-                .await
-                .unwrap();
-
-            let block_hash = in_block.block_hash();
-            let block = api.blocks().at(block_hash).await.unwrap();
-            let extrinsics = block.extrinsics().await.unwrap();
-            let extrinsic_details = extrinsics.iter().find(|e| e.is_signed()).unwrap();
-            extrinsic_details
-        }};
-    }
-
-    let transaction1 = submit_transfer_extrinsic_and_get_it_back!(1234);
+    let transaction1 =
+        submit_extrinsic_and_get_it_back(&api, DefaultExtrinsicParamsBuilder::new().tip(1234))
+            .await;
     let extensions1 = transaction1.transaction_extensions().unwrap();
 
     let nonce1 = extensions1.nonce().unwrap();
@@ -313,7 +315,9 @@ async fn decode_transaction_extensions_from_blocks() {
         .unwrap()
         .tip();
 
-    let transaction2 = submit_transfer_extrinsic_and_get_it_back!(5678);
+    let transaction2 =
+        submit_extrinsic_and_get_it_back(&api, DefaultExtrinsicParamsBuilder::new().tip(5678))
+            .await;
     let extensions2 = transaction2.transaction_extensions().unwrap();
     let nonce2 = extensions2.nonce().unwrap();
     let nonce2_static = extensions2.find::<CheckNonce>().unwrap().unwrap();
@@ -368,13 +372,69 @@ async fn decode_transaction_extensions_from_blocks() {
     {
         assert_eq!(e.name(), *expected_name);
     }
+}
 
-    // check that era decodes:
-    for extensions in [&extensions1, &extensions2] {
-        let era: Era = extensions
+#[cfg(fullclient)]
+#[subxt_test]
+async fn decode_block_mortality() {
+    let ctx = test_context().await;
+    let api = ctx.client();
+
+    // Explicit Immortal:
+    {
+        let tx =
+            submit_extrinsic_and_get_it_back(&api, DefaultExtrinsicParamsBuilder::new().immortal())
+                .await;
+
+        let mortality = tx
+            .transaction_extensions()
+            .unwrap()
             .find::<CheckMortality<SubstrateConfig>>()
             .unwrap()
             .unwrap();
-        assert_eq!(era, Era::Immortal)
+
+        assert_eq!(mortality, Era::Immortal);
+    }
+
+    // Explicit Mortal:
+    for for_n_blocks in [4, 16, 128] {
+        let tx = submit_extrinsic_and_get_it_back(
+            &api,
+            DefaultExtrinsicParamsBuilder::new().mortal(for_n_blocks),
+        )
+        .await;
+
+        let mortality = tx
+            .transaction_extensions()
+            .unwrap()
+            .find::<CheckMortality<SubstrateConfig>>()
+            .unwrap()
+            .unwrap();
+
+        assert!(matches!(mortality, Era::Mortal {
+            period,
+            phase: _, // depends on current block so don't test it.
+        } if period == for_n_blocks));
+    }
+
+    // Implicitly, transactions should be mortal:
+    {
+        let tx =
+            submit_extrinsic_and_get_it_back(&api, DefaultExtrinsicParamsBuilder::default()).await;
+
+        let mortality = tx
+            .transaction_extensions()
+            .unwrap()
+            .find::<CheckMortality<SubstrateConfig>>()
+            .unwrap()
+            .unwrap();
+
+        assert!(matches!(
+            mortality,
+            Era::Mortal {
+                period: 32,
+                phase: _, // depends on current block so don't test it.
+            }
+        ));
     }
 }