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

beefy-gadget is not guaranteed to see finality notification for every block. #254

Closed
tomusdrw opened this issue Jul 29, 2021 · 2 comments · Fixed by paritytech/substrate#10882
Labels
bug Something isn't working

Comments

@tomusdrw
Copy link
Contributor

tomusdrw commented Jul 29, 2021

Our assumption was that finality_notification is firing for all blocks that were finalized (even indirectly) - i.e. if the last finalized block was N and GRANDPA now finalizes N+10 we will get notifications for all blocks in range [N..N+10].

It does not seem to be the case and we need to make sure that the code is aware of that. Most notably the should_vote_on function needs to be changed. A possible solution is also outlined in #162

@tomusdrw tomusdrw added the bug Something isn't working label Jul 29, 2021
@adoerr
Copy link
Contributor

adoerr commented Jul 29, 2021

The issue is especially, how sc_service::build_network_future() does poll the finality notification stream future. Basically, finality notifications are aggregated.

The relevant code section is

		// We tweak the `Stream` in order to merge together multiple items if they happen to be
		// ready. This way, we only get the latest finalized block.
		stream::poll_fn(move |cx| {
			let mut last = None;
			while let Poll::Ready(Some(item)) = Pin::new(&mut finality_notification_stream).poll_next(cx) {
				last = Some(item);
			}
			if let Some(last) = last {
				Poll::Ready(Some(last))
			} else {
				Poll::Pending
			}
		}).fuse()
	};

So, if finality notifications N to N+10 are ready, only N+10 will be seen. This issue is magnified by the fact, that the finality notification stream future is polled as part of a fairly large future::select!() macro. The select macro does not offer any fairness guarantees in case multiple futures are ready.

@adoerr adoerr changed the title Finality Notification is not firing for all blocks. beefy-gadget is not guaranteed to see finality notification for every block. Jul 29, 2021
@tomusdrw tomusdrw added this to the Get BEEFY in shape milestone Nov 26, 2021
@andresilva
Copy link
Contributor

This should be fixed by paritytech/substrate#10639 now. We can handle the logic ourselves when multiple blocks are finalized at once.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants