Make snowbridge-ethereum-beacon-client generic over the network it is built for

[nazar-pc]

We wanted to use this at Subspace, but turned out there was a mix of constants and generic configuration parameters that were:

1. Somewhat confusing
2. Didn't allow to build both "minimal" and "mainnet" version of the crate in the same workspace

I resolved this by making a few data structures generic and adding constants to Config trait. If pallet is made instantiable (I didn't implement it in this PR as it might be more controversial) then it would be possible to have this pallet included multiple times in the same runtime with different configurations, which wasn't possible before.

It required using unstable feature, but since Substrate uses them too and nightly is already required, this seemed like a decent compromise.

Note: minimal feature is still there, but it is now only used for benchmaking and testing and has no effect otherwise.

[nazar-pc]

BTW, let me know if you want to see minimal feature gone, it is now possible to make all tests and benchmarks available at all times.

[vgeddes]

We can't really use unstable features (and especially this one, which is still in development) in the common-good BridgeHub parachain. That's where this pallet is eventually going to be installed.

But simple const generics, which were stabilized in Rust 1.51, are totally fine. So then would it not be possible to support both the minimal and mainnet using a more dynamic approach, like the following:

const A: u32 = 2;
const B: u32 = 3;

function foo<const FOO: u32>() { ... }

function main() {
  if ... {
    foo<A>();
  } else {
    foo<B>();
  }
}

Another option is doing away with the minimal feature. That will increase the duration of our integration tests though. But perhaps it is better to test always against mainnet configs. Let me think over it for a bit.

[nazar-pc]

Those features were only added so we can use constants defined on Config in function calls (that also have const generics) otherwise they are considered expressions in current Rust version for some reason ({} part). There are apparently more advanced versions of this that are not complete, but we literally use the most basic version of the feature possible, so I'd say the risk of using it is low.

And as I said before, runtime already requires nightly Rust (and you run tests on nightly Rust too), so I don't see an issue there either.

Dynamic approach means bigger runtime and less flexibility. With this PR you can provide whatever values you want (you might want to use Ethereum or any of the numerous forks), with "dynamic" approach we're limited to a few hardcoded options only.

Why are you saying that you can't use unstable features, is there a requirement for this somewhere?

[vgeddes]

Well the issue is that our snowbridge runtimes/parachain in this repo are just for testing. Our pallets are eventually going to be included in the BridgeHub runtime in the cumulus repo: https://github.com/paritytech/cumulus/tree/master/parachains/runtimes/bridge-hubs

So we'd have to ask the Cumulus team if they are willing to support the generic_const_exprs feature. I can't find a clear policy anywhere, but I suspect that Parity-developed core parachains can't use unstable features, even if the runtimes are built using nightly.

I believe Substrate relies on rust nightly mainly because it supports wasm compilation, rather than because of needing other unstable features.

[vgeddes]

If pallet is made instantiable (I didn't implement it in this PR as it might be more controversial) then it would be possible to have this pallet included multiple times in the same runtime with different configurations, which wasn't possible before.

I don't have a strong objection against making the pallet instantiable. Just bear in mind that operating even a single instance of the pallet consumes a lot of block weight due to BLS signature verification. I wouldn't add more 2-3 instances in a runtime (and that is still dependent on BLS host functions being merged into Substrate).

[nazar-pc]

Here is why they use nightly: paritytech/substrate#1252

TL;DR: It will soon become stable compatible: paritytech/substrate#13580

Still it seems to be useful to have ability to customize it and this is the only way. I don't see usage of nightly compiler as a big obstacle for Substrate ecosystem so far.

[vgeddes]

Thanks for those links, some that is news to me! I asked some Parity devs, and it was pointed out to me that paritytech/substrate#13580 pretty much answers this question. Using rust nightly is on the way out. And unstable features won't be accepted on the BridgeHub parachain, because it will add instability during compilation and possibly in production settings.

So I think we need to find another solution here for you.

[nazar-pc]

Got it, will close for now then, but if you have specific steps you'd like to make I might be able to help. Thanks for review!