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

rpc: Use the blocks pinning API for chainHead methods #13233

Merged
lexnv merged 48 commits into from
May 2, 2023
Merged

rpc: Use the blocks pinning API for chainHead methods #13233

Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
a72b8a8
rpc/chain_head: Add backend to subscription management
lexnv Jan 24, 2023
d672047
rpc/chain_head: Pin blocks internally and adjust testing
lexnv Jan 24, 2023
c9a0d7e
client/in_mem: Reference for the number of pinned blocks
lexnv Jan 24, 2023
3536c94
rpc/tests: Check in-memory references to pinned blocks
lexnv Jan 24, 2023
bdfdb7d
rpc/chain_head: Fix clippy
lexnv Jan 25, 2023
43d6825
rpc/chain_head: Remove unused comment
lexnv Jan 25, 2023
6bc2e87
rpc/chain_head: Place subscription handle under `Arc` and unpin block…
lexnv Jan 25, 2023
9015061
rpc/tests: Check all pinned blocks are unpinned on drop
lexnv Jan 25, 2023
09773d3
Apply suggestions from code review
lexnv Jan 27, 2023
4a38133
Update client/rpc-spec-v2/src/chain_head/subscription.rs
lexnv Jan 27, 2023
e509399
rpc/tests: Retry fetching the pinned references for CI correctness
lexnv Jan 27, 2023
3c2cc6c
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Jan 30, 2023
c6bfb6f
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
Feb 7, 2023
09fe732
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
Feb 14, 2023
b7dcacd
client/service: Use 512 as maximum number of pinned blocks
lexnv Feb 14, 2023
d67c6ed
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Mar 17, 2023
fa5323f
chain_head: Fix merging conflicts
lexnv Mar 17, 2023
852c302
rpc/chain_head: Adjust subscriptions to use pinning API
lexnv Mar 20, 2023
cdd1428
rpc/chain_head/tests: Test subscription management
lexnv Mar 20, 2023
ad9e921
rpc/chain_head: Adjust chain_head follow to the new API
lexnv Mar 20, 2023
866bb87
rpc/chain_head: Adjust chain_head.rs to the new API
lexnv Mar 20, 2023
b2e08ce
rpc/chain_head/tests: Adjust test.rs to the new API
lexnv Mar 20, 2023
b63f102
client/builder: Use new chainHead API
lexnv Mar 20, 2023
c46d7d4
rpc/chain_head: Fix documentation
lexnv Mar 20, 2023
7da0406
rpc/chain_head: Fix clippy
lexnv Mar 20, 2023
5561e56
client/in_mem: ChainHead no longer uses `in_mem::children`
lexnv Mar 21, 2023
d36f748
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Mar 23, 2023
ff3c9d5
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Mar 28, 2023
9781584
Update client/rpc-spec-v2/src/chain_head/subscription.rs
lexnv Mar 30, 2023
cc31043
Update client/rpc-spec-v2/src/chain_head/subscription.rs
lexnv Mar 30, 2023
efb14c4
Update client/rpc-spec-v2/src/chain_head/subscription.rs
lexnv Mar 30, 2023
10b61d5
Update client/rpc-spec-v2/src/chain_head/subscription.rs
lexnv Mar 30, 2023
c5266cc
chain_head: Add block state machine
lexnv Mar 30, 2023
f5a911f
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Mar 30, 2023
7bf9da0
Address feedback
lexnv Apr 24, 2023
6c054ba
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Apr 24, 2023
6a3f9e5
Use new_native_or_wasm_executor
lexnv Apr 24, 2023
d287533
chain_head: Remove 'static on Backend
lexnv Apr 25, 2023
6073ae2
chain_head: Add documentation
lexnv Apr 25, 2023
871dcb8
chain_head: Lock blocks before async blocks
lexnv Apr 25, 2023
083e3c3
chain_head_follower: Remove static on backend
lexnv Apr 25, 2023
1bb4b00
Update client/service/src/builder.rs
lexnv Apr 25, 2023
04a167c
Update client/service/src/builder.rs
lexnv Apr 25, 2023
cc24ad1
chain_head: Add BlockHeaderAbsent to the PartialEq impl
lexnv Apr 25, 2023
b3ca736
client: Add better documentation around pinning constants
lexnv Apr 25, 2023
3b1764f
chain_head: Move subscription to dedicated module
lexnv Apr 25, 2023
9cd53f1
Merge remote-tracking branch 'origin/master' into lexnv/chainhead_pin…
lexnv Apr 25, 2023
8b0eaff
subscription: Rename global pin / unpin functions
lexnv Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Apply suggestions from code review 
Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
@lexnv @bkchr
lexnv and bkchr authored Jan 27, 2023
commit 09773d354890e55d653147c484e2f044f128e56a
2 changes: 1 addition & 1 deletion client/rpc-spec-v2/src/chain_head/subscription.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl<Block: BlockT, BE: Backend<Block> + 'static> SubscriptionHandle<Block, BE>
let mut inner = self.inner.write();

if inner.blocks.len() == inner.max_pinned_blocks {
// We have reached the limit. However, the block can be already inserted.
// We reached the limit. However, the block could already be pinned.

if inner.blocks.contains(&hash) {
Copy link
Member

@bkchr bkchr Jan 27, 2023

Choose a reason for hiding this comment

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

Here is a logic error? If we try to pin the same block twice, the first unpin will unpin it in the backend (assuming it is only pinned by this subscription). Is this documented in the spec that when you receive the same block as best and some seconds later as finalized (without having called unpin in between), you only need to call unpin once?

Copy link
Contributor Author

@lexnv lexnv Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch! I couldn't find something super specific in the RPC spec regarding this, tho I was expecting users to call unpin only once (when they are no longer interested in using the block).

From the spec:

Note: A JSON-RPC client should call chainHead_unstable_unpin only after it is sure to no longer be interested in a certain block. This typically happens after the block has been finalized or pruned. There is no requirement to call chainHead_unstable_unpin as quickly as possible.

I was also thinking along the lines: the newBlock event is where the block is declared and pinned; and bestBlockChanged and finalized are updates about what happens with that block.

I think we could update the spec documentation for the unpin method to state that only block hashes reported by the newBlock event are supposed to be unpinned.

What do you think about it?

Copy link
Contributor

@jsdw jsdw Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was also thinking along the lines: the newBlock event is where the block is declared and pinned; and bestBlockChanged and finalized are updates about what happens with that block.

It sounds reasonable to me that:

  • a block is pinned just prior to a client being given a newBlock event,
  • later we might see best/finalised events emitted re that block (and no pinning happens at this point),
  • at any point after the newBlock, whenever a user would like to unpin that block hash they will call "unpin" once against the hash to unpin it.

But, if a newBlock is unpinned quickly and then a best/finalized event is emitted for it.. should it need unpinning again? I think offhand I'd go with "it's been unpinned so it stays unpinned, and those events are just notifications about the block you've already declared you don't care about any more".

But that would make it annoying for people to ignore all but best/finalized blocks! Because the simplest logic to do so would be to call "unpin" on all "newBlock" events and not on any best/finalized block events.

"unpin" could potentially take both a block hash and a type (new/best/finalized) so that you can opt-out of all block events you don't care about but still keep pinned the ones you do?

Or, a best/finalized notificiation could pin the block again, and "unpin" just takes the hash, but that runs the risk of race conditions (I "unpin" a newBlock but by the time the unpin request hits the node ,a bestBlock for that hash is emitted).

So two options then are:

  • Blocks are only pinned at "newBlock". Unpin will unpin, and best/finalized events may exist for blocks that have already been unpinned.
  • new/best/finalized blocks are treated as independent. "unpin" takes a hash and type to unpin exactly one of those.

Maybe there's a simpler way around this?

Copy link
Member

@bkchr bkchr Jan 27, 2023

Choose a reason for hiding this comment

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

I was also thinking along the lines: the newBlock event is where the block is declared and pinned; and bestBlockChanged and finalized are updates about what happens with that block.

I think we should pin on the first event we see (a client can only see bestBlock/finalized without newblock by having connected after the block was imported) and then unpin should be called when they are not interested anymore. We should update the spec to mention this behavior.

Copy link
Contributor Author

@lexnv lexnv Jan 30, 2023

Choose a reason for hiding this comment

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

Yep, we could update the spec to reflect something along the lines:
"Any block hash reported by chainHead's events is guaranteed to be pinned in memory and it requires a single chainHead_unstable_unpin call to unpin the block."

The spec states currently:

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.

I think we should pin on the first event we see

Yep, that's along the lines of how we pin internally for this chainhead subscription.

a client can only see bestBlock/finalized without newblock by having connected after the block was imported

Before using the subscriptions (best block and finalized) we are:

  • generating finalized event (and pin the finalized block)
  • generate newBlock event for each children of the finalized recursively (and pin the block)

I might be wrong, but I think its entirely possible that we can miss blocks (warp sync etc) and such we always pin the block on the first event that we get from the subscriptions.
In case of errors or jumps, the chainHead should produce the Stop events.

Let me know if that is correct, I ll take an action to create a PR to clarify a bit the spec, thanks!

Copy link
Contributor Author

@lexnv lexnv Feb 8, 2023

Choose a reason for hiding this comment

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

Another way would be to allow users to call unpin only for hashes reported by the Finalized event: paritytech/json-rpc-interface-spec#32. Let me know if this makes a bit more sense, thanks!

Copy link
Contributor

@jsdw jsdw Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

and then unpin should be called when they are not interested anymore.

What if they call unpin after receiving eg a newBlock, and then the server sends a finalized with the same hash? I assume that the block is still unpinned even though the client has been told about it again.

If I was only interested in finalized blocks, and did not want to have any non-finalized blocks pinned any longer than necessary, what would I do?

I gather from chatting with Alex that that what I'd do is hold off on calling "unpin" on anything until I see a finalized notification (which also lists all blocks that are "pruned"), and then I could use that list to unpin all blocks that are listed as "pruned" now that I know they aren't important any more. (This relies on that list of pruned blocks really just being a list of all blocks not in the finalized chain, since if they are pinned they wouldn't be pruned). Does that make sense?

Another way would be to allow users to call unpin only for hashes reported by the Finalized event: paritytech/json-rpc-interface-spec#32. Let me know if this makes a bit more sense, thanks!

I think github is being slow; this message popped up after I wrote mine :) Also seems like a very valid approach so long as the finalized event def lists all of the blocks produced since the last finalized event that aren't on the finalized chain (so we know what we can safely unpin now). A nice thing about this is that the client is never sent a hash to a block that's already been unpinned (since they can't unpin until they won't hear any more about the block anyway!).

return Ok(false)
Expand Down