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.

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

feat: Network and NodeMetadata Protobuf #16581

Merged
merged 11 commits into from
Nov 20, 2024

Conversation

thomas-swirlds-labs
Copy link
Contributor

@thomas-swirlds-labs thomas-swirlds-labs commented Nov 13, 2024

Description:

  • Defines a protobuf-based network object to replace the legacy config.txt file, fully encapsulating the information required to bootstrap a network with Threshold Signature Scheme (TSS) enabled and with the legacy address book completely replaced by Roster

Design and Motivations:

  • Eliminates the requirement for separate overrides or genesis configurations, providing a standardized format for storing metadata about all nodes in a network, including TssEncryptionKeys, Roster entries, and ledgerIds.
  • Note: this is a format for data that exists on disk, not in state or block streams, and can be regenerated during state freeze operations for network resets.
  • Each node in the network is represented by NodeMetadata, which includes fields such as the TssEncryptionKey (now removed from the Roster) and other information previously found in the Address Book.
  • This ensures compatibility for both Genesis startup (networks starting without state) and Network Transplant (starting from a previously saved state from another network).
  • tssEncryptionKey is required here because it has been removed from the Roster and will need to be create here with Tss Key Material
  • LedgerId is required, because it needs to be validated against when you start the new network

Related issue(s):

Fixes #16549

Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (develop@0461c65). Learn more about missing BASE report.
Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             develop   #16581   +/-   ##
==========================================
  Coverage           ?   63.39%           
  Complexity         ?    20193           
==========================================
  Files              ?     2538           
  Lines              ?    94260           
  Branches           ?     9873           
==========================================
  Hits               ?    59753           
  Misses             ?    30905           
  Partials           ?     3602           

Impacted file tree graph

---- 🚨 Try these New Features:

Copy link

codacy-production bot commented Nov 13, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -1.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (0461c65) 97025 63168 65.10%
Head commit (5e5a5d7) 97025 (+0) 63180 (+12) 65.12% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#16581) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

It isn't entirely clear (even after reading the linked issue and further linked Notion page) what these messages are for.

General suggestions:

  1. Detail in the PR description all of "what", "why", and "how" of the PR.
    • What is being done here, design/feature wise
    • Why is this needed, and why is it designed in this way
    • How is this to be used and how does it meet future needs
  2. Make sure that proto file specification always explains all of the relevant requirements for each message and field.
  3. In a proto file comment, add any relevant additional detail, possibly including the "what/why/how" from above, as applied to that particular file.
  4. Writing too much description is, in my experience, almost never a problem. The common problem is PRs, specifications, design documentation (i.e. code files), and related text that is woefully short and imprecise.

Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
@thomas-swirlds-labs
Copy link
Contributor Author

thomas-swirlds-labs commented Nov 15, 2024

It isn't entirely clear (even after reading the linked issue and further linked Notion page) what these messages are for.

General suggestions:

  1. Detail in the PR description all of "what", "why", and "how" of the PR.

    • What is being done here, design/feature wise
    • Why is this needed, and why is it designed in this way
    • How is this to be used and how does it meet future needs
  2. Make sure that proto file specification always explains all of the relevant requirements for each message and field.

  3. In a proto file comment, add any relevant additional detail, possibly including the "what/why/how" from above, as applied to that particular file.

  4. Writing too much description is, in my experience, almost never a problem. The common problem is PRs, specifications, design documentation (i.e. code files), and related text that is woefully short and imprecise.

@jsync-swirlds. Agreed. I've updated the description of the PR to include more detailed description of the motivation and design and give some more background on the what/why/how. Hopefully that should provide a little more clarity now

Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
petreze
petreze previously approved these changes Nov 15, 2024
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
jsync-swirlds
jsync-swirlds previously approved these changes Nov 19, 2024
Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

Well executed and thorough.
Thanks.

Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Copy link
Contributor

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Review applies to:
hapi/build.gradle.kts
hedera-node/hedera-app-spi/build.gradle.kts

@thomas-swirlds-labs thomas-swirlds-labs merged commit 53409a1 into develop Nov 20, 2024
52 of 68 checks passed
@thomas-swirlds-labs thomas-swirlds-labs deleted the 16549-network-metadata-protos branch November 20, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Network and NodeMetadata protobuf representation
6 participants