-
Notifications
You must be signed in to change notification settings - Fork 138
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Thomas Moran <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #16581 +/- ##
=============================================
- Coverage 63.37% 63.36% -0.02%
+ Complexity 20170 20151 -19
=============================================
Files 2532 2533 +1
Lines 94122 94125 +3
Branches 9848 9862 +14
=============================================
- Hits 59650 59639 -11
- Misses 30855 30863 +8
- Partials 3617 3623 +6 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
There was a problem hiding this 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:
- 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
- Make sure that
proto
file specification always explains all of the relevant requirements for each message and field. - In a proto file comment, add any relevant additional detail, possibly including the "what/why/how" from above, as applied to that particular file.
- 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 <[email protected]>
Signed-off-by: Thomas Moran <[email protected]>
Signed-off-by: Thomas Moran <[email protected]>
@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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions:
- If this is purely for on-disk data, it doesn't belong in the
state
folder. Placing it in state implies it will be stored in state; and it is not (cannot be) designed properly for both uses. - Why are we even using protobuf for this? On-disk startup files should be human readable and human editable to support operations teams, and protobuf is not a good format for that purpose. I would think a YAML file would be the best option, closely followed by JSON.
- It should be clearly documented in these files that this is an on-disk configuration file definition.
- The HAPI documentation guidelines do not apply to on-disk data formats; but that requires this definition to be well separated from the HAPI files (not in the
hapi
module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we specify the structure of the file in protobuf and still have a JSON representation on disk? Could it even be YAML, translated to JSON, then parsed by protobuf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to output JSON from a protobuf specification, but it's not a production option; that format is intended for debugging and development. In particular it's not guaranteed to round-trip deterministically.
It might be much easier to just create a Java record
class and have that output and input as YAML, using whichever YAML processor we prefer.
Protobuf-JSON format might work; I cannot say for certain either way, but it's definitely not what I would recommend.
One other thought. This is an internal private detail of the consensus node that should not be published in the HAPI API or presented to SDKs in the hedera-protobufs
repo. As such (and as noted above) this definition probably belongs in a different module (hedera-app, or perhaps the network service?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are producing this file by hand, that may be problematic. It's meant to be an automated export (probably at startup) and consumed as an override-network
in the transplant and reset use case. With the TSS information, there's a lot that can go wrong. It's all binary data and doesn't go well for by-hand assembly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TSS data is binary, and probably should be retained as such (e.g. base64 encoded).
The Node data, however, very likely needs to be edited every time (because the destination network won't have the same nodes), so the overall file format still needs to be editable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the reason I remember, we added a protobuf object was to be able to easily parse TSS key data. @tinker-michaelj thoughts ?
Description:
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 RosterDesign and Motivations:
TssEncryptionKeys
, Roster entries, andledgerIds
.NodeMetadata
, which includes fields such as theTssEncryptionKey
(now removed from the Roster) and other information previously found in the Address Book.tssEncryptionKey
is required here because it has been removed from the Roster and will need to be create here with Tss Key MaterialLedgerId
is required, because it needs to be validated against when you start the new networkRelated issue(s):
Fixes #16549