Skip to content

CheckOnlySudoAccount: Provide some tags #7838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bkchr merged 7 commits into from
Mar 10, 2025
Merged

CheckOnlySudoAccount: Provide some tags #7838

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dccf002
`CheckOnlySudoAccount`: Provide some tags
bkchr Mar 6, 2025
1904e0b
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] Mar 6, 2025
4d63c36
Update pr_7838.prdoc
bkchr Mar 6, 2025
8688eb2
vec
bkchr Mar 6, 2025
87043d7
Merge remote-tracking branch 'refs/remotes/origin/bkchr-sudo-extension'
bkchr Mar 6, 2025
58de5fc
Review comments
bkchr Mar 7, 2025
91fecc8
Remove `CheckNonce`
bkchr Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions prdoc/pr_7838.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
title: '`CheckOnlySudoAccount`: Provide some tags'
doc:
- audience: Runtime User
description: Let `CheckOnlySudoAccount` provide some tags to make the tx pool happy.
crates:
- name: pallet-sudo
bump: patch
6 changes: 4 additions & 2 deletions substrate/frame/sudo/src/extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// limitations under the License.

use crate::{Config, Key};
use alloc::vec;
use codec::{Decode, DecodeWithMemTracking, Encode};
use core::{fmt, marker::PhantomData};
use frame_support::{dispatch::DispatchInfo, ensure, pallet_prelude::TransactionSource};
Expand All @@ -31,7 +32,7 @@ use sp_runtime::{

/// Ensure that signed transactions are only valid if they are signed by sudo account.
///
/// In the initial phase of a chain without any tokens you can not prevent accounts from sending
/// In the initial phase of a chain without any tokens you cannot prevent accounts from sending
/// transactions.
/// These transactions would enter the transaction pool as the succeed the validation, but would
/// fail on applying them as they are not allowed/disabled/whatever. This would be some huge dos
Expand Down Expand Up @@ -89,7 +90,7 @@ where
fn validate(
&self,
origin: <<T as frame_system::Config>::RuntimeCall as Dispatchable>::RuntimeOrigin,
_call: &<T as frame_system::Config>::RuntimeCall,
call: &<T as frame_system::Config>::RuntimeCall,
info: &DispatchInfoOf<<T as frame_system::Config>::RuntimeCall>,
_len: usize,
_self_implicit: Self::Implicit,
Expand All @@ -110,6 +111,7 @@ where
Ok((

@gui1117 gui1117 Mar 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could also add a sufficient reference to the signer here automatically. Sounds cleaner than removing CheckNonce.

@bkchr bkchr Mar 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What you mean by this?

@gui1117 gui1117 Mar 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We remove CheckNonce because CheckNonce requires the account to exist.

But if we put CheckOnlySudoAccount before CheckNonce and CheckOnlySudoAccount creates the account with inc_sufficient (if it doesn't exist yet):

polkadot-sdk/substrate/frame/system/src/lib.rs

Line 1656 in 007565a

pub fn inc_sufficients(who: &T::AccountId) -> IncRefStatus {

Then the account exists and the nonce is checked.

@bkchr bkchr Mar 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And when is dec_sufficients called? :D

@gui1117 gui1117 Mar 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes true forget it :-), he could live forever

ValidTransaction {
priority: info.total_weight().ref_time() as TransactionPriority,
provides: vec![(who, call).encode()],

@gui1117 gui1117 Mar 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sometimes tag are prefixed to avoid collision, but here it is not necessary.

Suggested change
provides: vec![(who, call).encode()],
provides: vec![("Sudo", who, call).encode()],

@michalkucharczyk michalkucharczyk Mar 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dq: is call guaranteed to be unique here?
Otherwise we won't be able to send numerous sudo calls (with the same operation, e.g.: I can imagine someone calling same sudo operation few times like transfer funds).

@clangenb clangenb Mar 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAIU it is not, but for chains that also have the CheckNonce extension there is another unique tag provided. A chain with only the CheckOnlySudoAccount extension but without CheckNonce will have to work around this uniqueness guarantee (e.g., batch transactions).

@s0me0ne-unkn0wn s0me0ne-unkn0wn Mar 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dq: is call guaranteed to be unique here?
Otherwise we won't be able to send numerous sudo calls

Fair enough. Does it help, say, to add the block number here?

@clangenb clangenb Mar 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, not really I suppose, as it is going to be the identical too for the duration of one block. If the inherents are available, we could add the timestamp though.

@s0me0ne-unkn0wn s0me0ne-unkn0wn Mar 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as it is going to be the identical too for the duration of one block

True, but being able to make the same sudo calls once per block is better than not being able to do them at all 🤔

@michalkucharczyk michalkucharczyk Mar 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

but for chains that also have the CheckNonce

So I assume that chains w/o CheckNonce are rather for development purpose. And typically runtime should provide other mean to distinguish transaction sent from the same account, not necessarily responsibility of the sudo itself.

We could use hash of call to avoid sending all the bytes across a runtime boundary, also txpool will have smaller amount of data to track.

@bkchr bkchr Mar 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can imagine someone calling same sudo operation few times like transfer funds

Always to the same address?

@bkchr bkchr Mar 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Someone would need to send the exact same call multiple times. Just sending multiple transfer calls is not the same. Each call has the arguments directly "baked in".

@michalkucharczyk michalkucharczyk Mar 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I meant exactly the same call - for transfer that would be same amount, same address.

@bkchr bkchr Mar 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see this happening quite unlikely.

..Default::default()
},
(),
Expand Down