client/bond.go: Avoid dropping tier. #2460
Conversation
|
I'm not super confident about these changes, but they seem to work correctly. We want to reserve the same amount as bonded so we can re bond later I think? |
client/core/bond.go
Outdated
| @@ -259,6 +261,11 @@ func (c *Core) bondStateOfDEX(dc *dexConnection, bondCfg *dexBondCfg) *dexAcctBo | |||
|
|
|||
| tierDeficit := int64(state.targetTier) - state.tier | |||
| state.mustPost = tierDeficit + state.weakStrength - state.pendingStrength | |||
| // If weak bonds expiring would put the number of live and pending bonds | |||
| // to zero, replace the weak bonds. | |||
| if state.mustPost <= 0 && state.weakStrength >= state.liveStrength+state.pendingStrength { | |||
There was a problem hiding this comment.
The liveStrength does not include the bonds in the weakStrength category. Would it work if we just post 1 bond if state.liveStrength+state.pendingStrength == 0?
There was a problem hiding this comment.
Yeah. That may be more correct.
There was a problem hiding this comment.
The liveStrength does include weak bonds, so weakStrength is always <= liveStrength.
|
I was looking at something like this, @JoeGruffins. |
|
@buck54321 Great! Thanks! Will apply that to this pr if it's alright. How about the reserves changes here, do they look ok? They are in this pr because if the bonus tiers go up high enough we will stop reserving for the one bond we may need to post. |
JoeGruffins
force-pushed
the
testingclientbondcontinuous
branch
2 times, most recently
from
5051e26 to
97fba82
Compare
| // in split bonds that cost the user unnecessary tx fees. This solution | ||
| // is only a stop-gap. A proper solution will require more data from the | ||
| // API so we can know our bonus tiers. | ||
| bondTierDeficit := int64(state.targetTier) - state.liveStrength - state.pendingStrength + state.weakStrength |
There was a problem hiding this comment.
This basically just cancels out the benefit the user gets from good trading history. Their good trading history will not allow them to have less locked in bonds. If we do
if state.liveStrength - state.weakStrength + state.pendingStrength == 0 {
state.mustPost = 1
}then the user posts the minimum they need to maintain the tier they desire.
There was a problem hiding this comment.
This basically just cancels out the benefit the user gets from good trading history.
I don't follow. Can you give an example with numbers?
There was a problem hiding this comment.
targetTier = 2
currentTier = 4
liveStrength = 2
weakStrength = 2
pendingStrength = 0
In this situation, the original logic would post 0, and the updated logic would post 2. However we only need to post one to make sure that we have at least one live bond.
There was a problem hiding this comment.
Yeah. Because if weakStrength would ever bring tier below target tier, the original code will fire. I think one is always enough. It's difficult to reason about though, for some reason.
There was a problem hiding this comment.
@buck54321 is this what this is referring to?
// Note: For users with targetTier > 1, unknown bonus tiers can also
// cause us to broadcast renewal bonds that are too small, resulting
// in split bonds that cost the user unnecessary tx fees.
I'm not sure I understand.
There was a problem hiding this comment.
So in that scenerio, the extra bonds would be needed if our bonus tiers went down? Is this the same extra fees we would incur if we increased our target tier? You mean that they would expire at different times?
There was a problem hiding this comment.
Yes, they would expire at different times. Where normally we could send a single bond tx with bond weight 4 once every expiry, we would now be sending a weight 1 bond and the user would see their tier drop from 7 to 4 for some unknown reason. To fix it by broadcasting additional bonds would create "parallel tracks", adding more txs per expiry period.
There was a problem hiding this comment.
I see! Thank you.
@martonp ok with re-posting all weak bonds?
There was a problem hiding this comment.
Posting 2 is correct, imo. We should aim to maintain
bonded_tier, noteffective_tier = bonded_tier + bonus_tiers. In fact, any attempt to maintain effective tier for low-tier targets would ultimately fail to maintain bonded status, as @JoeGruffins was seeing in #2459.
I think it's not that bad to repost all the bonds and ignore the bonuses.. it's just a few extra bonds being posted, but my understanding was that the reason all orders were being canceled was that there were no bonds at all. If we just ensure that there is at least one bond at all times, wouldn't that be prevented?
There was a problem hiding this comment.
Oh. We're basing trading limits on bonded tier anyway, so that's the value we have to maintain.
Line 815 in 6a7d713
JoeGruffins
force-pushed
the
testingclientbondcontinuous
branch
from
97fba82 to
2c3c478
Compare
There was a problem hiding this comment.
I think we may have an issue with reserves handling in authDEX. We're basing dc.acct.tierChange on the effective tier.
Line 6723 in 6a7d713
and then below, in the case of a re-auth, we're basing our reserves updates on this value
Line 6768 in 6a7d713
but does that make sense with consideration for bonus tiers?
Similar concern for handleTierChangeMsg?
client/core/bond.go
Outdated
| // Reserve enough funds to recreate current bonds. | ||
| reserve := bondOverlap * uint64(acctBondState.liveStrength) * bondAsset.Amt |
There was a problem hiding this comment.
I'm having trouble wrapping my head around this change. Can you clue me in? I see you're eventually setting reserveDelta and acct.totalReserved, but usually we use something like the total from bondTotalInternal, which includes pending and expired bonds. The use of a bare liveStrength here is curious.
There was a problem hiding this comment.
The reason for the change is that now the acctBondState.tierChange may not be reliable to decide how much to reserve. If bonus tiers go up then we will start reserving 0 even though we need to reserve enough for at least one bond.
I think the original logic was created this way to allow for less reserves as we needed less because of bonus bond tiers, and I attempted to save that logic, but maybe we should just simply reserve target tiers worth?
The use of a bare liveStrength here is curious.
It was my thinking that we want enough to re-post all live bonds. That is strong and weak. I think strong and pending could also work, but pending, weak, and strong would be overkill, as we are making pending bonds in response to weak bonds so it would count those twice... Would the second return from bondTotalInternal be the same as liveStrength here?
There was a problem hiding this comment.
Hmm, should this be subtracted then from live bonds? bondOverlap indicates 2x target including live? If it was three then 3x? Also I guess this does not include fees we should reserve? Can clean this up better after v2 ConnectResult? I'm unclear on exactly how much we want to reserve. Do we want to count bonus tiers? We must count any negative tiers... If we are -1 because of bad behavior, we will always need to reserve target + 1.
There was a problem hiding this comment.
I think the original logic was created this way to allow for less reserves as we needed less because of bonus bond tiers, and I attempted to save that logic, but maybe we should just simply reserve target tiers worth?
Maybe, but trading limits scale with bonded tier. Bonus tiers are an illusion. I'm not certain why they exist at all, tbh. The server doesn't need to add them. They do nothing for the client as "tiers". Client should be more interested in score as a form of reputation and as a penalty buffer. I may propose removing these bonus tiers if I can get the upgrade logic worked out.
There was a problem hiding this comment.
Ok. Since I am unsure what the ultimate plan for the bonus tiers is will leave this for now. If you want to absorb usable changes here into #2471 or have a different plan please do. I can remove the reserves changes from this as well.
I don't have a strong opinion on whether the user should have expanded trading limits for good behavior.
There was a problem hiding this comment.
I guess I was thinking that it should look something like this. buck54321/dcrdex@04116f8...buck54321:dcrdex:reserves-adjustment
JoeGruffins
force-pushed
the
testingclientbondcontinuous
branch
from
2c3c478 to
04116f8
Compare
If client has good standing with the server, it will not preemptively create a bond before its only bond expires. Always restore weak bonds if they are the last bonds to avoid going to zero. Reserve enough coin to restore live bonds rather than tracking the delta.
JoeGruffins
force-pushed
the
testingclientbondcontinuous
branch
from
04116f8 to
8d0adcd
Compare
|
Looks ok. |
There was a problem hiding this comment.
I still think this is a little bit off, but this should fix the bug and we can work on finding better solutions. Thanks for digging in, @JoeGruffins
If client has good standing with the server, it will not preemptively create a bond before its only bond expires. Always restore weak bonds if they are the last bonds to avoid going to zero. Reserve enough coin to restore live bonds rather than tracking the delta.
closes #2459