Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add API for block pinning #12476

Closed
wants to merge 9 commits into from
Closed

Add API for block pinning #12476

wants to merge 9 commits into from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Oct 11, 2022

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.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Oct 11, 2022
@lexnv lexnv self-assigned this Oct 11, 2022
@lexnv lexnv requested review from bkchr and skunert October 11, 2022 17:48
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@bkchr bkchr requested a review from arkpar October 12, 2022 07:49
@arkpar
Copy link
Member

arkpar commented Oct 12, 2022

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.

@lexnv
Copy link
Contributor Author

lexnv commented Oct 12, 2022

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!

@arkpar
Copy link
Member

arkpar commented Oct 12, 2022

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?

@tomaka
Copy link
Contributor

tomaka commented Oct 12, 2022

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.

@bkchr
Copy link
Member

bkchr commented Nov 8, 2022

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.

@tomaka
Copy link
Contributor

tomaka commented Nov 8, 2022

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:

It works pretty well and is IMO a good pattern.

@stale
Copy link

stale bot commented Dec 9, 2022

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 stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 9, 2022
@stale stale bot closed this Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants