RC6 + update module weights #266

vedhavyas · 2020-08-17T10:00:54Z

RC6 Changes from RC3:

TargetedFeeAdjustment is removed and we are using the default one provided with RC6
WeightInfo can be passed to core pallets. While some pallets doesn't use them, pallets like democracy, balances make full use of this feature. I have added the default weight values taken from the substrate node.
SlashDefferedTime must be less than the UnbondingTime.
- Reason: If a validator gets slashed, the slash is deferred for 7day. In the meantime, they can start unbonding and transfer funds since the bonding time is also 7. Hence made SlashDefferedTime to 6 days instead

Weight Changes

With RC3, weights in substrate pallets have changed and so does the WeightToFee calculations. We would need to adjust our own module and our pallet weights accordingly.

How are new weights calculated:

With the new weight changes, the extrinsic bytes fee will be significantly higher since we charge 1 Micro rad per byte.
So for a balance transfer of 1 rad, the extrinsic payload would be around 97 bytes i.e 97 Micro rad just for Byte fee. With adding the remaining BaseFee, WeightFee, we would be charging some around 300 Micro RAD. Right now, we charge 100 Micro RAD.

We can reduce the weight fee coefficient. Making it 0, means, ignoring the weight completely, would still lead to 97 Micro Rad. and that defeats the who purpose of having weights to extrinsic.

Most of the substrate's internal weights are set as per Reads and Writes to DB, and total processing time as well. In order to charge the extrinsic correctly and keeping the new fees in par with the current fees we charge, I have updated

TransactionByteFee -> 0.01 Micro RAD
WeightFee Coefficient -> 315000

These changes will give us more control over how we charge the extrinsic based on their weights rather than extrinsic payload bytes.
With these changes, we will charge around 101.641 Micro RAD for 1 RAD balance transfer. But as the RAD number increases, so does the number of bytes in the extrinsic, which leads to slight increase in fees.

Using the balance.transfer as the base, I have calculated our module weights, multi-account, and chain bridge weights accordingly so that we are charging around the same fee we charging right now on Mainnet.

Anchor:

Module	Old Fee	New Fee
anchors.commit	100 Micro RAD	100.627
anchors.precommit	100 Micro RAD	101.612

Since anchors commit and pre-commit, have the same extrinsic payload bytes, the fee will be similar. but extrinsic that contain huge bytes payload will change much higher.

An example would be system.SetCode. Seems like there are no fees charged for this extrinsic when I check the recent runtime upgrade we made from subscan(correct me if I'm wrong here). With this new weights, we will charging around 632.358 milli RAD.

mikiquantum

LGTM, some minor clarifications.

runtime/src/impls.rs

runtime/src/lib.rs

mikiquantum · 2020-08-25T17:27:58Z

@vedhavyas following this:

WeightInfo can be passed to core pallets. While some pallets doesn't use them, pallets like democracy, balances make full use of this feature. I have added the default weight values taken from the substrate node.

Is it mandatory to pass this type to the runtime pallet? Wouldnt it use the default one if not passed?

type SystemWeightInfo = weights::frame_system::WeightInfo;

vedhavyas · 2020-08-25T18:43:36Z

@mikiquantum true! Updated now

vedhavyas · 2020-08-26T09:45:19Z

runtime/Cargo.toml

-substrate-pallet-multi-account = { version = "2.0.0-rc3-0", git = "https://github.com/centrifuge/substrate-pallet-multi-account", rev = "557b4da832a1d74ed286c3a3a4b20b7d80f47e05", default-features = false }
-chainbridge = { version = "0.0.1", git = "https://github.com/chainsafe/chainbridge-substrate.git", rev = "59fbb767de890b6dcb781fb9e6c4c84a967693ff" , default-features = false}
+substrate-pallet-multi-account = { version = "2.0.0-rc6", git = "https://github.com/centrifuge/substrate-pallet-multi-account", rev = "5b8d12bd0260551f18223001b2c5091ea92fc7d8", default-features = false }
+chainbridge = { version = "0.0.1", git = "https://github.com/vedhavyas/chainbridge-substrate.git", rev = "19df3f21889e60be286f532cd785d5d17ff61c4c" , default-features = false}


Will point to chain bridge once ChainSafe/chainbridge-substrate#64 is merged

philipstanislaus

Thanks @vedhavyas , just left a few comments regarding weights. 👍

philipstanislaus · 2020-08-26T10:23:03Z

runtime/src/anchor/mod.rs

@@ -118,7 +118,7 @@ decl_module! {
        /// # <weight>
        /// minimal logic, also needs to be consume less block capacity + cheaper to make the pre-commits viable.
        /// # </weight>
-        #[weight = 500_000]
+        #[weight = 193_000_000]


Instead of constants, what do you think about using benchmarking as Substrate does to determine the time complexity, and using T::DbWeight::get().reads_writes(1, 1) for storage access? See https://centrifuge.hackmd.io/Mb8IAGhHQ32DEl1GIoBkCQ#Action-items for details.

T::DbWeight::get().reads_writes(1, 1) implies we are doing only one read and one write for which is incorrect. Hence, I would like to keep our weights which were constant before as constants now to imply we are not considering the reads and writes but rather a chosen fixed weight @philipstanislaus

@vedhavyas keeping the weights constant is a bad idea, since we most likely missprice our extrinsics compared to substrate. The issue with that is an attack vector – if you find a severely underpriced extrinsic, you could just send a load of them and the block can't be executed within the time limit (Not sure what happens then though?).

Could you create a ticket to run benchmarks and use the actual reads/writes? Maybe link to that issue: paritytech/substrate#6168

philipstanislaus · 2020-08-26T10:24:24Z

runtime/src/impls.rs

+
+	fn polynomial() -> WeightToFeeCoefficients<Self::Balance> {
+		smallvec!(WeightToFeeCoefficient {
+			coeff_integer: 315000,


I noticed that you came to a different weight to fee constant (315000) from my calculation (625000) – are you sure your calculation is correct? I haven’t looked into it yet, just want to make sure that our fees will not suddenly be much cheaper than before.

Do verify this locally to ensure the fees are not lowered or made much cheaper than we are charging right now in mainnet @philipstanislaus

@vedhavyas done, I confirmed locally that anchor commits and balance transfers stay roughly the same. Thanks!

philipstanislaus · 2020-08-26T10:25:13Z

runtime/src/lib.rs

    impl_version: 0,
    apis: RUNTIME_API_VERSIONS,
-    transaction_version: 0,
+    transaction_version: 1,


Why did this change?

It seems like this is the convention I saw in the node-template. i.e starting the transaction_verison from 1 - https://github.com/paritytech/substrate/blob/master/bin/node-template/runtime/src/lib.rs#L101 @philipstanislaus

philipstanislaus

Thanks 👍

vedhavyas added 5 commits August 17, 2020 15:30

update module weights

571d527

more clarification

e3f04a3

Merge branch 'master' into feat/weights

4241520

update chainbridge dep

804fb42

update spec

46e8e73

vedhavyas marked this pull request as ready for review August 20, 2020 14:15

vedhavyas changed the title ~~WIP: update module weights~~ update module weights Aug 20, 2020

vedhavyas requested review from mikiquantum and philipstanislaus and removed request for mikiquantum August 20, 2020 14:15

mikiquantum approved these changes Aug 20, 2020

View reviewed changes

runtime/src/impls.rs Show resolved Hide resolved

runtime/src/lib.rs Show resolved Hide resolved

migrate to rc6

6accc3e

vedhavyas changed the title ~~update module weights~~ RC6 + update module weights Aug 25, 2020

remove weights and fallback to default

3bdfb6f

mikiquantum mentioned this pull request Aug 26, 2020

WIP: RC6 Migration #268

Closed

4 tasks

add sample fee calculation

0dfe241

vedhavyas commented Aug 26, 2020

View reviewed changes

move anchor rpc to rpc module

c435808

philipstanislaus suggested changes Aug 26, 2020

View reviewed changes

update deps

a909531

philipstanislaus approved these changes Sep 2, 2020

View reviewed changes

vedhavyas merged commit 9923181 into master Sep 2, 2020

vedhavyas deleted the feat/weights branch September 2, 2020 11:42

vedhavyas added the migrations label Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RC6 + update module weights #266

RC6 + update module weights #266

vedhavyas commented Aug 17, 2020 •

edited

Loading

mikiquantum left a comment

mikiquantum commented Aug 25, 2020

vedhavyas commented Aug 25, 2020

vedhavyas Aug 26, 2020

philipstanislaus left a comment

philipstanislaus Aug 26, 2020

vedhavyas Aug 26, 2020

philipstanislaus Sep 1, 2020

philipstanislaus Aug 26, 2020

vedhavyas Aug 26, 2020

philipstanislaus Sep 1, 2020

philipstanislaus Aug 26, 2020

vedhavyas Aug 26, 2020

philipstanislaus left a comment

RC6 + update module weights #266

RC6 + update module weights #266

Conversation

vedhavyas commented Aug 17, 2020 • edited Loading

RC6 Changes from RC3:

Weight Changes

How are new weights calculated:

mikiquantum left a comment

Choose a reason for hiding this comment

mikiquantum commented Aug 25, 2020

vedhavyas commented Aug 25, 2020

Choose a reason for hiding this comment

philipstanislaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipstanislaus left a comment

Choose a reason for hiding this comment

vedhavyas commented Aug 17, 2020 •

edited

Loading