feat: Configurable mortal extrinsic construction #204

emostov · 2020-12-14T04:14:38Z

Closes #202

public api updates

This PR allows for users to configure both mortal and immortal extrinsics through a new SignedOptions struct, where they can pass in the desired era_period for a mortal ext or None for immortal ext. I chose this design because I believe it will make it easier to append more options, such as tip (#187), with minimal maintenance.

This PR changes default extrinsic construction to Era::mortal, where the mortal_period defaults to 64 blocks. The mortal_period can be set via the client method set_mortal_period, where None indicates an immortal transaction.

This PR also exposes a DEFAULT_MORTAL_PERIOD to give users a reasonable starting point.

review notes

~~If there is no immediate major grumbles I go ahead and update tests~~ No need to update tests

src/lib.rs

niklasad1

Looks overall good but I don't know the rationale why mortal transactions wasn't implemented before

dvdplm

Good stuff here. My main question here is whether we should do more with the proc macro and ask users to annotate their structs with an attribute somehow, e.g.:

#[derive(Call)]
pub struct MyVeryOwnCall<'a, T: Balances> {
  pub to: &'a <T as System>::Address,
  pub amount: T::Balance,
  [#Call(signed_options)]
  pub era_period: u64, 
}

In other words, piggy back on the args parsing machinery we already have and add some annotation to guide its usage? Or maybe I'm completely wrong here and all extrinsics need the era info anyway?

proc-macro/src/call.rs

src/extrinsic/extra.rs

src/extrinsic/mod.rs

src/lib.rs

ascjones

I suggest moving the era into the Signer, in the same way that it handles the nonce. Instead of having this extra options argument to pass around.

Alternatively, or in addition to the above, this option and other similar could be provided as part of the Client, so would get applied to all calls from that client instance (if such an option is likely to be the same across all transactions).

ascjones · 2020-12-14T09:55:56Z

Good stuff here. My main question here is whether we should do more with the proc macro and ask users to annotate their structs with an attribute somehow, e.g.:
#[derive(Call)]
pub struct MyVeryOwnCall<'a, T: Balances> {
  pub to: &'a <T as System>::Address,
  pub amount: T::Balance,
  [#Call(signed_options)]
  pub era_period: u64, 
}
In other words, piggy back on the args parsing machinery we already have and add some annotation to guide its usage? Or maybe I'm completely wrong here and all extrinsics need the era info anyway?

I think it's good enough to have the global default and leave it up to the callers at runtime to configure the options per call.

emostov · 2020-12-14T19:05:46Z

I suggest moving the era into the Signer, in the same way that it handles the nonce. Instead of having this extra options argument to pass around.

Alternatively, or in addition to the above, this option and other similar could be provided as part of the Client, so would get applied to all calls from that client instance (if such an option is likely to be the same across all transactions).

I agree this should be moved to the Signer or Client as it would likely be the same across transactions for users. I think of mortality preferences being something non-specific to account, so in that sense I think it makes sense to put on the Client - but I think both setups would work.

emostov · 2020-12-14T22:48:41Z

@niklasad1, @dvdplm, @ascjones Updated: mortal_period now lives on the client. I think ready for another round of reviews

src/lib.rs

src/extrinsic/mod.rs

niklasad1 · 2020-12-23T10:32:59Z

src/metadata.rs

@@ -165,6 +171,28 @@ impl Metadata {
        }
        string
    }
+
+    /// Derive a mortal period
+    pub(crate) fn derive_mortal_period(&self) -> Result<u64, MetadataError> {


I was thinking a little bit about the concrete/hardcoded types u32 and u64 here which might not work for all runtimes. Ideally, we would read the types from the runtime but it seems quite complicated to get it to work and pallet-babe is not implemented for substrate-subxt. Thus, I think it's best to leave this out of this PR and open an issue or something.

However, for example if the runtime type parameter was passed in here we could do:

/// Derive a mortal period pub(crate) fn derive_mortal_period<T: Runtime>(&self) -> Result<TODO, MetadataError> { let block_hash_count = self .module("System") .and_then(|sys| sys.constant("BlockHashCount")) .and_then(|count| count.value::<<T as System>::BlockNumber>())?; let expected_block_time = self .module("Babe") .and_then(|babe| babe.constant("ExpectedBlockTime")) // babe-pallet trait doesn't exist yet.... .and_then(|e| e.value::<<T as Babe>::ExpectedBlockTime>())?; // maybe convert by try_into or something. match expected_block_time { 0 => Err(MetadataError::ZeroValue("Babe::ExpectedBlockTime")), expected_block_time => { Ok((BASELINE_MORTAL_PERIOD / expected_block_time) .next_power_of_two() .min(block_hash_count.next_power_of_two())) } } }

//cc @ascjones

As we speak I am working on upgrading substrate metadata to use scale-info for full type information and once we have that we should be able to do runtime specific builds for subxt

niklasad1 · 2020-12-23T10:37:42Z

src/metadata.rs

+            .map(Into::into)?;
+        let expected_block_time = self
+            .module("Babe")
+            .and_then(|babe| babe.constant("ExpectedBlockTime"))


Is it fair to assume that BABE is used for all runtimes?

No, I think Aura is popular as well... should look into a more generic solution

I think we can use TimeStamp::MinimumPeriod and double it

https://github.com/paritytech/substrate/blob/332399d16668a6c769f1a7db154bb9ea3b50e61c/frame/timestamp/src/lib.rs#L129-L133

src/lib.rs

niklasad1 · 2020-12-23T10:47:40Z

src/lib.rs

+        let mortal_period = if let Some(period) = self.mortal_period {
+            period
+        } else {
+            match metadata.derive_mortal_period() {


I had in mind to pass mortal_period into derive_mortal_period in order to check if it was valid but I have no strong opinions on it.

Honestly it is not even clear to me what would be an invalid mortal period a user could set. I think we could check if mortal_period <= BlockHashCount.next_power_of_two(), but the tx will still be processed by the node with larger values. Knowing that, it seems artificially limiting, especially if for some reason a runtime follows a different rule.

Fair enough, I was thinking of the initial annoyance, if the mortal_period > BlockHashCount

niklasad1 · 2020-12-23T10:54:36Z

Is this overkill?

It is fine from my side but you might want to print the actual error because it might fail for other reasons than mortal_period :)

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

…rate-subxt into zeke-mortal-ext

src/lib.rs

src/metadata.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

…rate-subxt into zeke-mortal-ext

dvdplm

I'm a bit out of my depth here, but left a few comments and questions.

src/metadata.rs

src/lib.rs

dvdplm · 2020-12-29T16:30:18Z

src/extrinsic/extra.rs

        (
            CheckSpecVersion(PhantomData, self.spec_version),
            CheckTxVersion(PhantomData, self.tx_version),
            CheckGenesis(PhantomData, self.genesis_hash),
-            CheckEra((Era::Immortal, PhantomData), self.genesis_hash),
+            CheckEra((self.era_info.0, PhantomData), era_hash),


This is a bit clunky. Ideally we'd have CheckEra(PhantomData, self.era_info) but that seems tricky to make work?

The inner tuple is what actually gets encoded, while the era_hash is not part of the CheckEra struct in Substrate and is just used here for implementing additional_signed. We could make a function like create_check_era(PhantomData, self.era_info), but it seems like that would just create unnecessary code.

Substrate FRAME nodes do not need the hash because they have a map of canon block number => block hash, which we do not have

Co-authored-by: David <dvdplm@gmail.com>

niklasad1

LGTM

ascjones · 2021-02-11T10:51:23Z

What's the status of this? Waiting for a review from @dvdplm?

dvdplm

Better late than never?

ascjones · 2021-07-15T07:11:28Z

Just needs merge conflicts fixing now

ascjones · 2022-01-26T09:54:00Z

Not sure as to the mergability of this since the recent refactoring. Certainly we need to functionality still.

ascjones · 2022-03-31T10:39:03Z

I believe this has now been superseded by #490 /cc @jsdw

feat: Mortal extrinsic construction

c9059d1

emostov added the enhancement New feature or request label Dec 14, 2020

emostov added 2 commits December 13, 2020 20:34

Remove unused proc macro token stuff

085eb5e

Remove TODO

00078ac

emostov requested review from ascjones and dvdplm December 14, 2020 04:36

Remove stray comment

a989932

dvdplm requested a review from niklasad1 December 14, 2020 07:04

niklasad1 reviewed Dec 14, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 14, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 14, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 14, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 14, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 14, 2020

View reviewed changes

dvdplm reviewed Dec 14, 2020

View reviewed changes

ascjones reviewed Dec 14, 2020

View reviewed changes

emostov added 5 commits December 14, 2020 11:34

Remove poorly panickable unwraps

633bd0f

Have create_signed() Pass era enum

47911b8

Update to set mortal_period in client

eef1929

Trivial cleaning

dc782e9

nightly fmt

ff265ec

Update comment

8f11c42

emostov requested review from niklasad1, dvdplm and ascjones December 14, 2020 23:38

niklasad1 reviewed Dec 15, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 15, 2020

View reviewed changes

src/extrinsic/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 23, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 23, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 23, 2020

View reviewed changes

emostov and others added 3 commits December 23, 2020 08:08

Update src/lib.rs

ecd674f

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

Use timestamp to calc block time; Misc. review resp.

fe8b441

Merge branch 'zeke-mortal-ext' of https://github.com/paritytech/subst…

6908adb

…rate-subxt into zeke-mortal-ext

niklasad1 reviewed Dec 24, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Dec 24, 2020

View reviewed changes

src/metadata.rs Show resolved Hide resolved

emostov and others added 5 commits December 24, 2020 10:05

Update src/lib.rs

d3b6971

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

Add MortalPeriod enum

7cae5d2

Merge branch 'zeke-mortal-ext' of https://github.com/paritytech/subst…

a8bbec4

…rate-subxt into zeke-mortal-ext

Refine enum usage

30a282f

Rename var

ab1cb3c

dvdplm reviewed Dec 29, 2020

View reviewed changes

emostov and others added 2 commits December 29, 2020 10:30

Apply suggestions from code review

900340f

Co-authored-by: David <dvdplm@gmail.com>

Address David feedback

ce36380

niklasad1 reviewed Jan 6, 2021

View reviewed changes

emostov requested a review from dvdplm January 25, 2021 02:33

paritytech deleted a comment from cla-bot-2021 bot Jun 3, 2021

dvdplm approved these changes Jul 14, 2021

View reviewed changes

h4x3rotab mentioned this pull request Nov 12, 2021

Additional parameters for SignedExtra #322

Merged

conectado mentioned this pull request Dec 15, 2021

Adds option to pass a mortality parameter to create_signed #371

Closed

ascjones closed this Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Configurable mortal extrinsic construction #204

feat: Configurable mortal extrinsic construction #204

emostov commented Dec 14, 2020 •

edited

Loading

niklasad1 left a comment •

edited

Loading

dvdplm left a comment

ascjones left a comment •

edited

Loading

ascjones commented Dec 14, 2020

emostov commented Dec 14, 2020 •

edited

Loading

emostov commented Dec 14, 2020 •

edited

Loading

niklasad1 Dec 23, 2020 •

edited

Loading

ascjones Jan 6, 2021

niklasad1 Dec 23, 2020

emostov Dec 23, 2020 •

edited

Loading

emostov Dec 23, 2020

niklasad1 Dec 23, 2020

emostov Dec 24, 2020

niklasad1 Dec 24, 2020

niklasad1 commented Dec 23, 2020

dvdplm left a comment

dvdplm Dec 29, 2020

emostov Dec 29, 2020

niklasad1 left a comment

ascjones commented Feb 11, 2021

dvdplm left a comment

ascjones commented Jul 15, 2021

ascjones commented Jan 26, 2022

ascjones commented Mar 31, 2022

feat: Configurable mortal extrinsic construction #204

feat: Configurable mortal extrinsic construction #204

Conversation

emostov commented Dec 14, 2020 • edited Loading

public api updates

review notes

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

ascjones left a comment • edited Loading

Choose a reason for hiding this comment

ascjones commented Dec 14, 2020

emostov commented Dec 14, 2020 • edited Loading

emostov commented Dec 14, 2020 • edited Loading

niklasad1 Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emostov Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Dec 23, 2020

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

ascjones commented Feb 11, 2021

dvdplm left a comment

Choose a reason for hiding this comment

ascjones commented Jul 15, 2021

ascjones commented Jan 26, 2022

ascjones commented Mar 31, 2022

emostov commented Dec 14, 2020 •

edited

Loading

niklasad1 left a comment •

edited

Loading

ascjones left a comment •

edited

Loading

emostov commented Dec 14, 2020 •

edited

Loading

emostov commented Dec 14, 2020 •

edited

Loading

niklasad1 Dec 23, 2020 •

edited

Loading

emostov Dec 23, 2020 •

edited

Loading