Round restart #288

kellpossible · 2021-06-20T19:18:51Z

Currently a work in progress:

TODO:

Handle situation where there are no replacement contributors and there are some remaining contributors after the drop of a contributor.
Update integration test to support starting contributors while a round is in progress, so the situation where there are no contributors left in the current round after a drop can be tested. Integration testing for round restart and multiple rounds ProvableHQ/aleo-setup-integration-test#4
Handle situation where there are no contributors left.
Round restart function #286 Manual round restart (which should possibly remove everyone and take people from the queue again? Or have an option for doing so).
~~Check what happens when a verifier drops (do the remaining verifiers pick up the dropped verifier's tasks successfully?).~~ We can do this in Handle accepted task handoff for verifiers #305
~~Handle situation where there are no verifiers left in the current round. Or at least document this in an issue.~~ We can do this in Handle accepted task handoff for verifiers #305
~~Handle situation where there are no replacement contributors left~~ (or at least document it for future implementation of the replacement contributor logic). Test the situation where contributor is dropped, and all replacement contributors are already occupied #309

A number of refactors in this PR are related to #267

- also make verifier more robust, upon failures it now resets its tasks

…n coordinator_state module

…y weren't being used consistently and were only used for testing, shouldn't be part of the public API. Removed ban DropParticipantData field

…ound

…ew participants

…test, more docs

apruden2008 · 2021-07-13T15:56:10Z

@kellpossible are we separating the unfinished items into new issues?

…round during drop

kellpossible · 2021-07-13T18:15:26Z

@apruden2008 I think so yes, I'll do that today, also I will properly report the problems that I encountered while testing the manual round restart in new issues

apruden2008 · 2021-07-15T15:24:39Z

phase1-coordinator/src/storage/storage.rs

+/// An enum containing a [Locator] or [LocatorPath].
+///
+/// **Note:** This can probably be refactored out in the future so
+/// that we only use [Locator].


I will open an issue for this

jules

LGTM!

…RUSTSEC-2021-0072

kellpossible · 2021-07-17T10:28:22Z

Pending a second review, and #308 hopefully this PR is good to go

ibaryshnikov · 2021-07-17T12:25:43Z

phase1-coordinator/src/coordinator_state.rs

+            .map(|(bucket_index, (participant, mut participant_info))| {
+                let bucket_id = bucket_index as u64;
+                let tasks = initialize_tasks(bucket_id, number_of_chunks, number_of_contributors as u64)?;
+                participant_info.restart_tasks(tasks, time)?;


It looks like one of the key parts of the change, is it? To reset the state of all contributors who is still in the round to the initial state similar to what we have on round start.

yep that's right

ibaryshnikov · 2021-07-17T16:54:41Z

phase1-coordinator/src/coordinator_state.rs

-                }));
+                // Restart the round because there are no verifiers
+                // left for this round.
+                let reset_round = self.current_verifiers.is_empty() && self.finished_verifiers.is_empty();


is it enough to have self.current_verifiers.is_empty()? Can you tell us more about the finished verifiers check here?

The verifiers in the round get moved from the current_verifiers list to the finished_verifiers list when all their tasks are complete. If only checking current_verifiers and all the verifiers are considered complete for whatever reason, then the round will still be reset.

at least that's how I was understanding it

Is this case possible: one verifier finished, another one dropped?. This check will be produce false therefore.

Thanks for raising questions about this line, I think it deserves more consideration.

I referenced it in #305

Yes it will produce false. Under the assumption that the remaining verifiers would be able to pick up the slack, but perhaps this isn't working yet and will be fixed in #305

ibaryshnikov · 2021-07-17T17:02:04Z

phase1-coordinator/src/objects/round.rs

@@ -783,13 +794,38 @@ impl Round {
        }
    }

+    /// Remove a contributor from the round.
+    pub(crate) fn remove_contributor_unsafe(


I hope we will remove "unsafe" and "try" from the function names one day. In case of remove_something_unsafe it's misleading because it's a safe function. And try_do_something() -> Result<...> doesn't need to have "try_" because it returns Result and it's clear that it can fail. And here the name is justified by the similarity to other remove_*_unsafe functions, which is ok for now.

I agree, I was just following convention in this case. I'm not sure how we would express the intended meaning. The function may not fail with a result, but my guess is the original authors were trying to say that calling this function might be dangerous in the wrong contexts (without being "unsafe" according to the strict Rust definition).

Edit: updated after re-reading your comment

work in progress reset round

c644d99

This was linked to issues Jun 20, 2021

Round doesn't restart when all contributors are dropped and there are no available replacement contributors #283

Closed

Round restart function #286

Closed

kellpossible added enhancement New feature or request bug Something isn't working labels Jun 20, 2021

kellpossible added 8 commits June 22, 2021 10:07

Merge branch 'storage-locator-path-refactor' into round-restart

a763b28

more work in progress round reset implementation

06e6747

CoordinatorState reset round implemented with unit test passing

b083183

work in progress round restart changes

be53664

- also make verifier more robust, upon failures it now resets its tasks

remove the ban variant fo the DropParticipant enum, small refactors i…

2ad879e

…n coordinator_state module

move the reset round responsibility into the drop_participant logic

bb360af

remove return locators from the drop from storage methods because the…

baf80cd

…y weren't being used consistently and were only used for testing, shouldn't be part of the public API. Removed ban DropParticipantData field

Merge branch 'master' into round-restart

b5ccbea

kellpossible mentioned this pull request Jul 4, 2021

Fix 257 failing drop participant integration tests #277

Closed

2 tasks

refactor the drop data to be more structured, remove drop data from r…

db3bacd

…ound

kellpossible changed the base branch from storage-locator-path-refactor to master July 4, 2021 10:17

kellpossible added 4 commits July 4, 2021 16:33

change to ResetCurrentRound action

70be9d1

reset round now returns an error if there is no current round

df35eaf

documentation for reset_current_round

b13fd7a

work in progress round reset rollback

8919b54

kellpossible requested review from apruden2008, emmorais and ibaryshnikov July 5, 2021 03:50

kellpossible mentioned this pull request Jul 5, 2021

Integration testing for round restart and multiple rounds ProvableHQ/aleo-setup-integration-test#4

Merged

kellpossible added 3 commits July 5, 2021 12:57

work in progress implementation of round reset/rollback to wait for n…

acab4e7

…ew participants

fix rolling back to previous round during round reset

debab47

fix small bug with going into rollback when it shouldn't, extra unit …

8459ef7

…test, more docs

kellpossible marked this pull request as ready for review July 12, 2021 12:33

kellpossible requested a review from jules July 12, 2021 12:34

changes for admin/manual round restart

2cafd2f

kellpossible mentioned this pull request Jul 12, 2021

Participant task timeout #252

Closed

fix failing unit tests, contributor id wasn't being removed from the …

5338f49

…round during drop

kellpossible mentioned this pull request Jul 13, 2021

Refactor round storage methods to return actions #303

Open

kellpossible mentioned this pull request Jul 13, 2021

Manual round restart fails, verifier uploads response during the round reset #304

Closed

apruden2008 reviewed Jul 15, 2021

View reviewed changes

apruden2008 mentioned this pull request Jul 15, 2021

Refactor storage access logic to use only [Locator] instead of either [Locator] or [LocatorPath] #306

Open

jules approved these changes Jul 15, 2021

View reviewed changes

update tokio to fix audit warning for https://rustsec.org/advisories/…

8e625e8

…RUSTSEC-2021-0072

Merge branch 'update-tokio-fix-audit-warning' into round-restart

0b9388e

ibaryshnikov reviewed Jul 17, 2021

View reviewed changes

ibaryshnikov approved these changes Jul 17, 2021

View reviewed changes

ibaryshnikov merged commit ec14631 into master Jul 17, 2021

kellpossible deleted the round-restart branch July 17, 2021 17:15

kellpossible mentioned this pull request Jul 17, 2021

Handle accepted task handoff for verifiers #305

Closed

kellpossible mentioned this pull request Sep 1, 2021

Refactor/303 round return storage actions #367

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round restart #288

Round restart #288

kellpossible commented Jun 20, 2021 •

edited

Loading

apruden2008 commented Jul 13, 2021

kellpossible commented Jul 13, 2021

apruden2008 Jul 15, 2021

jules left a comment

kellpossible commented Jul 17, 2021 •

edited

Loading

ibaryshnikov Jul 17, 2021

kellpossible Jul 17, 2021

ibaryshnikov Jul 17, 2021

kellpossible Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021

ibaryshnikov Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021

kellpossible Jul 17, 2021

ibaryshnikov Jul 17, 2021

kellpossible Jul 17, 2021 •

edited

Loading

Round restart #288

Round restart #288

Conversation

kellpossible commented Jun 20, 2021 • edited Loading

apruden2008 commented Jul 13, 2021

kellpossible commented Jul 13, 2021

Choose a reason for hiding this comment

jules left a comment

Choose a reason for hiding this comment

kellpossible commented Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellpossible Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibaryshnikov Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

kellpossible Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellpossible Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

kellpossible commented Jun 20, 2021 •

edited

Loading

kellpossible commented Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021 •

edited

Loading

ibaryshnikov Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021 •

edited

Loading

kellpossible Jul 17, 2021 •

edited

Loading