Skip to content

Conversation

@nmarley
Copy link

@nmarley nmarley commented Oct 24, 2019

Currently there is a bug in masternode status where the info (e.g. payout addr, vote addr) is not being updated when a ProUpRegTx is mined. This is currently only updated when the MN status is not "ready", if the MN is not in the valid list or if the operator key is changed.

This is an attempt to solve this by calling Init() in activemasternode if any of the MN's several fields are changed.

Another alternative would be to just call Init() regardless every block.

@UdjinM6 UdjinM6 added this to the 14.1 milestone Oct 24, 2019
@UdjinM6 UdjinM6 added the bug label Oct 24, 2019
@UdjinM6
Copy link

UdjinM6 commented Oct 24, 2019

Nice find! 👍 However, it doesn't feel right to go through the init process again when say voting address has changed because voting address has nothing to do with the state of a MN. The only (not yet covered) case when you'd want to do this would be IP address changes imo. On the other hand, detMNInfoChanged won't catch PoSe score changes and this info will stay outdated in masternode status output. I added a couple of patches (and a simple rpc test) on top of this one to address these issues, pls see https://github.com/UdjinM6/dash/commits/pr3176

@nmarley
Copy link
Author

nmarley commented Oct 24, 2019

👍 I like this better, originally tried doing something similar adding a comparison method to CDeterministicMNState but had compile issues so I abandoned it.

I have reset HEAD of this branch to your latest commit & force pushed.

@codablock
Copy link

IMHO the correct solution would be to remove mnListEntry completely and instead get the correct and up-to-date entry from the MN manager each time it's needed.

@codablock
Copy link

Here is an alternative implementation that removes the need to keep track of mnListEntry: 3e0ff6e
If you take it, I would also suggest to remove fba4687 again.

@UdjinM6
Copy link

UdjinM6 commented Oct 29, 2019

Agree, 3e0ff6e looks even better 👍

@nmarley
Copy link
Author

nmarley commented Oct 29, 2019

Thanks, I agree this is better, ff'd 3e0ff6e and reverted fba4687.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 9bc699f into dashpay:develop Oct 30, 2019
@nmarley nmarley deleted the activemn-update branch November 11, 2019 13:17
codablock pushed a commit to codablock/dash that referenced this pull request Nov 19, 2019
* Update activemn if protx info changed

* Add `==` and `!=` operators to CDeterministicMNState

* Only re-init active MN if its IP changed, changes to payout, voting etc. can be done without it

* Test `masternode status` updates

* Don't track mnListEntry anymore and instead get the DMN on demand

* Revert "Add `==` and `!=` operators to CDeterministicMNState"

This reverts commit fba4687.
MIPPL pushed a commit to biblepay/biblepay that referenced this pull request Nov 24, 2019
* commit '8b14adb206d76fbc6307385999a1c512052e93fa': (21 commits)
  [v0.14.0.x] Update release notes with change log (dashpay#3213)
  [v0.14.0.x] Bump version to 0.14.0.4 and draft release notes (dashpay#3203)
  Circumvent BIP69 sorting in fundrawtransaction.py test (dashpay#3100)
  Fix compile issues
  Merge bitcoin#11397: net: Improve and document SOCKS code
  Slightly optimize ApproximateBestSubset and its usage for PS txes (dashpay#3184)
  Update activemn if protx info changed (dashpay#3176)
  Actually update spent index on DisconnectBlock (dashpay#3167)
  Only track last seen time instead of first and last seen time (dashpay#3165)
  Merge bitcoin#17118: build: depends macOS: point --sysroot to SDK (dashpay#3161)
  Simulate BlockConnected/BlockDisconnected for PS caches
  Few fixes related to SelectCoinsGroupedByAddresses (dashpay#3144)
  Various fixes for mixing queues (dashpay#3138)
  Also consider txindex for transactions in AlreadyHave() (dashpay#3126)
  Ignore recent rejects filter for locked txes (dashpay#3124)
  Make orphan TX map limiting dependent on total TX size instead of TX count (dashpay#3121)
  Update/modernize macOS plist (dashpay#3074)
  Fix bip69 vs change position issue (dashpay#3063)
  Partially revert 3061 (dashpay#3150)
  Fix SelectCoinsMinConf to allow instant respends (dashpay#3061)
  ...

# Conflicts:
#	configure.ac
#	doc/man/biblepay-cli.1
#	doc/man/biblepay-qt.1
#	doc/man/biblepay-tx.1
#	doc/man/biblepayd.1
#	doc/release-notes.md
#	src/clientversion.h
#	src/wallet/wallet.cpp
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 25, 2021
* Update activemn if protx info changed

* Add `==` and `!=` operators to CDeterministicMNState

* Only re-init active MN if its IP changed, changes to payout, voting etc. can be done without it

* Test `masternode status` updates

* Don't track mnListEntry anymore and instead get the DMN on demand

* Revert "Add `==` and `!=` operators to CDeterministicMNState"

This reverts commit fba4687.
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants