[Bug/UX] Clicking "Start All" in Masternodes tab kicks *all* MNs to PRE_ENABLED state

[kyeno]

Not sure if this is a bug per-se, or just UX issue but nevertheless could be improved.

In a scenario when you run more than one masternode managed from the same controller wallet, and - let's say - one of the masternode dies on the server end, when you bring it back up and click "Start All" button it kicks literally all of your masternodes (including ENABLED ones) back to PRE_ENABLED state for some time. Soon after the previously ENABLED one gets back to ENABLED state.

Possibly a good solution would be skipping the ENABLED ones in the iteration loop, or redoing the buttons so "Start All" would just start inactive ones by default, and a small checkbox would change the function of the button to actually send the start command to all of them (including enabled).

[furszy]

In a scenario when you run more than one masternode managed from the same controller wallet, and - let's say - one of the masternode dies on the server end, when you bring it back up and click "Start All" button it kicks literally all of your masternodes (including ENABLED ones) back to PRE_ENABLED state for some time. Soon after the previously ENABLED one gets back to ENABLED state.

Hmm ok, shouldn't be trying to start masternodes that are already enabled or with the collateral spent in the mainchain. Will check it.

---

small checkbox would change the function of the button to actually send the start command to all of them (including enabled).

No need for a checkbox to force a start of already enabled/accepted masternodes (at least not in the current MN model, with the DMN introduction we will get different ways of updating a MN. Revoking keys access, updating the service address, changing the payout script etc). In the current model, there is no reason for an user to try to re-broadcast an already enabled/accepted MN if there is no service data being changed (the only reason could be if the backend is working badly. Not managing properly the mnb broadcast, mnb reception or MN heartbeat flow, but that is another sort of problem, not GUI, that we shouldn't be having after the recent improvements in the area -- check the v6.0 MN labeled PRs, there are some good fixes there --).

In other words, the user pressing the MN start button is just the initial flow trigger. After that, the backend has to manage the entire process, validating the data, the MN status and preventing the node from spamming the network with already accepted/known mnb messages. It isn't a direct message broadcast to the network as other peers could/will end up banning the user's node for spam if the user presses the start button repetitively.

[Liquid369]

So looking at everything now, those are being checked, but with extra conditionals. https://github.com/PIVX-Project/PIVX/blob/master/src/qt/pivx/masternodeswidget.cpp#L278 Heres the function;

if (!startMN(mne, strError)) {            
    amountOfMnFailed++;        
} else {            
    amountOfMnStarted++;       
}

I believe suitable to add the conditional check here before executing the startMN() function.

Making it like so

if (mnModel->isMNInactive(mnAlias))
    if (!startMN(mne, strError)) {            
        amountOfMnFailed++;        
    } else {            
        amountOfMnStarted++;       
    }

Because the link I provided above at line 285 we do

if (onlyMissing && !mnModel->isMNInactive(mnAlias)) 

Which in this instance I am unaware of passing the onlyMissing param havent dug enough, but this is also not escaping the loop when the Masternode is found as active/enabled. We either do a check for that and then return, rather than continue or the idea above to just add a conditional before the startMN() function call to avoid it if the node is indeed active.

Suitable?

[furszy]

no, IsMNInactive() only returns true if the MN expired or is in remove status (close to be kicked from the list). So, in that piece of code we would just block every new masternode start.

This needs something like:

// Only re-broadcast enabled MNs who had a service/data change.
if (mnModel->IsMnEnabled(mnAlias) && !mnModel->hasServiceDataChange(mneConf)) {
     continue;
}
// continue the flow, broadcast the MN..

[Liquid369]

Ah alright, I need to look more at these functions. I figured it was the isActive, but I see the IsMnEnabled.