Skip to content

chainHead/follow: Provide multiple block hashes to the initialized event #3445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lexnv merged 14 commits into from
Mar 6, 2024
Merged

chainHead/follow: Provide multiple block hashes to the initialized event #3445

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
21f0661
chainHead: Extend `Initialized` event to support multiple hashes
lexnv Feb 21, 2024
2c33377
chainHead: Provide more finalized blocks in the initial event
lexnv Feb 22, 2024
4869964
chainHead: Handle pinning during block collection
lexnv Feb 22, 2024
2b4f2fd
chainHead/events: Adjust test events
lexnv Feb 22, 2024
543bc92
chainHead/tests: Adjust test to include multiple hashes
lexnv Feb 22, 2024
7b66662
chainHead/tests: Apply rustfmt
lexnv Feb 22, 2024
c993cec
Update substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
lexnv Mar 4, 2024
3960b27
Update substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
lexnv Mar 4, 2024
fd877ed
Update substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
lexnv Mar 4, 2024
c8ff561
Update substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
niklasad1 Mar 4, 2024
3aa6c60
Merge remote-tracking branch 'origin/master' into lexnv/multiple-chai…
lexnv Mar 4, 2024
cbeecb2
chainHead: Replace zip with manual parent iter
lexnv Mar 4, 2024
626a33a
Merge branch 'master' into lexnv/multiple-chain-head-event
lexnv Mar 5, 2024
1989d41
Merge branch 'master' into lexnv/multiple-chain-head-event
lexnv Mar 6, 2024
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
chainHead: Handle pinning during block collection 
Signed-off-by: Alexandru Vasile <[email protected]>
  • Loading branch information
@lexnv
lexnv committed Feb 22, 2024
commit 486996488b2632cc8454a688dac0b141f8ed6986
46 changes: 19 additions & 27 deletions substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,14 @@ where
}

/// Get the in-memory blocks of the client, starting from the provided finalized hash.
///
/// The reported blocks are pinned by this function.
fn get_init_blocks_with_forks(
&self,
startup_point: &StartupPoint<Block>,
finalized: Block::Hash,
) -> Result<InitialBlocks<Block>, SubscriptionManagementError> {
let blockchain = self.backend.blockchain();
let leaves = blockchain.leaves()?;
let finalized = startup_point.finalized_hash;
let mut pruned_forks = HashSet::new();
let mut finalized_block_descendants = Vec::new();
Copy link
Contributor

@skunert skunert Feb 29, 2024

Choose a reason for hiding this comment

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

Not a comment related to this PR, but could we do some kind of sanity check of the distance between leaf and finalized block? This code assumes that they are close together, as they should be. However, on parachains for example, the finalization of para blocks is dependent on the relay chain.

If the embedded relay chain node syncs slower while the parachain node is already at the tip, between a leaf and the known finalized block could be millions of blocks (until relay chain has caught up).

This would lead to:

  1. The tree route being very costly
  2. Us delivering 1 million blocks via RPC

Maybe we should add some kind of safeguard against such situations.
Certainly an edge case however.

Copy link
Contributor Author

@lexnv lexnv Mar 4, 2024

Choose a reason for hiding this comment

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

Have create this PR to handle that edge-case: #3562 🙏

let mut unique_descendants = HashSet::new();
Expand All @@ -211,6 +212,8 @@ where

for pair in blocks.zip(parents) {
Copy link
Contributor

@niklasad1 niklasad1 Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated to this PR but I don't understand the logic with zip here.

Parents = blocks.len() + 1

Then the zip will ignore the last item in blocks is that intended?

Example what I mean:

    let mut a = vec![1, 2];
    let mut tmp = vec![3, 4];
    
    let b = std::iter::once(99).chain(tmp);
    
    let res: Vec<(usize, usize)> = a.into_iter().zip(b).collect();
    assert_eq!(res, vec![(1, 99), (2, 3)]);

Copy link
Contributor Author

@lexnv lexnv Mar 4, 2024

Choose a reason for hiding this comment

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

Yep, that makes sense! Indeed we are ignoring the last element of the parents, but that's on purpose since parents = [finalized, blocks[..]]

We are interested in grouping every block reported by blocks with its parent.

For example:

    let blocks = vec!["A", "B", "C"];
    let parents = vec!["gensiss", "A", "B", "C"];

    for pair in blocks.iter().zip(parents.iter()) {
        println!("block={} parent={}", pair.0, pair.1);
    }

This should produce the following result:

block=A parent=gensiss
block=B parent=A
block=C parent=B

Let me have a go at this to remove the zip to make the code easier to read 🙏

if unique_descendants.insert(pair) {
// The finalized block is pinned below.
self.sub_handle.pin_block(&self.sub_id, pair.0)?;
finalized_block_descendants.push(pair);
}
}
Expand All @@ -222,12 +225,22 @@ where
let Some(header) = blockchain.header(current_block)? else {
return Err(SubscriptionManagementError::BlockHeaderAbsent);
};
let mut finalized_block_hashes = VecDeque::with_capacity(10);

// Report at most `MAX_FINALIZDED_BLOCKS`. Note: The node might not have that many blocks.
let mut finalized_block_hashes = VecDeque::with_capacity(MAX_FINALIZDED_BLOCKS);

// Pin the finalized block.
self.sub_handle.pin_block(&self.sub_id, current_block)?;
finalized_block_hashes.push_front(current_block);
current_block = *header.parent_hash();

for _ in 0..MAX_FINALIZDED_BLOCKS - 1 {
let Ok(Some(header)) = blockchain.header(current_block) else { break };
// Block cannot be reported if pinning fails.
if self.sub_handle.pin_block(&self.sub_id, current_block).is_err() {
break
};

finalized_block_hashes.push_front(current_block);
current_block = *header.parent_hash();
}
Expand All @@ -244,36 +257,17 @@ where
startup_point: &StartupPoint<Block>,
) -> Result<(Vec<FollowEvent<Block::Hash>>, HashSet<Block::Hash>), SubscriptionManagementError>
{
let init = self.get_init_blocks_with_forks(startup_point)?;

let initial_blocks = init.finalized_block_descendants;
let init = self.get_init_blocks_with_forks(startup_point.finalized_hash)?;

// The initialized event is the first one sent.
let initial_blocks = init.finalized_block_descendants;
let finalized_block_hashes = init.finalized_block_hashes;
let mut iter = finalized_block_hashes.iter().rev();
let Some(finalized) = iter.next() else {
return Err(SubscriptionManagementError::BlockHeaderAbsent);
};
self.sub_handle.pin_block(&self.sub_id, *finalized)?;
let mut num_pinned = 1;
while let Some(remaining_hash) = iter.next() {
if self.sub_handle.pin_block(&self.sub_id, *remaining_hash).is_err() {
break
}

num_pinned += 1;
}

let to_skip = finalized_block_hashes.len() - num_pinned;
let finalized_block_hashes = finalized_block_hashes.into_iter().skip(to_skip).collect();

let finalized_block_hash = startup_point.finalized_hash;
self.sub_handle.pin_block(&self.sub_id, finalized_block_hash)?;

let finalized_block_runtime = self.generate_runtime_event(finalized_block_hash, None);

let initialized_event = FollowEvent::Initialized(Initialized {
finalized_block_hashes,
finalized_block_hashes: finalized_block_hashes.into(),
finalized_block_runtime,
with_runtime: self.with_runtime,
});
Expand All @@ -282,8 +276,6 @@ where

finalized_block_descendants.push(initialized_event);
for (child, parent) in initial_blocks.into_iter() {
self.sub_handle.pin_block(&self.sub_id, child)?;

let new_runtime = self.generate_runtime_event(child, Some(parent));

let event = FollowEvent::NewBlock(NewBlock {
Expand Down