Adds ability to configure default nonce #1557

gupnik · 2023-09-14T05:24:22Z

Fixes https://github.com/paritytech-secops/srlabs_findings/issues/35, closes #326

This PR introduces the ability to configure the default nonce. This will also be used if the account is reaped.

By default, the current behaviour is maintained and nonce starts from 0.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

h4x3rotab · 2023-09-14T08:47:18Z

Why using the block number as the start point of the nonce? If it's used to prevent nonce reset after the account is reaped, then it makes more sense to use the timestample or some larger number. After all, the nonce can be higher than the block number theoretically.

substrate/frame/system/src/lib.rs

xlc · 2023-09-14T09:27:11Z

Oh and make sure you still have tests to ensure the original behaivour is not impacted. Again, not everyone are going to use this and it is essential to the old functionality remains the same and working.

substrate/frame/balances/src/tests/currency_tests.rs

ggwpez · 2023-09-14T10:20:58Z

After all, the nonce can be higher than the block number theoretically.

Sure it could in theory, but this is a best effort approach that should prevent the most likely occurrences.

gupnik · 2023-09-27T04:11:26Z

@ggwpez @kianenigma Can we proceed with this? Haven't heard from @xlc yet on why someone would opt not to use this. Unless we see a clear benefit, I don't think it's worth maintaining this optionality.

xlc · 2023-09-27T04:30:02Z

Please. I don't know why I even need to give you a reason to ask you to not force a breaking change to EVERYONE. Ok, here is one: EVM compatibility.

gupnik · 2023-09-28T04:36:09Z

Please. I don't know why I even need to give you a reason to ask you to not force a breaking change to EVERYONE. Ok, here is one: EVM compatibility.

This is helpful. Thanks!

kianenigma

Let's make this configurable through frame_system so you can express how you wish new account nonces to be created.

…nonce-start

paritytech-cicd-pr · 2024-01-02T06:27:15Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4824508

ggwpez

I just now realize that we can already do this: type Nonce: Default allows us to already set a non-zero nonce upon creation - entirely without this merge request 🙈
But i guess since we often use types like u32, it would not be possible to implement another Default for it, so it should be fine.

prdoc/pr_1557.prdoc

ggwpez · 2024-01-04T11:41:20Z

substrate/frame/system/src/lib.rs

@@ -570,6 +575,9 @@ pub mod pallet {

 		/// The maximum number of consumers allowed on a single account.
 		type MaxConsumers: ConsumerLimits;
+
+		/// The default nonce when an account is created.
+		type DefaultNonce: Get<Self::Nonce>;


Is this intentionally not a #[pallet::constant]?

ggwpez · 2024-01-04T11:48:19Z

substrate/frame/system/src/lib.rs

+type AccountDataOf<T> = <T as Config>::AccountData;
+type AccountInfoOf<T> = AccountInfo<NonceOf<T>, AccountDataOf<T>>;
+
+pub struct GetDefaultAccountInfo<T>(PhantomData<T>);


Suggested change

pub struct GetDefaultAccountInfo<T>(PhantomData<T>);

/// Provides the `Default` for value an account that does not yet exist.

pub struct GetDefaultAccountInfo<T>(PhantomData<T>);

ggwpez · 2024-01-04T11:53:52Z

substrate/frame/system/src/lib.rs

@@ -463,6 +467,7 @@ pub mod pallet {
 		type RuntimeTask: Task;

 		/// This stores the number of previous transactions associated with a sender account.
+		#[pallet::no_default_bounds]


Now there are two notations of default, once from the Default bound here, and one from the DefaultNonce.

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

kianenigma

As @ggwpez said the Default is already pretty flexible, with a newtype that implements Deref { Target = u32 } it might be possibly without any breaking changes? If it works it seems much superior in terms of code design to me.

If not it looks good, but I expect to see system+relay chains to start using this. It is a good idea to expose a standard SetDefaultNonceToBlockNumber for them all to use.

will re-approve once alternative approach is explored.

kianenigma · 2024-04-01T08:44:20Z

@gupnik can we revive this one? 🙈 💀

gupnik · 2024-04-01T12:06:10Z

@gupnik can we revive this one? 🙈 💀

Thanks for the reminder. It's in my backlog but unable to get to it yet. Will revive this soon.

ggwpez · 2024-04-01T16:57:21Z

We dont actually need this, see polkadot-fellows/runtimes#248

gupnik · 2024-04-02T10:53:10Z

We dont actually need this, see polkadot-fellows/runtimes#248

Thanks. Closing this one.

* Unnecessary clone * Removed unused Enum value * Client - ConnectionParams wrapped with Arc + removed unnecessery clone * Client - ConnectionParams wrapped with Arc + removed unnecessery clone

ggwpez and others added 4 commits September 13, 2023 12:31

Use block number as nonce

2ef967f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fixes build

2c33657

Adds test

526d20b

Updates test

f496aab

gupnik added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 14, 2023

gupnik requested review from a team September 14, 2023 05:24

gupnik added 3 commits September 14, 2023 11:16

Uses TryFrom

644b48d

Fixes test

0dbac97

Fixes another test

0444b78

xlc reviewed Sep 14, 2023

View reviewed changes

substrate/frame/system/src/lib.rs Show resolved Hide resolved

substrate/frame/system/src/lib.rs Outdated Show resolved Hide resolved

Fixes another test

010f783

xlc reviewed Sep 14, 2023

View reviewed changes

substrate/frame/balances/src/tests/currency_tests.rs Outdated Show resolved Hide resolved

kianenigma reviewed Oct 3, 2023

View reviewed changes

Merge branch 'master' of github.com:paritytech/polkadot-sdk into oty-…

f7adbe3

…nonce-start

paritytech-review-bot bot requested a review from a team January 2, 2024 06:07

Adds ability to configure default nonce

8db0ef7

gupnik changed the title ~~Starts nonce from the current block number~~ Adds ability to configure default nonce Jan 2, 2024

gupnik added 3 commits January 2, 2024 11:57

Adds PrDoc

7042186

Fmt

624e983

Adds derive_impl in missing places

98e20ed

paritytech-review-bot bot requested a review from a team January 2, 2024 06:31

Fixes typo

7b4feec

ggwpez approved these changes Jan 4, 2024

View reviewed changes

Apply suggestions from code review

0cca9b6

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

kianenigma previously approved these changes Jan 15, 2024

View reviewed changes

kianenigma mentioned this pull request Apr 1, 2024

Consider non-zero default nonce polkadot-fellows/runtimes#248

Open

gupnik closed this Apr 2, 2024

gupnik mentioned this pull request Apr 16, 2024

[WIP] Add default nonce to frame_system::AccountInfo #4159

Closed

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds ability to configure default nonce #1557

Adds ability to configure default nonce #1557

gupnik commented Sep 14, 2023 •

edited by ggwpez

Loading

h4x3rotab commented Sep 14, 2023

xlc commented Sep 14, 2023

ggwpez commented Sep 14, 2023

gupnik commented Sep 27, 2023

xlc commented Sep 27, 2023 •

edited

Loading

gupnik commented Sep 28, 2023

kianenigma left a comment

paritytech-cicd-pr commented Jan 2, 2024

ggwpez left a comment

ggwpez Jan 4, 2024

ggwpez Jan 4, 2024

ggwpez Jan 4, 2024

kianenigma left a comment

kianenigma commented Apr 1, 2024

gupnik commented Apr 1, 2024

ggwpez commented Apr 1, 2024

gupnik commented Apr 2, 2024

	pub struct GetDefaultAccountInfo<T>(PhantomData<T>);
	/// Provides the `Default` for value an account that does not yet exist.
	pub struct GetDefaultAccountInfo<T>(PhantomData<T>);

Adds ability to configure default nonce #1557

Adds ability to configure default nonce #1557

Conversation

gupnik commented Sep 14, 2023 • edited by ggwpez Loading

h4x3rotab commented Sep 14, 2023

xlc commented Sep 14, 2023

ggwpez commented Sep 14, 2023

gupnik commented Sep 27, 2023

xlc commented Sep 27, 2023 • edited Loading

gupnik commented Sep 28, 2023

kianenigma left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Jan 2, 2024

ggwpez left a comment

Choose a reason for hiding this comment

ggwpez Jan 4, 2024

Choose a reason for hiding this comment

ggwpez Jan 4, 2024

Choose a reason for hiding this comment

ggwpez Jan 4, 2024

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma commented Apr 1, 2024

gupnik commented Apr 1, 2024

ggwpez commented Apr 1, 2024

gupnik commented Apr 2, 2024

gupnik commented Sep 14, 2023 •

edited by ggwpez

Loading

xlc commented Sep 27, 2023 •

edited

Loading