Fixes initialisation process on the web frontend #166

nkcr · 2022-09-13T09:24:34Z

coveralls · 2022-09-13T09:27:32Z

Pull Request Test Coverage Report for Build 3067527931

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-5.3%) to 58.5%

Totals
Change from base Build 3053251873:	-5.3%
Covered Lines:	2364
Relevant Lines:	4041

💛 - Coveralls

cmsigrist · 2022-09-13T12:17:44Z

web/frontend/src/pages/election/components/utils/useChangeAction.tsx

-      const promises = Array.from(nodeProxyAddresses.values()).map((proxy) => {
-        if (proxy !== '') {
+      const promises = Array.from(nodeProxyAddresses).map(([node, proxy]) => {
+        if (proxy !== '' && DKGStatuses.get(node) === NodeStatus.NotInitialized) {


I don't think this is enough to prevent the initialization of nodes that were already initialized, because of this cough beautiful Promise.all (I'm sorry .....) line 221, which either set them all up to Initialized or none...

I think one way to do this would be to use the component DKGStatusRow, so that each node initializes itself and polls it own status. useChangeAction could then be notified of the changes via the states in DKGStatuses ?

Luckily that works in this case, because "undefined" (and any other non-promise values) count as "resolved". 🙃

I think one way to do this would be to use the component DKGStatusRow, so that each node initializes itself and polls it own status. useChangeAction could then be notified of the changes via the states in DKGStatuses ?

Yes, that would be much better indeed.

Yes but it will return undefined only if the proxy address of the node could not be found (i.e proxy===''). Otherwise in pollDKG:

// Add a timeout if (attempts === maxAttempts) { throw new Error('Timeout'); } ... catch (e) { return reject(e); }

So if during the poll one of the node times out for some reason the Promise will be rejected thus the Promise.all will resolve to rejected for all of them. And the request to initialize will again be sent for all the nodes ...?

oh yes, that's true :/

I can try to solve this this coming week since it will be the start of term, but after that I probably won't have the time anymore 😅

I moved the DKG logic to DKGStatusRow, as suggested. That wasn't straightforward as there are probably still too much states spread around between useChangeAction.tsx, Show.tsx, and DKGStatusRow.tsx. Having your input on that would help, if you are willing to :)

It's already so much better ! But yes there are way too many states to be passed around, it's a bit cumbersome ....

jbsv

Thanks for sharing. However, there is nothing I can add to help here.
🍶 time...

nkcr · 2022-09-15T11:29:50Z

The setup process is quite shiny now:

Screen.Recording.2022-09-15.at.13.28.29.mov

cmsigrist

This is quite a rework ! I love the new states when doing the setup 🚀
But there are still some bugs, and I haven't tested all the cases yet 😅

web/frontend/src/components/utils/DKGStatus.tsx

web/frontend/src/pages/election/components/utils/useChangeAction.tsx

cmsigrist · 2022-09-15T11:53:09Z

web/frontend/src/pages/election/components/DKGStatusRow.tsx

+  // legitimate status.
+  useEffect(() => {
+    if (
+      ongoingAction === OngoingAction.Initializing &&


If a node is unreachable (either because it could not retrieve its proxy address or because the call to getDKGActor fails), the first intilization works. But then the Status of the election is stuck in Initial, and when clicking again on Initialize node, the process is stuck in this ongoing action (I was looking into how to fix this but haven't found a solution yet)

Well it's actually the same problem as before, the parents are not notified since there are no changes the second time we click on Initializing (neither the Status nor the DKGLoading states change...)

This should be solved in Show.tsx and inside the notifyDKGState function. There we can count the number of Initialized nodes (even if a node is already initialized, it will call this function at least once with its Initialized status), and update the status+ongoingAction once all nodes are initialized. What do you think ?

I'm not sure I understood exactly what you meant
There's already a useEffect that checks if all the node were initialized and transitions to the Status Initialized if so (in Show.tsx line 165). But it's not triggered because none of the entries in DKGStatuses change.
So yes if a node is already initialized, I think it should still call notifyDKGState so that it doesn't stay stuck in Initializing.

But I have no idea how it doesn't always remain stuck in Initializing because the setOngoingAction(ongoingAction.None) used to be handled in useChangeAction with the onFulfilled/onReject functions but is now missing for the case of the status Initialized... 😂

I fixed that with the DKGLoading. Even if the status doesn't change, the node will always update its DKGLoading and therefore notify the parent with notifyLoading. The parent at that time knows that something changed on its children, and can make the necessary checks.

@cmsigrist what about (added if (proxy === ''):

useEffect(() => { if ( ongoingAction === OngoingAction.Initializing && (status === NodeStatus.NotInitialized || status === NodeStatus.Failed || status === NodeStatus.Unreachable) ) { setDKGLoading(true); if (proxy === '') { setStatus(NodeStatus.Unreachable); setInfo('proxy empty'); setDKGLoading(false); return; } initializeNode() .then(() => { setInfo(''); setStatus(NodeStatus.Initialized); }) .catch((e: Error) => { setInfo(e.toString()); setStatus(NodeStatus.Failed); }) .finally(() => { setDKGLoading(false); }); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [ongoingAction]);

This works for the initialization (at least it passed all the failure cases I ran) ! 🎉

And then for the setup, I'm not sure if we're supposed to be able to retry setting up a node that has already failed the setup, but if this is the case, then it gets stuck in SettingUp.
It executes the onrejected part as expected (line 166):

pollDKGStatus(expectedStatus) .then( () => {}, (e: Error) => { setStatus(NodeStatus.Failed); setInfo(e.toString()); } ) .finally(() => setDKGLoading(false));

But it doesn't call notifyDKGState.

And then there's a minor bug where if we set up a first node and it fails but then set up another one which succeeds, the error message remains (the red little (!) next to the node status)

This works for the initialization (at least it passed all the failure cases I ran) ! 🎉

Yay 🥳

I'm not sure if we're supposed to be able to retry setting up a node that has already failed the setup, but if this is the case

Yes, we should.

But it doesn't call notifyDKGState.

It should, when doing setStatus(NodeStatus.Failed);, then the useEffect in line 68 calls notifyDKGState, at least if the state was updated. Was it what you meant ?

Yes it should, but I don't think it actually does, otherwise it wouldn't be stuck in Setting up ?

True. I simplified some elements on Show.ts and it seems to work now ... 🥵

web/frontend/src/pages/election/components/DKGStatusRow.tsx

web/frontend/src/pages/election/Show.tsx

pierluca

I'm too far from this code base to be of much help, but I had a quick look and it seems a lot of state is just passed around between components. As per our (offline) discussion @nkcr, I'm pretty sure introducing something like Redux or Akita could provide significant benefits.

I say this because then it would allow big chunks of the logic to be moved into dedicated services, dealing only with the network and the state, whereas the components would focus on showing what's in the state and triggering the services.
If the state is external, it can just be queried as needed.

pierluca · 2022-09-16T12:25:10Z

services/dkg/pedersen/handler.go

@@ -255,19 +260,23 @@ func (h *Handler) start(start types.Start, deals, resps *list.List, from mino.Ad
 // doDKG calls the subsequent DKG steps
 func (h *Handler) doDKG(deals, resps *list.List, out mino.Sender, from mino.Address) {
 	h.log.Info().Str("action", "deal").Msg("new state")
+	*h.status = dkg.Status{Status: dkg.Dealing}


If I understand correctly, this is unsafe, because while we only start Stream once, we could still receive multiple types.Start message. If so, this will be called multiple times.
I believe we have the same issue in dela.

Concretely, the impact on this line is minimal, but I'm concerned about the multiple starts in parallel 🤔 am I missing something ? do we have a test that verifies that we'll ignore the second start (on the same stream) if one is already running ?

That can technically happen if other nodes participating in the setup decide to act badly and send start messages.

pierluca · 2022-09-16T12:38:14Z

web/frontend/src/pages/election/Show.tsx

      if (!Array.from(nodeLoading.values()).includes(true)) {
        setDKGLoading(false);
+      } else {
+        setDKGLoading(true);


Suggested change

if (!Array.from(nodeLoading.values()).includes(true)) {

setDKGLoading(false);

} else {

setDKGLoading(true);

const someNodeLoading = Array.from(nodeLoading.values()).includes(true);

setDKGLoading(someNodeLoading);

pierluca · 2022-09-16T12:43:40Z

web/frontend/src/pages/election/Show.tsx

  useEffect(() => {
    if (nodeLoading !== null) {
      if (!Array.from(nodeLoading.values()).includes(true)) {
        setDKGLoading(false);
+      } else {
+        setDKGLoading(true);
      }
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps


in general exhaustive deps are a very good thing, shouldn't these issues be addressed ?
ignoring them has cost me a lot of time bug fixing, in the past :-/

We actually tried a couple of times to include all the dependencies with @badrlarhdir during the semester. But for some reasons, the useEffects were not triggered at the correct/expected moment when doing so... 🤔

\+ other fixes

cmsigrist · 2022-09-20T14:02:05Z

web/frontend/src/pages/election/Show.tsx

@@ -220,8 +251,6 @@ const ElectionShow: FC = () => {
                  setOngoingAction={setOngoingAction}
                  nodeToSetup={nodeToSetup}
                  setNodeToSetup={setNodeToSetup}
-                  DKGStatuses={DKGStatuses}
-                  setDKGStatuses={setDKGStatuses}


(I can't suggest changes on lines that were not modified). But line 225 and 239, it would be great to add:
DKGLoading && ongoingAction === OngoingAction.None &&
And line 230 and 240:
(!DKGLoading || ongoingAction !== OngoingAction.None) &&
So that the status bar still remain visible, same with the action during the initialization and setup (it currently disappears because of the change with the DKGLoading) ?

nkcr · 2022-09-21T12:21:09Z

I'm too far from this code base to be of much help, but I had a quick look and it seems a lot of state is just passed around between components. As per our (offline) discussion @nkcr, I'm pretty sure introducing something like Redux or Akita could provide significant benefits.

I say this because then it would allow big chunks of the logic to be moved into dedicated services, dealing only with the network and the state, whereas the components would focus on showing what's in the state and triggering the services. If the state is external, it can just be queried as needed.

Instead of pulling out the big boys, an interesting incremental step could be to use the native React context API to wrap all those properties.

cmsigrist

Woohoo it works ! 🎉

cmsigrist · 2022-09-21T15:24:33Z

web/frontend/src/pages/election/Show.tsx

@@ -206,7 +235,7 @@ const ElectionShow: FC = () => {
            <div className="font-bold uppercase text-lg text-gray-700 pb-2">{t('action')}</div>
            <div className="px-2">
              {DKGLoading && <LoadingButton>{t('actionLoading')}</LoadingButton>}{' '}


You forgot about this one 😬
DKGLoading && ongoingAction === OngoingAction.None &&

sonarqubecloud · 2022-09-21T15:44:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

nkcr · 2022-09-21T16:05:59Z

Woohoo it works ! 🎉

Yay 🥳
Thank you very much for the review, I owe you one 🍺

cmsigrist · 2022-09-21T16:09:02Z

Hahaha no problem (we were kind of fixing the mess I left anyway 😂) !

Fixes initialisation process on the web frontend

48a71a4

Fix #165

nkcr added dkg web frontend labels Sep 13, 2022

nkcr requested review from cmsigrist and jbsv September 13, 2022 09:54

cmsigrist reviewed Sep 13, 2022

View reviewed changes

jbsv reviewed Sep 13, 2022

View reviewed changes

Moves the DKG logic to DKGStatusRow

cda589a

nkcr force-pushed the initialisation-fix-web branch from 7ef4cd2 to cda589a Compare September 14, 2022 15:44

nkcr added 2 commits September 15, 2022 12:02

Adds new DKG states and continues the refactoring

958f702

Updates the mock

e7b7d1e

nkcr requested a review from cmsigrist September 15, 2022 12:30

cmsigrist reviewed Sep 15, 2022

View reviewed changes

web/frontend/src/pages/election/Show.tsx Show resolved Hide resolved

nkcr added 3 commits September 16, 2022 08:59

Merge branch 'main' into initialisation-fix-web

80a7b52

Fixes some edge cases

930019f

Adds missing translation

6c7f680

nkcr force-pushed the initialisation-fix-web branch from d008f58 to 6c7f680 Compare September 16, 2022 11:33

pierluca reviewed Sep 16, 2022

View reviewed changes

nkcr added 2 commits September 20, 2022 12:25

Fixes the case where a node is empty

3e3105d

\+ other fixes

Adds build info on the footer

e40e1ea

cmsigrist reviewed Sep 20, 2022

View reviewed changes

Fixes the setup state

5e97a5f

cmsigrist reviewed Sep 21, 2022

View reviewed changes

Adds missing condition while loading

3a6ab20

cmsigrist approved these changes Sep 21, 2022

View reviewed changes

nkcr merged commit 412809d into main Sep 22, 2022

nkcr deleted the initialisation-fix-web branch September 22, 2022 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes initialisation process on the web frontend #166

Fixes initialisation process on the web frontend #166

nkcr commented Sep 13, 2022

coveralls commented Sep 13, 2022 •

edited

Loading

cmsigrist Sep 13, 2022

nkcr Sep 13, 2022

cmsigrist Sep 13, 2022

nkcr Sep 14, 2022

cmsigrist Sep 14, 2022

nkcr Sep 14, 2022

cmsigrist Sep 15, 2022

jbsv left a comment

nkcr commented Sep 15, 2022

cmsigrist left a comment

cmsigrist Sep 15, 2022

cmsigrist Sep 15, 2022

nkcr Sep 15, 2022

cmsigrist Sep 15, 2022

nkcr Sep 16, 2022

nkcr Sep 20, 2022 •

edited

Loading

cmsigrist Sep 20, 2022 •

edited

Loading

nkcr Sep 20, 2022

cmsigrist Sep 20, 2022

nkcr Sep 21, 2022

pierluca left a comment •

edited

Loading

pierluca Sep 16, 2022

nkcr Sep 16, 2022

pierluca Sep 16, 2022

pierluca Sep 16, 2022

cmsigrist Sep 16, 2022

cmsigrist Sep 20, 2022

nkcr commented Sep 21, 2022

cmsigrist left a comment

cmsigrist Sep 21, 2022

sonarqubecloud bot commented Sep 21, 2022

nkcr commented Sep 21, 2022

cmsigrist commented Sep 21, 2022

Fixes initialisation process on the web frontend #166

Fixes initialisation process on the web frontend #166

Conversation

nkcr commented Sep 13, 2022

coveralls commented Sep 13, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3067527931

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbsv left a comment

Choose a reason for hiding this comment

nkcr commented Sep 15, 2022

cmsigrist left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcr Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

cmsigrist Sep 20, 2022 • 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

pierluca left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcr commented Sep 21, 2022

cmsigrist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 21, 2022

nkcr commented Sep 21, 2022

cmsigrist commented Sep 21, 2022

coveralls commented Sep 13, 2022 •

edited

Loading

nkcr Sep 20, 2022 •

edited

Loading

cmsigrist Sep 20, 2022 •

edited

Loading

pierluca left a comment •

edited

Loading