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

fix/chain_head: Ensure correct events for finalized branch #13632

Merged
lexnv merged 8 commits into from
Mar 22, 2023
Merged

fix/chain_head: Ensure correct events for finalized branch #13632

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
12c3235
chain_head/follow: Ensure correct events for finalized branch
lexnv Mar 17, 2023
c6f9691
Merge remote-tracking branch 'origin/master' into lexnv/fix_chainhead…
lexnv Mar 17, 2023
df6a438
Reenable tests
lexnv Mar 17, 2023
5f4907b
Do some clean ups and add some more docs
bkchr Mar 21, 2023
d070b31
Merge remote-tracking branch 'origin/master' into lexnv/fix_chainhead…
bkchr Mar 21, 2023
26f2e8c
Fix gramatic
bkchr Mar 22, 2023
c56a02a
Update client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
bkchr Mar 22, 2023
3d223a4
rpc/chain_head: Introduce error for absent headers
lexnv Mar 22, 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
Next Next commit
chain_head/follow: Ensure correct events for finalized branch 
Signed-off-by: Alexandru Vasile <[email protected]>
  • Loading branch information
@lexnv
lexnv committed Mar 17, 2023
commit 12c323596eb3b44ea4a9fcd9a96ea03d2d8563e9
45 changes: 20 additions & 25 deletions 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 @@ -373,31 +373,26 @@ where
continue
}

match self.best_block_cache {
Some(best_block_hash) => {
// If the best reported block is a children of the last finalized,
// then we had a gap in notification.
let ancestor = sp_blockchain::lowest_common_ancestor(
&*self.client,
*last_finalized,
best_block_hash,
)?;

// A descendent of the finalized block was already reported
// before the `NewBlock` event containing the finalized block
// is reported.
if ancestor.hash == *last_finalized {
return Err(SubscriptionManagementError::Custom(
"A descendent of the finalized block was already reported".into(),
))
}
self.best_block_cache = Some(*hash);
},
// This is the first best block event that we generate.
None => {
self.best_block_cache = Some(*hash);
},
};
// Make sure we have not encountered a gap in notifications.
// Note: Let `generate_import_events()` populate the cache.
if let Some(best_block_hash) = self.best_block_cache {
// If the best reported block is a children of the last finalized,
// then we had a gap in notification.
let ancestor = sp_blockchain::lowest_common_ancestor(
&*self.client,
*last_finalized,
best_block_hash,
)?;

// A descendent of the finalized block was already reported
// before the `NewBlock` event containing the finalized block
// is reported.
if ancestor.hash == *last_finalized {
return Err(SubscriptionManagementError::Custom(
"A descendent of the finalized block was already reported".into(),
))
}
}
Copy link
Member

@bkchr bkchr Mar 18, 2023

Choose a reason for hiding this comment

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

I don't get this piece of code? How do you know that there wasn't any new best block event for the last finalized block?

And why do you have this as part of of the for loop while it doesn't depend on any from the loop?

Copy link
Contributor Author

@lexnv lexnv Mar 20, 2023

Choose a reason for hiding this comment

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

Hey

How do you know that there wasn't any new best block event for the last finalized block?

We depend on the pin_block which returns true if this is the first time we "pin" the block and false if the block was previously pinned. We call pin_block twice for each block: from the import and finalized branches.

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

Lines 364 to 369 in df6a438

for (hash, parent) in finalized_block_hashes.iter().zip(parents) {
// This block is already reported by the import notification.
if !self.sub_handle.pin_block(*hash)? {
continue
}

And why do you have this as part of of the for loop while it doesn't depend on any from the loop?

For any blocks that we have not seen yet we want to generate the NewBlock, except for the last block for which we want to generate the NewBlock + BestBlock events. For the last block, I've added the extra check to make sure we catch the following case:

[Block 0] -  [Block 1] - [Block 2] -  ...
                          ^^^ T0: Block import branch reports block 2 for the first time 

^^^^^^^^^^^^^^^ T1:  Finalized branch reports 0 and 1 for the first time

We could enter this path only if the BlockImport notification is not generated for 0, 1; but somehow we receive the BlockImport for block 2. And at the same time we get Finalized for 0 and 1 (blocks that we have not been reported by BlockImport).

This check may be an overcautious one, let me know what you think of it, thanks!

Copy link
Member

@bkchr bkchr Mar 21, 2023

Choose a reason for hiding this comment

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

For me the flow was a little bit hard to follow. I pushed a commit that improves the flow and adds some more docs. I hope that is okay.


// This is the first time we see this block. Generate the `NewBlock` event; if this is
// the last block, also generate the `BestBlock` event.
Expand Down