polkadot-omni-node-lib: enable tx storage inherent data provider setup

[iulianbarbu]

Description

Closes paritytech/polkadot-bulletin-chain#82.

Integration

• Node developers could enable the transaction storage inherent data provider setup by using --enable-tx-storage-idp flag. This is especially useful in the context of bulletin chain.
• Node developers will set up the network idle_connection_timeout to 1h when using --ipfs-server flag, again, useful in the context of bulleting chain.

Review Notes

Implementation notes

A con of this work is that the current setup exposes bulletin node specific logic in polkadot-omni-node-lib (the --enable-tx-storage-idp flags, which isn't necessarily useful for every instantiation of polkadot-omni-node-lib). Ideally this logic would be implemented via a generic abstraction.

paritytech/polkadot-bulletin-chain#82 covers a few ways of doing it in the comments:

1. API for defining logic to be considered in polkadot-omni-node-lib based on custom args,
2. plugin API which uses wasm modules with logic for inherent data providers,
3. metadata based detection of tx-storage pallet, and enablement of the inherent data provider without the flags,
4. flags guarded enablement of tx storage inherent data provider

The discussions converged towards exposing the flags through polkadot-omni-node-lib, used by polkadot-parachain, which will eventually be used to run the bulletin node. It is mostly because it is the simplest implementation that unblocks the testing of bulletin based on polkadot-parachain.

Blockers (with any of the implementations)

Huh, a very simple fix, I overcomplicate it for a day: aea846e.

the blocker

A blocker of this PR to be actually useful is the following error:

error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce`
   --> cumulus/polkadot-omni-node/lib/src/nodes/aura.rs:596:36
    |
596 |             create_inherent_data_providers: move |parent, ()| async move {
    |                                             ^^^^^^^^^^^^^^^^^ this closure implements `FnOnce`, not `Fn`
...
600 |                             &*client_clone,
    |                               ------------ closure is `FnOnce` because it moves the variable `client_clone` out of its environment
...
633 |         Self::launch_slot_based_collator(params);
    |         ---------------------------------------- the requirement to implement `Fn` derives from here
    |
    = note: required for `{closure@aura.rs:596:36}` to implement `CreateInherentDataProviders<Block, ()>`
    = note: the full name for the type has been written to '/home/ubuntu/repos/polkadot-sdk/target/release/deps/polkadot_omni_node_lib-52a270c00284da3c.long-type-58975863800009441
90.txt'
    = note: consider using `--verbose` to print the full type name to the console

It indicates to an error where if we need the client, for being able to construct an inherent data provider,,the created closure here will be constrained to a FnOnce, which is incompatible with the type requested by slot based or lookahead collators, which is a Fn. The fix for this isn't straight forward based on my trials. I am still trying to figure out a way forward.

[iulianbarbu]

/cmd prdoc --audience "Node Dev"

[bkontur]

@iulianbarbu is it possible to call runtime api exposed by the runtime (sp_api::decl_runtime_apis! / sp_api::impl_runtime_apis) at this moment? With client_clone? Would it be expensive? Do we have similar call here around?

Something like:

client.runtime_api().get_storage_period()

runtime.new_instance()?.call("get_storage_period", [])

[iulianbarbu]

Why is the storage period runtime API call relevant? Not sure how expensive it gets from a block building standpoint, it is something we need to measure. But what is this get_storage_period? Can't really find it in the codebase.

[bkontur]

But what is this get_storage_period? Can't really find it in the codebase.

Yes, you cannot, because it is not implemented yet: paritytech/polkadot-bulletin-chain#60

The problem is that the new_data_provider uses a hard-coded pub const DEFAULT_STORAGE_PERIOD: u32 = 100800;, which forces the runtime to use the same value. This value corresponds to 7 days for 6-second blocks. I want to make this constant configurable and driven by the runtime — exposed via a runtime API. For example, if we switch to 2-second blocks but still want to keep the 7-day period, we won’t need to coordinate and restart the collators with a new hard-coded constant and so on. Basically, I want to have possibility to set this constant with arbitrary value without affecting the collators or anything.

I think new_data_provider is doing already two calls, the client.number and client.block_indexed_body, so it shouldn't be a problem to add another call to get the constant from the runtime. I will try this Monday on the top of this PR, which is looking good so far.

[bkontur]

@iulianbarbu merged as a part of #10662
plus 3b575f9