Unexpected behaviour of block import_notification_stream

[ffarall]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

From my understanding, there seems to be a misalignment between the commented (expected) behaviour of the import_notification_stream from BlockchainEvents (code here), and the actual code. I'm going to explain my understanding of what the code should do according to the comment, and what it actually does. Please correct me if I'm wrong.

The Code

This code snippet is where the notifications are sent through the streams:

match import_notification_action {
			ImportNotificationAction::Both => {
				trigger_storage_changes_notification();
				self.import_notification_sinks
					.lock()
					.retain(|sink| sink.unbounded_send(notification.clone()).is_ok());

				self.every_import_notification_sinks
					.lock()
					.retain(|sink| sink.unbounded_send(notification.clone()).is_ok());
			},
			ImportNotificationAction::RecentBlock => {
				trigger_storage_changes_notification();
				self.import_notification_sinks
					.lock()
					.retain(|sink| sink.unbounded_send(notification.clone()).is_ok());

				self.every_import_notification_sinks.lock().retain(|sink| !sink.is_closed());
			},
			ImportNotificationAction::EveryBlock => {
				self.every_import_notification_sinks
					.lock()
					.retain(|sink| sink.unbounded_send(notification.clone()).is_ok());

				self.import_notification_sinks.lock().retain(|sink| !sink.is_closed());
			},
			ImportNotificationAction::None => {
				// This branch is unreachable in fact because the block import notification must be
				// Some(_) instead of None (it's already handled at the beginning of this function)
				// at this point.
				self.import_notification_sinks.lock().retain(|sink| !sink.is_closed());

				self.every_import_notification_sinks.lock().retain(|sink| !sink.is_closed());
			},
		}

So depending on the value of import_notification_action, both notification streams (import_notification_stream and every_import_notification_stream) are used, or just one of them.

The value of import_notification_action is defined here:

			let import_notification_action = if should_notify_every_block {
				if should_notify_recent_block {
					ImportNotificationAction::Both
				} else {
					ImportNotificationAction::EveryBlock
				}
			} else {
				ImportNotificationAction::RecentBlock
			};

should_notify_recent_block is responsible for determining if import_notification_stream should be triggered, and its value is determined in this line:

		// Notify when we are already synced to the tip of the chain
		// or if this import triggers a re-org
		let should_notify_recent_block = make_notifications || tree_route.is_some();

The second operand of the logical OR (tree_route) is what one would expect:

		let tree_route = if is_new_best && info.best_hash != parent_hash && parent_exists {
			let route_from_best =
				sp_blockchain::tree_route(self.backend.blockchain(), info.best_hash, parent_hash)?;
			Some(route_from_best)
		} else {
			None
		};

In other words, if there is a new best block which belongs to a fork branch that is not the current one, this is a re-org, so tree_route will be Some.

The first operand though (make_notifications), is defined here:

		// this is a fairly arbitrary choice of where to draw the line on making notifications,
		// but the general goal is to only make notifications when we are already fully synced
		// and get a new chain head.
		let make_notifications = match origin {
			BlockOrigin::NetworkBroadcast | BlockOrigin::Own | BlockOrigin::ConsensusBroadcast =>
				true,
			BlockOrigin::Genesis | BlockOrigin::NetworkInitialSync | BlockOrigin::File => false,
		};

This means that make_notifications will only be false iff the origin of the block is one of BlockOrigin::Genesis | BlockOrigin::NetworkInitialSync | BlockOrigin::File. Which means that, for example, make_notifications will still be true if a block is imported from the network, and that block belongs to a fork branch that is not the current best branch, and is not the new best (doesn't cause a re-org). Given that should_notify_recent_block is determined by an OR, regardless of tree_route, should_notify_recent_block will still be true for this non-reorging block.

Behaviour from the Code

My interpretation from this analysis is that:

1. During the initial syncing process of the node, make_notifications will be false, so should_notify_recent_block will be true if somehow there is a re-org during the syncing process.
2. After the initial sync, make_notifications will essentially always be true, so should_notify_recent_block will always be true regardless of whether the block is the new best of the current branch, a non-new best from another fork branch, or a new best from another branch that causes a re-org.

This means that the tree_route condition is relevant only to notify about re-orgs during a syncing process.

Commented Behaviour of import_notification_stream

This is the comment on import_notification_stream in the code:

	/// Get block import event stream.
	///
	/// Not guaranteed to be fired for every imported block, only fired when the node
	/// has synced to the tip or there is a re-org. Use `every_import_notification_stream()`
	/// if you want a notification of every imported block regardless.
	fn import_notification_stream(&self) -> ImportNotifications<Block>;

Based on this comment, my intuition for the behaviour of import_notification_stream is:

1. It should start notifying block imports after the initial sync.
2. After the initial sync, it should notify of new best blocks in the current best branch.
3. It should not notify of block imports from a fork branch that don't cause a re-org.
4. It should notify of block imports from another fork branch that do cause a re-org.

Conclusion

Either the behaviour interpreted from the code is the expected one, and the comment is not clear about it, or the comment reflects the intended behaviour, but the code doesn't.

Personally, I feel it is counterintuitive if the expected behaviour is to notify every block after the initial sync (re-org or not), but during the initial sync, notify only of re-orgs. So I'm inclined to believe that the comment reflects the intended behaviour, and the code is wrong. But of course, there are use cases that I might not be considering.

Whatever the case, I'm open to open a PR and help make this behaviour clearer. Either by updating the comment on import_notification_stream, or fixing the code.

Thank you for your time!

Steps to reproduce

Not needed

[bkchr]

Maybe you want to make the comment more clear to you. However,

Only fired when the node has synced to the tip or there is a re-org

means to me that the notification is either send when you are at the tip of the chain (for every block) or there is a re-org (which is only relevant when you are not at the tip of the chain).

• It should not notify of block imports from a fork branch that don't cause a re-org.

• It should notify of block imports from another fork branch that do cause a re-org.

The first one not being true and the second one being true because we notify for every imported block.

[ffarall]

Thank you for the clarification @bkchr ! Of course this is based on my interpretation of the comment, so yes, the comment wasn't clear at least to me.

I'm curious to understand the reasoning behind this. Why would a re-org only be relevant when you're not synced to the tip? My thinking was that, if you don't care about being notified of blocks before syncing to the tip, why would you be only interested in blocks during that sync that cause a re-org?

[ffarall]

A colleague helped me find the reasoning in this PR.

[bkchr]

If you like you can open a pr to make the comment more clear to you and others.

[ffarall]

@bkchr I just did that. No rush at all in reviewing it or anything, but hope this helps. Thanks again for your clarification.