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

Add collateral, owner and voting addresses to masternode list table #3207

Merged
merged 7 commits into from
Nov 22, 2019

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 19, 2019

I did not notice any slowdown or GUI lags despite using GetUTXOCoin here (which locks cs_main), please test to see if that's the case for you too. This PR also tweaks table headers to match RPC output a bit better and unifies somewhat related code a bit.

Thanks to Craig Durham for the suggestion.

@UdjinM6 UdjinM6 added this to the 15 milestone Nov 19, 2019
Copy link
Collaborator

@thephez thephez left a comment

Choose a reason for hiding this comment

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

Slightly-tested ACK

@codablock
Copy link

@UdjinM6 did you also test this on mainnet regarding potential performance losses?

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 20, 2019

I only tried changing filter back and forth to trigger list update first. The debug timer I added to logs showed an increase like from 150-200ms on avg to 200-250ms on avg which I wasn't able to notice by eyes.

But now I tried reindexing... and it's dead... :/

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 20, 2019

Can't find any good solution to this besides just skipping updates until blockchain is synced and caching all the things but even then the first update takes like 1.5 sec which is way too much.

Closing.

@UdjinM6 UdjinM6 closed this Nov 20, 2019
@UdjinM6 UdjinM6 removed this from the 15 milestone Nov 20, 2019
@codablock
Copy link

@UdjinM6 storing the collateral address/destination in the MN object coming from the MN manager could be a viable solution. But then we need to take care of compatibility in evodb, not sure how much effort that'd be.

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 20, 2019

Found smth :)

@UdjinM6 UdjinM6 reopened this Nov 20, 2019
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 21, 2019

I tested this by reindexing a couple of times and works pretty good it seems. MN list UI updates take ~200-300ms so calling them during reindex (and locking cs_main for this purpose) every 3 sec would "steal" up to ~10%, c68669b fixes this by dropping the impact 10x to ~1% which sounds acceptable. I'm still not sure how this would perform under regular heavy load though. I'm going to keep this PR open with no milestone for now and see how my local branch performs when/if there is another stresstest.

@codablock
Copy link

Holding cs_main all the time in updateDIP3List is dangerous IMHO as it can easily lead to slow down everything else. What about 3a325ba as an alternative?

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 21, 2019

Good idea 👍 Applied your patch, will test.

@codablock
Copy link

@UdjinM6 any test results already?

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 22, 2019

I tried reindexing a couple of time and cs_main takes like 10% in general during the process (the other 90% is gui i.e. it's 10x improvement over my code :) ). Initial GUI update on a synced wallet is still lag-ish though:

cs_main lock: 2358ms, total: 2780ms
cs_main lock: 39ms, total: 359ms
cs_main lock: 8ms, total: 238ms
cs_main lock: 7ms, total: 194ms
cs_main lock: 8ms, total: 188ms
cs_main lock: 9ms, total: 210ms
cs_main lock: 9ms, total: 343ms

And it differs from time to time - can take from 1 to 4 sec on my machine.

@codablock
Copy link

How did you measure these times?

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 22, 2019

int64_t nStart = GetTimeMillis();
///cs_main stuff
int64_t nMainLock = GetTimeMillis();
/// gui stuff
int64_t nEnd = GetTimeMillis();
LogPrintf("%s -- cs_main lock: %dms, total: %dms\n", __func__, nMainLock - nStart, nEnd - nStart);

@codablock
Copy link

Strange, I'm unable to reproduce the multi-second lag on startup with a fully synced wallet. Using the logging you provided I get just a few ms for each updateDIP3List invocation, even for the first one. I also tested with DEBUG_LOCKORDER to make sure that this is not the issue here.

@codablock
Copy link

Ahh sorry, just realized that I probably shouldn't test this on testnet but on mainnet... 🙈

codablock
codablock previously approved these changes Nov 22, 2019
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.

tested ACK

The initial lag on my machine is much lower (a few 100ms instead of multiple secs). I assume this is dependent on hardware performance. I tend to accept this lag and then later perform a proper refactoring for this masternode list that utilizes caches and other tricks to speed up stuff.

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 22, 2019

No initial GUI lag after 79cbbd4 on my machine anymore:

Filling coin cache with masternode UTXOs...
Filling coin cache with masternode UTXOs: done in 2249ms
updateDIP3List -- cs_main lock: 5ms, total: 243ms
updateDIP3List -- cs_main lock: 7ms, total: 275ms

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.

re-utACK

@UdjinM6 UdjinM6 added this to the 15 milestone Nov 22, 2019
@@ -831,6 +831,20 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
// GetMainSignals().UpdatedBlockTip(chainActive.Tip());
pdsNotificationInterface->InitializeCurrentBlockTip();

{
// Get all UTXOs for each MN collateral in one go so that we can fill coin cache early
// and reduce further locking overhead for cs_main in other parts of code inclluding GUI
Copy link

Choose a reason for hiding this comment

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

nit: including

Copy link
Author

Choose a reason for hiding this comment

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

Sigh.. I literally hate keyboard on recent macs more and more every day...

Copy link

@nmarley nmarley 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 482a549 into dashpay:develop Nov 22, 2019
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
…ashpay#3207)

* Add collateral, owner and voting addresses to masternode list table

* Adjust column names in masternode list table

* Slightly refactor updateDIP3List()

* Lock cs_main in updateDIP3List early to avoid GUI frezes

* Update MN list in GUI 10 times less often while blockchain is still syncing

* Move GetUTXO calls outside of main update loop

* Fill coin cache for masternode UTXOs on start
FornaxA pushed a commit to ioncoincore/ion that referenced this pull request Jul 6, 2020
…ashpay#3207)

* Add collateral, owner and voting addresses to masternode list table

* Adjust column names in masternode list table

* Slightly refactor updateDIP3List()

* Lock cs_main in updateDIP3List early to avoid GUI frezes

* Update MN list in GUI 10 times less often while blockchain is still syncing

* Move GetUTXO calls outside of main update loop

* Fill coin cache for masternode UTXOs on start

Signed-off-by: cevap <[email protected]>
@UdjinM6 UdjinM6 deleted the newcolumnsinmnlist branch November 26, 2020 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants