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

Run integrity_test in externalities #13726

Closed
ggwpez opened this issue Mar 27, 2023 · 7 comments · Fixed by #14546
Closed

Run integrity_test in externalities #13726

ggwpez opened this issue Mar 27, 2023 · 7 comments · Fixed by #14546
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. T1-runtime This PR/Issue is related to the topic “runtime”. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@ggwpez
Copy link
Member

ggwpez commented Mar 27, 2023

We should probably run all of then inside externalities. In the past I was concerned about unintended consequences, but it just annoyed us again in: paritytech/polkadot#6957

Since there a parameter type was configured to be storage instead of const, so it would fail outside externalities.

@ggwpez ggwpez added I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder labels Mar 27, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Apr 17, 2023

Note: There are cases were we explicitly want to run something outside of externalities.
For example complicated weight annotations in pallet alliance use config constants.
These could be factored out into helper functions and ran outside of externalities as a test to ensure that they dont access state.
Otherwise there is a chance that someone configures them to a storage parameter_type!

@bkchr
Copy link
Member

bkchr commented Apr 21, 2023

I don't see any problem in using storage parameter_type! if there is a justification. Before we started to make all of them "const' all of these parameters where stored in the storage. However, it doesn't make any sense to read something from the storage that isn't ever changing. But some implementations may have a reason to have some value dynamic.

@ggwpez
Copy link
Member Author

ggwpez commented Apr 21, 2023

So you mean that reading storage in weight annotations is fine? Or how does your comment relate to mine?
Someone on SE pointed out that Polkadot does this.

@bkchr
Copy link
Member

bkchr commented Apr 21, 2023

I mean reading storage for the weight needs to be done with a lot of caution to not open a DDOS door.

The linked weight looks wrong as the weight should not depend on the number of digests.

@juangirini juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 7, 2023
@atenjin
Copy link
Contributor

atenjin commented Jul 11, 2023

In fact in our project, we are using a storage to config the max weight..
So we are blocked at here (integrity_test not in Externalities).

Though you may not agree our practice that using a storage to config the weight, we are using substrate not for public using, in fact just in some corner case. So I agree this may have a hidden danger for DDOS, but I think we know what we are doing for now.

Anyway, even we do not consider about the weight, I think for the integrity_test, we may also need to access some storage in the test, so I think let the integrity_test in Externalities is necessary.

@ggwpez
Copy link
Member Author

ggwpez commented Jul 11, 2023

You can always create externalities in the integrity_test, like here https://github.com/paritytech/substrate/pull/13092/files so this should not block you.
Although we can also run the test in externalities per default, the only catch would be that the genesis state is not configurable, which could be weird.

@atenjin
Copy link
Contributor

atenjin commented Jul 11, 2023

@ggwpez oh thanks, but in fact our panic appears from transaction-payment pallet 🤣🤣🤣
so add it as default for all pallet is more better.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. T1-runtime This PR/Issue is related to the topic “runtime”. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants