Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Weight annotation. #3157

Merged
merged 45 commits into from
Jul 25, 2019
Merged

Weight annotation. #3157

merged 45 commits into from
Jul 25, 2019

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Jul 21, 2019

TLDR; Closes #2431

This is potentially the final phase of a bunch of PRs regarding weight -> fee -> updates over time. Especially during the final phase, we changed a thing or two during implementation that might not be 100% in line with the specs. Furthermore, the spec itself is not sophisticated and does not cover all the details. First, quoting the important parts of the specs:

Reflection on research

There will be several types of transactions, with different fee levels. This fee differentiation is used to reflect the different costs in resources incurred by transactions, and to encourage/discourage certain types of transactions.

  • We currently have different variations of Normal and Operational dispatches. Normal ones have less quota, pay the full fee and have a priority proportional to their weight. Operational ones can use the full block weight limit (extra 25% atm but can be changed), don't pay the size-fee (again, can be changed) and have maximum priority.

Note that more priority -> more inclusion probability, not a guarantee.

Within each level, different fee levels, as the spec requires, can be defined by simply tuning the weight value in the Fixed variant.

There will be an additional space in each block that is reserved only for crucial or urgent transactions, so that they can be included even if the block is full. Fishermen txs that report misbehaviours would be an example of crucial transaction.

  • Simply Operational transaction.

A transaction fee includes a base fee and a variable fee that is proportional to the number of bytes to be put on chain.

  • Weight-wise, we dropped this half-way through the development and only have fixed weights. It is quite hard to know how much data a transaction will put on chain (e.g. if a key does not exist a storage ::insert is actually putting something while if it already exists, it is not adding any data to the state). Nonetheless, we can add this along the way since the weight interface has access to the input parameters of each dispatch and can examine them. I imagine that we would, at some point, add something like InputOrderOfNormal(X) as opposed to FixedNormal(Y) weight types where the weight of the former is calculated as size_of_some_input_parameter ^ X or size_of_some_input_parameter * X.

Also, we brought back the old substrate fee system which works next to the weight-fee system and takes the length of the transaction into account (base + size * num_bytes). This is promising since now that weights are input-independent, we have some mechanism to protect (and charge money) for processing large transactions. To sum, the final fee is something like: len_fee + weight_fee [+ tip].

Slow adjusting mechanism.

  • Works well. The only question is where to configure the target fullness level (25% -- currently hardcoded as a constant). Also, note that this target fullness level is entirely independent of the quota distinction of Normal and Operational. It simply looks at the full block weight and tweaks the weight fee based on being above or below a threshold.
                                           Block                                         
                              +-------------------------------+                          
                              |                               |                          
                              |                25%            |                          
                              |    Reserved for Operational   |                          
                              --------------------------------|                          
                              |                               |                          
                              |                               |                          
                              |                               |                          
                              |                               |                          
                              |                               |                          
                              |                               |                          
                              |                               |    25%                   
                              |                               | ----------               
                              |                               |weight update target         
                              |                               |                          
                              |                               |                          
                              +-------------------------------+                          

The transaction fee is considered a base price. There will be a different field in the transaction called tip, and a user is free to put any amount of tokens in it or leave it at zero. Block producers can charge both the fee and tip, so they have an incentive to include transactions with large tips.

  • we do this by adding the fee to the priority which, as mentioned, increases the chance of inclusion. Yet, I think this has to be researched more. We are trivially adding two numbers (Balance, Priority) which may or may not be of the same scale.

Not in specs but I remember from some conversations that we might want to allow tips to also allow transactions to go above their quota limit

  • This was a wrong statement. We don't have this and don't want it. Tips only give you a better chance of inclusion. Limits will stay fixed.

Technical challenges left

As mentioned above, there are (at least) two related parameters (system's 3/4 for Normal dispatches and WeightMultiplierUpdate's target) that need to be configured somewhere and I wasn't certain where they belong to. for the former, it makes sense for it to be an associated constant in the system module. But, I want approval on it first since it is a daunting refactor of all tests that use the system module.

Weight values.

The spec only defines the notion of weights but does not indicate how to compute them. We used a minimal methodology to do so with one major goal in mind: be as conservative as possible and take the worst-case scenario of everything into account.

The simple methodology for obtaining weight values is as follows:

  • We benchmarked as many of the dispatchable functions in substrate/Polkadot runtime as possible.

    • Benchmarks are using the native execution environment.
    • Column: #1 Native Runtime Exec Time
  • We assume the target block time to be 2 seconds.

  • We assume that we want each block to be at most full by 25% (same as the weight update target mentioned before) to be conservative. target_block_fullness = 0.25.

  • Due to 1- errors in benchmarks 2- the fact that benchmarks are not really measuring the worst-case scenario 3- caching 4- non-runtime code which is not measured (and evidence showed that it was quite time consuming), we immediately take 1 second apart for any unexpected behavior or delays. Hence, the available time for dispatchable functions to execute is assumed to be 1 second per block.

  • Since weights are (or better to say: should be) a representation of the time it takes to execute a function, we then apply the target_block_fullness to this time, yielding 250_000_000ns as the available_block_execution_time.

  • We define the total weight units available per block to be 1_000_000_000. Applying the same target_block_fullness :

    • total_block_weight = 1_000_000_000.
    • available_block_weight = 250_000_000.
  • We then multiply all execution times values by a wasm-factor, an estimate of how slower web-assembly can be. Currently, this value is 5.

    • Column: Ratio of available block time
  • We divide the benchmarked execution time of each function by available_block_execution_time to obtain what ratio of the block time it consumes.

    • Column: Wasm Runtime Exec Time
  • We multiply each obtained wasm-execution-time ratio per function, by the obtained available_block_weight. This yields the actual weight of each dispatch.

    • Column: Basic Weight
  • We then round + scale-up these numbers by multiplying them to a value between [1,10] manually, in order to:

    • Take the actual implementation into account. Some transactions get a bigger bump here in weight since we know from the code that they are expensive and the benchmarks might not showcase that.
    • Be a bit more pessimistic and round up everything. It is better to be safe than sorry.
    • Column: Basic weight - manual
  • We choose a very small number to be the fee for the absolute cheapest (but not free) transactions: 0.1 Cent given that 1 Unit is 100 Dollars. Based on this, the weights should be scaled one last time to obtain this final fee. This number happens to be 1000.

    • This is not yet implemented in the code yet.
  • Finally, note that:

    • In the code the weights are factored by 1000 for simplicity. As mentioned, there should be a factor of 1000 added as well to correct the fee values.

The above is the outcome of collaboration with @keorn and @mattrutherford and mostly their work. The final results are here:
https://docs.google.com/spreadsheets/d/1h0RqncdqiWI4KgxO0z9JIpZEJESXjX_ZCK6LFX6veDo/edit?usp=sharing

Screenshot 2019-07-21 at 12 42 20

The # To Fill Target Block, # To Exhaust Block and Weight Fee in Cents are interesting.

Of course, this is merely something to start with and I strongly believe that this matter should be dedicatedly researched in more detail.

I will rebase this PR to master if #3102 gets merged in.

gavofyork and others added 25 commits July 11, 2019 13:24
Also Remove old extrinsic types.
* Weight signed extension.

* Revert a bit + test for check era.

* Update Cargo.toml

* Update node/cli/src/factory_impl.rs

* Update node/executor/src/lib.rs

* Update node/executor/src/lib.rs
* working poc added.

* some fixes.

* Update doc.

* Fix all tests + final logic.

* more refactoring.

* nits.

* System block limit in bytes.

* Silent the storage macro warnings.

* More logic more tests.

* Fix import.

* Refactor names.

* Fix build.

* Update srml/balances/src/lib.rs

* Final refactor.
// For the system to be configured in a sane way, `TARGET_BLOCK_FULLNESS` should always be less than
// the ratio that `system` module uses to find normal transaction quota.
/// The block saturation level. Fees will be updates based on this value.
pub const TARGET_BLOCK_FULLNESS: Perbill = Perbill(1_000_000_000 / 4);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the items that we might want to place elsewhere. I think it actually looks okay to be in this crate node/runtime but preferably in a separate consts.rs file? We will probably have other configurable constants in the future as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did as explained for now. Moved some constants to a new file.

Copy link
Member

Choose a reason for hiding this comment

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

language seems to flip between 1/4 of block is reserved for operation transactions and 3/4 of block is reserved. here, for example TARGET_BLOCK_FULLNESS is 25%, indicating 3/4 reserved for operational transactions. however, in the diagram at the top of this page, it says:

25% ... Reserved for Operational

Please consult research team and get this consistent and correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly what I addressed in the comment above this line:

// CRITICAL NOTE: The system module maintains two constants: a _maximum_ block weight and a
// _ratio_ of it yielding the portion which is accessible to normal transactions (reserving the rest
// for operational ones). `TARGET_BLOCK_FULLNESS` is entirely independent and the system module is
// not aware of if, nor should it care about it. This constant simply denotes on which ratio of the
// _maximum_ block weight we tweak the fees. It does NOT care about the type of the dispatch.

To rephrase: They are entirely separate.

System module knows:

  • Each block can be a total of X weight units.
  • It will keep 25% of this only for operational transactions and the rest for normal.

Weight Multiplier:

  • Asks the system module for the total weight units per block.
  • Assumes that if we are 25% of this limit then we are relatively congested and should slightly increase the weight fees.

The only adjustment I see is that the Weight multiplier should query the system module for the available-to-normal-txs portion of weights and set its goal for 25% of that. But I still think it makes more sense like this. Weight multiplier should not care about Operational (heck, it even doesn't know they exist). Only note will be, as commented in the code:

// For the system to be configured in a sane way, `TARGET_BLOCK_FULLNESS` should always be less than
// the ratio that `system` module uses to find normal transaction quota.

Agreeing that the research team should verify the numbers.

Copy link
Member

Choose a reason for hiding this comment

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

ok cool

// maximum that is available to normal transactions. Hence,
let max_weight = <MaximumBlockWeight as Get<u32>>::get() / 4;
let ideal_weight = (max_weight / 4) as u128;
let max_weight = <MaximumBlockWeight as Get<u32>>::get();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that here we are simply getting all the weights, not just the Normal portion.

node/runtime/src/impls.rs Outdated Show resolved Hide resolved
pub const MaximumBlockWeight: Weight = 4 * 1024 * 1024;
pub const MaximumBlockLength: u32 = 4 * 1024 * 1024;
pub const MaximumBlockWeight: Weight = 1_000_000_000;
pub const MaximumBlockLength: u32 = 5 * 1024 * 1024;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that the normal size limit of the node is now 5mb.

@kianenigma
Copy link
Contributor Author

@bkchr only a few of your nits are still to be resolved:

  • Having the fill_block().
  • Tests that have some prints and are sort of a simulation. I feature-gated them in a stress-test feature.

Rest seems fine.

Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

fill_block should contain #![cfg(test)]. Also, println! should be replaced with a proper logger.

/// Note that this is a standard, _potentially-panicking_, implementation. Use `Saturating` trait for
/// safe subtraction.
/// Note that this is a standard, _potentially-panicking_, implementation. Use `Saturating` trait
/// for safe subtraction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should safety be the default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The priority was to be consistent with how the std operations work. This implementation might panic. There are saturating/checked ones that don't.

@@ -102,8 +96,9 @@ pub fn native_version() -> NativeVersion {

parameter_types! {
pub const BlockHashCount: BlockNumber = 250;
pub const MaximumBlockWeight: Weight = 4 * 1024 * 1024;
pub const MaximumBlockLength: u32 = 4 * 1024 * 1024;
pub const MaximumBlockWeight: Weight = 1_000_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma what about using powers of 2 instead?

node/runtime/src/constants.rs Show resolved Hide resolved
if wm == WeightMultiplier::from_rational(-1, 1) { break; }
iterations += 1;
}
println!("iteration {}, new wm = {:?}. Weight fee is now zero", iterations, wm);
Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma what about using debug! from the log crate?

Suggested change
println!("iteration {}, new wm = {:?}. Weight fee is now zero", iterations, wm);
debug!("iteration {}, new wm = {:?}. Weight fee is now zero", iterations, wm);

node/executor/src/lib.rs Show resolved Hide resolved
node/runtime/src/impls.rs Show resolved Hide resolved
subkey/src/main.rs Show resolved Hide resolved
/// A big dispatch that will disallow any other transaction to be included.
// TODO: this must be preferable available for testing really (not possible at the moment).
#[weight = SimpleDispatchInfo::MaxOperational]
fn fill_block(origin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn fill_block(origin) {
fn fill_block(origin) {
#![cfg(test)]

See this Playground example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get the suggestion here; The problem is that:

  • decl_module!{} does not accept such compiler flags.
  • anything that even wraps the internals of the function is useless since the function itself exists and can be called and has a super high weight.
    • The function is not doing anything anyhow.

srml/system/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

@demimarie-parity I replied in one place about the logging matter. Rest either fixed or argued why not. Since your comments were mostly (except for if it is possible to use cfg[test]) not related to the logic but rather style, would be nice if you can re-approve since I am planning I am merging this first thing tmr morning and it is a blocker for v2-k. thanks.

(I don't know why there was a push here but I can recover from the minor build issues arisen from it :p)

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Can go in once resolved and all green.

@bkchr bkchr merged commit f9e5a37 into master Jul 25, 2019
@bkchr bkchr deleted the kiz-annotate-weights branch July 25, 2019 09:58
@gnunicorn gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020
kianenigma added a commit to kianenigma/seeding that referenced this pull request Sep 27, 2022
# Membership Request 

Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 

1. Maintaining and developing the staking sub-system.
2. General FRAME development, especially testing and quality assurance. 
3. Polkadot-native side-projects. 
4. Education 

> My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915

### Staking system

I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. 

Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users.

I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. 

Some notable contributions: 

- paritytech/substrate#4517
- paritytech/substrate#7910
- paritytech/substrate#6242
- paritytech/substrate#9415
- paritytech/polkadot#3141
- paritytech/substrate#11212
- paritytech/substrate#12129

### FRAME 

Historically, I have contributed a variety of domains in FRAME, namely: 

- Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157
- Early version of the transaction fee system
- Primitive arithmetic types paritytech/substrate#3456
- Council election pallet paritytech/substrate#3364

Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. 

These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs:

- paritytech/substrate#8038
- paritytech/substrate#9788
- paritytech/substrate#10174

Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. 

### Polkadot-Native Side Projects

I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. 

- https://github.com/substrate-portfolio/polkadot-portfolio/
- https://github.com/kianenigma/polkadot-basic-notification/
- https://github.com/paritytech/polkadot-scripts/
- https://github.com/paritytech/substrate-debug-kit/

### Education 

Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. 

---

With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well.
bkchr added a commit to polkadot-fellows/seeding that referenced this pull request Sep 27, 2022
# Membership Request 

Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 

1. Maintaining and developing the staking sub-system.
2. General FRAME development, especially testing and quality assurance. 
3. Polkadot-native side-projects. 
4. Education 

> My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915

### Staking system

I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. 

Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users.

I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. 

Some notable contributions: 

- paritytech/substrate#4517
- paritytech/substrate#7910
- paritytech/substrate#6242
- paritytech/substrate#9415
- paritytech/polkadot#3141
- paritytech/substrate#11212
- paritytech/substrate#12129

### FRAME 

Historically, I have contributed a variety of domains in FRAME, namely: 

- Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157
- Early version of the transaction fee system
- Primitive arithmetic types paritytech/substrate#3456
- Council election pallet paritytech/substrate#3364

Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. 

These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs:

- paritytech/substrate#8038
- paritytech/substrate#9788
- paritytech/substrate#10174

Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. 

### Polkadot-Native Side Projects

I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. 

- https://github.com/substrate-portfolio/polkadot-portfolio/
- https://github.com/kianenigma/polkadot-basic-notification/
- https://github.com/paritytech/polkadot-scripts/
- https://github.com/paritytech/substrate-debug-kit/

### Education 

Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. 

---

With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well.

Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine transaction (fee) weights by transaction types
8 participants