Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

Convert Node to Parachain #73

Merged
merged 38 commits into from
Sep 7, 2021
Merged

Convert Node to Parachain #73

merged 38 commits into from
Sep 7, 2021

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Aug 30, 2021

This is a WIP attempt to turn the canvas-node into a Cumulus based parachain. Right now
I'm using the substrate-parachain-template as a reference.

There is a problem with this though. The substrate-parachain-template is using
Substrate branch polkadot-v0.9.9 and the canvas-node needs to follow a fairly recent
version of Substrate in order for the contract functionality to work.

We've worked around this by creating a branch based off Substrate's polkadot-v0.9.9
which cherry-picks the pallet-contracts commit which we need. We've then used this
Substrate branch in BEEFY, Polkadot, and Cumulus.

One thing we can do is wait for Substrate's polkadot-v0.9.10 branch to be cut (which
should happen this week) and that would make it a bit easier to keep the right
dependencies synced up across the three repos.

It looks like the polkadot-v0.9.10 won't have the required changes for us. While the custom
branches work for now, we should really move away from them quickly once polkadot-v0.9.11
is cut.

@HCastano HCastano changed the title Convern Node to Parachain Convert Node to Parachain Aug 30, 2021
@cmichi
Copy link
Contributor

cmichi commented Sep 1, 2021

Wdyt about this?

  • The parachain-template is currently synced to polkadot-v0.9.9.
  • My understanding is that we need only this commit for supporting a contracts parachain: paritytech/substrate@bd69793.
  • We could create a Substrate branch which we branch off Substrate's polkadot-v0.9.9 branch. Maybe something like $name-polkadot-v0.9.9-with-contracts-support.
  • Then we could cherry-pick that one commit to our branch and use that branch in our node's Cargo.toml. The changes in that commit also extend to some other parts of the Substrate codebase, but on first glance it doesn't look problematic.
  • After polkadot-v0.9.10 is released and the parachain-template has been updated, we migrate those dependencies.

@HCastano
Copy link
Contributor Author

HCastano commented Sep 1, 2021

The cherry-picked branch sounds like a good idea! We need to make sure that Cumulus and Polkadot are also up to date. This means we need to:

  1. Create $name-polkadot-v0.9.9-with-contracts-support branch on Substrate
  2. Create a name-polkadot-v0.9.9-... branch on Cumulus which uses the Substrate name-polkadot-v0.9.9-... branch
  3. Create a name-polkadot-v0.9.9-... branch on Polkadot which uses the Substrate name-polkadot-v0.9.9-... branch
  4. Finally, we can use the name-polkadot-v0.9.9-... in the canvas-node and hope everything works out 🤞

node/src/rpc.rs Outdated Show resolved Hide resolved
node/src/chain_spec.rs Outdated Show resolved Hide resolved
node/src/chain_spec.rs Outdated Show resolved Hide resolved
node/src/service.rs Outdated Show resolved Hide resolved
@HCastano
Copy link
Contributor Author

HCastano commented Sep 3, 2021

@TriplEight Hey Denis, do you know why the Linux pipeline is failing? The error is:

cp: cannot stat '/ci-cache/canvas-node/targets/73/build-linux/release/canvas': No such file or directory

@nuke-web3
Copy link
Contributor

Hey guys - I am sure you are aware of the limitations on the PoV size that might block you here, right?
paritytech/substrate#9354

I also am under the impression that other optimizations will be needed and (possibly) some work on the sand-boxing upstream for the contracts pallet in the context of parachains/PoV that are missing.

Happy to be wrong here, many people are looking for contract enabled parachains 😁

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend setting all of these to zero to prevent eviction of contracts:

type TombstoneDeposit = TombstoneDeposit;
type DepositPerContract = DepositPerContract;
type DepositPerStorageByte = DepositPerStorageByte;
type DepositPerStorageItem = DepositPerStorageItem;
type RentFraction = RentFraction;
type SurchargeReward = SurchargeReward;

Also we should set resolver = "2" in our root Cargo.toml. Substrate does that, too.

node/Cargo.toml Outdated Show resolved Hide resolved
node/src/chain_spec.rs Show resolved Hide resolved
node/Cargo.toml Outdated Show resolved Hide resolved
node/Cargo.toml Outdated Show resolved Hide resolved
runtime/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because we shouldn't enable unstable interface by default.

Copy link
Contributor

@riusricardo riusricardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far.

node/src/chain_spec.rs Outdated Show resolved Hide resolved
node/src/chain_spec.rs Outdated Show resolved Hide resolved
node/src/chain_spec.rs Outdated Show resolved Hide resolved
// One XCM operation is 1_000_000 weight - almost certainly a conservative estimate.
pub UnitWeightCost: Weight = 1_000_000;
// One UNIT buys 1 second of weight.
pub const WeightPrice: (MultiLocation, u128) = (X1(Parent), UNIT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constant WeightPrice is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from the parachain template (and is also unused there). I'm gonna leave it here just to keep the diff smaller, but it should be addressed upstream next time the template gets updated

@HCastano HCastano marked this pull request as ready for review September 6, 2021 22:21
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the runtime configuration it looks good to me.

runtime/src/lib.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
HCastano and others added 3 commits September 6, 2021 17:42
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
We'll be creating an updated config later on.
// Give your base currency a unit name and decimal places
let mut properties = sc_chain_spec::Properties::new();
properties.insert("tokenSymbol".into(), "CAN".into());
properties.insert("tokenDecimals".into(), 12.into());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we want to use the relay chain currency here and not have a separate one. Does anybody know if it's sufficient to just remove the properties altogether?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep the properties here and align them with the relay-chain we'll connect to.
E.g. Kusama

let mut properties = sc_chain_spec::Properties::new();
properties.insert("tokenSymbol".into(), "KSM".into());
properties.insert("tokenDecimals".into(), 12.into());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to do native token teleport in order to use the relay-chain token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means we do not need our own faucet but can just point users to the rococo faucet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means we do not need our own faucet but can just point users to the rococo faucet?

That's a good point, hmm, is it even possible to have dedicated parachain endowed accounts then? 🤔

@cmichi cmichi merged commit 38effce into master Sep 7, 2021
@cmichi cmichi deleted the hc-add-parachain-template branch September 7, 2021 08:39
@HCastano HCastano mentioned this pull request Sep 22, 2021
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants