-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use Foucoco runtime with instant seal #462
Conversation
It would be nice to have this how-to at the bottom of the README.md. 👌 |
There are a couple of changes in this PR that are pure formatting changes in unrelated code. How come, is the code on the main branch currently not correctly formatted or is did you reformat the code with different parameters? Makes it harder to review this PR. |
It seems that actually the current code on main is not correctly formatted. For that reason I added this issue. Is it possible to revert the formatting changes in unrelated code to keep this PR focussed and then correct the formatting when working on the linked issue? |
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 can't evaluate the code in node/service.rs
, it's just code I don't understand. Was this just inspired by taking the code from foucoco-standalone? Is it possible to unify this further? I can imagine that it becomes a major pain point when updating Polkadot versions.
1607e16
to
2b7e533
Compare
@b-yap Totally! I was waiting to see if in review something came up and we decided to change the commands or implementation, as soon as we define it I will add this to the README.md. @TorstenStueber Regarding formatting, I realized after the fact that it was a bad idea for reviewing the changes, so I rolled back the format and will maybe do at the end before merging. The idea was to maintain the same configurations of services we have right now for the production chain, as well as the same standalone uses without duplicating all the code. I tried to unify as much as I could the common initialization between both here, but it became cumbersome due mostly to |
Okay, I appreciate your effort to try to unify the code. Then let's use it as it is now – as long as it works that way 👍. |
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'm not a big fan of the standalone naming but I don't necessarily have a better word.
How about:
foucoco-testing
foucoco-instant
- or just
dev
. We previously had adev
runtime but removed it as we never used it. We can reintroduce the name and call itdev
while pointing to the foucoco runtime with a different chain spec.
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 tested it locally and can confirm that it works. I ran the chain with
cargo run -p pendulum-node -- --chain foucoco-standalone --instant-seal
and was able to do a simple transfer that got executed instantly.
@gianfra-t let's add some details how to run the chain with instant-seal to the README then.
The README looks good, thanks @gianfra-t 👍 seems like something went wrong in your merge commit, that's why the CI is complaining. |
Ohh yes I was hoping I caught all conflicts. My vs code was buggy and not showing them. |
Thanks! |
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.
Looks good to me now.
Closes tasks/#235.
Node update notes:
This does involve changes to the node of course, but it is not required for collators to update since it involves modifications for testing the chain locally.
Modifications
Implementation of a new flag
instant-seal
that allows to run a local node with the current foucoco runtime on instant seal mode consensus, like foucoco standalone.By not specifying the new flag the node should behave the same as before the changes.
Also modifies the genesis config to add the necessary token allowances used currently in the standalone chain.
Most relevant code changes
foucoco_standalone_config
genesis config by default.foucoco-standalone
.Running in standalone mode
To run:
cargo run -p pendulum-node -- --chain {x}-spec-raw.json --instant-seal
.Most other flags are irrelevant when
instant-seal
is used.For instance, to run with the standalone genesis config from
chain_spec.rs
:cargo run -p pendulum-node -- --chain res/foucoco-standalone-spec-raw.json --instant-seal
or simply :
cargo run -p pendulum-node -- --chain foucoco-standalone --instant-seal
to use whatever current config in
chain_spec.rs
is defined for foucoco standalone.Tests
The new standalone chain was tested by executing the deployment script for
nabla
from wasm-deploy.