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

Make System Events Private from the Runtime #9619

Merged
4 commits merged into from
Aug 26, 2021
Merged

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Aug 24, 2021

We do not want to allow any Pallet to accidentally read from the events deposited into the runtime.

Doing so can have a huge impact on the PoV size of a block, and currently this is a very easy way to shoot yourself in the foot as a Parachain.

This PR removes the "easy" access to the storage item outside of tests and benchmarking.

polkadot companion: paritytech/polkadot#3712

@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 24, 2021
if <frame_system::Pallet<T>>::events()
//
// We can read the events here because offchain worker doesn't affect PoV.
if <frame_system::Pallet<T>>::read_events_i_know_what_i_am_doing()
Copy link
Contributor

Choose a reason for hiding this comment

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

writing a small note here to make it a new issue: This can actually be removed. While it is an offchain read with no PoV consequence or time limit, we can achieve the same by storing the previous phase in some offchain storage, and then checking the new phase here. Whenever previous_phase != Off && new_phase == Off we call unsigned::kill_ocw_solution::<T>();.

@gui1117
Copy link
Contributor

gui1117 commented Aug 25, 2021

As far as understand reading events is ok as long as it is done after the first removal in frame_system::initialize.
Because in this case it only reads the element stored by the runtime thus it shouldn't be needed in the PoV. But I may be wrong.

But this PR also solve this problem.

frame/system/src/lib.rs Outdated Show resolved Hide resolved
frame/system/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

code is good to me, but doc should be improved IMO, and maybe we can actually read events without leaking into PoV, as long as they were removed before, but I'm not sure and in general I would advise not to read them inside the consensus runtime code.

@shawntabrizi
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Aug 26, 2021

Trying merge.

@ghost ghost merged commit 69e5b50 into master Aug 26, 2021
@ghost ghost deleted the shawntabrizi-private-events branch August 26, 2021 13:55
Copy link

@jaysonmald35 jaysonmald35 left a comment

Choose a reason for hiding this comment

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

Pls help

@bkchr
Copy link
Member

bkchr commented Aug 27, 2021

As far as understand reading events is ok as long as it is done after the first removal in frame_system::initialize.
Because in this case it only reads the element stored by the runtime thus it shouldn't be needed in the PoV. But I may be wrong.

But this PR also solve this problem.

This is right, reading from the overlay doesn't include anything into the PoV.

@gui1117
Copy link
Contributor

gui1117 commented Aug 27, 2021

Pls help

what is your question ?

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants