emit event on remark #8120

xlc · 2021-02-15T03:17:35Z

Partially address paritytech/polkadot-sdk#350

This is required to use remark for proof-of-ownership.

Will need new benchmark cc @shawntabrizi

polkadot companion: paritytech/polkadot#2528

shawntabrizi · 2021-02-15T04:57:20Z

@xlc this will probably not be accepted. Remarks are not meant to have any state or anything. They should be and remain as minimal as possible. If you want something like this, I think it would be better to just make a new extrinsic or even better maybe a new pallet with much more "arbitrary data storage" features like the "comment pallet" i had envisioned: https://github.com/paritytech/substrate/compare/shawntabrizi-comment-pallet

Anyway, that is my two cents on this PR

xlc · 2021-02-15T07:26:02Z

This does not store data, just emit event.
This is required to actually make remark useful.
There are plenty other txs can be used to put arbitrary data in a block anyway.
But I am fine to make this into a new call. Will wait to see what others think.

kianenigma · 2021-02-16T13:47:45Z

I think remark is already useful as it is being used by some teams for NFT AFAIK, and I can see other purposes where it could work fine without events. Based on your other issue, I understand that this is only an issue for proxies, multisigs, and other call-wrappings that make it rather difficult (with the current limited tools at hand) to know if the remark succeeded or not. Correct?

if so, then adding a remark_noted() which also emits an event will be an acceptable compromise.

But I still don't say why with proxies or multisigs it becomes hard to verify that an account sent a remark or not.

Swader · 2021-02-16T17:14:33Z

We use remarks for rmrk.app, yes. An event would make no difference for us, we have to match against extrinsicSuccess events anyway so we already check if the remark was noted that way.

shawntabrizi · 2021-02-16T17:26:19Z

I think Bruno is completely on point here. Using this event versus any other extrinsic success event or processing the included txs in a block doesn't really make sense to me.

Having some persistent storage could make sense, but needs a new extrinsic and some economics around users cleaning up that data.

xlc · 2021-02-16T22:23:26Z

The best practice is always check for specific events, not extrinsic success.

Otherwise it is very difficult to check if a remark is "executed", think about scheduler, batch, multisig, derive, proxy, delayed proxy and all the combinations and future additions. You simply cannot use extrinsic success to infer if remark is executed without false negative.

And the main reason I want this is for proof-of-ownership, i.e. for multisgn / proxy account to proof some offchain components someone does control the said account. Otherwise there is no easy way other than transfer some DOT to a random generated account, which is less than ideal.

Swader · 2021-02-17T09:44:17Z

This is true, we do currently jump through some hoops to find remarks in batches. For our immediate case this would not be helpful (we've handled those edge cases) but I can see the benefit in other scenarios we intend to develop, and the scenarios @xlc mentions.

gui1117 · 2021-02-17T10:47:56Z

we usually take a deposit when we store something on chain as long as it is stored.
But event are stored in only one block.
I don't think we need to ask for a deposit and then give it back the next block. (But we can still do it.)

The current PR is fine to me, but I may miss an economical issue.

kianenigma · 2021-02-19T13:52:42Z

(why are we not adding a remark_with_event, which is slightly more expensive but emits the event?)

xlc · 2021-02-20T03:55:47Z

@kianenigma updated

kianenigma · 2021-02-20T08:11:16Z

/benchmark runtime pallet frame_system

parity-benchapp · 2021-02-20T08:11:17Z

Error running benchmark: remark

stdout

fatal: ambiguous argument 'origin/remark': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]'

xlc · 2021-02-20T08:43:15Z

yeah benchmark from fork is not supported. someone will need to make a branch in this repo to run benchmarks and I can merge the results into here

bkchr · 2021-02-21T18:49:31Z

@shawntabrizi why can the both not push to foreign branches, as long as the author enabled pushing members?

NikVolf · 2021-02-22T07:20:13Z

@shawntabrizi why can the both not push to foreign branches, as long as the author enabled pushing members?

Running on our CI anything that can be manipulated from fork should be approached with extreme caution

bkchr · 2021-02-22T11:06:31Z

@shawntabrizi why can the both not push to foreign branches, as long as the author enabled pushing members?

Running on our CI anything that can be manipulated from fork should be approached with extreme caution

We already run the complete CI on our machines, that could involve anything and you can do much more as with this bot.

NikVolf · 2021-02-23T12:21:38Z

We already run the complete CI on our machines, that could involve anything and you can do much more as with this bot.

CI does not have push access to substrate

shawntabrizi · 2021-02-25T03:43:11Z

@shawntabrizi why can the both not push to foreign branches, as long as the author enabled pushing members?

The bot can be updated to support foreign branches, but I am not sure how to do it, and haven't spent time to look into it.

Happy to have a PR, but I was waiting to delegate to our new CI person.

Running on our CI anything that can be manipulated from fork should be approached with extreme caution

The solution here IMO is just have only Parity employees be able to trigger the bot.

shawntabrizi · 2021-02-25T03:45:58Z

frame/system/src/lib.rs

+		#[pallet::weight(T::SystemWeightInfo::remark_with_event(remark.len() as u32))]
+		pub(crate) fn remark_with_event(origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
+			ensure_signed(origin)?;
+			Self::deposit_event(Event::Remarked(remark));


I still do not see what this ACTUALLY does.

Like events should be for presenting data which may not easily be accessible, for example that an extrinsic completed using fork path A versus fork path B.

In this case, all the information you need is already on chain and accessible as easily as an even it:

Extrinsic with Remark data is in the block header

Success event of the remark is in events already

So what am I missing? Is this just lazily trying to use the events API in JavaScript? if so, then we could probably program in the JS API side this functionality.

No. Please see my previous comment

The best practice is always check for specific events, not extrinsic success.

Otherwise it is very difficult to check if a remark is "executed", think about scheduler, batch, multisig, derive, proxy, delayed proxy and all the combinations and future additions. You simply cannot use extrinsic success to infer if remark is executed without false negative.

And the main reason I want this is for proof-of-ownership, i.e. for multisgn / proxy account to proof some offchain components someone does control the said account. Otherwise there is no easy way other than transfer some DOT to a random generated account, which is less than ideal.

Alice is doing the best practice to secure her account that she have anonymous account + time delayed proxy + revoke proxy + multisig proxy.

Now she want to use a service that require her to provide proof of ownership.

She cannot use her account to sign message as there is no private key for anonymous proxy.

She can use the time delayed proxy to issue a proof as form of system.remark to indicate she does control this account.

Now on the verification side, how would you be able to detect if the system.remark is executed and not cancelled? How do you know which account is the system.remark in been executed with?

How could Alice use this setup to interact with RMRK.app?

remark is executed without false negative.

Why do we care about false negative? We should be worried about false positive. In any of the scenarios you describe above, if the full batch/multisig/proxy etc is completed successfully, then the remark is easy enough to verify.

Batch will always give extrinsic success. Multisig requires x success. Time delayed proxy execution could fail even if all extrinsic are success. Only check extrinsic success is certainly going to cause some bugs.

And how do I even know if a proxy call contains system.remark? Remember, we can arbitrarily nest calls. batch in proxy in multisig in batch.

It is possible to code up something that works now, but it will have issue in future if we introduce some new way of nesting.

bkchr · 2021-02-25T17:03:08Z

frame/system/src/lib.rs

+		#[pallet::weight(T::SystemWeightInfo::remark_with_event(remark.len() as u32))]
+		pub(crate) fn remark_with_event(origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
+			let who = ensure_signed(origin)?;
+			Self::deposit_event(Event::Remarked(who, remark));


But is it really a good idea to put the full remark into the event?

Do you have a suggestion? Put hash? Limit the remark length?

gavofyork · 2021-02-26T08:40:16Z

frame/system/src/lib.rs

+		#[pallet::weight(T::SystemWeightInfo::remark_with_event(remark.len() as u32))]
+		pub(crate) fn remark_with_event(origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
+			let who = ensure_signed(origin)?;
+			Self::deposit_event(Event::Remarked(who, remark));


should be the hash of remark, not remark itself.

gavofyork · 2021-02-26T12:48:25Z

probably needs a master merge to get it green.

xlc · 2021-02-27T09:52:34Z

Looks like all green now.

…ark-remark-event

…/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=frame_system --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/system/src/weights.rs --template=./.maintain/frame-weight-template.hbs

shawntabrizi · 2021-02-27T20:15:03Z

@xlc please integrate the benchmark results from ^

frame/system/src/lib.rs

shawntabrizi · 2021-02-27T21:43:53Z

bot merge

ghost · 2021-02-27T21:43:57Z

Waiting for commit status.

ghost · 2021-02-27T22:13:34Z

Checks failed; merge aborted.

Co-authored-by: Parity Benchmarking Bot <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]>

xlc force-pushed the remark branch from 487c217 to c3d42be Compare February 15, 2021 03:32

emit event on remark

5d0907d

xlc force-pushed the remark branch from c3d42be to 5d0907d Compare February 15, 2021 03:47

xlc added 2 commits February 20, 2021 16:49

update to use remark_with_event

bc3e674

manually updating weight file

17c80d7

fix

a3b6603

Merge remote-tracking branch 'origin/master' into remark

13f2c52

paritytech-ci added the D2-breaksapi label Feb 24, 2021

shawntabrizi reviewed Feb 25, 2021

View reviewed changes

add origin to Remarked

31bb301

shawntabrizi approved these changes Feb 25, 2021

View reviewed changes

kianenigma approved these changes Feb 25, 2021

View reviewed changes

gui1117 added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Feb 25, 2021

bkchr reviewed Feb 25, 2021

View reviewed changes

shawntabrizi self-requested a review February 25, 2021 17:13

gavofyork reviewed Feb 26, 2021

View reviewed changes

gavofyork added A5-grumble and removed A8-mergeoncegreen A0-please_review Pull request needs code review. labels Feb 26, 2021

emit Hash instead of remark

fab54e6

paritytech-ci added the D2-breaksapi label Feb 26, 2021

gavofyork added A8-mergeoncegreen and removed A5-grumble labels Feb 26, 2021

Merge remote-tracking branch 'origin/master' into shawntabrizi-benchm…

c7613af

…ark-remark-event

shawntabrizi mentioned this pull request Feb 27, 2021

Benchmark Remark with Event #8222

Closed

Update frame/system/src/weights.rs

ab88a33

shawntabrizi reviewed Feb 27, 2021

View reviewed changes

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

Update frame/system/src/lib.rs

200d71b

shawntabrizi approved these changes Feb 27, 2021

View reviewed changes

gui1117 merged commit 2dd569a into paritytech:master Feb 28, 2021

xlc deleted the remark branch February 28, 2021 10:06

jam10o-new pushed a commit to jam10o-new/substrate that referenced this pull request Feb 28, 2021

emit event on remark (paritytech#8120)

80a9616

Co-authored-by: Parity Benchmarking Bot <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]>

Swader mentioned this pull request Mar 3, 2021

Mapping events to extrinsics rmrk-team/rmrk-tools#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emit event on remark #8120

emit event on remark #8120

xlc commented Feb 15, 2021 •

edited

Loading

shawntabrizi commented Feb 15, 2021

xlc commented Feb 15, 2021

kianenigma commented Feb 16, 2021 •

edited

Loading

Swader commented Feb 16, 2021

shawntabrizi commented Feb 16, 2021

xlc commented Feb 16, 2021 •

edited

Loading

Swader commented Feb 17, 2021

gui1117 commented Feb 17, 2021

kianenigma commented Feb 19, 2021

xlc commented Feb 20, 2021

kianenigma commented Feb 20, 2021

parity-benchapp bot commented Feb 20, 2021 •

edited

Loading

xlc commented Feb 20, 2021

bkchr commented Feb 21, 2021

NikVolf commented Feb 22, 2021

bkchr commented Feb 22, 2021

NikVolf commented Feb 23, 2021

shawntabrizi commented Feb 25, 2021

shawntabrizi Feb 25, 2021

xlc Feb 25, 2021 •

edited

Loading

xlc Feb 25, 2021 •

edited

Loading

shawntabrizi Feb 25, 2021

xlc Feb 25, 2021 •

edited

Loading

bkchr Feb 25, 2021

xlc Feb 25, 2021

gavofyork Feb 26, 2021

gavofyork commented Feb 26, 2021

xlc commented Feb 27, 2021

shawntabrizi commented Feb 27, 2021 •

edited

Loading

shawntabrizi commented Feb 27, 2021

ghost commented Feb 27, 2021

ghost commented Feb 27, 2021

emit event on remark #8120

emit event on remark #8120

Conversation

xlc commented Feb 15, 2021 • edited Loading

shawntabrizi commented Feb 15, 2021

xlc commented Feb 15, 2021

kianenigma commented Feb 16, 2021 • edited Loading

Swader commented Feb 16, 2021

shawntabrizi commented Feb 16, 2021

xlc commented Feb 16, 2021 • edited Loading

Swader commented Feb 17, 2021

gui1117 commented Feb 17, 2021

kianenigma commented Feb 19, 2021

xlc commented Feb 20, 2021

kianenigma commented Feb 20, 2021

parity-benchapp bot commented Feb 20, 2021 • edited Loading

xlc commented Feb 20, 2021

bkchr commented Feb 21, 2021

NikVolf commented Feb 22, 2021

bkchr commented Feb 22, 2021

NikVolf commented Feb 23, 2021

shawntabrizi commented Feb 25, 2021

shawntabrizi Feb 25, 2021

Choose a reason for hiding this comment

xlc Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

xlc Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

shawntabrizi Feb 25, 2021

Choose a reason for hiding this comment

xlc Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

bkchr Feb 25, 2021

Choose a reason for hiding this comment

xlc Feb 25, 2021

Choose a reason for hiding this comment

gavofyork Feb 26, 2021

Choose a reason for hiding this comment

gavofyork commented Feb 26, 2021

xlc commented Feb 27, 2021

shawntabrizi commented Feb 27, 2021 • edited Loading

shawntabrizi commented Feb 27, 2021

ghost commented Feb 27, 2021

ghost commented Feb 27, 2021

xlc commented Feb 15, 2021 •

edited

Loading

kianenigma commented Feb 16, 2021 •

edited

Loading

xlc commented Feb 16, 2021 •

edited

Loading

parity-benchapp bot commented Feb 20, 2021 •

edited

Loading

xlc Feb 25, 2021 •

edited

Loading

xlc Feb 25, 2021 •

edited

Loading

xlc Feb 25, 2021 •

edited

Loading

shawntabrizi commented Feb 27, 2021 •

edited

Loading