-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add parachains modules to Westend and Kusama runtimes #2854
Conversation
22403a6 to
ed5b2d2
Compare
| dispute_post_conclusion_acceptance_period: 1 * HOURS, | ||
| dispute_max_spam_slots: 2, | ||
| dispute_conclusion_by_time_out_period: 1 * HOURS, | ||
| no_show_slots: 2, |
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.
Are we doing okay with only two here? We briefly discussed this but then I think other changed superseded anything considered there, I still like tow if nothing else problematic.
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 thought we'd settled on 2, but I could see bumping it up to 3
runtime/kusama/src/lib.rs
Outdated
| } | ||
|
|
||
| parameter_types! { | ||
| pub const ParaDeposit: Balance = 5 * DOLLARS; |
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.
TODO
runtime/kusama/src/lib.rs
Outdated
| hrmp_max_message_num_per_candidate: 10, | ||
| validation_upgrade_frequency: 1 * DAYS, | ||
| validation_upgrade_delay: EPOCH_DURATION_IN_BLOCKS, | ||
| max_pov_size: 5 * 1024 * 1024, |
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.
We should use the constant MAX_POV_SIZE here to maintain consistency.
| parameter_types! { | ||
| pub const ParaDeposit: Balance = 5 * DOLLARS; | ||
| pub const DataDepositPerByte: Balance = deposit(0, 1); | ||
| pub const MaxCodeSize: u32 = 5 * 1024 * 1024; // 10 MB |
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.
runtime/kusama/src/lib.rs
Outdated
| type IsReserve = (); | ||
| type IsTeleporter = (); | ||
| type LocationInverter = LocationInverter<Ancestry>; | ||
| type Barrier = (); |
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.
TODO: set
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.
AllowPaidFromParachains, probably
| dispute_max_spam_slots: 2, | ||
| dispute_conclusion_by_time_out_period: 1 * HOURS, | ||
| no_show_slots: 2, | ||
| n_delay_tranches: 40, |
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've never added enough to https://github.com/w3f/research-internal/issues/515 to explain this number, but it's sensitive.. too high and too many no shows bog us down.. too low and we're not secure..
I worked out n_active_validators / n_delay_tranches = 2.25 once, although I should redo that computation, so n_delay_tranches = 89 sounds good for kusama right now, but not sure about westend.
We'll want a separate number which is the number of tranches after which we give up and declare the candidate shitty but not yet invalid, and fork the relay chain to dump it.
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'd aim for high rather than low as:
- vast majority of nodes are not malicious, better safe than sorry
- our test networks perform quite well with even hundreds of validators on approval-checking
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 fine with high initially, especially before I've written anything concrete.
If we add 5 validators per tranche, aka per no show, then an adversary with 20% could censor many candidates. As a larger concern, we increase risks that too many existing validators run on insufficient hardware or connections, and could no show by accident, but this requires other techniques I guess.
runtime/kusama/src/lib.rs
Outdated
| dispute_max_spam_slots: 2, | ||
| dispute_conclusion_by_time_out_period: 1 * HOURS, | ||
| no_show_slots: 2, | ||
| n_delay_tranches: 40, |
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.
n_delay_tranches = 89 here. https://github.com/paritytech/polkadot/pull/2854/files#r610163186
| n_delay_tranches: 40, | ||
| zeroth_delay_tranche_width: 0, | ||
| needed_approvals: 15, | ||
| relay_vrf_modulo_samples: 1, |
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.
We want roughly needed_approvals * n_parachains = (relay_vrf_modulo_samples + 1 / n_delay_tranches) * n_active_validators = relay_vrf_modulo_samples * n_active_validators + 2.25
Although the + could disappear if we changed zeroth_delay_tranche_width = 0 to mean delay VRF criteria start in tranche 1, but maybe not worth the trouble.
|
/benchmark runtime westend slots |
|
Error running benchmark: rh-parachains-runtime Merge conflict merging branch to master! |
|
/benchmark runtime westend slots |
|
Finished benchmark for branch: rh-parachains-runtime Benchmark: Benchmark Runtime Westend Pallet cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=slots --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/ ResultsPallet: "slots", Extrinsic: "force_lease", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
gavofyork
left a comment
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.
Aside from the comments appears ok.
node/service/src/chain_spec.rs
Outdated
| pallet_sudo: westend::SudoConfig { | ||
| key: endowed_accounts[0].clone(), | ||
| }, | ||
| //TODO: Don't use 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.
Using the default here seems as good as anything else.
| pub const DOTS: Balance = 1_000_000_000_000; | ||
| pub const DOLLARS: Balance = DOTS / 300; | ||
| pub const CENTS: Balance = DOLLARS / 100; | ||
| pub const UNITS: Balance = 1_000_000_000_000; |
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.
Removed the questionable DOLLARS to leave the (slightly more vague) CENTS and GRAND.
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.
Not everywhere though? (still in Polkadot, Rococo, and Test-Runtime)
| // The AccountId32 location type can be expressed natively as a `Signed` origin. | ||
| SignedAccountId32AsNative<KusamaNetwork, Origin>, | ||
| // A system child parachain, expressed as a Superuser, converts to the `Root` origin. | ||
| ChildSystemParachainAsSuperuser<ParaId, Origin>, |
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.
Do we want this right now?
We seems we will not use it with statemint, so imo better to exclude it for now.
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.
Statemint won't be a system parachain
| SovereignSignedViaLocation<LocationConverter, Origin>, | ||
| ChildParachainAsNative<parachains_origin::Origin, Origin>, | ||
| SignedAccountId32AsNative<WestendNetwork, Origin>, | ||
| ChildSystemParachainAsSuperuser<ParaId, Origin>, |
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.
also can be removed here for now
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 it's best to leave in - the whole point of system parachains is that they can do this.
shawntabrizi
left a comment
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 left comments
Co-authored-by: Shawn Tabrizi <[email protected]>
* master: (98 commits) Companion PR for #8414 - Remove OffencesWeightSoftLimit (#2966) Companion for Substrate#8663 (#2945) Add parachains modules to Westend and Kusama runtimes (#2854) Make KeyStore optional (#2964) v0.9.0 prep (#2959) Implement null weight trader (#2957) feat: add proc macro to reduce overseer mock boilerplate (#2949) Allow normal accounts to execute XCM for withdraw+teleport (#2953) Required weight is returned in case it exceeds limit. (#2952) Bump version, bump substrate & update benchmarks in preparation for v0.8.31 (#2938) Bump BEEFY (#2946) Send statements to own backing group first (#2927) Bump (#2942) Add Dev Config for Rococo and Wococo (#2928) change junction parachain id from named field to unnamed field (#2940) fix link (#2941) Add XCM Origin and converter (#2896) Add Statemint teleport (#2934) CI: build colander on rococo branch too (#2936) Bump BEEFY (#2937) ...
TODO:
HostConfigurationParachainHostAPI to use theruntime_api_implmodule