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

optimize extrinsics decoding and storage indexing #462

Merged

Conversation

xcaptain
Copy link
Contributor

@xcaptain xcaptain commented Apr 29, 2022

  1. Ignore extrinsics decoding errors and move forward rather than abort and retry.
  2. Index storage in ascending order and with a fixed limit(1000) to avoid too many blocks not in storage table.
  3. Construct wasm executor from runtime configs rather than hard-coded values.

@xcaptain xcaptain changed the title ignore extrinsics decode failure optimize extrinsics decoding and storage indexing Apr 29, 2022
@insipx
Copy link
Collaborator

insipx commented May 1, 2022

This looks good to me, some simple and effective changes

@xcaptain
Copy link
Contributor Author

xcaptain commented May 4, 2022

Added a small improvement, please take a look again.

@jsdw
Copy link
Collaborator

jsdw commented May 18, 2022

I think you'll need to run/resolve cargo fmt and cargo clippy. I'm not sure what's going on with the tests offhand!

@xcaptain
Copy link
Contributor Author

xcaptain commented May 18, 2022

The new commit may fix the CI error. The reason is in previous commits I changed behavior of missing_storage_blocks, add one more condition: 9ca2795#diff-c1534283c28a073545083cd49a8459f91e52899eb9289b7098327c814d061dc7R199

Shouldn't add the `block_num >= (select max(block_num) ...)` condition
in the previous commits, in some situations `extrinsics` and `storage`
table may not increase in ascending order
@xcaptain
Copy link
Contributor Author

xcaptain commented May 18, 2022

Considered twice, it's better not to add the AND block_num >= (SELECT MAX()...) condition. Adding it won't make the sql execution faster.
@insipx @jsdw please take a look again

@insipx
Copy link
Collaborator

insipx commented May 19, 2022

LGTM. I would just make sure that if archive stops unexpectedly or is stopped, then we don't miss any storage blocks since we specify that we restore at MAX(block_num).

As long as we always have contiguous blocks in storage it's ok to index from MAX(block_num) in storage. Have you ran archive to test & make sure that this holds? @xcaptain

@xcaptain
Copy link
Contributor Author

@insipx I'm currently using with the > max(block_num)... condition, it works well. But maybe the reason is I only have one substrate-archive process running.I'm afraid that in some scenes multiple processes update the storage table will make it not contiguous which then causes storage missing.

I don't know how to simulate the scene, it just occurred to me that adding this condition won't give us too much benefit but may introduce potential problems.

@insipx
Copy link
Collaborator

insipx commented May 19, 2022

If you're running with only one storage indexer than I believe you are correct. If a later block finishes execution before an earlier block and is inserted first, then a gap is created which restore_missing_blocks would miss if indexing from MAX(block_num)

@insipx insipx merged commit 1c80b3b into paritytech:master May 19, 2022
@xcaptain xcaptain deleted the feature/ignore-extrinsics-decode-failure branch May 20, 2022 02:28
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.

3 participants