Operational Transaction; second attempt #3138
Conversation
srml/system/src/lib.rs
Outdated
| let added_weight = info.weight; | ||
| let next_weight = current_weight.saturating_add(added_weight); | ||
| let limit = match info.class { | ||
| DispatchClass::Operational => T::MaximumBlockWeight::get(), |
There was a problem hiding this comment.
Should there be any limit at all for operational transactions?
There was a problem hiding this comment.
There is: the idea is that there is a maximum block weight limit. Normal transaction are only allowed to fill a quarter of that (or any ratio). The operational ones can fill the whole limit. In essence, the limit for operational ones is now 4 times more.
There was a problem hiding this comment.
@AlistairStewart is this right? ^^^
There was a problem hiding this comment.
I think @lsaether @keorn must verify this. But based on my discussion with them it is certainly correct that:
If a block has X weight units available, in normal conditions, we only allow 1/4 to be filled.
What's new is that:
- What we added as operational txs that can spike to the maximum. This was concluded based on our discussion @gavofyork and not in the specs. But I think it is a reasonable solution for the problem of always allowing important reports to get in.
- We should also allow tipped transactions to go beyond the limit (specs clearly say that). But since we don't have realistic weight numbers yet it is hard to know exactly how. Although we can also simply add a fixed number of extra weight units to the threshold of tipped transactions and let them compete for it.
There was a problem hiding this comment.
This wasn't my understanding at all. We were thinking of havin reserved space or even no limit for important transactions. But the 1/4 was the target fullness that we adjusted transaction fees for, not a hard limit. Tips just prioitize transactions in case the maximum has been reached. But maybe @keorn or @AlfonsoCev have a later design.
srml/balances/src/lib.rs
Outdated
| fn compute_fee(len: usize, weight: Weight, tip: T::Balance) -> T::Balance { | ||
| // length fee | ||
| let len = T::Balance::from(len as u32); | ||
| let len_fee = T::TransactionBaseFee::get() |
There was a problem hiding this comment.
should be mutated based on DispatchInfo. i.e. have fn pay_base_fee() -> bool in DispatchInfo, to allow the possibility of truly zero-cost transactions.
There was a problem hiding this comment.
I thought of such a design a while ago but the problem for that would be that all of the parameters need to be passed in, since the DispatchInfo does not have access to them. It will be: DispatchInfo::get_base_fee<B>(len: usize, base_fee: B, byte_fee: B)
There was a problem hiding this comment.
(unless if we want to move fee-related associated constants stuff into to core, which I don't see good)
There was a problem hiding this comment.
I think the best is to set the design as follows:
Signed Extension only sees DispatchClass and won't interact with SimpleDispatchInfo directly (since it actually can't see it). If you want truly zero-cost transactions: This should be reflected in a new DispatchClass::UserFree , DispatchClass::OperationalFree. A bit verbose but keeps things where they belong.
btw. I am not sure if we want such transactions actually. You can have a zero-weight-fee, the decision to bring back the traditional base + byte is what is preventing us from having it. It might still be sensible to say: you might get away with the weight fee, but you always pay byte fee (it should be smaller anyhow).
srml/balances/src/lib.rs
Outdated
| impl<T: Trait<I>, I: Instance + Clone + Eq> SignedExtension for TakeFees<T, I> { | ||
| type AccountId = T::AccountId; | ||
| type AdditionalSigned = (); | ||
| fn additional_signed(&self) -> rstd::result::Result<(), &'static str> { Ok(()) } | ||
|
|
||
| fn pre_dispatch( |
There was a problem hiding this comment.
why introduce this? by default it just calls validate.
There was a problem hiding this comment.
Because I am proposing that maybe the logic in them is slightly different: In validate:
- Can just call
ensure_can_withdraw. - Omit the imbalance stuff.
If not reasonable, then it should be reverted and only use validate().
There was a problem hiding this comment.
fair enough, but then maybe there should be an auxilliary to deduplicate the code.
srml/system/src/lib.rs
Outdated
| let current_len = Module::<T>::all_extrinsics_len(); | ||
| let added_len = len as u32; | ||
| let next_len = current_len.saturating_add(added_len); | ||
| if next_len > T::MaximumBlockSize::get() { |
There was a problem hiding this comment.
probably want to do the same as with weight and have 3/4 reserved for operational transactions.
…e into kiz-operational-tx-2
* Make extrinsics extensible. Also Remove old extrinsic types. * Rest of mockup. Add tips. * Fix some build issues * Runtiem builds :) * Substrate builds. * Fix a doc test * Compact encoding * Extract out the era logic into an extension * Weight Check signed extension. (#3115) * 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 * Don't use len for weight - use data. * Operational Transaction; second attempt (#3138) * 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. * Bump transaction version * Fix weight mult test. * Fix more tests and improve doc. * Bump. * Make some tests work again. * Fix subkey. * Remove todos + bump. * Ignore expensive test. * Bump.
* Make extrinsics extensible. Also Remove old extrinsic types. * Rest of mockup. Add tips. * Fix some build issues * Runtiem builds :) * Substrate builds. * Fix a doc test * Compact encoding * Extract out the era logic into an extension * Weight Check signed extension. (#3115) * 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 * Don't use len for weight - use data. * Operational Transaction; second attempt (#3138) * 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. * Bump transaction version * Fix weight mult test. * Fix more tests and improve doc. * Bump. * Make some tests work again. * Fix subkey. * Remove todos + bump. * First draft of annotating weights. * Refactor weight to u64. * More refactoring and tests. * New convert for weight to fee * more tests. * remove merge redundancy. * Fix system test. * Bring back subkey stuff. * a few stress tests. * fix some of the grumbles. * Final nits. * Update srml/system/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> * Scale weights by 1000. * Bump. * Fix decl_storage test.
to be merged with #3102
As mentioned: Instead of passing down merely a weight down the line:
If all looks good I should finalize it by: