Based on enabled statement store PR: make default behavior configurable: enable by default only in polkadot-parachain.#8421
Conversation
iulianbarbu
left a comment
iulianbarbu
left a comment
There was a problem hiding this comment.
In general, I see where this goes and even if I feel I don't agree with some of the implementation details (no comment left from me yet because I want to explore a few different things on my own when the time comes), I still believe the direction in the PR is the sweet spot - which IIUC is to consider the statement store arg an extra Cli arg, which would be added to a node that uses polkadot-omni-node-lib via some associated or generic types, somewhat resembling dependency injection.
I was part of some offline discussions in omni-node related calls and I can't say we identified a serious need for a "generic"/"extandable" extra cli arg support for polkadot-omni-node-lib based nodes. Adding polished/intentional support for it requires some more upfront thinking and requirements gathering IMO. The solutions we choose can be limited (e.g. people can add extra args to the node in a way resembling dependency injection, via predefined types, implement only by omni-node-lib, and not very configurable by users, almost close to inflexible) or somewhat flexible, where people would augment the CLI but they'd also be able to implement custom services, or add custom RPC endpoints. There isn't a need for the second option (and based on my understanding that has been attempted in the past via a Service trait, which made life of users and devs complicated), while for the first option, I personally see it useful but low prio (also it can be started anytime we decide, it should not be a one door decision to pick it or delay it in favour of a "hardcoded" alternative, where we simply add the needed flag in omni node, to enable a needed service).
Hope it makes sense, and I would be happy to continue the discussions offline if something's not clear. For now I would say we can close this PR and at most open an issue where we describe the idea of extra CLI args - this can also wait for actual users to open it too IMO.
|
we settled for disabled by default. |
…8076) In `polkadot-omni-node-lib`, when starting the node, when the runtime for the best hash contains the runtime api: `ValidateStatement`, then we start the statement store. The statement store is an off-chain data-store for signed statements accessible via RPC and offchain worker. It uses the runtime api to get the allowance associated to an account. This takes effect in `polkadot-omni-node` and `polkadot-parachain` and any node depending on `polkadot-omni-node-lib`. </br> </br> ---- ### Old description: We want to enable statement store for people chain. This PR introduces and enables the statement store and add a new argument `disable_statement_store` in `polkadot-omni-node-lib`. This takes effect in `polkadot-omni-node` and `polkadot-parachain` and any node depending on `polkadot-omni-node-lib`. The reasoning is following other service such as `offchain_worker` the default behavior is to support it, otherwise I expect not many collator to explictly opt-in (but I can be wrong assuming this). Do system chain collator use `omni-node` or `polkadot-parachain`? we could otherwise only enable it for `polkadot-parachain`. Or we could also create a new binary for people chain and enable it only for this new binary, and remove people chain from `polkadot-parachain`. If the runtime doesn't support the API then it any submission will fail with `Error calling into the runtime`. It would be ideal to target the next cut-off for this issue/PR given that it can take time for node operator to update their node. To have a configurable CLI and default behavior for statement-store, I did in this PR: #8421 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Sebastian Kunert <skunert49@gmail.com> Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
…8076) In `polkadot-omni-node-lib`, when starting the node, when the runtime for the best hash contains the runtime api: `ValidateStatement`, then we start the statement store. The statement store is an off-chain data-store for signed statements accessible via RPC and offchain worker. It uses the runtime api to get the allowance associated to an account. This takes effect in `polkadot-omni-node` and `polkadot-parachain` and any node depending on `polkadot-omni-node-lib`. </br> </br> ---- ### Old description: We want to enable statement store for people chain. This PR introduces and enables the statement store and add a new argument `disable_statement_store` in `polkadot-omni-node-lib`. This takes effect in `polkadot-omni-node` and `polkadot-parachain` and any node depending on `polkadot-omni-node-lib`. The reasoning is following other service such as `offchain_worker` the default behavior is to support it, otherwise I expect not many collator to explictly opt-in (but I can be wrong assuming this). Do system chain collator use `omni-node` or `polkadot-parachain`? we could otherwise only enable it for `polkadot-parachain`. Or we could also create a new binary for people chain and enable it only for this new binary, and remove people chain from `polkadot-parachain`. If the runtime doesn't support the API then it any submission will fail with `Error calling into the runtime`. It would be ideal to target the next cut-off for this issue/PR given that it can take time for node operator to update their node. To have a configurable CLI and default behavior for statement-store, I did in this PR: #8421 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Sebastian Kunert <skunert49@gmail.com> Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
2 participants
Based on: #8076
Another way to add the statement-store in the omni-node: Have a new configurable CLI in
CliConfigand allow nodes to choose the default behavior: enabled by default or disabled by default.