Allow both consensus and runtime to limit block building #1581
Conversation
srml/system/src/lib.rs
Outdated
| @@ -283,6 +283,14 @@ impl<T: Trait> Module<T> { | |||
| storage::unhashed::get(well_known_keys::EXTRINSIC_INDEX) | |||
| } | |||
|
|
|||
| /// Gets a total length of all executed extrinsics. | |||
| pub fn extrinsics_len() -> u32 { | |||
| let index = Self::extrinsic_index().unwrap_or_default(); | |||
There was a problem hiding this comment.
I'm really hasitant about computing that after every extrinsic, but was unsure if it's ok to use storage for that. Would be storage::unhashed most likely alike EXTRINSIC_INDEX.
Thoughts?
There was a problem hiding this comment.
Better to store the length rather than the data. There will be extrinsics of > 1MB in length and getting them in and out of storage quickly eats up heap and probably has no good effect on the effectiveness of the trie db cache.
gavofyork
added
A5-grumble
and removed
A0-please_review
tomusdrw
added
A0-please_review
| // Verify the signature is good. | ||
| let xt = uxt.check(&Default::default()).map_err(internal::ApplyError::BadSignature)?; | ||
|
|
||
| // Check the size of the block if that extrinsic is applied. | ||
| if <system::Module<System>>::all_extrinsics_len() + encoded_len as u32 > internal::MAX_TRANSACTIONS_SIZE { |
There was a problem hiding this comment.
it would be cool to make this generic so it's not only by block size -- so it could be a dynamic gas limit or something similar. srml-contract could have an inherent for NewGasLimit which always has to be within some range of the old gas limit. And an implementation of the ExtrinsicLimiter trait that checks if the total gas used in the block is up to the gas limit.
There was a problem hiding this comment.
@tomusdrw could we file an issue for that?
There was a problem hiding this comment.
@pepyakin how are srml_contract gas limits currently handled?
There was a problem hiding this comment.
Right now, the node is greedy, it will cram how many extrinsics it can in a block. At the same time, gas block limit in place so the first extrinsics will drain the gas up to the block limit and then all following extrinsics will fail because of lack of gas available in the block.
There was a problem hiding this comment.
later extrinsics should at least fail with a "transaction not valid in this block (but maybe in the future)" error. they should definitely not execute with no side-effects similarly to a failed transfer.
There was a problem hiding this comment.
Yeah, I'm going to do a follow up PR on this to convert this error: https://github.com/paritytech/substrate/blob/master/srml/contract/src/gas.rs#L209 into FullBlock. This would be a temporary patch before #1651 provides a more generic solution.
gavofyork
added
A3-needsresolving
and removed
A0-please_review
|
rebased on master |
…1581) * Limit block size in runtime, * Add test for basic authorship. * Store length of extrinsics instead of computing it. * Don't rely on note_extrinsic * Use hashed version of storage and write test. * Recompile runtime.
Closes #1354
basic_authorship.evaluation)Durationis passed to the proposer from consensus to prevent missing a slot.