Skip to content
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

Guarding wallet access from init and print error for unknown MN collaterals #2218

Merged
merged 2 commits into from
Feb 28, 2021

Conversation

furszy
Copy link

@furszy furszy commented Feb 26, 2021

Two changes:

  1. Guarding pwalletMain access from init.cpp, so in the future we could enable pivxd to run without the wallet.
  2. BugFix: Wallet::LockCoin verifying that the MN collateral utxo is in the wallet's map before add it to the locked set. Printing the correct error if the transaction is unknown for the wallet.

@furszy furszy self-assigned this Feb 26, 2021
Fuzzbawls
Fuzzbawls previously approved these changes Feb 27, 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 fbe9200

@Fuzzbawls
Copy link
Collaborator

needs rebase now

@Fuzzbawls Fuzzbawls added Startup Wallet startup changes and/or improvements Wallet labels Feb 27, 2021
@Fuzzbawls Fuzzbawls added this to the 5.1.0 milestone Feb 27, 2021
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.

The second commit introduces a subtle risk.
With this change, if a controller's wallet is zapped, and then reindexes/resyncs, then the masternode collaterals are not locked during init.
Thus they could be inadvertently spent, when the reindex completes.

A similar (but less likely) situation can happen in this case:

  1. Create a new controller from scratch. Before it completes the sync, get a receiving address.
  2. From another client, send the collateral to the receiving address created in (1), and get the txid
  3. Add the txid to the masternode.conf of the controller (which still hasn't completed the sync, nor added the tx to his wallet) and restart it.
  4. When the sync completes, the masternode owner could think that the collateral is locked, but it isn't.

@furszy
Copy link
Author

furszy commented Feb 27, 2021

good points.
Will modify the second commit to not block the output append, have thought on some ways to cover the presented scenarios but it simply doesn't worthy it as it would add an extra overhead during sync/rescan processes for such a small benefit.

@random-zebra
Copy link

Agreed.
Further, DMNs will remove the masternode.conf file and locking will be automatic, so there is no need to overthink this.

@furszy
Copy link
Author

furszy commented Feb 27, 2021

pushed + rebased on master.

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.

utACK de6f052

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.

re-ACK de6f052

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Startup Wallet startup changes and/or improvements Wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants