decred / dcrpool

decred mining pool
ISC License
31 stars 28 forks source link

pool: Correct racy paymgr processing logic. #396

Closed davecgh closed 1 year ago

davecgh commented 1 year ago

This corrects the mutex logic in the payment manager that keeps track of whatever or not payments are already being processed.

In particular, the code that is being updated previously checked and set the processing flag under two separate locks of the mutex which could easily result in the first check returning false for two concurrent goroutines followed both both setting it to true and executing which would defeat the purpose of the check.

davecgh commented 1 year ago

Given that payDividends is the only func which accesses the Mutex, I think this can be really simplified. Mutex can just be locked once at the start of the func and then unlocked in a defer.

I originally changed it that way, as I also highly suspect that's true, however I haven't dug into the call paths yet and didn't want to change the semantics until I verify, so I opted for this approach.

davecgh commented 1 year ago

Looking a little more closely, I see that that payDividends is the result of receiving a message on the paymentCh which itself is sent via processPayments. Then processPayments is used in the chain state config and is invoked as a goroutine from handleChainUpdates for connected blocks.

Since blocks can arrive at any time, and it's executed as a goroutine, it is possible that payDividends is still running. Changing it to a mutex for the entire function would cause it to end up running the entirety of payDividends for every connected block instead of intentionally skipping rapid blocks and processing them as a batch as it does with the current semantics.