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

frame_system::remark: Allow any kind of origin#14260

Merged
bkchr merged 2 commits intomasterfrom
bkchr-remark-every-origin
Jun 4, 2023
Merged

frame_system::remark: Allow any kind of origin#14260
bkchr merged 2 commits intomasterfrom
bkchr-remark-every-origin

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented May 29, 2023

There should be no downside in allowing any kind of origin for remark.

@bkchr
frame_system::remark: Allow any kind of origin
b98eaef 
There should be no downside in allowing any kind of origin for `remark`.
@bkchr bkchr added A0-please_review Pull request needs code review. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels May 29, 2023
@bkchr bkchr requested review from a team May 29, 2023 15:07
@bkchr bkchr added the C1-low PR touches the given topic and has a low impact on builders. label May 29, 2023
@rzadp rzadp mentioned this pull request May 29, 2023
Closed
ggwpez
ggwpez approved these changes May 29, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

So in theory this is fine, but in practice this can be used to inflate indexers / archive nodes.

@bkchr
Copy link
Member Author

bkchr commented May 30, 2023

but in practice this can be used to inflate indexers / archive nodes.

What?

@ggwpez
Copy link
Member

ggwpez commented May 30, 2023

but in practice this can be used to inflate indexers / archive nodes.

What?

Ah i thought it would allow unsigned TX, but it does not. Okay 👍

sacha-l
sacha-l approved these changes May 30, 2023
View reviewed changes
@bkchr bkchr merged commit e0752b1 into master Jun 4, 2023
@bkchr bkchr deleted the bkchr-remark-every-origin branch June 4, 2023 21:15
This was referenced Jun 9, 2023
Merged
Merged
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@bkchr @nathanwhit
frame_system::remark: Allow any kind of origin (paritytech#14260)
0f6aaa2 
* frame_system::remark: Allow any kind of origin

There should be no downside in allowing any kind of origin for `remark`.

* Fix tests
@shawntabrizi
Copy link
Member

shawntabrizi commented Jul 24, 2023

Sorry, dumb question: Why does this not allow unsigned? Do we not allow unsigned by default?

@command-bot

This comment was marked as spam.

Sign in to view
@shawntabrizi shawntabrizi mentioned this pull request Jul 24, 2023
Closed
@ggwpez
Copy link
Member

ggwpez commented Jul 24, 2023 •
edited
Loading

Sorry, dumb question: Why does this not allow unsigned? Do we not allow unsigned by default?

Who would then pay the TX fee?
I think to support unsigned tx, the pallet needs a dedicated validate_unsigned hook to manually do the spam protection.

@bkchr
Copy link
Member Author

bkchr commented Jul 24, 2023

Sorry, dumb question: Why does this not allow unsigned? Do we not allow unsigned by default?

Unsigned transactions are rejected by default. As @ggwpez said you need a proper validate_unsigned to accept them.

@shawntabrizi
Copy link
Member

shawntabrizi commented Jul 24, 2023

great, makes sense, i just didnt remember explicitly.

@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

6 more reviewers

@xlc xlc xlc left review comments

@sam0x17 sam0x17 sam0x17 approved these changes

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez ggwpez approved these changes

@sacha-l sacha-l sacha-l approved these changes

@lexnv lexnv lexnv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”.

Projects

Runtime / FRAME
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bkchr @ggwpez @shawntabrizi @xlc @sam0x17 @kianenigma @sacha-l @lexnv