substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Published 20 hours ago • verified publisher icon paritytech

Storage change notifications rework

Open koute opened this issue 3 years ago • 6 comments

This PR reworks the way storage change notifications work.

Old behavior (before this PR):

  1. A new subscription is made through the RPC endpoint; the values for keys to which the subscription was made are fetched from the current best block and sent to the client.
  2. On every imported block (regardless of what kind of block it is) all of the matching storage changes are sent to the client.

New behavior (after this PR):

  1. A new subscription is made through the RPC endpoint; the values for keys to which the subscription was made are fetched from the current best block and sent to the client. (So same behavior as before.)
  2. On every new imported best block: a) if the previously sent block is a direct parent of this block send all of the matching storage changes to the client. (So same behavior as before.) b) if the previously sent block is not a direct parent of this block fetch all of the keys to which the client is subscribed from the storage and send them. (This might generate superfluous notifications but ensures that the clients' view of the storage is consistent.)

Fixes https://github.com/paritytech/substrate/issues/10743

Internal changes/implementation notes

  • Previously the internal subscription mechanism supported subscribing to child storage changes, but you couldn't subscribe to those through the RPC (the changes were filtered out) and this functionality is not used anywhere else. Since it was essentially dead code I didn't bother adding it back in this rewrite.

  • The previously unbound queues for storage notifications are now bounded. When the number of pending messages exceeds the maximum capacity of the queue it is simply cleared and the values are fetched from the storage once it's unclogged. (So basically it acts just as the 2b case.)

  • All of the necessary fetches from the storage are done on the receiving end to allow for multiple subscribers to fetch the values they need in parallel and not bottleneck the block import codepath.

  • The subscriptions track the current best block. I know we'd like to remove the concept of best blocks from the client, but I've made it follow the best block anyway due to the following reasons:

    • It closest matches what the code was doing originally (that is: sending out notifications for freshly imported blocks instead of waiting for e.g. finalization)
    • It should be more efficient; if there are no chain reorganizations it makes it possible to not have to query the storage and instead we just use the list of changes generated when the block was imported to generate the notifications. (So it works as it used to work.)

    We can always change it later; the code was deliberately written in a way to make that relatively easy to do.

  • Behavior of wildcard subscribers is unchanged; I've just split that up into its own separate file and its own stream type.

koute avatar May 10 '22 08:05 koute

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 12 '22 09:06 stale[bot]

@koute could you merge master?

bkchr avatar Jun 13 '22 08:06 bkchr

Okay, I've retriggered the test and they all pass now.

koute avatar Jun 14 '22 11:06 koute

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jul 15 '22 01:07 stale[bot]

not stale

koute avatar Jul 15 '22 07:07 koute

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Sep 14 '22 20:09 stale[bot]

Ah, oops. :cold_sweat: Not stale. Sorry, I will get back to this PR soon-ish.

koute avatar Sep 29 '22 05:09 koute

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Oct 29 '22 09:10 stale[bot]

Yes, still active.

bkchr avatar Oct 29 '22 22:10 bkchr

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Nov 29 '22 01:11 stale[bot]

not stale

the-right-joyce avatar Nov 29 '22 11:11 the-right-joyce

The CI pipeline was cancelled due to failure one of the required jobs. Job name: cargo-check-benches Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2104673

paritytech-cicd-pr avatar Dec 01 '22 04:12 paritytech-cicd-pr

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Dec 31 '22 04:12 stale[bot]

not stale

koute avatar Jan 09 '23 04:01 koute

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Feb 08 '23 14:02 stale[bot]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 16 '23 01:03 stale[bot]