substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Add API for block pinning

Open lexnv opened this issue 3 years ago • 4 comments

The block pinning is a feature required by the RPC Spec V2.

The block pinning feature should be called for subscribers of the chainHead_unstable_follow method.

This PR exposes two methods:

  • pin_block - Ensure the given block is never pruned while it has a positive reference count
  • unpin_block - Decrease the reference count of the block. When the reference hits zero the block is subject to pruning

When a block is pinned it enters the pinning queue (Backend::pinned_blocks) and increments its reference count. While a block is in the pinning queue will not be pruned, regardless of the pruning settings.

When a block is unpinned the block can enter the pruning queue (Backend::pruning_queue) iff:

  • reference count is zero
  • pruning was delayed for the block previously

The PR builds upon the state_at function that is not keeping the block's body around.

Testing Done

  • test_pinned_blocks_on_finalize
  • test_pinned_blocks_on_finalize_with_fork
  • custom testing with the new RPC PoC branch

Part of #12475.

lexnv avatar Oct 11 '22 17:10 lexnv

I guess this is mostly needed to pin unfinalized blocks.

A lot of this logic, including maintaining pinned set and reference counting is already in StateDb. Would it be possible to reuse it? StateDb could be made to expose a set of pinned blocks.

arkpar avatar Oct 12 '22 08:10 arkpar

I guess this is mostly needed to pin unfinalized blocks.

Yep, that should be the main use case the way I see it. It might also be possible for the client to keep around a finalized block that would exceed the pruning window of BlocksPruning::Some(value).

A lot of this logic, including maintaining pinned set and reference counting is already in StateDb. Would it be possible to reuse it? StateDb could be made to expose a set of pinned blocks.

That does indeed make sense, I'll have a go at it!

Thanks for the feedback!

lexnv avatar Oct 12 '22 09:10 lexnv

There a couple of additional issues with this PR:

  1. The spec:

The current finalized block reported in the initialized event, and each subsequent block reported with a newBlock event, is automatically considered by the JSON-RPC server as pinned.

How exactly is this going to be implemented with this API? The RPC layer can't just call pin when it receives the new block notification. The notification is asynchronous, and by the time it is consumed the block may be already gone.

  1. If the node quits or crashes with some blocks pinned, they won't ever be removed. This is not a problem for state data, as it is copied into memory for pinned blocks in state-db, but it looks like a problem for bodies and justifications. There are a few of ways to fix this:
  • Mark pinned blocks in the database somehow and process them on start.
  • Prune them right away, but save the content in memory so it can be served.
  • Don't bother with pinning block bodies at all.

It seems to me it that the whole thing should rather be implemented as a simple delay before blocks are garbage collected. Instead of discarding all unfinalized sibling chains of some block N as soon as it is finalized, we'd discard them when e.g. N+64 is finalized.

@tomaka @bkchr what do you think?

arkpar avatar Oct 12 '22 10:10 arkpar

How exactly is this going to be implemented with this API? The RPC layer can't just call pin when it receives the new block notification. The notification is asynchronous, and by the time it is consumed the block may be already gone.

We could check whether the block still exists when processing the notification, and ignore that notification if it doesn't. The important thing is that we report a consistent view for new blocks, so it's safe to not report new blocks if we are sure that we will not report its descendants later on.

it looks like a problem for bodies and justifications

Note that the API doesn't give you a way to retrieve justifications right now, and I don't think it's a useful feature for the chainHead API but could be added to the archive API later on.

Prune them right away, but save the content in memory so it can be served.

This seems sensible to me. As far as I know we don't expect the block bodies and all to occupy a lot of memory.

It seems to me it that the whole thing should rather be implemented as a simple delay before blocks are garbage collected. Instead of discarding all unfinalized sibling chains of some block N as soon as it is finalized, we'd discard them when e.g. N+64 is finalized.

This works as well. The server can send a "stop" if a JSON-RPC client still has a block N pinned when N+64 is finalized.

tomaka avatar Oct 12 '22 11:10 tomaka

How exactly is this going to be implemented with this API? The RPC layer can't just call pin when it receives the new block notification. The notification is asynchronous, and by the time it is consumed the block may be already gone.

While this is right, this is also right for everything else in the node. IMO it would probably be nice to have all blocks pinned that are still in some "notification" in the node. When the last notification for a block is dropped, the pinning could be stopped.

bkchr avatar Nov 08 '22 20:11 bkchr

While this is right, this is also right for everything else in the node. IMO it would probably be nice to have all blocks pinned that are still in some "notification" in the node. When the last notification for a block is dropped, the pinning could be stopped.

This is what I've been doing in smoldot, for what it's worth:

Everything that needs to be aware of the latest blocks needs to use an API similar to the chainHead_unstable API, plus a name in order to identify offenders that keep blocks pinned for too long:

  • https://github.com/paritytech/smoldot/blob/a25bd32cde94d6f7c3e4024607df52ea6db6472b/bin/light-base/src/runtime_service.rs#L190-L214
  • https://github.com/paritytech/smoldot/blob/a25bd32cde94d6f7c3e4024607df52ea6db6472b/bin/light-base/src/runtime_service.rs#L575-L594
  • https://github.com/paritytech/smoldot/blob/a25bd32cde94d6f7c3e4024607df52ea6db6472b/bin/light-base/src/runtime_service.rs#L374-L384

It works pretty well and is IMO a good pattern.

tomaka avatar Nov 08 '22 21:11 tomaka

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 09 '22 01:12 stale[bot]