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

pool: Remove handlePayments goroutine. #424

Closed
wants to merge 1 commit into from

Conversation

jholdstock
Copy link
Member

Previously a long-lived goroutine named handlePayments would buffer calls to processPayments and run them sequentially. This adds unnecessary complexity without any obvious benefit.

In addition, it appears as though the only purpose of the done channel in the paymentMsg was to facilitate testing, which means this change makes it redundant and it can be removed.

Previously a long-lived goroutine named handlePayments would buffer
calls to processPayments and run them sequentially. This adds
unnecessary complexity without any obvious benefit.

In addition, it appears as though the only purpose of the done channel
in the paymentMsg was to facilitate testing, which means this change
makes it redundant and it can be removed.
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I'm not completely against approving this because it's really not much worse than what is already there, but the overall logic here looks incorrect to me, even though it probably won't experience too many issues due to the inherit delays in block solving.

Notably, it seems like payments really should be processed in order, but the code is just launching a goroutine to process them, which means they can be processed out of order if there are back to back payments too quickly.

From looking at the code, I highly suspect the original intention was to ensure payments could be processed concurrently with all other operations (notably processing chain updates, providing work, etc), and most notably in order, which was the reason for having a separate goroutine with a channel to serialize the input.

However, because it isn't designed to deal with backlog properly, it would almost positively have ended up blocking on the send side in processPayments from the chain updates anyway and causing issues (e.g. due to leading to blocking other things) , and so I'd wager that scenario was probably hit and in order to "paper over" it, processPayments is being launched as a goroutine. However, as mentioned, that really is incorrect logic because it means there is no longer a guaranteed proper order.

@jholdstock jholdstock closed this Dec 14, 2023
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.

2 participants