Apply time preference check for all lower TimeFrames #12356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
turbolay
commented
Feb 3, 2024
| { | ||
| if (FeeRateMedianTimeFrame == default) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (RoundStatusUpdater.CoinJoinFeeRateMedians.TryGetValue(FeeRateMedianTimeFrame, out var medianFeeRate)) | ||
| if (RoundStatusUpdater.CoinJoinFeeRateMedians.ContainsKey(FeeRateMedianTimeFrame)) |
There was a problem hiding this comment.
This check is more or less useless now but I didn't want to change the behaviour.
molnard
reviewed
Feb 5, 2024
molnard
reviewed
Feb 6, 2024
|
Sorry if irrelevant but I don't see PR that is merging this to the release branch. Can you verify if it is merged, if not then create a PR for that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem found by @Kruwed
tl;dr: When you are willing to wait for several days for the best moment of the month, you are also willing to wait few extra hours for the best moment of the day
Currently,
IsRoundEconomiconly takes into consideration theMedianFeeRaterelated to the exactTimeFrameselected in user's Coinjoin settings,But in cases where a
FeeRateMedianfor a shortTimeFrameis lower than theFeeRateMedianfor a longerTimeFrame, and the user has the longerTimeFramein its settings,IsRoundEconomicwill allow coinjoin.Example:
Median past day, 24 sat/vbyte
Median past week, 27 sat/vbyte
Median past month, 35 sat/vbyte
The user has
Weeklyselected, the current coinjoin round's fee rate is 26 sat/vbCurrently,
IsRoundEconomicwill returntrue, while it would be better to wait for the current round fee rate to also go below the daily median.I know that this problem is a bit tricky to understand, I had to wrap my head around it for quite some time.
What this PR is doing is to check for the current
FeeRateMedianof all lower or equalTimeFrameof what the user has in settings. That way,IsRoundEconomicwill also return false until all the current round's fee rate goes below theMedianFeeRatefor all lowerTimeFrame.I wanted to write a test but I didn't manage to do it without changing the method to
staticor struggling to instantiateCoinJoinClient, if anyone has a suggestion on how to do it I would gladly write the test.