Skip to content

feat: add collator peer ID to ParachainInherentData#8708

Merged
alindima merged 6 commits intomasterfrom
sw10pa/collator-peer-id-inherent
Jun 2, 2025
Merged

feat: add collator peer ID to ParachainInherentData#8708
alindima merged 6 commits intomasterfrom
sw10pa/collator-peer-id-inherent

Conversation

@sw10pa
Copy link
Contributor

@sw10pa sw10pa commented May 30, 2025

Issue

[#7749] Collator Protocol Revamp: send PeerId via UMP

Description

Adds an optional collator_peer_id field to the new version of ParachainInherentData struct from PR #8299. This field is currently unused and defaults to None, but is added proactively to avoid introducing yet another version of the inherent data after the current release.

Next Steps

In a follow-up PR:

  • Populate collator_peer_id on the collator side;
  • Handle the field in the parachain-system pallet and sends a UMP signal to the relay chain.

@sw10pa sw10pa requested review from alindima and skunert May 30, 2025 10:48
@sw10pa sw10pa self-assigned this May 30, 2025
@sw10pa sw10pa added the T9-cumulus This PR/Issue is related to cumulus. label May 30, 2025
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

LGTM!

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/15345509551
Failed job name: cargo-clippy

bkchr
bkchr previously requested changes May 30, 2025
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.

I'm against adding some things to structures without having the underlying approach approved.

@sw10pa
Copy link
Contributor Author

sw10pa commented May 30, 2025

I'm against adding some things to structures without having the underlying approach approved.

Hi @bkchr, what do you mean by "underlying approach approved"? We are going to use this attribute to store collator peer ID, as described in #7749.

@alindima
Copy link
Contributor

I'm against adding some things to structures without having the underlying approach approved.

The underlying approach is the whole collator protocol revamp, which has been reviewed for quite some time (I sent you the doc and several fellows reviewed it, as well as someone from the auditing team) and the implementation is under way (parts of it already merged). We're doing an incremental development plan to avoid big bang implementations that are hard to review

@alindima alindima added this pull request to the merge queue Jun 2, 2025
Merged via the queue into master with commit 25d6e88 Jun 2, 2025
257 of 260 checks passed
@alindima alindima deleted the sw10pa/collator-peer-id-inherent branch June 2, 2025 08:45
@github-project-automation github-project-automation bot moved this from Backlog to Completed in parachains team board Jun 2, 2025
pub relay_parent_descendants: Vec<RelayHeader>,
/// Contains the collator peer ID, which is later sent by the parachain to the
/// relay chain via a UMP signal to promote the reputation of the given peer ID.
pub collator_peer_id: Option<ApprovedPeerId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

@sw10pa just a dq, I am not sure about now, when we add this field here, doesn't it change encoding or with None no? I am thinking about when the runtime with this change is enacted, what about older validators/collators without this change, do they still work? Will the ParachainInherentData with collator_peer_id: None produce the same data as ParachainInherentData without collator_peer_id field? Will both validators/collators produce the same state_root or whatever? Other words, is this change backwards-compatible? Will the older validators/collators still work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@skunert added this new inherent data type in #8299 in a backwards compatible way. This new data type is written at a different key. This PR piggy-backs on that logic.

  • Old collators, old runtime. This is the present state, which works just fine.
  • Old collators, new runtime. New runtime will attempt to decode both data types. If the new one fails, it will fall back to the old one.
  • New collators supply both inherent data types (most of the data being duplicated). Old runtime will decode the old one, new runtime will decode the new one

Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

LGTM

ordian added a commit that referenced this pull request Jun 4, 2025
* master:
  omni-node: fix `benchmark pallet` to work with `--runtime` (#8594)
  Handle and suppress "New unknown `FromSwarm` libp2p event" warning (#8731)
  Implement detailed logging for XCM failures (#8724)
  [pallet-revive] contract's nonce starts at 1 (#8734)
  sync/fix: Clear gap sync on known imported blocks (#8445)
  [PoP] Add personhood tracking pallets (#8164)
  client/net: Use litep2p as the default network backend (#8461)
  Unflake `returns_status_for_pruned_blocks` (#8709)
  [AHM] Report the weights of epmb pallet to expose kusama and polkadot weights (#8704)
  Remove all XCM dependencies from `pallet-revive` (#8584)
  Docker master image tag fix (#8711)
  Record ed as part of the storage deposit (#8718)
  [pallet-revive] update dry-run logic (#8662)
  feat: add collator peer ID to ParachainInherentData (#8708)
  Nest errors in pallet-xcm (#7730)
  pallet-assets ERC20 precompile (#8554)
  Broker: Introduce min price + adjust renewals to lower market. (#8630)
  [AHM] Staking async fixes for XCM and election planning (#8422)
  Staking (EPMB): Add defensive error handling to voter snapshot creation and solution verification (#8687)
pgherveou pushed a commit that referenced this pull request Jun 11, 2025
## Issue
[[#7749] Collator Protocol Revamp: send PeerId via
UMP](#7749)

## Description
Adds an optional `collator_peer_id` field to the new version of
`ParachainInherentData` struct from [PR
#8299](#8299). This field
is currently unused and defaults to `None`, but is added proactively to
avoid introducing yet another version of the inherent data after the
current release.

## Next Steps
### In a follow-up PR:
- Populate `collator_peer_id` on the collator side;
- Handle the field in the `parachain-system` pallet and sends a UMP
signal to the relay chain.
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
## Issue
[[#7749] Collator Protocol Revamp: send PeerId via
UMP](#7749)

## Description
Adds an optional `collator_peer_id` field to the new version of
`ParachainInherentData` struct from [PR
#8299](#8299). This field
is currently unused and defaults to `None`, but is added proactively to
avoid introducing yet another version of the inherent data after the
current release.

## Next Steps
### In a follow-up PR:
- Populate `collator_peer_id` on the collator side;
- Handle the field in the `parachain-system` pallet and sends a UMP
signal to the relay chain.
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

Status: Done
Status: Completed

Development

Successfully merging this pull request may close these issues.

7 participants