Skip to content

Conversation

@furszy
Copy link

@furszy furszy commented Aug 25, 2021

Fix impossibility to start Masternodes using the GUI.

This was introduced by beb1cf6 (curiously enough, no one noticed it up until now..), the walletModel is provided to the widget in loadWalletModel, so it can't be used in the MasternodesWidget constructor to construct the mnmodel object.

@furszy furszy added the GUI label Aug 25, 2021
@furszy furszy added this to the 5.3.0 milestone Aug 25, 2021
@furszy furszy self-assigned this Aug 25, 2021
@furszy furszy added the Bug label Aug 25, 2021
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 85b926e

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch. We need to backport this to the 5.3 branch. utACK 85b926e and merging...

@random-zebra random-zebra added the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Aug 25, 2021
@random-zebra
Copy link

(curiously enough, no one noticed it up until now..)

I believe that's because masternodes can still be individually started with the GUI.
What is actually broken is the "Start All" functionality.
This is because, if walletModel is null inside MNModel (as it is with this bug), then collateralTxAccepted is not populated during MNModel::updateMNList(), and, therefore, MasterNodesWidget::startAll fails the isMNCollateralMature check.
This check is missing in MasterNodesWidget::startAlias (we should probably add it there too), so this bug has been undetected for so long.

@ambassador000
Copy link

I believe that's because masternodes can still be individually started with the GUI.

Correct. Can confirm that I successfully started a masternode through GUI individually using the v5.3.0rc1.

@random-zebra random-zebra merged commit 8ac28b8 into PIVX-Project:master Aug 25, 2021
@furszy
Copy link
Author

furszy commented Aug 25, 2021

This check is missing in MasterNodesWidget::startAlias (we should probably add it there too), so this bug has been undetected for so long.

Hmm, not really. We are doing the same check there, you actually just found another bug :).

startAlias() is called from onEditMNClicked() which accesses to the MN model row through the usage of the model index, WAS_COLLATERAL_ACCEPTED column, which.. uses the walletModel and the collateral accepted map (same as startAll), but.. this functions returns true if the collateral is not found instead of returning false :).

So.. the single MN start is working because of another bug actually..

@random-zebra
Copy link

Yeah...

if (!collateralTxAccepted.value(txHash) && walletModel) {
    return walletModel->getWalletTxDepth(rec->vin.prevout.hash) > 0;
}

furszy added a commit that referenced this pull request Aug 26, 2021
0e1839e [Refactor] check scriptsig size in CheckBlockSignature (random-zebra)
c3ec5e9 [BUG] Correct CCSV opcode handling in script evaluation (random-zebra)
fb7e513 [GUI] MasternodesWidget Fix: missing walletModel in mnmodel constructor. (furszy)
cbb77f8 Add mainnet DNS seeder from furszy. (furszy)

Pull request description:

  Back porting the last PRs for the coming v5.3.0 production release:

  List:
  * #2524.
  * #2529.
  * #2532.

ACKs for top commit:
  Fuzzbawls:
    utACK 0e1839e
  random-zebra:
    utACK 0e1839e

Tree-SHA512: 2dd07b0cc79b5a2f2791393ad37ed369bc72ac3450d168befc812bb8cf1336f3b59ef4d3cce6f346eeb1b2d826368c2cae3ccc04ee82b807958e6479ca6ae95b
@random-zebra random-zebra removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Aug 27, 2021
Fuzzbawls added a commit that referenced this pull request Sep 2, 2021
… MNModel::data

f0fd8ac Refactor: remove duplicated coll. confirmation check in MNModel::data (random-zebra)

Pull request description:

  Follow up to the discussion in #2529.
  If walletModel is null, then `MNModel::data` returns true at `WAS_COLLATERAL_ACCEPTED`.
  Fix the bug removing the duplicated check (`getWalletTxDepth > MasternodeCollateralMinConf()` is already checked every 30 seconds in `MNModel::updateMNList`).

ACKs for top commit:
  furszy:
    utACK f0fd8ac
  Fuzzbawls:
    utACK f0fd8ac

Tree-SHA512: 63065b4ae40403107d738d25ebf6616e99df4373b5fbe4d6b924fecddeef4189f455a91e1cbb48e8c85458481ab519420b49210d6e9f5284c6f35c6e53b35d4e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants