Skip to content
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.

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

chainHead: Track reported blocks to capture notification gaps #5856

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Sep 27, 2024

There are cases during warp sync or re-orgs, where we receive a notification with a block parent that was not reported in the past. This PR extends the tracking state to catch those cases and report a Stop event to the user.

This PR adds a new state to the RPC-v2 chainHead to track which blocks have been reported.

In the past we relied on the pinning mechanism to provide us details if a block is pinned or not.
However, the pinning state keeps the minimal information around for pinning. Therefore, unpinning a block will cause the state to disappear.

Closes: #5761

@lexnv lexnv added I2-bug The node fails to follow expected behavior. T3-RPC_API This PR/Issue is related to RPC APIs. labels Sep 27, 2024
@lexnv lexnv self-assigned this Sep 27, 2024
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv changed the title wip: chainHead: Track reported blocks to capture notification gaps chainHead: Track reported blocks to capture notification gaps Sep 27, 2024
@lexnv lexnv marked this pull request as ready for review September 27, 2024 13:43
@@ -71,6 +69,8 @@ pub struct ChainHeadFollower<BE: Backend<Block>, Block: BlockT, Client> {
current_best_block: Option<Block::Hash>,
/// LRU cache of pruned blocks.
pruned_blocks: LruMap<Block::Hash, ()>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Believe that pruned_blocks are no longer needed (#5605), will tackle this in another PR

@@ -96,6 +96,9 @@ impl<BE: Backend<Block>, Block: BlockT, Client> ChainHeadFollower<BE, Block, Cli
pruned_blocks: LruMap::new(ByLength::new(
MAX_PINNED_BLOCKS.try_into().unwrap_or(u32::MAX),
)),
announced_blocks: LruMap::new(ByLength::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside of this approach is that we never remove anything manually and just rely on the LRU to remove items. However, that means that over time we will have the maximum amount of items in every LRU for every subscription. But in reality probably does not matter, for these 512 items thats approx. 16kb of memory usage then 🤷

// For the finalized blocks we use `peek` to avoid moving the block counter to the front.
// This effectively means that the LRU acts as a FIFO queue. Otherwise, we might
// end up with scenarios where the last inserted finalized block is removed.
self.blocks.get(block).is_some() || self.finalized.peek(block).is_some()
Copy link
Member

@niklasad1 niklasad1 Oct 3, 2024

Choose a reason for hiding this comment

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

I think it's quite "strange/hard to understand" use a LRU for the finalized blocks when it's treated as a FIFO queue.

Personally I would just use a fixed size ring buffer but probably less efficient to do lookups.

An alternative is create a wrapper type such as:

/// A wrapper type to utilize a LruMap as FIFO queue.
struct MostRecentFinalizedBlocks(LruMap<Block::Hash, ()>);

impl FinalizedBlocksFifo {
    fn insert(&mut self, block: Block::Hash) {
        self.finalized.insert(block, ());
     }

    fn contains(&mut self, block: &Block::Hash) -> bool {
        self.0.peek(block).is_some()
    }
}

fn was_announced(&mut self, block: &Block::Hash) -> bool {
// For the finalized blocks we use `peek` to avoid moving the block counter to the front.
// This effectively means that the LRU acts as a FIFO queue. Otherwise, we might
// end up with scenarios where the last inserted finalized block is removed.
Copy link
Member

@niklasad1 niklasad1 Oct 3, 2024

Choose a reason for hiding this comment

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

Suggested change
// end up with scenarios where the last inserted finalized block is removed.
// end up with scenarios where the "finalized block" in the end of LRU is overwritten which may not necessarily be the oldest finalized block i.e, possible that "get" promotes an older finalized block because it was accessed more recently


/// Check if the block is contained in the LRU cache.
fn contains(&mut self, block: &Block::Hash) -> Option<&()> {
// For the finalized blocks we use `peek` to avoid moving the block counter to the front.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's sufficient with the comments on the struct itself to explain that the LRU is used as FIFO queue but maybe move some of the comment here to struct itself?

but up to you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Niklas! That sounds good to me! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON-RPC: chainHead_v1_follow Subscription Sends newBlock events with unannounced parentBlockHash
3 participants