Skip to content

[Infra] Fullnode on LocalNet#727

Merged
okdas merged 12 commits intomainfrom
fullnode-localnet
May 12, 2023
Merged

[Infra] Fullnode on LocalNet#727
okdas merged 12 commits intomainfrom
fullnode-localnet

Conversation

@okdas
Copy link
Contributor

@okdas okdas commented May 3, 2023

Description

Adds a fullnode (an unstaked validator) to the LocalNet

Issue

Fixes #719

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Adds full node to LocalNet
  • Makes helm chart agnostic of the protocol actor it runs
  • Adds more private keys for other actors (except fisherman)

Testing

  • make develop_test; if any code changes were made
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made
  • remote DevNet

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@okdas okdas added the infra Core infrastructure - not protocol related label May 3, 2023
@okdas okdas self-assigned this May 3, 2023
@reviewpad reviewpad bot added the small label May 3, 2023
@reviewpad reviewpad bot added large and removed small labels May 4, 2023
@okdas okdas added cl validate Run the changelog validation workflow e2e-devnet-test Runs E2E tests on devnet labels May 4, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The changelog validation failed with the following output:
Latest date in charts/CHANGELOG.md is incorrect.
Latest: 2023-05-03, Current: 2023-05-05

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

@github-actions github-actions bot dismissed their stale review May 5, 2023 22:47

The check succeeded, dismissing the review comment.

@okdas okdas added the gpt review Triggers a code review from https://github.com/anc95/ChatGPT-CodeReview label May 5, 2023
@okdas okdas marked this pull request as ready for review May 5, 2023 22:48
@okdas okdas requested a review from Olshansk May 5, 2023 22:49
@okdas
Copy link
Contributor Author

okdas commented May 5, 2023

e2e passed on remote (sorry no CI for this yet)
image

Copy link
Collaborator

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Some minor NITS + 1 question but nothing blocking so don't hesitate to merge in!

@Olshansk
Copy link
Collaborator

@okdas Following up on our offline conversation, let's fix this up and and get it over the finish line!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The changelog validation failed with the following output:
Latest date in charts/CHANGELOG.md is incorrect.
Latest: 2023-05-05, Current: 2023-05-12

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.


## [0.0.0.40] - 2023-05-04

- Added `network_id` parameter to the node config files

Choose a reason for hiding this comment

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

Overall, the patch seems to be updating the changelog with developments made in the project. Below are some suggestions and potential risk factors:

  1. Bug Risk: Mention of "private keys for all (except fisherman) actors". Storing private keys in a repository or sharing them among multiple users is a significant security concern.

Improvement Suggestion: If these private keys are meant for testing purposes only, clarify this in the changelog or remove them entirely and use a more secure method for handling private keys, such as environment variables or secret management tools.

  1. Improvement Suggestion: Add more context about the introduced changes. Providing details on how the updated features function and interact within the system can help other developers understand the rationale and usage of the new code.

For example:

  • Explain the significance of excluding the 'fisherman' actor.
  • Describe how the debug_keybase package update will impact the system.
  • Clarify the role of the full node (non-staked validator), and its implications on the network.

It would be helpful to have access to the actual code changes to provide a more detailed review, but based on the information provided in the submitted patch, these are the primary concerns and suggestions.

@github-actions github-actions bot dismissed their stale review May 12, 2023 19:05

The check succeeded, dismissing the review comment.


## [0.0.0.3] - 2023-04-19

- Rename `pocket-validator` to `pocket`, as we are not going to ship different artifacts, but will have just one.

Choose a reason for hiding this comment

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

The code patch provided aims to update the project's changelog with two major modifications:

  1. Adding a new version [0.0.0.4] - 2023-05-12 that includes:
    a. Addition of nodeType parameter to the Helm chart, making it actor-agnostic.
    b. Modifications in other parts where the validator is mentioned to prevent confusion with non-validator actors.

  2. An earlier version update [0.0.0.3] - 2023-04-19 which indicates renaming of pocket-validator to pocket.

Since this is only a changelog update, no actual code is shown or modified. You can go ahead and commit this change if the listed modifications accurately reflect the updates made to the project. Make sure to verify the functionality of the changes made in the actual codebase through appropriate testing, and check for any potential issues or improvements there.

@okdas okdas requested a review from Olshansk May 12, 2023 19:06

## [0.0.0.41] - 2023-05-08

- Updated Dockerfiles using outdated go version to 1.19

Choose a reason for hiding this comment

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

Here's a brief code review of the provided patch:

  1. The patch adds a new version (0.0.0.42) to the changelog with the date 2023-05-12.

  2. The changes include:

    • Adding private keys for all actors (except fisherman)
    • Changing the debug_keybase package to support multiple YAML secrets in one YAML file
    • Adding a full node (non-staked validator)

Potential issues and suggestions:

  1. Storing private keys directly in the code base can be a security risk. Consider using a secure key management system or environment variables to avoid exposing sensitive information.

  2. It is unclear if there are any access control mechanisms if the private keys are visible in the project. Make sure to implement proper access control to prevent unauthorized use of the keys.

  3. Regarding the debug_keybase change, ensure proper parsing and validation of the multiple YAML secrets. This can help minimize potential bugs or vulnerabilities when handling the new format.

  4. For the newly added full node, thoroughly test its integration with the existing architecture to confirm proper functioning and detect any compatibility issues or edge cases.

@okdas
Copy link
Contributor Author

okdas commented May 12, 2023

@okdas Following up on our offline conversation, let's fix this up and and get it over the finish line!

Now it should be good to merge. :)

@okdas okdas merged commit a06d875 into main May 12, 2023
@okdas okdas deleted the fullnode-localnet branch May 12, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cl validate Run the changelog validation workflow e2e-devnet-test Runs E2E tests on devnet gpt review Triggers a code review from https://github.com/anc95/ChatGPT-CodeReview infra Core infrastructure - not protocol related waiting-for-review

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Infrastructure] localnet config for full nodes

2 participants