Add pallet dev mode

[sam0x17]

Description

This PR fixes #8965 by adding an optional dev_mode argument to the frame support pallet attribute macro, allowing #[pallet(dev_mode)] as a usable syntax. Specifying pallet(dev_mode) will allow the developer to place the pallet in development mode, which relaxes a number of constraints, provides defaults for a number of things that are not super relevant when a pallet is under construction (including weights), and disables accidental production compilation of the pallet.

Thus dev mode creates an optimal environment for tinkering with a pallet while it is under development and lowers the bar for new developers to be able to approach pallet development.

General behavior:

• Pallets can be marked with #[pallet(dev_mode)] and a compiler error will be issued if anything other than dev_mode is specified as an arg.

For pallets marked with #[pallet(dev_mode)], this PR will:

• set a default weight if one is not specified (0)
• automatically set up a default MaxEncodedLen / PoV size if it is not set up. Currently I handle this by simply marking all storages as #[pallet::unbounded] if the parent pallet is in dev mode, but I am totally open to other approaches
• anything else people think might be convenient to be set up by default for a dev-mode pallet

#8965 also lists a few things that should never appear in a production pallet. The plan is that for these things we will throw a compile error if a pallet is built in production mode with any of these features:

• Transactional Extrinsics (don't know the details on this yet)
• () or test impl for many traits (will have to do some more research to get the full list)

Related Issues

• Production / Non-Production Feature Flags for FRAME Pallets and Runtime #8965

[kianenigma]

Can you re-iterate here as well what practical effects this has on the end-user i.e. the developer, as the PR stands now? the original issue is a bit vague to me and I am not sure if all are implemented here.

[sam0x17]

@kianenigma I copied over and expanded on some things listed in the original issue in the description at the top.

One thing I have realized is not fleshed out yet is how to handle the unaudited pallets @shawntabrizi mentions in the original issue -- perhaps we should do some sort of system where we mark a pallet that has been audited with #[pallet(audited)] and only then does it become available for use in production? This would be a breaking change but it would protect runtime devs from using unaudited pallets in production and would allow us to gatekeep a bit within the pallets we maintain based on auditing results. I.e. now people will have 100% confidence that if they can use a pallet in production, it has been audited. For outisde devs that maintain their own production pallets, they can implement whatever process they want (or not) for adding the #[pallet(audited)] attribute, but will need to add it to be able to run a pallet in production mode. Otherwise I don't see how we can do what shawn is implying here. Thoughts?

Could be a separate issue + PR. I don't think it has to be tied to this specifically.

[sam0x17]

Note that my chosen approach for dealing with MaxEncodedLen is simply marking all storages on dev mode pallets with #[pallet::unbounded]. Totally open to alternative approaches however

[sam0x17]

@shawntabrizi do you have any examples of traits that should get default impls as you mentioned in the issue?

[shawntabrizi]

@sam0x17 nothing at the moment. The only main features you should implement to start with is just not needing the Weight attribute and not needing to have bounded storage.

automatically set up a default MaxEncodedLen / PoV size if it is not set up. Currently I handle this by simply marking all storages as #[pallet::unbounded] if the parent pallet is in dev mode, but I am totally open to other approaches

Is it possible to add this only if you detect the storage is unbounded?

[ggwpez]

Feels to me like we are abusing cfg(test) since IIUC this implies that you can never deploy a dev-mode pallet ever?

[sam0x17]

Yeah this is why I worry that approach might be heavy handed. Also doing it this way creates this blocker with trybuild: dtolnay/trybuild#206

[sam0x17]

OK I've explored these options fully now and it looks like it would neither be practical nor desirable to emit a compile-time error if dev_mode is enabled outside of cargo test.

This is for the following reasons:

• There are legitimate scenarios where someone will run dev mode pallets locally (not using cargo test) anyway as part of the tinkering process, so I don't think it makes sense to disable this capability
• There is no good way of detecting if we are in production. Rust doesn't provide a #[cfg()] equivalent for features, and although we could do something like

  substrate/client/consensus/slots/build.rs

  Line 23 in 547e856

  println!("cargo:rustc-cfg=build_type=\"{}\"", profile);

  theoretically, @bkchr doesn't think this would be a good idea because there is nothing that actually enforces that there is a production feature (I take this to mean that it would still be possible for people to "deploy" a dev mode pallet without enabling the production feature, so in practice this wouldn't prevent anything)
• While it is easy to issue the compiler error when we aren't run via cargo test, some limitations of trybuild make it extremely difficult to test this behavior (see how to specify that tests should be run with particular cfg directives? dtolnay/trybuild#206) so we would have to bend over backwards to test this compiler error that I think might not be desirable anyway

I would say a better approach might be to issue a compiler warning if someone tries to use dev mode outside of cargo test, however to do this we would have to use #![feature(proc_macro_diagnostic)] (see rust-lang/rust#54140) which we currently do not use anywhere in substrate AFAIK, so this would be yet another unstable feature we'd need to rely on and I know that's contentious right now.

If people are ok with me using proc_macro_diagnostic, we could go this route. If so, it might even be a good idea to just always issue a warning when dev mode is enabled to ensure people are using it properly instead of gating it based on whether we are in #[cfg(test)].

Otherwise, we could just merge this as is, perhaps with some additional doc text emphasizing that this should not be used in production. For now I've gone ahead and reverted the commits that emitted the compile-time error, but I can easily re-add this and tweak it a bit if we want to do a warning and go that route.

thoughts? @ggwpez @KiChjang @shawntabrizi

[KiChjang]

Using compiler features is out of the question here, since we explicitly run on stable rust.

[sam0x17]

ready for final review :)

[ggwpez]

Works very well, great!

I am thinking about where to best show it off:

• New example pallet in frame/examples/dev-mode
• In the node-template template-pallet
• In the dev-hub repo substrate-node-template

Not sure which one to pick. Do you have a reservation @shawntabrizi ?

Another thing about how to prevent production usage of the pallet would be to add a new Trait or Trait function to a pallet which describes if it is dev-mode or not. Then the runtime can use that in its runtime_integrity_test and we dont need to rely on compiler feature magic et al.

[ggwpez]

Is it common to put HTML into the docs?

[sam0x17]

Right now AFAIK there is no built-in way of doing a warning block unless something about the syntax of your code creates a doc warning (which isn't the case here), so the common workaround is to just use the same CSS classes rust docs use when there is a real warning and hard-code the HTML for now.

rust-lang/rust#73935 has tracked it since 2020

The actual HTML I use here was based on an SO answer that I am now having trouble locating, but yes it is using some of the built-in classes to achieve this warning look

TLDR: yes, for warnings

[shawntabrizi]

awesome

[sam0x17]

bot merge

[bkchr]

weight_attrs.push(FunctionAttr::Weight(syn::parse_quote!(0))

Would be the same.