Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Message codec indexes #7437

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Fix Message codec indexes #7437

merged 3 commits into from
Feb 4, 2025

Conversation

serban300
Copy link
Contributor

Fixes #7400

@serban300 serban300 added the I2-bug The node fails to follow expected behavior. label Feb 3, 2025
@serban300 serban300 self-assigned this Feb 3, 2025
@lexnv
Copy link
Contributor

lexnv commented Feb 3, 2025

Nice catch here!

I believe we are no longer using the message and the code seems to be forgotten during a refactoring. Instead, we rely on the protobuf encoding for networking messages.

The message is annotated with unused here:

polkadot-sdk/substrate/client/network/src/protocol/message.rs

Lines 28 to 34 in 7971f95

#[allow(unused)]
pub type Message<B> = generic::Message<
<B as BlockT>::Header,
<B as BlockT>::Hash,
<<B as BlockT>::Header as HeaderT>::Number,
<B as BlockT>::Extrinsic,
>;

External users from crates-io are not able to access this structure either:

polkadot-sdk/substrate/client/network/src/lib.rs

Line 248 in 7971f95

mod protocol;

I would opt for removing the entire substrate/client/network/src/protocol/message.rs file

cc @dmitry-markin Would you happen to know what's the story behind this enum? 🤔

@serban300 serban300 added the R0-silent Changes should not be mentioned in any release notes label Feb 3, 2025
@serban300 serban300 mentioned this pull request Feb 3, 2025
Open
@serban300
Copy link
Contributor Author

Thank you @lexnv for the suggestion ! Done.

lexnv
lexnv approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for catching this 🙏

@gui1117 gui1117 added this pull request to the merge queue Feb 4, 2025
@gui1117 gui1117 removed this pull request from the merge queue due to a manual request Feb 4, 2025
github-merge-queue bot pushed a commit that referenced this pull request Feb 4, 2025
@serban300
Fix Message codec indexes (#7437)
f2a7c53 
Fixes #7400
@gui1117 gui1117 added this pull request to the merge queue Feb 4, 2025
Merged via the queue into paritytech:master with commit d6aa157 Feb 4, 2025
205 of 209 checks passed
@dmitry-markin
Copy link
Contributor

Wasn't there a plan to delete the message.rs file completely?

@gui1117
Copy link
Contributor

gui1117 commented Feb 4, 2025

Wasn't there a plan to delete the message.rs file completely?

I may have pushed the button too fast, but if we can remove more unused code, let's continue in another PR

@serban300 serban300 added the A4-needs-backport Pull request must be backported to all maintained releases. label Feb 5, 2025
github-actions bot pushed a commit that referenced this pull request Feb 5, 2025
@serban300 @github-actions
Fix Message codec indexes (#7437)
9b1c299 
Fixes #7400

(cherry picked from commit d6aa157)
@paritytech-cmd-bot-polkadot-sdk
Copy link

Successfully created backport PR for stable2407:

@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Feb 5, 2025
Merged
github-actions bot pushed a commit that referenced this pull request Feb 5, 2025
@serban300 @github-actions
Fix Message codec indexes (#7437)
2795848 
Fixes #7400

(cherry picked from commit d6aa157)
@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Feb 5, 2025
Merged
@paritytech-cmd-bot-polkadot-sdk
Copy link

Successfully created backport PR for stable2409:

github-actions bot pushed a commit that referenced this pull request Feb 5, 2025
@serban300 @github-actions
Fix Message codec indexes (#7437)
38f34fd 
Fixes #7400

(cherry picked from commit d6aa157)
@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Feb 5, 2025
Merged
@paritytech-cmd-bot-polkadot-sdk
Copy link

Successfully created backport PR for stable2412:

@serban300 serban300 mentioned this pull request Feb 5, 2025
Open
@serban300 serban300 had a problem deploying to subsystem-benchmarks February 5, 2025 09:34 — with GitHub Actions Failure
EgorPopelyaev pushed a commit that referenced this pull request Feb 5, 2025
@paritytech-cmd-bot-polkadot-sdk @serban300 @ggwpez
[stable2407] Backport #7437 (#7465)
dc31da0 
Backport #7437 into `stable2407` from serban300.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Serban Iorga <serban@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
EgorPopelyaev pushed a commit that referenced this pull request Feb 5, 2025
@paritytech-cmd-bot-polkadot-sdk @serban300
[stable2409] Backport #7437 (#7466)
35f4c11 
Backport #7437 into `stable2409` from serban300.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

---------

Co-authored-by: Serban Iorga <serban@parity.io>
EgorPopelyaev pushed a commit that referenced this pull request Feb 5, 2025
@paritytech-cmd-bot-polkadot-sdk @serban300
[stable2412] Backport #7437 (#7467)
54a4ec0 
Backport #7437 into `stable2412` from serban300.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

---------

Co-authored-by: Serban Iorga <serban@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lexnv lexnv lexnv approved these changes

@gui1117 gui1117 gui1117 approved these changes

Assignees

@serban300 serban300

Labels
A4-needs-backport Pull request must be backported to all maintained releases. I2-bug The node fails to follow expected behavior. R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate codec index in Message
4 participants
@serban300 @lexnv @dmitry-markin @gui1117