Switch to decoding extrinsics inside runtime instead of kate RPC to a… #182
Conversation
rpc/kate-rpc/src/lib.rs
Outdated
| @@ -131,6 +137,24 @@ where | |||
| } | |||
| } | |||
|
|
|||
| fn app_id_from_opaque(opaque: &OpaqueExtrinsic) -> Result<AppId, String> { | |||
There was a problem hiding this comment.
This fn is unused, I just left it (temporarily) to demonstrate how to only extract the AppId, without decoding the call and triggering BoundedVec limit check in native code.
There was a problem hiding this comment.
maybe to add some todo so we can delete it later if it is not used?
There was a problem hiding this comment.
Let's delete it as we do not use it.
| @@ -173,8 +197,13 @@ where | |||
| .block | |||
| .extrinsics() | |||
| .iter() | |||
| .filter_map(|opaque| UncheckedExtrinsic::try_from(opaque).ok()) | |||
There was a problem hiding this comment.
The problem arises here, filter_map shouln't be used here, there is no correct behavior that skips any of the extrinsics. In this instance (the runtime upgrade bug for increasing tx size), the UncheckedExtrinsic::try_from fails because the decoding happens in the native code, which has an old tx size (BoundedVec bound), while the submitted extrinsic with bigger size was submitted and was checked by runtime and accepted into block.
| .filter_map(|opaque| UncheckedExtrinsic::try_from(opaque).ok()) | ||
| .map(AppExtrinsic::from) | ||
| .map(|e| { | ||
| let appid = submitted_data::extract_app_id::<Runtime>(e).unwrap_or(AppId(0)); |
There was a problem hiding this comment.
Here we decode the xt inside the runtime, so no problem occurs. Also, we don't actually need the extrinsic decoded, we only need AppID of it and use the (already) encoded value to construct the matrix, commitment, etc. The same can also be achieved in native code manually (see example fn above). We would gain some perormance, but we would lose the flexibility of the runtime upgrade changes. Also, results are chached per block, so the performance penalty is somewhat ammortized. Also keep in mind that if the extrinsic doesn't have an AppID, 0 is used. This is expected and is the case for all unsigned extrinsics, while the signed ones apart from Avail Submit Data use 0 as default as well.
There was a problem hiding this comment.
Hi @luka-ethernal
Are you sure this is running inside the runtime (WASM part)?.
I thought the only way to do it is though the self.client().runtime_api().<function>, where the executor decides if we can use native or wasm.
There was a problem hiding this comment.
Well, it has been a month, so I'm no longer sure of anything. But what I'm sure is that I've tested the code and it indeed does invoke runtime api at some point, as the extrinsic gets properly decoded. This means we're no longer tripping the BoundedVec<> limit, which means it's using the constant from newest runtime.
| let calls = block | ||
| .extrinsics() | ||
| .iter() | ||
| .filter_map(|opaque| UncheckedExtrinsic::try_from(opaque).ok()) |
There was a problem hiding this comment.
This seemed to suffer from the same problem, except here, we need calls to be decoded, app_id is not enough. The decoded extrinsics are then sent to runtime to filter out just Avail Submit Data extrinsics, and the proof is then created (also inside runtime). Upon closer inspection, there is already a public function called extrinsics_proof that instead does the decoding also in runtime, which solves the problem. However, that was probably used to save memory, because in this instance, all the extrinsics are sent over to runtime, instead of just Avail data. There's a todo there about exact length iterator requirements, which makes me think that.
There was a problem hiding this comment.
would be good to see what are benchmark performance on extrinsics_proof vs calls_proof
luka-ethernal
requested review from
fmiguelgarcia,
0xSasaPrsic,
markopoloparadox,
prabal-banerjee,
Leouarz and
aterentic-ethernal
|
Hey guys, looking for your comments/discussion on this PR, regarding fixing the bug about runtime upgrades. This PR solves the problem, but there are multiple ways to go about this, and for some, I'm not aware of the performance inpact it may have. I've tested the branch to make sure it works, but I didn't do any benchmarking. |
luka-ethernal
force-pushed
the
luka/fix-proof-generation-on-runtime-upgrade
branch
from
9296ee8
to
5d62401
Compare
|
About this commit Is doing it within the runtime slower than native? Is caching and other features still work the same? Is there any regression test related to this? |
| .filter_map(|opaque| UncheckedExtrinsic::try_from(opaque).ok()) | ||
| .map(AppExtrinsic::from) | ||
| .map(|e| { | ||
| let appid = submitted_data::extract_app_id::<Runtime>(e).unwrap_or(AppId(0)); |
There was a problem hiding this comment.
nit: Could we dedup this from line 201 and 370?
…void problems with runtime upgrades. Don't silently filter out extrinsics. Log issues.
Co-authored-by: Francisco Miguel García <fmiguelgarcia@users.noreply.github.com>
luka-ethernal
force-pushed
the
luka/fix-proof-generation-on-runtime-upgrade
branch
from
30d61f6
to
6d7a0a4
Compare
luka-ethernal
force-pushed
the
luka/fix-proof-generation-on-runtime-upgrade
branch
from
6d7a0a4
to
5944738
Compare
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #182 +/- ##
===========================================
- Coverage 50.05% 49.90% -0.15%
===========================================
Files 71 71
Lines 10063 10093 +30
===========================================
Hits 5037 5037
- Misses 5026 5056 +30
☔ View full report in Codecov by Sentry. |
aterentic-ethernal
requested review from
aterentic
and removed request for
aterentic
…void problems with runtime upgrades. Don't silently filter out extrinsics. Log issues.