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
EFM Recovery Integration Tests: Part 1 #6156
EFM Recovery Integration Tests: Part 1 #6156
Changes from all commits
d9a99c9
ebe557a
0d5dcf5
a842760
c3a05be
0d0378c
f0670d0
ee7fa73
09f29e7
96b9776
d48d438
d75354b
b93e8ac
ec964a8
d0c40f2
d3d7af6
4be8e65
62028cd
7a3b6b9
4ba9d6a
1a02397
7c6d36b
d5da087
d70de6e
b1c23ed
53b2bbf
04880e7
d052730
9cff7d5
5943125
fc2355d
c549f36
9e062d1
582ca24
777267f
0d41389
b64a5ca
a46ae41
78fb541
3b6d707
4db4afc
d2fb6bc
0d225b7
216ca7e
a650821
e9532ef
a0df4a2
c331ab2
01bfe0d
4a2a888
a982d13
3aaf62f
3b5c953
a614c62
f1db034
00e2f40
df64765
5a4cd70
1eb81e8
c60abe3
b51a73f
2ae7bd7
5b92090
34a49bf
772133f
d1fab36
1d2e034
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
appreciate the clear documentation:
flow-go/cmd/bootstrap/run/epochs.go
Lines 91 to 92 in 3b6d707
currentEpochIdentities
is listed incurrentEpochDKG
. That is one part of the requirement 👍currentEpochDKG
that is not incurrentEpochIdentities
. I think your current code would not recognize this and only list the nodes that are incurrentEpochIdentities
. Then, the set of consensus nodes for the recovery epoch would be smaller than the set of nodes incurrentEpochDKG
, so we have an inconsistent configuration.Implications:
For recovery epochs, we might need to be able to include consensus nodes that are ejected. I am not entirely sure -- we should Talk to Tarak about this edge case.
If it is indeed a problem, I would be inclined account for this in the
EpochRecover
struct, by including anIdentifierList
where we list the nodeIDs for all nodes that are ejected.I would like to have a test case specifically covering this case for the different node roles. I think implementing this test would be beyond the scope of this PR. So if you could create an issue that would be great.
In absence of a proper solution so far, could you
currentEpochIdentities
. After running through thisfor
loop. This counter must equalcurrentEpochDKG.Size()
. Otherwise, the consensus committee is inconsistent with the DKG and we should not proceed.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.
This is my understanding -- we do need to include consensus nodes that are ejected. The committee and indices need to be exactly the same.
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.
that is also my understanding. Though I still have the vague hope that this might not be absolutely necessary. Trying to set up a conversation with Tarak...
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.
Copying some context here:
Slack thread