-
Notifications
You must be signed in to change notification settings - Fork 924
Enable statement store in polkadot-omni-node and polkadot-parachain #8076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
64f012b
to
92057c9
Compare
4acb5fe
to
cad2edc
Compare
/cmd prdoc --audience node_dev --audience node_operator |
This is ready to be looked at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the offline discussion, should we disable the statement store by default ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
All GitHub workflows were cancelled due to failure one of the required jobs. |
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
inpolkadot-omni-node-lib
.This takes effect in
polkadot-omni-node
andpolkadot-parachain
and any node depending onpolkadot-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
orpolkadot-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