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

emit event on remark #8120

Merged
merged 12 commits into from
Feb 28, 2021
Merged

emit event on remark #8120

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5d0907d
emit event on remark
xlc Feb 15, 2021
bc3e674
update to use remark_with_event
xlc Feb 20, 2021
17c80d7
manually updating weight file
xlc Feb 20, 2021
a3b6603
fix
xlc Feb 20, 2021
13f2c52
Merge remote-tracking branch 'origin/master' into remark
xlc Feb 24, 2021
31bb301
add origin to Remarked
xlc Feb 25, 2021
ed29400
trigger CI
xlc Feb 25, 2021
fab54e6
emit Hash instead of remark
xlc Feb 26, 2021
c7613af
Merge remote-tracking branch 'origin/master' into shawntabrizi-benchm…
Feb 27, 2021
b83441a
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Feb 27, 2021
ab88a33
Update frame/system/src/weights.rs
shawntabrizi Feb 27, 2021
200d71b
Update frame/system/src/lib.rs
shawntabrizi Feb 27, 2021
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
6 changes: 6 additions & 0 deletions frame/system/benchmarking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ benchmarks! {
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), remark_message)

remark_with_event {
let b in 0 .. *T::BlockLength::get().max.get(DispatchClass::Normal) as u32;
let remark_message = vec![1; b as usize];
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), remark_message)

set_heap_pages {
}: _(RawOrigin::Root, Default::default())

Expand Down
17 changes: 15 additions & 2 deletions frame/system/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,6 @@ pub mod pallet {
///
/// # <weight>
/// - `O(1)`
/// - Base Weight: 0.665 µs, independent of remark length.
/// - No DB operations.
/// # </weight>
#[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))]
pub(crate) fn remark(origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo {
Expand Down Expand Up @@ -450,6 +448,19 @@ pub mod pallet {
storage::unhashed::kill_prefix(&prefix);
Ok(().into())
}

/// Make some on-chain remark and emit event.
///
/// # <weight>
/// - `O(1)`
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
/// - 1 event.
/// # </weight>
#[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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xlc xlc Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xlc xlc Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xlc xlc Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Ok(().into())
}
}

/// Event for the System pallet.
Expand All @@ -466,6 +477,8 @@ pub mod pallet {
NewAccount(T::AccountId),
/// An \[account\] was reaped.
KilledAccount(T::AccountId),
/// On on-chain \[remark\] happened.
Remarked(Vec<u8>),
}

/// Old name generated by `decl_event`.
Expand Down
7 changes: 7 additions & 0 deletions frame/system/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub trait WeightInfo {
fn set_storage(i: u32, ) -> Weight;
fn kill_storage(i: u32, ) -> Weight;
fn kill_prefix(p: u32, ) -> Weight;
fn remark_with_event(b: u32, ) -> Weight;
}

/// Weights for frame_system using the Substrate node and recommended hardware.
Expand Down Expand Up @@ -85,6 +86,9 @@ impl<T: crate::Config> WeightInfo for SubstrateWeight<T> {
.saturating_add((845_000 as Weight).saturating_mul(p as Weight))
.saturating_add(T::DbWeight::get().writes((1 as Weight).saturating_mul(p as Weight)))
}
fn remark_with_event(_b: u32, ) -> Weight {
(1_973_000 as Weight)
}
}

// For backwards compatibility and tests
Expand Down Expand Up @@ -119,4 +123,7 @@ impl WeightInfo for () {
.saturating_add((845_000 as Weight).saturating_mul(p as Weight))
.saturating_add(RocksDbWeight::get().writes((1 as Weight).saturating_mul(p as Weight)))
}
fn remark_with_event(_b: u32, ) -> Weight {
(1_973_000 as Weight)
}
}