Per-transaction weight for srml #2799

kianenigma · 2019-06-05T11:43:03Z

How it works:

decl_module! now understands #[weight = $x:expr]
weight values, per transaction, are attached to Call and fetched and examined in executive's apply_extrinsic.
The weight used to indicate block fullness,

I've added a usage example in srml/example. The documentation there should help.

For now, this is technically not changing any logic since the default #[weight] annotation puts {base_weight: 0, byte_weight: 1} and MAX_TRANSACTION_WEIGHT is 4 * 1024 * 1024 => we are still enforcing 4mb block size (current limit is 4mb per block).

Up next

As soon as possible:

Annotations should be added based on benchmarking results (@mattrutherford @lsaether @keorn) and the correct value for MAX_TRANSACTION_WEIGHT.
base_fee and byte_fee would be deprecated in favor of fee_multiplier and henceforth: fee = weight * fee_multiplier. fee_multiplier has some embedded logic and must be updated per-block based on the size of previous blocks. Block fullness targeting with fee multiplier #2430 explains it.

Then:

Lacks some testing primitives (real client and real runtime) that have to be added by the time all of the above checkpoints are done. Either I or @tomusdrw will do it.
At last: Optional tip in the transaction.

xlc · 2019-06-05T12:11:49Z

It is possible somehow incorporated this with smart contract transactions? The (maximum) weight of a smart call is not determine by size, but rather by parameter gas limit.
Same for many other transactions, the resource usage is determine by the parameters, have little to do with size.

kianenigma · 2019-06-05T12:19:19Z

It is possible somehow incorporated this with smart contract transactions? The (maximum) weight of a smart call is not determine by size, but rather by parameter gas limit.
Same for many other transactions, the resource usage is determine by the parameters, have little to do with size.

As far as I know, no, since this is meant to be sth flexible (for general substrate users) and at the end of the day to serve Polkadot's fee and block fullness target, both of which being radically different between the runtime abstraction and smart contract afaik. cc @pepyakin ^

core/sr-primitives/src/weights.rs

tomusdrw · 2019-06-05T12:28:36Z

core/sr-primitives/src/weights.rs

+	fn calculate_weight(self) -> Weight {
+		match self {
+			TransactionWeight::Basic(w) => w,
+			TransactionWeight::Max => (4 * 1024 * 1024, 0),


Should this use a constant?

Also it seems pretty tricky to define it like that - it means that this transaction can only be included solo in a block. The documentation though kind of implies that it can be included whenever, but when it's included no more transactions can.

I --honestly-- didn't bother now with a constant since even the value needs to change. Same goes for the semantic meaning (your point is valid, it should be less than the actual maximum). These are placeholders until some actual benchmarking results come out.

At the end of the day, weight should not really be directly connected/correlated with block size, so having the maximum of it as 4 mb is already moot.

Will make it a const, but really have no idea for the value. The documentation is correct (we want some weight that says okay this goes in but nothing else), implementation cannot be correct for now.

tomusdrw · 2019-06-05T12:30:45Z

core/sr-primitives/src/weights.rs

+/// a _dispatchable_ function (anything inside `decl_module! {}`) that can be weighted.
+/// A type implementing this trait can _optionally_ be passed to the as
+/// `#[weight = X]`. Otherwise, default implementation will be used.
+pub trait WeighableTransaction {


I don't feel this trait is very useful. The WeighableCall interfaces is limitting this stuff to linear cost of base and bytes , I think that it might be worth allowing users to specify different cost calculations that are based on the call and it's encoded len, which is not possible currently.

I still think it's unnecessary. WeighableCall and WeighableTransaction look almost identical - they just have different method names and in transaciton self is consumed (for no apparent reason).

I'd unify this into Weighable and implement for multiple things if needed.

xlc · 2019-06-05T12:48:33Z

I don’t think this flexible because otherwise it should be able support contract case no problem.

srml/example/Cargo.toml

core/sr-primitives/src/generic/checked_extrinsic.rs

srml/support/src/dispatch.rs

bkchr · 2019-06-05T14:15:45Z

srml/support/src/dispatch.rs

 					]),
 					documentation: DecodeDifferent::Encode(&[]),
 				},
+				FunctionMetadata {


Do we need the weight in the metadata?

CC @jacogr

no clue. I just added it to pass the test tbh and assumed it happens by default.

If this is affecting fee calculation (down the line), it should be made available. (There is obviously a downstream effect for any metadata version bumps, which this should be if the structure changes - at least the JS and Python metadata parsers should be updated to cater for the new v5)

@jacogr, for now, it is not affecting the fee calculation and at least in this PR I don't intend to do that but rather build the building blocks. Though, for sure, very soon the fees will be affected by the weight. Not sure what this means for js/client side.

Ok, so then I would suggest not yet exposing the weights on the metadata. For each change on the metadata formats, we basically -

need to bump the metadata version

downstream projects need to implement new versions of the Metadata (unlike Substrate, they need to support all. This also includes stuff like Polkascan, just like the polkadotjs/api they support from v0 all the way up to current v4)

I'm not against a v5 bump now the works needs to be done at some point anyway, however if things may change again and end up with a v6, maybe not - especially not if the stuff is not used atm.

Without the metadata changes (where there go down the wire), as it stands (and yes, looked at what the code does), there is no impact as of yet.

Excited to see how this evolves, it is a really cool approach.

@bkchr how can one prevent this information to sneak into metadata then?

pepyakin · 2019-06-05T14:23:15Z

Thanks for the heads-up @xlc. I think the current plan for the smart-contract is to tap on the weight mechanism, although we would need to add some customization here.

tomusdrw

Looks good! Few more minor nitpicks.

I'm wondering if weight should affect transaction fee or transaction priority as well - seems it's currently not taken into account, however you can fill up the entire block and pay little for some kinds of transactions (i.e. short encoded_len, but high weight)

tomusdrw · 2019-06-11T07:03:27Z

core/sr-primitives/src/weights.rs

+/// a _dispatchable_ function (anything inside `decl_module! {}`) that can be weighted.
+/// A type implementing this trait can _optionally_ be passed to the as
+/// `#[weight = X]`. Otherwise, default implementation will be used.
+pub trait WeighableTransaction {


I still think it's unnecessary. WeighableCall and WeighableTransaction look almost identical - they just have different method names and in transaciton self is consumed (for no apparent reason).

I'd unify this into Weighable and implement for multiple things if needed.

srml/executive/src/lib.rs

srml/staking/src/lib.rs

srml/system/src/lib.rs

tomusdrw · 2019-06-11T07:10:48Z

core/sr-primitives/src/weights.rs

+	fn calculate_weight(self, len: usize) -> Weight {
+		match self {
+			TransactionWeight::Basic(base, byte) => base + byte * len as Weight,
+			TransactionWeight::Max => 3 * 1024 * 1024,


I just realised that we could calculate max given current block weight (we would have to pass it to the function along with len) - then you could just return whatever is left in a block.

I don't like it that much though, due to additional argument that has to be passed so if we have a better solution for longer term or we are satisfied enough with this, then let's just leave it as is.

What you propose will actually work (haven't tried but shouldn't have a problem).

The potential long term solution is the other approach I explained here. I think in that design the weight calculation is internal to the Module<T> and can simply query system. I am still not fully convinced of how that should work (+ if it is absolutely needed) hence rather get this stable and clean first to be able to proceed to fee + tip steps and then refactor if needed (hopefully a decision I won't regret ♻️)

kianenigma · 2019-06-11T09:49:32Z

I'm wondering if weight should affect transaction fee or transaction priority as well - seems it's currently not taken into account, however you can fill up the entire block and pay little for some kinds of transactions (i.e. short encoded_len, but high weight)

Yeah, these are the next steps, see the top PR description for the roadmap.

tomusdrw

lgtm!

bkchr

Some last nitpicks.

core/sr-primitives/src/weights.rs

bkchr · 2019-06-11T20:15:38Z

node/runtime/src/lib.rs

 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
 	spec_version: 92,


I think we should bump the spec.

srml/example/src/lib.rs

core/sr-primitives/src/weights.rs

Co-Authored-By: Bastian Köcher <[email protected]>

Co-Authored-By: Amar Singh <[email protected]>

Co-Authored-By: Bastian Köcher <[email protected]>

…tx-weight-fee

* debug checkpoint. * new * Worked. * Worked and weight propagated to executive. * Works with some tests. * Cleanup debug prints. * More cleanup. * Undo more logs. * Undo a few more. * Fix build. * Allow len to be used in weight calculation. * Remove noop function from dispath. * Cleanup. * Unify traits. * Update docs and nits. * line width * Update core/sr-primitives/src/weights.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update core/sr-primitives/src/weights.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update core/sr-primitives/src/weights.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update core/sr-primitives/src/weights.rs Co-Authored-By: Amar Singh <[email protected]> * Update srml/example/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Final cleanup. * Fix build.

kianenigma added 10 commits May 31, 2019 17:37

debug checkpoint.

769c9bf

new

eb97440

Worked.

9a1b4f9

Worked and weight propagated to executive.

04a64ad

Works with some tests.

0dad22b

Cleanup debug prints.

f546e10

Master.into()

89dd83b

More cleanup.

dea4232

Undo more logs.

2ba3087

Master.into()

da9b861

kianenigma requested review from bkchr and tomusdrw June 5, 2019 11:43

devops-parity added the A4-gotissues label Jun 5, 2019

Undo a few more.

6f8f2e5

kianenigma added A0-please_review Pull request needs code review. and removed A4-gotissues labels Jun 5, 2019

devops-parity added the A4-gotissues label Jun 5, 2019

tomusdrw reviewed Jun 5, 2019

View reviewed changes

Fix build.

64796e2

gui1117 reviewed Jun 5, 2019

View reviewed changes

srml/example/Cargo.toml Outdated Show resolved Hide resolved

bkchr reviewed Jun 5, 2019

View reviewed changes

kianenigma added 2 commits June 5, 2019 17:43

Allow len to be used in weight calculation.

07a72cb

Remove noop function from dispath.

f53ef4b

kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jun 5, 2019

Cleanup.

620e64e

kianenigma added A0-please_review Pull request needs code review. and removed A4-gotissues labels Jun 9, 2019

tomusdrw reviewed Jun 11, 2019

View reviewed changes

Unify traits.

56495ab

kianenigma added 2 commits June 11, 2019 11:55

Update docs and nits.

ae124ea

line width

389e225

kianenigma requested a review from tomusdrw June 11, 2019 12:03

tomusdrw approved these changes Jun 11, 2019

View reviewed changes

4meta5 mentioned this pull request Jun 11, 2019

WIP: Transaction Fee Multiplier #2847

Closed

bkchr approved these changes Jun 11, 2019

View reviewed changes

4meta5 reviewed Jun 12, 2019

View reviewed changes

core/sr-primitives/src/weights.rs Outdated Show resolved Hide resolved

kianenigma and others added 8 commits June 12, 2019 13:01

Update core/sr-primitives/src/weights.rs

66e957e

Co-Authored-By: Bastian Köcher <[email protected]>

Update core/sr-primitives/src/weights.rs

1658bc6

Co-Authored-By: Bastian Köcher <[email protected]>

Update core/sr-primitives/src/weights.rs

c05f0f9

Co-Authored-By: Bastian Köcher <[email protected]>

Update core/sr-primitives/src/weights.rs

18f8242

Co-Authored-By: Amar Singh <[email protected]>

Update srml/example/src/lib.rs

5f69fe4

Co-Authored-By: Bastian Köcher <[email protected]>

Final cleanup.

152c1ab

Master.into()

f14236b

Merge branch 'tx-weight-fee' of github.com:paritytech/substrate into …

dcdad9d

…tx-weight-fee

devops-parity added the B2-breaksapi label Jun 12, 2019

Fix build.

18cecd1

kianenigma added A1-onice and removed A0-please_review Pull request needs code review. B2-breaksapi labels Jun 12, 2019

kianenigma merged commit 56b0273 into master Jun 12, 2019

kianenigma deleted the tx-weight-fee branch June 12, 2019 12:38

4meta5 mentioned this pull request Jun 12, 2019

Transaction Fee Multiplier #2854

Merged

3 tasks

kianenigma mentioned this pull request Jun 23, 2019

Tipped Transaction Type. #2930

Closed

3 tasks

Per-transaction weight for srml #2799

Per-transaction weight for srml #2799

Uh oh!

Conversation

kianenigma commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it works:

Up next

Uh oh!

xlc commented Jun 5, 2019

Uh oh!

kianenigma commented Jun 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlc commented Jun 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepyakin commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma commented Jun 11, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kianenigma commented Jun 5, 2019 •

edited

Loading

pepyakin commented Jun 5, 2019 •

edited

Loading