Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Transaction versioning in the RuntimeVersion #5582

Merged
merged 13 commits into from
Apr 17, 2020
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Apr 8, 2020

This just adds the transaction_version field in the RuntimeVersion, which just tracks the compatibility of dispatchables, useful for hardware wallets.

Polkadot companion: paritytech/polkadot#987

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Apr 8, 2020
@gavofyork gavofyork added this to the 2.0 milestone Apr 8, 2020
/// index.
///
/// It need *not* change when a new module is added or when a dispatchable is added.
pub transaction_version: u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to move to the bottom, or we brick everything^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that will not be enough. We need to change the version of the Core runtime api and make sure to put the changed_in attribute above the version function.

The old version function should have the old RuntimeVersion struct as return value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

@xlc
Copy link
Contributor

xlc commented Apr 8, 2020

This is already a transaction version

const TRANSACTION_VERSION: u8 = 4;

And this could fix #3925 ?

@gavofyork
Copy link
Member Author

They are quite different things; the TRANSACTION_VERSION constant refers to the version code placed in each transaction and is the version of the transaction's data format, taking into account any extensions it might have. Different versions imply that transactions cannot be decoded in the same way. The transaction_version field of the RuntimeVersion struct, OTOH, refers to the semantics of the dispatchables. Different versions here imply an specific incompatibility within the dispatchables. Generally this will be the removal of a module or a dispatchable within a module. It could also mean an alteration in the argument list.

The specific semantics are:

  • When removing or changing a dispatchable/pallet (including reordering) that was available within the first released runtime of the current runtimes's transaction version, then transaction_version of the new release must be bumped.
  • When adding a dispatchable/pallet, transaction_version need not be bumped.

In both cases, spec_version must be bumped. This implies that modules which are introduced into a runtime which is released after the first runtime of that transaction_version may actually be changed or removed without bumping the transaction_version. Bumping the transaction_version therefore amounts to "baking in the current set of dispatchables as the API".

@xlc
Copy link
Contributor

xlc commented Apr 11, 2020

I see. It should be possible to create some automation to detect if we need to bump transaction version by comparing metadata from wasm? Manually tracking it is error prone.

Also maybe we need a better name? because have two different thing with exact same name is confusing.

@gavofyork
Copy link
Member Author

Yeah that should be possible to automate; you'd just need to check if there's an alteration/removal to existing pallets, dispatchables, including their docs.

@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 13, 2020
@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 13, 2020
@gavofyork gavofyork requested a review from bkchr April 13, 2020 17:10
@gavofyork gavofyork requested a review from pepyakin as a code owner April 13, 2020 19:56
@gnunicorn
Copy link
Contributor

seems like tests need updating

@gavofyork gavofyork requested a review from tomusdrw as a code owner April 14, 2020 16:26
@gavofyork gavofyork requested a review from NikVolf as a code owner April 14, 2020 21:27
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some test and also found a bug with these tests. I think we should test syncing Kusama and if that works it should be mergable.

@tomusdrw tomusdrw removed their request for review April 15, 2020 07:35
@bkchr bkchr removed their assignment Apr 15, 2020
@gnunicorn gnunicorn added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants