feat: Protobufs for exact weight TSS #17230
Conversation
Signed-off-by: Neeharika-Sompalli <[email protected]>
Neeharika-Sompalli
requested review from
timo0,
OlegMazurov,
povolev15,
tinker-michaelj and
edward-swirldslabs
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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17230 +/- ##
=========================================
Coverage 64.34% 64.34%
- Complexity 20941 20946 +5
=========================================
Files 2555 2555
Lines 96171 96176 +5
Branches 10055 10054 -1
=========================================
+ Hits 61878 61885 +7
+ Misses 30655 30653 -2
Partials 3638 3638
... and 17 files with indirect coverage changes |
Signed-off-by: Neeharika-Sompalli <[email protected]>
| * This value MUST be set.<br/> | ||
| * This value MUST be greater than zero.<br/> | ||
| */ | ||
| uint64 construction_id = 1; |
There was a problem hiding this comment.
Is this a roster hash? If not, why not use the roster hash? What associates a particular construction with a roster if the roster hash is not used?
| * maximum universe size. A hinTS key is an "extended" public key; | ||
| * that is, a BLS public key combined with hints a signature | ||
| * aggregator can use to prove its aggregate public key by a SNARK | ||
| * for the aggregate secret key. |
There was a problem hiding this comment.
I wasn't aware of an aggregate secret key. Where is this term coming from?
| * This value MUST be set.<br/> | ||
| * This value MUST be greater than zero.<br/> | ||
| */ | ||
| uint64 construction_id = 1; |
There was a problem hiding this comment.
I assume that this is technically a different construction id for the HistoryService even though it is most likely in-synch with the construction id of the HintsService. I think both ids can be the roster hash.
| * This value MUST NOT be empty.<br/> | ||
| * This value MUST contain a valid proof key. | ||
| */ | ||
| bytes proof_key = 1; |
There was a problem hiding this comment.
Is this the Schnorr key? It's a public key. Should we add the term public to it?
I would prefer the term history_signing_public_key or something like that.
The proof use the key directly? Or is the key used to generate signatures and the signatures are input into the proof?
| * certain maximum size M = 2^k; maps to the node's party id | ||
| * in the range [0, M). | ||
| */ | ||
| message NodeSchemeId { |
There was a problem hiding this comment.
Where is this used? There is no protobuf data structure that uses this. Where is the mapping to the node's party id taking place?
| proto.Timestamp aggregation_time = 5; | ||
| /** | ||
| * If set, the preprocessed hinTS keys that were approved by nodes | ||
| * holding at least 1/3 of the weight in the source roster. |
There was a problem hiding this comment.
is source the candidate roster? Normally source and target are directed from old (source) to new (target). Maybe we could call them current and candidate to avoid confusion?
| * This value MUST be set.<br/> | ||
| * This value MUST NOT be empty. | ||
| */ | ||
| repeated PartyAssignment party_assignments = 7; |
There was a problem hiding this comment.
If we stored the size in the construction, we wouldn't have the size duplicated in so many more data structure instances.
| * This value MUST be set.<br/> | ||
| * This value MUST NOT be empty. | ||
| */ | ||
| bytes proof_public_key = 3; |
There was a problem hiding this comment.
How is this different than the proof_key in the history_proof_key_publication.proto?
| * roster transition phase; so the node should begin using it | ||
| * then. | ||
| */ | ||
| bytes next_key = 4; |
There was a problem hiding this comment.
Are these two different keys belonging to two different constructions? I don't understand why they are being paired like this. And they are not associated to particular constructions.
| * This value MUST be set.<br/> | ||
| * This value MUST NOT be empty. | ||
| */ | ||
| bytes key = 2; |
There was a problem hiding this comment.
Is this key value a duplicate of a key contained in the ProofKeySet? Is it a duplicate of the proof_key value in the publication?
| * The unique identifier for the proof roster construction that this | ||
| * vote is associated with. | ||
| */ | ||
| uint64 construction_id = 1; |
There was a problem hiding this comment.
if the construction id was a roster hash, does this data structure evaporate as the roster matching the hash would indicate all the valid node ids?
| * The id of any node that already voted for the exact tagged proof roster | ||
| * this node is voting for. | ||
| */ | ||
| uint64 congruent_node_id = 2; |
There was a problem hiding this comment.
I'm still not following the purpose of referencing the vote of another node? data redundancy reduction? Would voting on a hash of the proof be sufficient data reduction?
| /** | ||
| * A set of Schnorr keys for a node; that is, the key the node is | ||
| * currently using and the key it party wishes to use for the next | ||
| * proof roster assembly. |
There was a problem hiding this comment.
What is a proof roster? I am reading as adjective noun. It's a roster in support of a proof? I can make more sense of the term roster proof being a proof that is in support of a roster.
| * This value MUST be set.<br/> | ||
| * This value MUST NOT be empty. | ||
| */ | ||
| repeated ProofKey proof_keys = 4; |
There was a problem hiding this comment.
Is source here the current roster or the candidate roster?
| uint64 construction_id = 1; | ||
| /** | ||
| * The hash of the roster whose weights are used to determine when | ||
| * certain thresholds are during construction. |
There was a problem hiding this comment.
which thresholds?
| /** | ||
| * The hash of the roster whose weights are used to assess progress | ||
| * toward obtaining Schnorr keys for parties that hold at least a | ||
| * strong minority of the stake in that roster. |
There was a problem hiding this comment.
strong minority is defined as? The candidate roster needs >2/3 weight of keys to be present before any node should move forward with computing the proof. The current roster needs to have >=1/3 weight on acceptance votes for the network to be able to adopt the roster proof.
| * <p> | ||
| * This value MUST be set. | ||
| */ | ||
| HistoryAssembly assembly = 1; |
There was a problem hiding this comment.
Should this be a hash of the assembly?
I don't understand the difference between the terms assembly and construction. They are synonyms, each non-descriptive terms. The high level design document doesn't explain what an assembly is.
| /** | ||
| * A recorded history assembly signature. | ||
| */ | ||
| message RecordedHistoryAssemblySignature { |
There was a problem hiding this comment.
No hash of what is being signed?
There was a problem hiding this comment.
The proto file layout is quite good. The field specifications, in particular, are generally good.
We could improve the style and structure of the descriptions a bit, and most of the messages would benefit from more detail and more information to explain what the message is for, how it is used, etc...
I've made suggestions in a subset of items as examples. These are not exhaustive, and are intended to serve as guidance for improvement.
Entirely at your discretion, of course.
If you are uncertain what I am suggesting, or I can be of assistance in adding more rigor and detail, I am happy to discuss in chat or in a huddle.
| */ | ||
| message HintsAggregationVoteTransactionBody { | ||
| /** | ||
| * The id of the hinTS construction this vote is for.<br/> |
There was a problem hiding this comment.
We're following some older habits here. Not incorrect so much as a bit inconsistent with the broader base of proto files.
| * The id of the hinTS construction this vote is for.<br/> | |
| * A hinTS construction identifier.<br/> | |
| * This is the identifier of the hinTS construction that this vote transaction | |
| * seeks to approve. |
| * construction. | ||
| * <p> | ||
| * This value MUST be set.<br/> | ||
| * This value MUST be greater than zero.<br/> |
There was a problem hiding this comment.
The line break is not needed on the last line
| * This value MUST be greater than zero.<br/> | |
| * This value MUST be greater than zero. |
| * The vote the node is casting for the aggregation of the hints | ||
| * in the given construction (which should be ongoing). | ||
| * <p> | ||
| * This value MUST be set.<br/> |
There was a problem hiding this comment.
| * The vote the node is casting for the aggregation of the hints | |
| * in the given construction (which should be ongoing). | |
| * <p> | |
| * This value MUST be set.<br/> | |
| * A pre-processed keys vote.<br/> | |
| * This is a vote cast by a particular node supporting the aggregation of | |
| * the hints in a given construction (which should be ongoing). | |
| * <p> | |
| * This value is REQUIRED. |
| * The base-2 logarithm "k" of the maximum universe size for this | ||
| * publication. |
There was a problem hiding this comment.
Another example
| * The base-2 logarithm "k" of the maximum universe size for this | |
| * publication. | |
| * A base-2 logarithm "universe" size.<br/> | |
| * This is the base-2 logarithm "k" of the maximum universe size for this | |
| * hinTS key publication. |
| * A transaction body to publish a node's hinTS partial signature on | ||
| * a particular message for a certain construction id. |
There was a problem hiding this comment.
We're trying to say everything in a single sentence here, when we should probably be presenting more detail with less complex language.
Note the suggestion for more detail, which should be replaced with additional information that a third-party wishing to validate or reconstruct the process from these transactions might find useful.
| * A transaction body to publish a node's hinTS partial signature on | |
| * a particular message for a certain construction id. | |
| * A transaction body to publish a single node's hinTS partial signature.<br/> | |
| * This presents a partial signature on a particular message. The signature is | |
| * valid for a particular hinTS construction id. | |
| * << Any additional detail for this transaction body >> | |
| * | |
| * This message MUST specify a construction identifier.<br/> | |
| * This message MUST specify the message that is signed.<br/> | |
| * This message MUST include a signature value.<br/> | |
| * The current hinTS process SHALL support BLS partial signatures | |
| * and Alt-BN-128 keys |
| * This value MUST NOT be empty. | ||
| */ | ||
| repeated PartyAssignment party_assignments = 7; | ||
| } |
|
Thanks for the review @edward-swirldslabs @jsync-swirlds . |
Fixes #17194
Sub-PR of #17215
Adds protobufs for exact-weight TSS