Enable statement store in polkadot-omni-node and polkadot-parachain#8076
Enable statement store in polkadot-omni-node and polkadot-parachain#8076
Conversation
64f012b to
92057c9
Compare
4acb5fe to
cad2edc
Compare
|
/cmd prdoc --audience node_dev --audience node_operator |
|
This is ready to be looked at |
georgepisaltu
left a comment
There was a problem hiding this comment.
Looking at the code, I think we could get away with always enabling the statement store (i.e. not having it be an Option in the node spec) since it would just be inaccessible if the runtime API doesn't allow it, but I'm happy to move forward with this.
| /// and OCW. | ||
| /// It uses the runtime api to get the allowance associated to an account. | ||
| #[arg(long)] | ||
| pub disable_statement_store: bool, |
There was a problem hiding this comment.
As per the offline discussion, should we disable the statement store by default ?
There was a problem hiding this comment.
If we disable by default I fear nobody will enable it. But maybe it will be enough with the few that will enable it.
I feel going into a dedicated binary for people chain seems the better then no?
There was a problem hiding this comment.
I think that having a dedicated binary for the people chain would create more complications. We would need to release one more binary, change all the tests that involve the rococo-people chain to use the dedicated binary, change the deployment process only for the people chain. I don't know.
Or maybe we should just enable the statement-store by default as it is done here. I'm not sure which solution is the best. I'm curious what others think as well.
There was a problem hiding this comment.
I have also this other PR #8421 that make the CLI configurable for the statement store. This way we wouldn't need to maintain a different service in a different binary. But it still requires a dedicated process for the release.
There was a problem hiding this comment.
I would go for support in the existing polkadot-omni-node-lib instead for a new binary too.
If we disable by default I fear nobody will enable it.
Would it be widely useful for parachains? If no, only the ones requiring it will enable it, and that should be fine. I would rather go for the reverse logic here.
|
After thinking about it some more, I am in favor of not overengineering this problem. @gui1117 To move this forward I propose to just check if the If that does not work for you we can also enable it by default as proposed, but checking the runtime API seems like the most efficient solution to me. Example: let requires_statement_store = client
.runtime_api()
.has_api::<dyn ValidateStatement<Self::Block>>(best_hash)
.ok()
.unwrap_or_default(); |
It is a great solution, I guess this check is done when starting the node, so if it syncs from genesis or a snapshot that doesn't contain the |
Just wondering, would same thing work for offchain worker too - and maybe in other places? Sounds more elegant than asking node operators to provide flags (IIUC the flag removal is implied by your suggestion).
Is it though? What would that recent view be/how would we start the node from it? This sounds as if we can start node from a given state instantly, so that the runtime API check succeeds. |
|
Yeah if started from genesis the runtime-api might not be there. On restart though it should work if you use the
If a node component is tightly coupled to some runtime-API, then it makes sense. And yep, I was thinking that with the detection we don't need any flags. |
Does this assume that the DB already contains the infos regarding the block where the runtime API was added? I guess node operators could wait for the nodes to import a certain finalized block with the runtime upgrade and a new API before restarting. This should be the most frequent situation. For the cases where a restart happens with a DB that misses some relevant blocks, then operators still need to wait for the syncing, which means the restarted node will not enable the wanted service, yet. I guess it is important for node operators to check the last finalized block on the node before restarting, and then, they should have their own ways of checking the liveliness of the enabled service (e.g logs on each restarted node). I am mostly concerned with how visible it is for operators that the service is enabled. It is not anymore as simple as start your node and don't worry about a service being started, given the used CLI, but it shouldn't be far from it for most frequent usage. |
|
I forgot to include the host function, this may makes the implementation more difficult, I will see how to declare the host function conditionally. |
Looking at the substrate node, we only register the extension for the offchain worker. So this current commit we should have a similar implementation in both substrate node and omni-node. Calling from the statement-store host fn from the STF should fail.
iulianbarbu
left a comment
There was a problem hiding this comment.
Nothing outstanding, just a few suggestions
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
fb9b088 to
6e74940
Compare
|
ok, this is now written with a new cli arg: I still wish to have it in the upcoming release but maybe people will refuse given it is the frozen window, anyway I don't think I am in a position to enforce a decision on this matter so I will let release and node team decide. |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-8076-to-stable2506
git worktree add --checkout .worktree/backport-8076-to-stable2506 backport-8076-to-stable2506
cd .worktree/backport-8076-to-stable2506
git reset --hard HEAD^
git cherry-pick -x 18cd0b4d8ff6a2eea379a3b166fe21aec180778f
git push --force-with-lease |
…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`. 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> (cherry picked from commit 18cd0b4)
Backport #8076 into `stable2506` from gui1117. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> 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> Co-authored-by: Egor_P <egor@parity.io>
…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>
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-nodeandpolkadot-parachainand any node depending onpolkadot-omni-node-lib.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_storeinpolkadot-omni-node-lib.This takes effect in
polkadot-omni-nodeandpolkadot-parachainand any node depending onpolkadot-omni-node-lib.The reasoning is following other service such as
offchain_workerthe 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-nodeorpolkadot-parachain? we could otherwise only enable it forpolkadot-parachain. Or we could also create a new binary for people chain and enable it only for this new binary, and remove people chain frompolkadot-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