ensure #[pallet::constant] is actually constant

[xlc]

It is possible to annotate a field with #[pallet::constant] which is actually not a constant and causing unexpected behaviour.

We should have a way to assert the pallet constant are actually constant at compile time. Not exactly sure how it can be implemented but there must be some way.

Related: #934 AcalaNetwork/Acala#2184

[nazar-pc]

We actually (ab)use the fact that it isn't actually a constant (at least in a sense that we can have different values without updating runtime) for implementing dynamic cost of storage (our LengthToFee is not constant, it is derived from consensus and changes every block).

Otherwise we'll need to refactor and make LengthToFee non-constant and that has significant implications.

[xlc]

In that case it should not be annotated with #[pallet::constant] and provide a different way to expose the value.

[nazar-pc]

Well, Bastian suggested that it is fine as long as value stays the same for the duration of a particular block, so we went with that.
I did find it counter-intuitive as well.

[xlc]

Unstable metadata wouldn't cause any onchain issue, but I am sure @Slesarew will reach out to you one day complaining it breaks parity signer.

And it will cause more trouble with #291 paritytech/substrate#10057

[bkchr]

Well, Bastian suggested that it is fine as long as value stays the same for the duration of a particular block, so we went with that. I did find it counter-intuitive as well.

Did I? :D I can not remember :P

To the issue, I'm with you @xlc, but it will maybe also be hard to solve.Using the example of @nazar-pc, we may would need to introduce LengthToFeeStatic and a LengthToFeeDynamic. While one needs to be set and the other one could be None or something. However, I think there are more of these things hidden in the code base.

A trick to check if constants are really constant could be to get each constant and return an error if we accessed the storage while doing so.

[xlc]

I guess the short term solution will be have #[pallet::constant] generate an integrity test that calls get and ensure it doesn't access storages.

[nazar-pc]

But in our protocol it does for LengthToFee and will break our codebase,

[xlc]

It is not a constant so you should be fine

https://github.com/paritytech/substrate/blob/a0a40adbf57b47b445012c6cbca940c991c94197/frame/transaction-payment/src/lib.rs#L287-L288

[nazar-pc]

Indeed 🤦‍♂️

It must have been something else then, but I can't recall it right now. But this would be a significant change either way.

Ideally we'd use trait T { const fn foo(); }, but it isn't stable yet.

[xlc]

I can totally see this breaks a lot of chains, so it could be an opt-in feature to start with.

[kianenigma]

pallet::constant in its current form should really be renamed pallet::metadata foremost, as it does not imply being constant and instead is about just being exposed in metadata.

[xlc]

Metadata should be constant within a version. Otherwise, it won't play nicely with features like #291

[bkchr]

Metadata should be constant within a version. Otherwise, it won't play nicely with features like #291

Yeah, I think to ensure this I would introduce some kind of "no state access" runtime api calls. Then we could move the metadata to use this. If there would be a state access, it would fail.

[pepyakin]

Yeah, I think to ensure this I would introduce some kind of "no state access" runtime api calls. Then we could move the metadata to use this. If there would be a state access, it would fail.

I am pretty sure that the fact that metadata is not a static blob is a mistake, in the same way as it was mistake to make the version a function. Thus I think #2745 is the right way to solve it long term.

[muharem]

For dynamic parameters, we can have a different attribute, and metadata will return a default value and it's location (storage key) in the storage.

I have an issue for it, if sounds good I will work on it - #3238