Skip to content

UncheckedExtrinsic: lazily decode the call#7902

Closed
serban300 wants to merge 7 commits intoparitytech:masterfrom
serban300:unchecked-extrinsic
Closed

UncheckedExtrinsic: lazily decode the call#7902
serban300 wants to merge 7 commits intoparitytech:masterfrom
serban300:unchecked-extrinsic

Conversation

@serban300
Copy link
Copy Markdown
Contributor

@serban300 serban300 commented Mar 13, 2025

Related to #4255

This PR changes some core logic in order to lazily decode the RuntimeCall inside an UncheckedExtrinsic

This is a significant rework of #4296. The main problem with #4296 was that it was always decoding a call at least twice. This PR comes with a different approach, that avoids the need to decode twice.

@serban300 serban300 added the T17-primitives Changes to primitives that are not covered by any other label. label Mar 13, 2025
@serban300 serban300 self-assigned this Mar 13, 2025
@serban300 serban300 marked this pull request as ready for review March 13, 2025 09:29
@serban300 serban300 requested a review from a team as a code owner March 13, 2025 09:29
@serban300 serban300 force-pushed the unchecked-extrinsic branch 5 times, most recently from 368c27a to 4e1cafb Compare March 13, 2025 13:36
@serban300 serban300 changed the title [Draft][WIP] UncheckedExtrinsic: lazy decoding + set a memory limit when decoding the call [Draft][WIP] UncheckedExtrinsic: lazily decode and limit the call Mar 13, 2025
@serban300 serban300 force-pushed the unchecked-extrinsic branch 2 times, most recently from 520f415 to 099a094 Compare March 13, 2025 16:42
@serban300 serban300 force-pushed the unchecked-extrinsic branch from 099a094 to dbee656 Compare March 14, 2025 07:50
@serban300 serban300 added the I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. label Mar 14, 2025
@serban300 serban300 force-pushed the unchecked-extrinsic branch 2 times, most recently from 506a5bf to 51b5cb4 Compare March 14, 2025 13:32
@serban300 serban300 force-pushed the unchecked-extrinsic branch from 51b5cb4 to 1b70876 Compare March 26, 2025 07:34
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 26, 2025 07:35
@serban300 serban300 force-pushed the unchecked-extrinsic branch 4 times, most recently from bb5a405 to 7a77636 Compare March 28, 2025 06:31
@serban300 serban300 changed the title [Draft][WIP] UncheckedExtrinsic: lazily decode and limit the call UncheckedExtrinsic: lazily decode and limit the call Mar 28, 2025
@serban300
Copy link
Copy Markdown
Contributor Author

Marked the PR as ready for review

@bkchr @ggwpez @gui1117 since you have been the main reviewers of the previous issues related to DecodeWithMemTracking, could you also take a look on this PR when you have time please ?

@serban300 serban300 force-pushed the unchecked-extrinsic branch 2 times, most recently from 5930635 to 000d75c Compare July 11, 2025 09:01
@serban300 serban300 force-pushed the unchecked-extrinsic branch 4 times, most recently from c729ad0 to c8053ff Compare July 14, 2025 07:51
github-merge-queue bot pushed a commit that referenced this pull request Jul 14, 2025
Stumbled upon this while working on other issue
(#7902). I thought I
might need to change the `CheckInherentsResult` and this deduplication
would have made everything easier. Probably changing
`CheckInherentsResult` won't be needed in the end, but even so it would
be nice to reduce the duplication.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@redzsina redzsina moved this from Scheduled to Backlog in Security Audit (PRs) - SRLabs Jul 14, 2025
@serban300 serban300 force-pushed the unchecked-extrinsic branch 6 times, most recently from c9f3ede to 5fe90bf Compare July 16, 2025 07:42
serban300 and others added 7 commits July 31, 2025 09:37
Define:
- TransactionPayment::query_info_from_runtime_api()
- TransactionPayment::query_fee_details_from_runtime_api()
Define:
- InherentDataExt::check_extrinsics_from_runtime_api()
@serban300 serban300 force-pushed the unchecked-extrinsic branch from 5fe90bf to ce076ac Compare July 31, 2025 06:42
Copy link
Copy Markdown
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When looking at this pr again, we only need the lazy decoding in two situations:

  1. execute_block
  2. validate_block

Instead of concentrating the "fallout" on these two situations, right now we just change everything. However, I don't see the need given that we only need this in two situations. Or do I miss something?

Something like is_inherent and other calls should not be changed at all, because when we reach them the extrinsic should be already decoded and available.

@serban300
Copy link
Copy Markdown
Contributor Author

When looking at this pr again, we only need the lazy decoding in two situations:

1. `execute_block`

2. `validate_block`

Instead of concentrating the "fallout" on these two situations, right now we just change everything. However, I don't see the need given that we only need this in two situations. Or do I miss something?

Something like is_inherent and other calls should not be changed at all, because when we reach them the extrinsic should be already decoded and available.

Thanks for the suggestion ! Opened a new PR that implements this approach: #9480

Closing this PR in favor of the new one

@serban300 serban300 closed this Aug 15, 2025
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
…8234)

Closes #7360 

This PR:
- sets a memory limit when decoding the `RuntimeCall` inside an
`UncheckedExtrinsic`
- simplifies some logic

This PR splits some logic from #7902. The purpose of doing this is to
make it possible to review the logic incrementally, which should be
easier.
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
Stumbled upon this while working on other issue
(#7902). I thought I
might need to change the `CheckInherentsResult` and this deduplication
would have made everything easier. Probably changing
`CheckInherentsResult` won't be needed in the end, but even so it would
be nice to reduce the duplication.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2025
Related to #4255

Addresses
#7902 (review)

This PR enables us to lazily decode the block extrinsics when needed.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T17-primitives Changes to primitives that are not covered by any other label. T18-zombienet_tests Trigger zombienet CI tests.

Projects

Development

Successfully merging this pull request may close these issues.

5 participants