Skip to content

Combine standalone and parachain node#204

Merged
crystalin merged 47 commits into
masterfrom
joshy-unify-binary
Feb 1, 2021
Merged

Combine standalone and parachain node#204
crystalin merged 47 commits into
masterfrom
joshy-unify-binary

Conversation

@JoshOrndorff

@JoshOrndorff JoshOrndorff commented Jan 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

joelamouche commented Jan 26, 2021

Copy link
Copy Markdown
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?

nanocryk
nanocryk reviewed Jan 26, 2021
View reviewed changes
Comment thread node/src/dev_service.rs Outdated
@crystalin

crystalin commented Jan 26, 2021

Copy link
Copy Markdown
Collaborator

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

@tgmichel

tgmichel commented Jan 26, 2021

Copy link
Copy Markdown
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

joelamouche commented Jan 26, 2021

Copy link
Copy Markdown
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 mentioned this pull request Jan 26, 2021
Closed
3 tasks
@crystalin crystalin mentioned this pull request Jan 28, 2021
Merged
3 tasks
@JoshOrndorff JoshOrndorff marked this pull request as ready for review January 29, 2021 21:30
peektism
peektism reviewed Jan 30, 2021
View reviewed changes
Comment thread node/src/inherents.rs Outdated
peektism
peektism reviewed Jan 30, 2021
View reviewed changes
Comment thread node/src/inherents.rs Outdated
peektism
peektism reviewed Jan 30, 2021
View reviewed changes
Comment thread node/src/dev_service.rs
peektism
peektism reviewed Jan 30, 2021
View reviewed changes
Comment thread node/src/service.rs Outdated
@crystalin

crystalin commented Feb 1, 2021

Copy link
Copy Markdown
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
@JoshOrndorff @peektism
Update node/src/inherents.rs
98153f5 
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
@JoshOrndorff @peektism
Update node/src/inherents.rs
e271bef 
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
@JoshOrndorff JoshOrndorff mentioned this pull request Feb 1, 2021
Closed
@crystalin

crystalin commented Feb 1, 2021

Copy link
Copy Markdown
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

JoshOrndorff commented Feb 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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.

JoshOrndorff
JoshOrndorff commented Feb 1, 2021
View reviewed changes
Comment thread node/src/inherents.rs Outdated
@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
@peektism peektism mentioned this pull request Feb 2, 2021
Merged
@JoshOrndorff JoshOrndorff mentioned this pull request Mar 12, 2021
Merged
@JoshOrndorff JoshOrndorff mentioned this pull request Nov 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crystalin crystalin crystalin approved these changes

@tgmichel tgmichel Awaiting requested review from tgmichel

+3 more reviewers

@notlesh notlesh notlesh left review comments

@nanocryk nanocryk nanocryk left review comments

@peektism peektism peektism left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JoshOrndorff @joelamouche @crystalin @tgmichel @notlesh @nanocryk @peektism