Skip to content

Conversation

@serban300
Copy link
Contributor

@serban300 serban300 commented Oct 28, 2025

Resolves #7749

@serban300 serban300 self-assigned this Oct 28, 2025
@serban300 serban300 changed the title [WIP] Send PeerId via UMP [WIP][Draft] Send PeerId via UMP Oct 28, 2025
@serban300 serban300 added the T9-cumulus This PR/Issue is related to cumulus. label Oct 28, 2025
@serban300 serban300 force-pushed the send-approved-peer-ump-signal branch 2 times, most recently from f19389c to 97bebcf Compare October 28, 2025 13:26
@serban300 serban300 force-pushed the send-approved-peer-ump-signal branch from 97bebcf to 730dfc3 Compare October 28, 2025 14:47
@serban300 serban300 force-pushed the send-approved-peer-ump-signal branch from 730dfc3 to 239752a Compare October 28, 2025 15:55
@serban300 serban300 changed the title [WIP][Draft] Send PeerId via UMP Send PeerId via UMP Oct 29, 2025
@serban300 serban300 requested a review from alindima October 29, 2025 07:08
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

Thanks @serban300 . Looks good, left a few comments.

/// This method expects to receive a `Vec` of at most 64 bytes. Otherwise, it panics.
pub fn unchecked_new_approved_peer_id(bytes: Vec<u8>) -> ApprovedPeerId {
ApprovedPeerId::try_from(bytes)
.expect("unchecked_new_approved_peer_id() should receive a sequence of at most 64 bytes")
Copy link
Contributor

@sandreim sandreim Oct 29, 2025

Choose a reason for hiding this comment

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

Not really nice this panics. Maybe Option better ?

relay_chain_state,
relay_parent_descendants,
collator_peer_id: None,
collator_peer_id: Some(collator_peer_id),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the only place where we should convert using unchecked_new_approved_peer_id, and keep using the client network primitive PeerId across the node.

CumulusDigestItem::find_core_info(&frame_system::Pallet::<T>::digest())
{
ump_signals.push(
UMPSignal::SelectCore(core_info.selector, core_info.claim_queue_offset).encode(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should double check before merging if UMPSignal::ApprovedPeer is supported by current RC runtimes.

// Take the pending UMP signals.
let mut ump_signals = PendingUpwardSignals::<T>::take();
// Append the core selector signal.
if let Some(core_info) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks a bit inconsistent now that you added PendingUpwardSignals, why not put UMPSignal::SelectCore there as well ?

Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

LGTM

@alindima alindima requested a review from skunert November 3, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T9-cumulus This PR/Issue is related to cumulus.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collator Protocol Revamp: send PeerId via UMP

3 participants