Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Epoch Extensions in HotStuff Committee #6154
Support Epoch Extensions in HotStuff Committee #6154
Changes from 22 commits
426fa9a
59ed5fa
c605b49
0eff8a1
33e199a
f2f9192
0b1efe2
52dee4b
ef21d09
0c83d60
11c9fd6
0fe06d0
64619f3
4b26f72
80f569b
7dd487d
2ac1c0e
66c3b84
465d66f
d724014
c456b82
430a00c
05498d0
b400f38
80339f8
4fbd98b
d4d743a
3e1b46f
2b0d7e2
486c076
5d59791
4c47895
62c5d69
66f6e7f
cd6e188
43754d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
would suggest adding TODO to indicate that epochs will only be exposed once they are committed.
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.
See #6154 (comment)
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.
In my opinion, this should yield an error
ErrNextEpochNotCommitted
Though, this would require an update of the
EpochQuery
API ... so maybe add a todo to the issue is mentioned in my previous comment.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.
See #6154 (comment)
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.
In my opinion,
setupEpoch
simply cannot be a valid representation ofprotocol.Epoch
as it misses a bunch of the important information. We have the convention that an epoch essentially only starts to exist after the EpochCommit Service event has been finalized. I think we had the plan to remove any exposure of uncommitted epochs from the major APIs. Though,NewSetupEpoch
is still used and returned in some of the broadly-used interfaces.extensions
in thesetupEpoch
because it further increases the inconsistency of protocol definition and implementation. Instead, I would include the extensions intocommittedEpoch
and override theFinalView()
method inherited fromsetupEpoch
.setupEpoch
as a return value from theEpochQuery
API and only return epochs once they are committed. Is such issue already exists, maybe add a reference to this comment. If not, would you mind creating such issue please? 🙏 🙇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.
👍 Agree, will move the extensions to
committedEpoch
.Epoch APIs cannot entirely avoid exposing data for un-committed epochs, because the node software requires access to a subset of un-committed epoch data in order to commit that epoch, by completing the DKG and generating root cluster QCs.
There are other ways we can improve the epoch APIs, but we will need something similar to the progressive disclosure of
setupEpoch
andcommittedEpoch
. For example, we could hide some info from the EpochSetup event until the epoch is committed (like the view range), and we could use different interface types to represent the two states.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.
Added this tech debt issue as a place to capture these ideas for improving the API: #6191
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.
In my opinion, this method should be deprecated for the scope of this Pr, including a TODO to remove it entirely
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.
See #6154 (comment)