Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Provide multi addresses instead of p2p peer ids to the manifest file #1272

Conversation

Stefan-Ethernal
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal commented Mar 7, 2023

Description

This PR changes the way we are expecting validators' network parameters to be specified when creating the manifest file. Prior to this, when specifying validators via CLI flags, only P2P peer id was expected, and then when genesis configuration was generated multi-address was assuming the local host IP address.

Now manifest generation expects entire multi-address to be provided if case genesis validators are being specified via CLI flag. Otherwise, if secrets are provided on local file storage, multi addresses are being constructed with localhost IP addresses.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Please complete this section if you ran manual tests for this functionality, otherwise delete it

Documentation update

Please link the documentation update PR in this section if it's present, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

@Stefan-Ethernal Stefan-Ethernal added the feature New update to Polygon Edge label Mar 7, 2023
@Stefan-Ethernal Stefan-Ethernal self-assigned this Mar 7, 2023
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Will the multiaddr be output by the polybft-secrets command?

@Stefan-Ethernal
Copy link
Collaborator Author

Will the multiaddr be output by the polybft-secrets command?

Nope, it just prints out P2P node id. The thing is that AFAIK, we don't keep IP addresses and ports inside secrets, but only P2P node IDs, so we are not able to reconstruct the entire multiaddress.

@vcastellm
Copy link
Contributor

Nope, it just prints out P2P node id. The thing is that AFAIK, we don't keep IP addresses and ports inside secrets, but only P2P node IDs, so we are not able to reconstruct the entire multiaddress.

Ok, so it's the user that should build the multiaddr format for feeding the manifest command, right? We should specify that in docs

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@Stefan-Ethernal
Copy link
Collaborator Author

Ok, so it's the user that should build the multiaddr format for feeding the manifest command, right? We should specify that in docs

Yes, that is correct. 👍

@Stefan-Ethernal
Copy link
Collaborator Author

We should specify that in docs

Updated README on my end: fa743f1

@Stefan-Ethernal Stefan-Ethernal merged commit 39dc415 into develop Mar 8, 2023
@Stefan-Ethernal Stefan-Ethernal deleted the EVM-490-read-the-full-multi-address-string-in-manifest-command branch March 8, 2023 12:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New update to Polygon Edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants