refactor: minimize locking in ChainLocks Cleanup

[PastaPastaPasta]

Issue being fixed or feature implemented

Simple changes, just look at the two commits: first we minimize scope of cs_main to what actually needs it. Then we change where we update the lastCleanupTime to right after we check it to minimize any chance of two threads entering at the same time.

What was done?

How Has This Been Tested?

Built, ran for a bit

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[knst]

can it change behavior in case if Cleanup() will take significant time?

It can cause of new iteration of Cleanup start immediately after previous iteration is done if cs_main will be busy for awhile and cleaning process will take a long time due to that.

[PastaPastaPasta]

It in theory could; but in practice I don't see how it possibly could. I think Cleanup is only done every 30 seconds; and I don't see it being a problem if cleanup happens after extra 30 seconds.

[knst]

possible if you debug application and run it step-by-step, but I guess that is not really important

[UdjinM6]

Then we change where we update the lastCleanupTime to right after we check it to minimize any chance of two threads entering at the same time.

Doubt 9184edb would make any difference because TrySignChainTip() is the only place Cleanup() is called in and we call that one via a dedicated scheduler to avoid multiple parallel runs already

dash/src/llmq/chainlocks.cpp

Lines 206 to 208 in 9184edb

	// don't call TrySignChainTip directly but instead let the scheduler call it. This way we ensure that cs_main is
	// never locked and TrySignChainTip is not called twice in parallel. Also avoids recursive calls due to
	// EnforceBestChainLock switching chains.

[PastaPastaPasta]

Then we change where we update the lastCleanupTime to right after we check it to minimize any chance of two threads entering at the same time.

Doubt 9184edb would make any difference because TrySignChainTip() is the only place Cleanup() is called in and we call that one via a dedicated scheduler to avoid multiple parallel runs already

dash/src/llmq/chainlocks.cpp

Lines 206 to 208 in 9184edb

	// don't call TrySignChainTip directly but instead let the scheduler call it. This way we ensure that cs_main is
	// never locked and TrySignChainTip is not called twice in parallel. Also avoids recursive calls due to
	// EnforceBestChainLock switching chains.

Agreed; it just feels the more correct way to update it

[UdjinM6]

utACK

[UdjinM6]

...but needs rebase to fix tests :)