-
Notifications
You must be signed in to change notification settings - Fork 379
Unify RelayChainInterface error handling and introduce async #909
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
Looks good, but can still be a little bit improved here and there.
@@ -222,6 +218,7 @@ impl TryFrom<&'_ CollationSecondedSignal> for BlockAnnounceData { | |||
/// chain. If it is at the tip, it is required to provide a justification or otherwise we reject | |||
/// it. However, if the announcement is for a block below the tip the announcement is accepted | |||
/// as it probably comes from a node that is currently syncing the chain. | |||
#[derive(Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what do we need the clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having lifetime issues while moving function calls to the async block in the validate method: /~https://github.com/paritytech/cumulus/pull/909/files#diff-0fe3ce0d7087ba7f979d9065d8d812b23562d2f4ed5341719aa8480dc48c74cdR345
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see. Can you then make the handle_empty_block_announce_data
async and not have it return a future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
client/pov-recovery/src/lib.rs
Outdated
}); | ||
|
||
session_index_result | ||
.and_then(|v| Ok(pending_availability_result?.map(|pa| (pa, v)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.and_then(|v| Ok(pending_availability_result?.map(|pa| (pa, v)))) | |
.and_then(|v| pending_availability_result.map(|pa| (pa, v))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion does not work here as it changes the return type. However, I tried to improve the readability of this section
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
This PR includes more changes in preparation of network communication with the relay chain:
RelayChainInterface
methods asyncRelayChainError
with enum variants for different errorsnew_best_notification_stream
to the interface. Over RPC, we only receive the header instead ofBlockImportNotification
. In some places we were checking if a new block is the new best. These places now consumenew_best_notification_stream
.This PR is not expected to change behaviour.