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

backend(fix): Remove only finalized blocks from the event window #1356

Merged
merged 4 commits into from Jan 11, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jan 11, 2024

This PR ensures that the window of events we feed to every new subscription contains the appropriate NewBlock and BestBlock events.

Before this PR, the block_events_from_last_finalized window of events was cleared out on every Finalized event.
However, this approach could miss reporting NewBlocks that will later become Finalized. Therefore, the unstable driver could report a block as Finalized and never report it as NewBlock first.

Considering the following edge case:

      0x1 -----> 0x2 -----> 0x3 -----> 0x4

T0    init       new         new       new
T1               fin                 
  • T0: The chainHead subscription is started, 0x1 0x2 0x3 0x4 are announced
    • 0x1 is the Initialized event
    • block_events_from_last_finalized contains: 0x2 0x3 0x4 as NewBlocks
  • T1: 0x2 is reported as Finalized
    • Before this PR the block_events_from_last_finalized is cleared our entirely

If a new subscription is started after T1; it will receive an Initialized event with the 0x2 block. However, it will never receive 0x3 and 0x4 as NewBlocks. After around 6 seconds, the 0x3 is reported in a Finalized event. This breaks the expectations of the rpc-v2 spec.

To mitigate this behavior, the window of events is not entirely cleaned out. Instead, only blocks reported as Finalized or pruned are removed from the window.
The last finalized block will be reported as Initialized by our driver, therefore there is no need to report NewBlock and BestBlock events for it. If the Finalized event reported multiple finalized hashes, we only care about the state at the head of the chain, therefore it is correct to remove those as well. Idem for the pruned hashes; they will never be reported again and we remove them from the window of events.

Testing Done

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner January 11, 2024 12:08
shared
.block_events_for_new_subscriptions
.retain(|ev| match ev {
FollowEvent::NewBlock(new_block_ev) => {
Copy link
Member

@niklasad1 niklasad1 Jan 11, 2024

Choose a reason for hiding this comment

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

Can you add a comment here why on NewBlock and BestBlockChanged are removed here if it's pruned or finalized?

The last finalized block will be reported as Initialized by our driver, therefore there is no need to report NewBlock and BestBlock events for it. If the Finalized event reported multiple finalized hashes, we only care about the state at the head of the chain, therefore it is correct to remove those as well. Idem for the pruned hashes; they will never be reported again and we remove them from the window of events.

@@ -251,7 +250,7 @@ impl<Hash: BlockHash> Shared<Hash> {
// New subscriptions will be given this init message:
shared.current_init_message = Some(ev.clone());
// Clear block cache (since a new finalized block hash is seen):
shared.block_events_from_last_finalized.clear();
shared.block_events_for_new_subscriptions.clear();
Copy link
Member

Choose a reason for hiding this comment

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

yeah, alright when Initialized event is generated we can conclude that no messages regarding the next block are missed.

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

I could only review on my phone but this looks like a good approach to me; good job!

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Contributor Author

lexnv commented Jan 11, 2024

Thanks for the review guys! I'll keep my eyes on the CI for a couple of days to make sure no other errors appear 🙏

@niklasad1 niklasad1 merged commit d004789 into master Jan 11, 2024
11 checks passed
@niklasad1 niklasad1 deleted the lexnv/fix-outofoder-follow-events branch January 11, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants