Unchecked extrinsic: lazily decode the Call#4296
Unchecked extrinsic: lazily decode the Call#4296serban300 wants to merge 12 commits intoparitytech:masterfrom serban300:unchecked-extrinsic
Conversation
serban300
added
R0-no-crate-publish-required
serban300
changed the title
serban300
changed the title
|
I think we can do a quick check on how many times the decode_function is getting called per each extrinisc? Just add a log statement and run the node and submit transaction and check the logs. If it is 2 or 3, I think it is fine. If it is say 10, then we should optimize something. |
Sounds good. Will do that. |
|
Added a log statement and started a Rococo + Bridge Hub zombienet. This is what I'm getting in the bridge hub collator logs for a random block for example: Some calls are decoded 5 times, some 7 times, some 1 time. I'll check where this calls come from. Later edit: 21 times for |
|
Did some optimizations in this commit Now it's something like: The number of decodings for each function is down to around 8 times. But from what I understand this is only happening for inherents because of calls to |
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
We should be more careful with these changes. They introduce a lot of extra decodings that mainly can be prevented if we rewrite some code.
|
|
||
| /// Implementation for unchecked extrinsic. | ||
| impl<Address, Call, Signature, Extra> GetDispatchInfo | ||
| impl<Address, Call: Encode + Decode, Signature, Extra> GetDispatchInfo |
There was a problem hiding this comment.
| impl<Address, Call: Encode + Decode, Signature, Extra> GetDispatchInfo | |
| impl<Address, Call: Decode, Signature, Extra> GetDispatchInfo |
There was a problem hiding this comment.
Did you check if we maybe can remove this implementation? That we need to decode here again the call to get this info is a little bit inconvenient.
There was a problem hiding this comment.
We can't remove it. It is needed here. One option would be to cache the dispatch info.
| // We use the dedicated `is_inherent` check here, since just relying on `Mandatory` dispatch | ||
| // class does not capture optional inherents. | ||
| let is_inherent = System::is_inherent(&uxt); | ||
| if !<frame_system::Pallet<System>>::inherents_applied() && !System::is_inherent(&uxt) { |
There was a problem hiding this comment.
So here we decode the function and then do it directly again inside check below.
There was a problem hiding this comment.
Fixed together with the comment regarding EnsureInherentsAreFirst. Added a way of caching the decoded function.
| pub trait ExtrinsicCall: sp_runtime::traits::Extrinsic { | ||
| /// Get the call of the extrinsic. | ||
| fn call(&self) -> &Self::Call; | ||
| fn call(&self) -> Self::Call; |
There was a problem hiding this comment.
We should probably return Result here to make it more obvious that this maybe includes decoding or maybe any better approach.
There was a problem hiding this comment.
I wouldn't return result since it would complicate things and normally it should also be decodable. We could rename the method maybe. WDYT ?
| if let Some(call) = IsSubType::<_>::is_sub_type(&call) { | ||
| if <#pallet_names as ProvideInherent>::is_inherent(&call) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
EnsureInherentsAreFirst below should be able to be replaced with some logic in Executive. Otherwise we are decoding twice.
| input.remaining_len()?.and_then(|a| before_length.map(|b| (b, a))) | ||
| let mut input = CopyReader::new(input); | ||
| // We have to check that the function decodes correctly. | ||
| Call::decode(&mut input)?; |
There was a problem hiding this comment.
We really finally need to get some proper skip implemented.
There was a problem hiding this comment.
Could you expand on this please ? I'm not sure if I understand.
There was a problem hiding this comment.
If you mean to just skip the encoded function bytes without decoding it, the problem is that we will have to return a Result<> from the methods that decode the function and:
- I'm not sure how hard it will be to propagate this
Result<>to all the call sites - I'm not sure if it's ok to panic at a later time for example if the function can't be decoded
I will look into it
There was a problem hiding this comment.
Ok, I think I finally got it. I replaced this with Call::skip(&mut input), but from what I understand it still does decode internally. I agree we should implement a proper skip, but maybe in a separate PR ? I guess it's something that we should do in the parity-scale-codec crate, right ?
| if let Some(call) = xt.call().is_sub_type() { | ||
| match call { | ||
| crate::Call::set_validation_data { data: validation_data } => | ||
| return validation_data.clone(), | ||
| _ => {}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Please keep the old logic with an early return on finding the set_validation_data. Now we decode multiple calls before we check them.
There was a problem hiding this comment.
Also you don't need to clone validation_data because you own the call
There was a problem hiding this comment.
Please keep the old logic with an early return on finding the
set_validation_data. Now we decode multiple calls before we check them.
Reverted to something closer to the old logic.
Also you don't need to clone
validation_databecause you own thecall
The problem is that is_sub_type() returns a reference. And the call is owned by the current method. I couldn't find a straightforward way to avoid cloning. There would be options like:
- adding a
try_into_sub_type()method - Returning the
calland executingis_sub_type()again from the calling function - moving this logic outside of the function. I see that it's only used once.
- some
unsafe {}code block maybe
Moving the logic out of the function seems like the easiest thing right now in my opinion.
serban300
left a comment
serban300
left a comment
There was a problem hiding this comment.
Addressed only some of the easier comments so far
|
|
||
| /// Implementation for unchecked extrinsic. | ||
| impl<Address, Call, Signature, Extra> GetDispatchInfo | ||
| impl<Address, Call: Encode + Decode, Signature, Extra> GetDispatchInfo |
There was a problem hiding this comment.
We can't remove it. It is needed here. One option would be to cache the dispatch info.
| pub trait ExtrinsicCall: sp_runtime::traits::Extrinsic { | ||
| /// Get the call of the extrinsic. | ||
| fn call(&self) -> &Self::Call; | ||
| fn call(&self) -> Self::Call; |
There was a problem hiding this comment.
I wouldn't return result since it would complicate things and normally it should also be decodable. We could rename the method maybe. WDYT ?
| input.remaining_len()?.and_then(|a| before_length.map(|b| (b, a))) | ||
| let mut input = CopyReader::new(input); | ||
| // We have to check that the function decodes correctly. | ||
| Call::decode(&mut input)?; |
There was a problem hiding this comment.
Could you expand on this please ? I'm not sure if I understand.
| if let Some(call) = xt.call().is_sub_type() { | ||
| match call { | ||
| crate::Call::set_validation_data { data: validation_data } => | ||
| return validation_data.clone(), | ||
| _ => {}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Please keep the old logic with an early return on finding the
set_validation_data. Now we decode multiple calls before we check them.
Reverted to something closer to the old logic.
Also you don't need to clone
validation_databecause you own thecall
The problem is that is_sub_type() returns a reference. And the call is owned by the current method. I couldn't find a straightforward way to avoid cloning. There would be options like:
- adding a
try_into_sub_type()method - Returning the
calland executingis_sub_type()again from the calling function - moving this logic outside of the function. I see that it's only used once.
- some
unsafe {}code block maybe
Moving the logic out of the function seems like the easiest thing right now in my opinion.
5 participants
Related to #4255
I hope I understood the idea correctly.
There are a 2 things I should mention:
I think in the initial discussion, the idea was to store the encoded
Calldirectly as a blob, without decoding it. However I think we need to decode it, in order to check if it's valid. I'm talking about this line. If we're not sure if it's valid, thedecode_function()method will have to return aResultand I don't know if this would be acceptable for some of the call sites.With this new approach we have to decode the
Calleach time it's needed. So there can be a performance penalty more or less significant depending on how often the decoded call is actually needed. If it's not accessed very often, this implementation should be ok. If it's accessed often, we can try to cache the decoded call also. However for caching the decoded call there are 2 options, and both are problematic:decode_call()method to take amutreference toself. And in many places we don't have access to themut UncheckedExtrinsicno-stdenvironment