Skip to content

CheckOnlySudoAccount: Provide some tags#7838

Merged
bkchr merged 7 commits into
masterfrom
bkchr-sudo-extension
Mar 10, 2025
Merged

CheckOnlySudoAccount: Provide some tags#7838
bkchr merged 7 commits into
masterfrom
bkchr-sudo-extension

Conversation

@bkchr

@bkchr bkchr commented Mar 6, 2025

Copy link
Copy Markdown
Member

Close: #7816

@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Mar 6, 2025
@bkchr bkchr requested a review from a team as a code owner March 6, 2025 23:29
@bkchr

bkchr commented Mar 6, 2025

Copy link
Copy Markdown
Member Author

/cmd prdoc --audience runtime_user --bump patch

@paritytech-workflow-stopper

Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13710370798
Failed job name: test-linux-stable-int

Comment thread substrate/frame/sudo/src/extension.rs Outdated
Ok((
ValidTransaction {
priority: info.total_weight().ref_time() as TransactionPriority,
provides: vec![(who, call).encode()],

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()],

Comment thread substrate/frame/sudo/src/extension.rs Outdated
Ok((
ValidTransaction {
priority: info.total_weight().ref_time() as TransactionPriority,
provides: vec![(who, call).encode()],

@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.

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).

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).

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

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.

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

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.

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?

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".

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.

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.

@clangenb

clangenb commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

It will only close the #7816 if you also remove the CheckNonce TxExtension from glutton, but it unblocks the issue at least. ;)

@@ -110,6 +111,7 @@ where
Ok((

@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.

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

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

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):

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

Then the account exists and the nonce is checked.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T2-pallets This PR/Issue is related to a particular pallet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[glutton] can't send extrinsics on glutton as accounts do not exist

7 participants