chainHead: Error on duplicate unpin hashes

[lexnv]

This PR addresses an issue where calling chainHead_unpin with duplicate hashes could lead to unintended side effects.

This backports: paritytech/json-rpc-interface-spec#135

While at it, have added a test to check that the global reference count is decremented only once on unpin.

cc @paritytech/subxt-team

[bkchr]

Maybe we should error on duplicated hashes instead?

[lexnv]

Yep that sounds good to me, I'll create a small PR in the spec to clarify this (part of paritytech/json-rpc-interface-spec#133)

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5209970

[tadeohepperle]

Nice! :)

[davxy]

Just curious. What is the unintended consequence if there are duplicate hashes?

I ask because the only thing you do after the check is to call self.subscriptions.unpin_blocks(). And if this is where the unintended behavior is triggered then maybe you should add the check there (i.e. in the called function) as it may be called somewhere else

[lexnv]

The side-effect before this PR was that duplicate hashes could decrement the global counter of the blocks multiple times:

polkadot-sdk/substrate/client/rpc-spec-v2/src/chain_head/subscription/inner.rs

Lines 778 to 781 in aa68ea5

	// Block have been removed from the subscription. Remove them from the global tracking.
	for hash in hashes {
	self.global_unregister_block(hash);
	}

Have moved the duplicate check in the unpin_blocks logic.

With this check enforced, we'll return an error before doing any other work. The downside of moving the check in the unpin_blocks is that we'll perform the check under a lock. However, considering that we don't expect a large number of blocks for the most common use-case; and that this closes the gap on potentially calling the unpin_blocks from other code paths; I'd say it is a reasonable thing to do.

Thanks for the review everyone! 🙏