chainHead/fix: Report bestBlock events only for newBlock reports

[lexnv]

The #5512 has surfaced that we reported a BestBlock event for a block not previously reported via NewBlock.

This is because of a race between:

• the stream of events that announces new blocks
• self.client.info().best_block

It is possible that client.info() contains newer information than the information polled from the block stream (that may be lagging).

To mitigate this, instead of relying on the client's info use the last finalized block to emit a new event.

There are two cases when a new best block event is emitted:

• The best block is in the pruned list and is reported immediately
• The best block is not a descendant of the last finalized block

Closes: #5512

Thanks @jsdw and @josepot for helping debug this 🙏

cc @paritytech/subxt-team

[josepot]

First of all, thank you for your quick response and for addressing this so promptly!

I’d like to clarify a few points regarding the generation of multiple BestBlock events for the same block after this PR:

After this PR, multiple BestBlock events may be generated for the same block. The RPC-v2 spec does not mention that the BestBlock must be a descendant of the previously generated BestBlock, because it can simply be on a different fork. Therefore, it is ok to generate multiple BestBlock events for the same block.
However, the spec mentions that a BestBlock event must be generated after a NewBlock event.

I’m not entirely sure I fully understood your intent here, but I have some concerns. It's important to note that the actual name of the event is bestBlockChanged. In my opinion, the exact same event should only be emitted multiple times if there is a valid reason -specifically, if the best block has genuinely changed, such as due to a switch between different branches.

While the spec doesn't explicitly prohibit emitting the same event multiple times for an unchanged best block, I believe it's not in the spirit of the spec to do so. The event’s name, bestBlockChanged, suggests that it should only be triggered when there is an actual change. Emitting redundant or noisy events when the best block hasn’t changed could be misleading and I think it's something we should avoid.

That said, I believe this behavior wouldn’t break anything in PAPI, but it could break things for other libraries, as the spec implies that the event is meant to signal an actual change in the best block:

The bestBlockChanged event indicates that the given block is now considered to be the best block.

bestBlockHash is guaranteed to be equal either to the current finalized block hash, or to a block reported in an earlier newBlock event.

Also, the spec states:

This list of notifications makes it very easy for a JSON-RPC client to follow just the best block updates (listening to just bestBlockChanged events).

Given these points, it seems reasonable to expect that bestBlockChanged events are only emitted when there is a meaningful change. Generating this event without a genuine change could potentially create unnecessary noise for clients monitoring these updates.

cc @tomaka

[tomaka]

It's important to note that the actual name of the event is bestBlockChanged. In my opinion, the exact same event should only be emitted multiple times if there is a valid reason -specifically, if the best block has genuinely changed, such as due to a switch between different branches.

I don't think we should focus too much on the name. All APIs always have small prints that aren't conveyed by the name of the thing itself.
Yes, emitting multiple identical events in a row contradicts the word "changed" and technically it is misleading, but I really don't think that this "misleading-ness" a big deal.

Whether it is allowed to emit multiple identical events in a row should be explicitly written in the spec.
Given that the best block can change at any time and in unpredictable ways, I can't manage to find a situation where it would be troublesome for the JSON-RPC client if the best block changed to what it already is, and so it should IMO simply be allowed but slightly discouraged.

[lexnv]

Have modified the code logic to take care of the following edge-cases:

• The best block is in the pruned list and is reported immediately
• The best block is not a descendant of the last finalized block

We now should not emit extra BestBlock events if the current best block is a descendant of the finalized block. Thanks for the review everyone 🙏

[niklasad1]

bot fmt

[command-bot]

@niklasad1 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7233685 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-87d08829-fd10-4af6-94e0-3048bd7c6e0e to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@niklasad1 Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7233685 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7233685/artifacts/download.

[bkchr]

I don't get why we need this. If the best block was not pruned, it needs to be a descendant of the finalized block.