Try fix duplicate header relay

[fewensa]

In our tests, it was found that if multiple bridges are started, many meaningless repeated header relays will be sent. After making this modification, this problem will be alleviated.

Related:

• Fix duplicate header relay darwinia-network/darwinia-messages-substrate#12
• message_race_loop issue darwinia-network/darwinia-messages-substrate#10
• message_race_loop still update required header to latest even if no new nonces on source_finalized_updated. darwinia-network/bridger#344

[svyatonik]

Hey! Thanks for the PR, but I have some questions:

1. what do you mean by "multiple bridges" - multiple relay processes that are serving the same lane? Is it possible to illustrate this case with test or at least with some text/diagram here? Is it only specific to the case when two relayers are running?
2. the only difference between latest_updated_confirm_nonce_at_source and latest_confirmed_nonces_at_source.back().map(|_, n| n) is that: latest_updated_confirm_nonce_at_source is never set to None - am I right?
3. so far, this PR looks to me like revert of Keep multiple latest confirmed nonces at source in messages relay #719, which has replaced Option<MessageNonce> with VecDeque<HeaderId, MessageNonce>. So imo it resurrects the issue, described in Keep multiple latest confirmed nonces at source in messages relay #719 (unless I'm missing something);
4. I've tried to read related issues and PRs, still not 100% sure, what's the issue is (hope you'll make it clear by introducing a test or detailed description/diagram, mentioned in (1)). But probably the 'problem' you see may be in this code. IIUC, it'll ask for more headers even if there are no new messages to deliver, but still some rewards are not confirmed to the target chain. Could you, please, give it a try? I mean - change code so that it'll always return self.strategy.required_source_header_at_target(current_best)? It may be incorrect and will need a double-check + possibly proper fix, just want to see if it solves your problem

[fewensa]

1. Yes, "multiple bridges" is means "multiple relayers".
2. The charts you can click darwinia bridgecrabgranpa submit_finality_proof extrinsics to see our bridge header relay records. too more header relay at the same time sent by multiple bridges/relayers.
3. I guess the reason is:

You can read the log in this issue darwinia-network/bridger#344

[2021-12-06T23:53:35Z TRACE finality_relay::finality_loop] Considering range of headers (8186014; 8186010]
[2021-12-06T23:53:35Z TRACE finality_relay::finality_loop] Can not improve selected Crab finality proof None. No unjustified headers and recent proofs
[2021-12-06T23:53:35Z DEBUG messages_relay::message_lane_loop] Received state from Crab node: ClientState { best_self: HeaderId(8186018, 0x7315a7e75dce411c14b8d5e3f128ac0da1630496507111dfbf2a259aae8bf713), best_finalized_self: HeaderId(8186015, 0x4727ea887eef10242d2701b1483ffe8d98a41e2e1530f13fc6183dd1fee58804), best_finalized_peer_at_best_self: HeaderId(6225408, 0xea1db9acef190c683e953e79186d20d5e72a725701c5d1bd5f15f0aedbb4f75b) }
[2021-12-06T23:53:35Z DEBUG messages_relay::message_race_loop] Asking Crab::MessagesDelivery about best message nonces

Considering range of headers (8186014; 8186010]

The source chain block number is great than source_confirmed_in_target_chain. so there

parity-bridges-common/relays/messages/src/message_race_delivery.rs

Line 390 in 88d684d

.map(|(id, _)| id.0 < oldest_header_number_to_keep)

can not pass condition the result is latest_confirmed_nonces_at_source not pop front.
then

parity-bridges-common/relays/messages/src/message_race_delivery.rs

Lines 414 to 420 in 88d684d

	let latest_confirmed_nonce_at_source = self
	.latest_confirmed_nonces_at_source
	.iter()
	.take_while(|(id, _)| id.0 <= best_finalized_source_header_id_at_best_target.0)
	.last()
	.map(|(_, nonce)| *nonce)?;
	let target_nonces = self.target_nonces.as_ref()?;

will always return stored latest confirm block in latest_confirmed_nonces_at_source

[svyatonik]

Thanks for the explanation. Still, imo this fix reverts #719, which is not good.

4. I've tried to read related issues and PRs, still not 100% sure, what's the issue is (hope you'll make it clear by introducing a test or detailed description/diagram, mentioned in (1)). But probably the 'problem' you see may be in this code. IIUC, it'll ask for more headers even if there are no new messages to deliver, but still some rewards are not confirmed to the target chain. Could you, please, give it a try? I mean - change code so that it'll always return self.strategy.required_source_header_at_target(current_best)? It may be incorrect and will need a double-check + possibly proper fix, just want to see if it solves your problem

I've made a quick and dirty fix for that - please see #1328. Is it possible for you to test if it is fixing your issue (I mean - instead of fix from this PR)? #1328 is not in final stage yet - it is definitely missing tests and I need to think whether it breaks something else, but I believe, it shall fix your issue.

[fewensa]

Thanks, I'll test use your pull request.

[fewensa]

Follow #1328
Change code to https://github.com/darwinia-network/darwinia-bridges-substrate/tree/fewensa-upstream-duplicate-header-relay