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

Change max number of pinned blocks to max number of finalized pinned blocks #2373

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 12, 2022

Close #2217

As suggested here: #2215 (comment)

Since the user can't control the number of non-finalized blocks, it makes sense for the non-finalized blocks to not be counted towards the limit. It is entirely smoldot's fault if the number of non-finalized blocks becomes too high (well, it might be because of a problem with the chain itself, but basically it's smoldot's problem and not the JSON-RPC client's problem).

The limit is now a NonZeroUsize. Previously, if the limit passed as parameter was too low, we would immediately close the subscription. Now, it is impossible to have a limit that is too low, because we always start with exactly 1 finalized pinned block, which is always <= the value inside of a NonZeroUsize.

Note that this doesn't require any change to https://github.com/paritytech/json-rpc-interface-spec/, as the number of pinned blocks is at the discretion of the implementation.

I've also reduced the hardcoded limits at some places, since we no longer take the non-finalized blocks into account.

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
      +11616 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h1d6e99c02f476918
      -11616 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::he90f8ecfe84fa919
      +11199 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h70d54b7c5a373556
      -11199 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hdc88d3c398a0e8a6
       +4643 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h08a2a396caea48e9
       -4643 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h62daf6aecbea5593
       -2567 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hefda7bc9d6db4d0e
       +2529 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h2df0001c0fbc5b31
       +1998 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h881937e0a6dbbdaa
       -1998 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hce2f06a5c9dc4086
       +1909 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hd1292a04ca2b80a2
       -1909 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hd560655a9837dc90
       +1752 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h459ab1ea5671f3ab
       -1752 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hbd017c6f38a5bb7e
       -1535 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hb22b3a289a10ae37
       +1535 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hc1451464f5297055
        -971 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h62aecaac7e805db7
        +971 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hb73c4ab43511774c
        +475 ┊ core::ptr::drop_in_place<core::future::from_generator::GenFuture<smoldot_light_base::sync_service::parachain::start_parachain<smoldot_light_wasm::platform::Platform>::{{closure}}>>::ha085459af51c7049
        -475 ┊ core::ptr::drop_in_place<core::future::from_generator::GenFuture<smoldot_light_base::sync_service::parachain::start_parachain<smoldot_light_wasm::platform::Platform>::{{closure}}>>::hf4fb754a89535b68
         +90 ┊ ... and 91 more.
         -29 ┊ Σ [111 Total Rows]

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Jun 14, 2022
@mergify mergify bot merged commit 1004195 into paritytech:main Jun 14, 2022
@tomaka tomaka deleted the fix-max-pinned branch June 14, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out pinned block limit mechanism in chainHead_unstable_follow
2 participants