Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Unify RelayChainInterface error handling and introduce async #909

Merged
merged 33 commits into from
Jan 25, 2022

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Jan 18, 2022

This PR includes more changes in preparation of network communication with the relay chain:

  • Make RelayChainInterface methods async
  • Make methods return results (Over network, all operations could fail)
  • Logging of errors is moved out of the interface to the caller side
  • Unify error handling by introducing RelayChainError with enum variants for different errors
  • Introduce new_best_notification_stream to the interface. Over RPC, we only receive the header instead of BlockImportNotification. In some places we were checking if a new block is the new best. These places now consume new_best_notification_stream.

This PR is not expected to change behaviour.

@skunert skunert added B0-silent Changes should not be mentioned in any release notes C1-low 📌 labels Jan 18, 2022
@skunert skunert requested a review from bkchr January 18, 2022 12:34
Copy link
Member

@bkchr bkchr left a 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.

primitives/parachain-inherent/src/client_side.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-local/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-local/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-local/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-interface/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-local/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-local/src/lib.rs Outdated Show resolved Hide resolved
@@ -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)]
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

});

session_index_result
.and_then(|v| Ok(pending_availability_result?.map(|pa| (pa, v))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.and_then(|v| Ok(pending_availability_result?.map(|pa| (pa, v))))
.and_then(|v| pending_availability_result.map(|pa| (pa, v)))

Copy link
Contributor Author

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

client/relay-chain-interface/Cargo.toml Outdated Show resolved Hide resolved
@skunert skunert merged commit a963055 into master Jan 25, 2022
@skunert skunert deleted the relay-chain-interface-error-handling branch January 25, 2022 17:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants