Skip to content
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

Combine standalone and parachain node #204

Merged
merged 47 commits into from
Feb 1, 2021
Merged

Combine standalone and parachain node #204

merged 47 commits into from
Feb 1, 2021

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Jan 25, 2021

This PR combines the standalone and parachain nodes into a single binary application 🤯

This removes the binary application known as the standalone node, moving its most important features (--dev service and manual seal) into the existing parachain binary. When run in --dev mode the node will start a special dev service (copied from the former standalone node) and insert mocked relay chain validation data. The runtime will accept this mocked data, and there will be no real relay chain backing the node.

This does not preserve every single feature we had in the standalone node. By removing non-critical features we reduce the size of our codebase considerably, and eliminate nearly all remaining duplicated code.

What is preserved?

  • Everything the parachain node could do before it can still do now
  • The --dev flag with it's manual seal mode (we may add instant seal in the future)

What is discarded

  • Aura and Grandpa.
  • The entire standalone runtime feature. There is now only one runtime
  • The light client (which probably didn't work anyway cite)

Is there something left for follow-up PRs ?

  • Support instant seal. Contract developers may want to spin up a local node for quick development (Like they do with ganache, and like our docs describe). Instant seal will make that a much smoother development process.
  • Explore to what extent the service files can be combined.

What value does it bring to the blockchain users?

Only a single binary to work with whether joining a parachain, launching a parachain, or testing against a simple local node.

Checklist

  • ❌ Does it require a purge of the network?
  • ❌ You bumped the runtime version if there are breaking changes in the runtime ?
  • ✔️ Does it require changes in documentation/tutorials ?

@joelamouche
Copy link
Contributor

Looks good! But I don't understand that comment very well:
"Specifically it removes the ability to start a non-parachain production-ready blockchain network."
We will still be able to spin up a local blockchain without a relaychain, right?

node/src/dev_service.rs Outdated Show resolved Hide resolved
@crystalin
Copy link
Collaborator

If we don't support manual seal, how are we going to test so far ?

@tgmichel
Copy link
Contributor

@crystalin They way I understand this PR is that manual seal still supported through the parachain binary (using the --dev flag, that sets up a special "dev" service), but normal standalone node support is dropped. Is that the idea @JoshOrndorff ?

@joelamouche
Copy link
Contributor

But the tests use the standalone...
Dropping standalone support means switching to using the polkadot-launch package for testing.
Moreover, how does manual seal work for a parachain? The relay chain is waiting for the seal in the same way the parachain is?

@JoshOrndorff JoshOrndorff marked this pull request as ready for review January 29, 2021 21:30
node/src/inherents.rs Outdated Show resolved Hide resolved
node/src/inherents.rs Outdated Show resolved Hide resolved
node/src/service.rs Outdated Show resolved Hide resolved
@crystalin
Copy link
Collaborator

@JoshOrndorff The test is easy to fix if we want to but reflects a problem. The --dev part (aka standalone) is using the parachaunUpgrade.setValidationData inherent. Is this something we want to have ?
In one sense, it is good to have a similar inherent from both our dev and parachain but on the other side having parachain specific inherent in a non-parachain context is weird.
What's your opinion ?

JoshOrndorff and others added 2 commits January 31, 2021 21:45
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
@crystalin
Copy link
Collaborator

If you plan to keep the setValidationData provider for the --dev, you can merge crystalin-unify-binary-fix-tests to fix the test

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Feb 1, 2021

@crystalin thanks for looking into the tests 🙏.

The approach I took in this PR is to treat the parachain runtime as the main product. Thus, in this branch, it is the only runtime. No more standalone feature. In order to make the --dev service work without an actual relay chain, I introduced the MockValidationDataInherentDataProvider which provides fake inherent data.

https://github.com/PureStake/moonbeam/blob/e271bef6844326eb1ea2399585ad55f2da9d5450/node/src/inherents.rs#L117-L123

I think it makes sense for the tests to reflect this. Making these changes in the tests will also prepare them for a future where they are run against an actual parachain which would also have that inherent.

node/src/inherents.rs Outdated Show resolved Hide resolved
@JoshOrndorff JoshOrndorff added A8-mergeoncegreen Pull request is reviewed well. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes C3-medium Elevates a release containing this PR to "medium priority". labels Feb 1, 2021
@crystalin crystalin merged commit 1a8a62b into master Feb 1, 2021
@crystalin crystalin deleted the joshy-unify-binary branch February 1, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes C3-medium Elevates a release containing this PR to "medium priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants