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

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

Merged
merged 48 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 25 additions & 3 deletions client/api/src/in_mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ impl<Block: BlockT> blockchain::Backend<Block> for Blockchain<Block> {
}

fn children(&self, _parent_hash: Block::Hash) -> sp_blockchain::Result<Vec<Block::Hash>> {
unimplemented!()
Ok(Default::default())
}

fn indexed_transaction(&self, _hash: Block::Hash) -> sp_blockchain::Result<Option<Vec<u8>>> {
Expand Down Expand Up @@ -626,20 +626,36 @@ where
states: RwLock<HashMap<Block::Hash, InMemoryBackend<HashFor<Block>>>>,
blockchain: Blockchain<Block>,
import_lock: RwLock<()>,
pinned_blocks: RwLock<HashMap<Block::Hash, i64>>,
}

impl<Block: BlockT> Backend<Block>
where
Block::Hash: Ord,
{
/// Create a new instance of in-mem backend.
///
/// # Warning
///
/// For testing purposes only!
pub fn new() -> Self {
Backend {
states: RwLock::new(HashMap::new()),
blockchain: Blockchain::new(),
import_lock: Default::default(),
pinned_blocks: Default::default(),
}
}

/// Return the number of references active for a pinned block.
///
/// # Warning
///
/// For testing purposes only!
pub fn pin_refs(&self, hash: &<Block as BlockT>::Hash) -> Option<i64> {
let blocks = self.pinned_blocks.read();
blocks.get(hash).map(|value| *value)
}
}

impl<Block: BlockT> backend::AuxStore for Backend<Block>
Expand Down Expand Up @@ -789,11 +805,17 @@ where
false
}

fn pin_block(&self, _: <Block as BlockT>::Hash) -> blockchain::Result<()> {
fn pin_block(&self, hash: <Block as BlockT>::Hash) -> blockchain::Result<()> {
let mut blocks = self.pinned_blocks.write();
blocks.entry(hash).and_modify(|counter| *counter += 1).or_insert(1);
lexnv marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
}

fn unpin_block(&self, _: <Block as BlockT>::Hash) {}
fn unpin_block(&self, hash: <Block as BlockT>::Hash) {
let mut blocks = self.pinned_blocks.write();
blocks.entry(hash).and_modify(|counter| *counter -= 1).or_insert(-1);
}
}

impl<Block: BlockT> backend::LocalBackend<Block> for Backend<Block> where Block::Hash: Ord {}
Expand Down
1 change: 1 addition & 0 deletions client/rpc-spec-v2/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,5 @@ substrate-test-runtime = { version = "2.0.0", path = "../../test-utils/runtime"
sp-consensus = { version = "0.10.0-dev", path = "../../primitives/consensus/common" }
sp-maybe-compressed-blob = { version = "4.1.0-dev", path = "../../primitives/maybe-compressed-blob" }
sc-block-builder = { version = "0.10.0-dev", path = "../block-builder" }
sc-service = { version = "0.10.0-dev", features = ["test-helpers"], path = "../service" }
assert_matches = "1.3.0"
21 changes: 11 additions & 10 deletions client/rpc-spec-v2/src/chain_head/chain_head.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,15 @@ use sp_runtime::{
use std::{marker::PhantomData, sync::Arc};

/// An API for chain head RPC calls.
pub struct ChainHead<BE, Block: BlockT, Client> {
pub struct ChainHead<BE: Backend<Block> + 'static, Block: BlockT, Client> {
Copy link
Contributor

@jsdw jsdw Jan 25, 2023

Choose a reason for hiding this comment

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

Just a nit; nowadays I'm leaning towards not putting any trait bounds on generic params in structs unless they are needed for the definition of the struct (though I've been back and forth on this myself), so I guess I'd lean towards removing the bounds if possible (but curious on what others think!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I do like that approach. One nice thing about not having bounds on the structure is that we do not contaminate every structure containing that type.

For soundness reasons I presume, the compiler requires the bounds for the Drop impl to also be present on the struct.
The SubscriptionHandle implements Drop that needs access to the unpin_block method defined by Backend<Block> trait.
And unfortunately, this then leaks into SubscriptionManagement and into ChainHead itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this ; TIL Drop impls with trait bounds need matching ones on the struct :)

lexnv marked this conversation as resolved.
Show resolved Hide resolved
/// Substrate client.
client: Arc<Client>,
/// Backend of the chain.
backend: Arc<BE>,
/// Executor to spawn subscriptions.
executor: SubscriptionTaskExecutor,
/// Keep track of the pinned blocks for each subscription.
subscriptions: Arc<SubscriptionManagement<Block>>,
subscriptions: Arc<SubscriptionManagement<Block, BE>>,
/// The hexadecimal encoded hash of the genesis block.
genesis_hash: String,
/// The maximum number of pinned blocks allowed per connection.
Expand All @@ -77,7 +77,7 @@ pub struct ChainHead<BE, Block: BlockT, Client> {
_phantom: PhantomData<Block>,
}

impl<BE, Block: BlockT, Client> ChainHead<BE, Block, Client> {
impl<BE: Backend<Block> + 'static, Block: BlockT, Client> ChainHead<BE, Block, Client> {
/// Create a new [`ChainHead`].
pub fn new<GenesisHash: AsRef<[u8]>>(
client: Arc<Client>,
Expand Down Expand Up @@ -129,7 +129,7 @@ impl<BE, Block: BlockT, Client> ChainHead<BE, Block, Client> {
fn generate_initial_events<Block, BE, Client>(
client: &Arc<Client>,
backend: &Arc<BE>,
handle: &SubscriptionHandle<Block>,
handle: &SubscriptionHandle<Block, BE>,
runtime_updates: bool,
) -> Result<Vec<FollowEvent<Block::Hash>>, SubscriptionManagementError>
where
Expand Down Expand Up @@ -314,13 +314,14 @@ async fn submit_events<EventStream, T>(

/// Generate the "NewBlock" event and potentially the "BestBlockChanged" event for
/// every notification.
fn handle_import_blocks<Client, Block>(
fn handle_import_blocks<BE, Client, Block>(
client: &Arc<Client>,
handle: &SubscriptionHandle<Block>,
handle: &SubscriptionHandle<Block, BE>,
runtime_updates: bool,
notification: BlockImportNotification<Block>,
) -> Result<(FollowEvent<Block::Hash>, Option<FollowEvent<Block::Hash>>), SubscriptionManagementError>
where
BE: Backend<Block> + 'static,
Block: BlockT + 'static,
Client: CallApiAt<Block> + 'static,
{
Expand Down Expand Up @@ -370,12 +371,13 @@ where

/// Generate the "Finalized" event and potentially the "BestBlockChanged" for
/// every notification.
fn handle_finalized_blocks<Client, Block>(
fn handle_finalized_blocks<BE, Client, Block>(
client: &Arc<Client>,
handle: &SubscriptionHandle<Block>,
handle: &SubscriptionHandle<Block, BE>,
notification: FinalityNotification<Block>,
) -> Result<(FollowEvent<Block::Hash>, Option<FollowEvent<Block::Hash>>), SubscriptionManagementError>
where
BE: Backend<Block> + 'static,
Block: BlockT + 'static,
Client: HeaderBackend<Block> + HeaderMetadata<Block, Error = BlockChainError> + 'static,
{
Expand Down Expand Up @@ -474,9 +476,8 @@ where
return Err(err)
},
};

// Keep track of the subscription.
let Some((rx_stop, sub_handle)) = self.subscriptions.insert_subscription(sub_id.clone(), runtime_updates, self.max_pinned_blocks) else {
let Some((rx_stop, sub_handle)) = self.subscriptions.insert_subscription(sub_id.clone(), runtime_updates, self.max_pinned_blocks, self.backend.clone()) else {
// Inserting the subscription can only fail if the JsonRPSee
// generated a duplicate subscription ID.
debug!(target: "rpc-spec-v2", "[follow][id={:?}] Subscription already accepted", sub_id);
Expand Down
Loading