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
Adding notion of a recovery owner for network recovery #6705
base: main
Are you sure you want to change the base?
Adding notion of a recovery owner for network recovery #6705
Changes from 19 commits
07b152f
7cd6011
580670c
4df08bd
491aed2
41e16fd
90cbdfb
1c1bda9
2f7ff7f
7d0fe02
607b8a4
986138a
0ae745c
33b736b
96838b7
7104982
7639bf9
8bd2d95
e5021f1
0dcc258
e0bc7a7
b28b452
8dc3f06
01df49a
7546e41
754adc3
123387b
1228c1d
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.
I believe we support opening deliberately unrecoverable services, and although I am not aware of current use cases, they have come up as potential use cases in the past, so I think we want to leave that open as a possibility.
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.
@achamayou not sure which lines you wanted me to change here. I added (a) a check to ensure that if recovery_owner has a value then enc_pub_key must also have a value else throw and (b) count the member_with_pubk_count while skipping the recovery_owner members.
(a) is like a configuration issue while (b) is only ensuring the correctness of the existing check that the count of recovery members and supplied or calculated default recovery threshold values are sane else the logic already throws below.
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 service will check that at least one..."
^ I don't believe this is true now, and I don't want it to become true, because it precludes creating un-recoverable systems, which we think may be desirable in some cases.
This is something that an operator can quite trivially preclude by modifying the
transition_service_to_open()
transition if they wish to do so, there is no reason to hardcode it outside the constitution.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.
@achamayou "The service will check that at least one..." this check that there must be atleast 1 recovery member (aka participant) and recovery threshold cannot exceed that number existed before this PR and continues to work today. So as of now you cannot open a service that has 0 recovery participants. I preserved the check in main.cpp using members_with_pubk_count and there are checks in internal table access::set_recovery_threshold, remove_member and open_service that continue to work as before. Having owners has not changed the checks around recovery threshold and recovery members (participants).