-
Notifications
You must be signed in to change notification settings - Fork 498
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
Improve CoinJoinClient by extracting OutputRegistration Phase related progress #7533
Conversation
Tested with 10 wallets, seems to be working OK. I know this specific PR is not a big change but I am also testing the new time distribution of requests. https://mempool.space/testnet/tx/2fcb11a7554a4280decfe23f7fb540327dace1e444063b26f4c738644ecf818e |
CoinJoinClient.cs improves its Code Health from 8.36 -> 8.83 |
Code LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK b4c34dc
runs smooth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very good.
var outputRegistrationEndTime = DateTimeOffset.UtcNow + (remainingTime * 0.8); | ||
var readyToSignEndTime = outputRegistrationEndTime + remainingTime * 0.2; | ||
|
||
using CancellationTokenSource outputRegPhaseTimeoutCts = new(remainingTime + TimeSpan.FromMinutes(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the magic 1min comes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra margin, the value coming from common sense - synchronization delays with the coordinator. Maybe it is too much, but we are on the right side of the horse because under no circumstances should the client cancels the operations accidentally before it was canceled by the backend.
Lucas ACK-ed on a meeting. |
based on #7529
I've also added a phase-specific timeout cancellationToken.