From 3abd734e9c2666ff28d542d77d812c5129fbd700 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 25 Apr 2023 17:57:55 +0300 Subject: [PATCH 1/4] removed obsolete check that is superseded by the unblock checks below --- relays/messages/src/message_race_delivery.rs | 42 +------------------- 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/relays/messages/src/message_race_delivery.rs b/relays/messages/src/message_race_delivery.rs index 8a667fdbdb8..bea8d68ebd8 100644 --- a/relays/messages/src/message_race_delivery.rs +++ b/relays/messages/src/message_race_delivery.rs @@ -343,31 +343,6 @@ where // There's additional condition in the message delivery race: target would reject messages // if there are too much unconfirmed messages at the inbound lane. - // The receiving race is responsible to deliver confirmations back to the source chain. So - // if there's a lot of unconfirmed messages, let's wait until it'll be able to do its job. - let latest_received_nonce_at_target = target_nonces.latest_nonce; - let confirmations_missing = - latest_received_nonce_at_target.checked_sub(latest_confirmed_nonce_at_source); - match confirmations_missing { - Some(confirmations_missing) - if confirmations_missing >= self.max_unconfirmed_nonces_at_target => - { - log::debug!( - target: "bridge", - "Cannot deliver any more messages from {} to {}. Too many unconfirmed nonces \ - at target: target.latest_received={:?}, source.latest_confirmed={:?}, max={:?}", - MessageDeliveryRace::

::source_name(), - MessageDeliveryRace::

::target_name(), - latest_received_nonce_at_target, - latest_confirmed_nonce_at_source, - self.max_unconfirmed_nonces_at_target, - ); - - return None - }, - _ => (), - } - // Ok - we may have new nonces to deliver. But target may still reject new messages, because // we haven't notified it that (some) messages have been confirmed. So we may want to // include updated `source.latest_confirmed` in the proof. @@ -375,6 +350,7 @@ where // Important note: we're including outbound state lane proof whenever there are unconfirmed // nonces on the target chain. Other strategy is to include it only if it's absolutely // necessary. + let latest_received_nonce_at_target = target_nonces.latest_nonce; let latest_confirmed_nonce_at_target = target_nonces.nonces_data.confirmed_nonce; let outbound_state_proof_required = latest_confirmed_nonce_at_target < latest_confirmed_nonce_at_source; @@ -808,22 +784,6 @@ mod tests { ); } - #[async_std::test] - async fn message_delivery_strategy_selects_nothing_if_too_many_confirmations_missing() { - let (state, mut strategy) = prepare_strategy(); - - // if there are already `max_unconfirmed_nonces_at_target` messages on target, - // we need to wait until confirmations will be delivered by receiving race - strategy.latest_confirmed_nonces_at_source = vec![( - header_id(1), - strategy.target_nonces.as_ref().unwrap().latest_nonce - - strategy.max_unconfirmed_nonces_at_target, - )] - .into_iter() - .collect(); - assert_eq!(strategy.select_nonces_to_deliver(state).await, None); - } - #[async_std::test] async fn message_delivery_strategy_includes_outbound_state_proof_when_new_nonces_are_available() { From 94dc4f805de41e721d39553b11cb632fefc0061a Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 25 Apr 2023 18:01:02 +0300 Subject: [PATCH 2/4] if messages race transaction submit has failed, do not restart loop. Instead, wait for new best nonces from target node and retry selection. That's because submit has probably failed because other relayer has submitted same nonces --- relays/messages/src/message_race_delivery.rs | 5 +++++ relays/messages/src/message_race_loop.rs | 23 +++++++++++++++++--- relays/messages/src/message_race_strategy.rs | 4 ++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/relays/messages/src/message_race_delivery.rs b/relays/messages/src/message_race_delivery.rs index bea8d68ebd8..06e02a06017 100644 --- a/relays/messages/src/message_race_delivery.rs +++ b/relays/messages/src/message_race_delivery.rs @@ -561,6 +561,11 @@ where self.strategy.source_nonces_updated(at_block, nonces) } + fn reset_best_target_nonces(&mut self) { + self.target_nonces = None; + self.strategy.reset_best_target_nonces(); + } + fn best_target_nonces_updated, TargetHeaderIdOf

>>( &mut self, nonces: TargetClientNonces, diff --git a/relays/messages/src/message_race_loop.rs b/relays/messages/src/message_race_loop.rs index be7d5b46756..3df9eef4960 100644 --- a/relays/messages/src/message_race_loop.rs +++ b/relays/messages/src/message_race_loop.rs @@ -195,6 +195,9 @@ pub trait RaceStrategy: Debug { at_block: SourceHeaderId, nonces: SourceClientNonces, ); + /// Called when we want to wait until next `best_target_nonces_updated` before selecting + /// any nonces for delivery. + fn reset_best_target_nonces(&mut self); /// Called when best nonces are updated at target node of the race. fn best_target_nonces_updated>( &mut self, @@ -542,14 +545,22 @@ pub async fn run, TC: TargetClient

>( P::target_name(), ); - race_state.reset_nonces_to_submit(); race_state.nonces_submitted = Some(artifacts.nonces); target_tx_tracker.set(artifacts.tx_tracker.wait().fuse()); }, &mut target_go_offline_future, async_std::task::sleep, || format!("Error submitting proof {}", P::target_name()), - ).fail_if_error(FailedClient::Target).map(|_| true)?; + ).fail_if_connection_error(FailedClient::Target)?; + + // in any case - we don't need to retry submitting the same nonces again until + // we read nonces from the target client + race_state.reset_nonces_to_submit(); + // if we have failed to submit transaction AND that is not the connection issue, + // then we need to read best target nonces before selecting nonces again + if !target_client_is_online { + strategy.reset_best_target_nonces(); + } }, target_transaction_status = target_tx_tracker => { match (target_transaction_status, race_state.nonces_submitted.as_ref()) { @@ -699,7 +710,13 @@ pub async fn run, TC: TargetClient

>( .fuse(), ); } else if let Some(source_required_header) = source_required_header.clone() { - log::debug!(target: "bridge", "Going to require {} header {:?} at {}", P::source_name(), source_required_header, P::target_name()); + log::debug!( + target: "bridge", + "Going to require {} header {:?} at {}", + P::source_name(), + source_required_header, + P::target_name(), + ); target_require_source_header .set(race_target.require_source_header(source_required_header).fuse()); } else if target_best_nonces_required { diff --git a/relays/messages/src/message_race_strategy.rs b/relays/messages/src/message_race_strategy.rs index 718c296391c..a2da4a3a010 100644 --- a/relays/messages/src/message_race_strategy.rs +++ b/relays/messages/src/message_race_strategy.rs @@ -254,6 +254,10 @@ impl< ) } + fn reset_best_target_nonces(&mut self) { + self.best_target_nonce = None; + } + fn best_target_nonces_updated< RS: RaceState< HeaderId, From ec0ed529f567441373299c698867655f0a61b767 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 25 Apr 2023 18:01:45 +0300 Subject: [PATCH 3/4] reset nonces_to_submit and nonces_submitted if at least one of selected/submitted nonces is already at target --- relays/messages/src/message_race_strategy.rs | 48 ++++++++++++++++---- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/relays/messages/src/message_race_strategy.rs b/relays/messages/src/message_race_strategy.rs index a2da4a3a010..e08b1211131 100644 --- a/relays/messages/src/message_race_strategy.rs +++ b/relays/messages/src/message_race_strategy.rs @@ -270,19 +270,37 @@ impl< ) { let nonce = nonces.latest_nonce; + // if **some** of nonces that we have selected to submit already present at the + // target chain => select new nonces let need_to_select_new_nonces = race_state .nonces_to_submit() - .map(|nonces| *nonces.end() <= nonce) + .map(|nonces| nonce >= *nonces.start()) .unwrap_or(false); - if need_to_select_new_nonces { + if need_to_select_new_nonces && race_state.nonces_to_submit().is_some() { + log::trace!( + target: "bridge", + "Latest nonce at target is {}. Clearing nonces to submit: {:?}", + nonce, + race_state.nonces_to_submit(), + ); + race_state.reset_nonces_to_submit(); } + // if **some** of nonces that we have submitted already present at the + // target chain => select new nonces let need_new_nonces_to_submit = race_state .nonces_submitted() - .map(|nonces| *nonces.end() <= nonce) + .map(|nonces| nonce >= *nonces.start()) .unwrap_or(false); - if need_new_nonces_to_submit { + if need_new_nonces_to_submit && race_state.nonces_submitted().is_some() { + log::trace!( + target: "bridge", + "Latest nonce at target is {}. Clearing submitted nonces: {:?}", + nonce, + race_state.nonces_submitted(), + ); + race_state.reset_nonces_submitted(); } @@ -419,10 +437,15 @@ mod tests { let mut state = TestRaceStateImpl::default(); let mut strategy = BasicStrategy::::new(); state.nonces_to_submit = Some((header_id(1), 5..=10, (5..=10, None))); - strategy.best_target_nonces_updated(target_nonces(7), &mut state); + // we are going to submit 5..=10, so having latest nonce 4 at target is fine + strategy.best_target_nonces_updated(target_nonces(4), &mut state); assert!(state.nonces_to_submit.is_some()); - strategy.best_target_nonces_updated(target_nonces(10), &mut state); - assert!(state.nonces_to_submit.is_none()); + // any nonce larger than 4 invalidates the `nonces_to_submit` + for nonce in 5..=11 { + state.nonces_to_submit = Some((header_id(1), 5..=10, (5..=10, None))); + strategy.best_target_nonces_updated(target_nonces(nonce), &mut state); + assert!(state.nonces_to_submit.is_none()); + } } #[test] @@ -430,10 +453,15 @@ mod tests { let mut state = TestRaceStateImpl::default(); let mut strategy = BasicStrategy::::new(); state.nonces_submitted = Some(5..=10); - strategy.best_target_nonces_updated(target_nonces(7), &mut state); + // we have submitted 5..=10, so having latest nonce 4 at target is fine + strategy.best_target_nonces_updated(target_nonces(4), &mut state); assert!(state.nonces_submitted.is_some()); - strategy.best_target_nonces_updated(target_nonces(10), &mut state); - assert!(state.nonces_submitted.is_none()); + // any nonce larger than 4 invalidates the `nonces_submitted` + for nonce in 5..=11 { + state.nonces_submitted = Some(5..=10); + strategy.best_target_nonces_updated(target_nonces(nonce), &mut state); + assert!(state.nonces_submitted.is_none()); + } } #[async_std::test] From 754bc3dd2d2e677e1d17c9f970e1d355d1305b78 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 25 Apr 2023 18:40:36 +0300 Subject: [PATCH 4/4] removed extra check --- relays/messages/src/message_race_strategy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/relays/messages/src/message_race_strategy.rs b/relays/messages/src/message_race_strategy.rs index e08b1211131..5c8f9a162b4 100644 --- a/relays/messages/src/message_race_strategy.rs +++ b/relays/messages/src/message_race_strategy.rs @@ -276,7 +276,7 @@ impl< .nonces_to_submit() .map(|nonces| nonce >= *nonces.start()) .unwrap_or(false); - if need_to_select_new_nonces && race_state.nonces_to_submit().is_some() { + if need_to_select_new_nonces { log::trace!( target: "bridge", "Latest nonce at target is {}. Clearing nonces to submit: {:?}", @@ -293,7 +293,7 @@ impl< .nonces_submitted() .map(|nonces| nonce >= *nonces.start()) .unwrap_or(false); - if need_new_nonces_to_submit && race_state.nonces_submitted().is_some() { + if need_new_nonces_to_submit { log::trace!( target: "bridge", "Latest nonce at target is {}. Clearing submitted nonces: {:?}",