Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ignore extrinsicIndex in multiBlockMigrations event #1541

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Nov 9, 2024

Description

Closes #1538

Issue

In the blocks endpoint, we return a list of extrinsics included in the queried block and under each extrinsic their related event. However, during multiBlockMigrations we encounter cases where events reference extrinsic indexes that do not exist.
In the code, this situation is represented by having an event with :

  • phase set as ApplyExtrinsic here (which means we expect an extrinsic associated with this event)
  • then we retrieve the extrinsic index here
  • the index retrieved in this case is 2 but there is no extrinsic in index 2 so we return an error msg

Proposed Solution

During multiBlockMigrations :

  • we ignore the extrinsic index since it is a non existent one anyway
  • we also ignore the multiblockmigrations / MigrationAdvanced event (shown in Subscan here) since we cannot associate it with any valid/existing extrinsic.

In the code, we do that by :

  • not throwing an error if we find an event with a non existent extrinsic during multiBlockMigrations
  • push the event only if the extrinsic is present.

This does not break any past logic since the error message will be thrown in all other cases.

Testing

While connected to Shiden network (e.g. through wss://rpc.shiden.astar.network), query block 7675808 http://127.0.0.1:8080/blocks/7675808.

Before the fix we would get :

Missing extrinsic 2 in block 0xc26eb7b19378e953c80ef9749df2b5ae67a7bdb17b3ad9b9a0ee9c1810438a9f

After the fix :

...
...
"extrinsics": [
    {
      "method": {
        "pallet": "timestamp",
        "method": "set"
      },
     ...
     ...
      },
      "events": [
        {
          "method": {
            "pallet": "system",
            "method": "ExtrinsicSuccess"
          },
...
...
{
      "method": {
        "pallet": "parachainSystem",
        "method": "setValidationData"
      },
...
...
"events": [
        {
          "method": {
            "pallet": "system",
            "method": "ExtrinsicSuccess"
          },

The response now returns correctly the 2 extrinsics present in this block and their associated events. There is also the multiblockmigrations / MigrationAdvanced event (mentioned earlier) but we chose to ignore it since we cannot attach it to any valid/existing extrinsics.

Downside

This solution has the downside that we are not showing an existing event.

Long term Solution

As soon as this PR paritytech/polkadot-sdk#3666 gets merged, I will expect that multiBlockMigrations events will have phase set as ApplyInherent so we will not have this issue for future blocks. However this fix needs to be kept for historic blocks.

Todos

@Imod7 Imod7 requested a review from a team as a code owner November 9, 2024 08:33
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@IkerAlus IkerAlus left a comment

Choose a reason for hiding this comment

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

LGTM

@Imod7 Imod7 merged commit 45c4b1f into master Nov 10, 2024
17 checks passed
@Imod7 Imod7 deleted the domi-fix-phase branch November 10, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sidecar fails to handle multi block migration events
3 participants