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.

Sign up for GitHub

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

client/bond.go: Avoid dropping tier. #2460

Merged
merged 3 commits into from
Sep 1, 2023
Merged

client/bond.go: Avoid dropping tier. #2460

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39fe28f
client/bond.go: Avoid dropping tier.
JoeGruffins Aug 4, 2023
307e5c6
work around bonus tier bug
buck54321 Aug 4, 2023
8d0adcd
alternative reserves adjustment formula
buck54321 Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 72 additions & 28 deletions client/core/bond.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ type dexAcctBondState struct {
bondAssetID uint32
maxBondedAmt uint64
mustPost int64
inBonds uint64
}

// bondStateOfDEX collects all the information needed to determine what
Expand All @@ -227,6 +228,8 @@ func (c *Core) bondStateOfDEX(dc *dexConnection, bondCfg *dexBondCfg) *dexAcctBo
} else {
if int64(bond.LockTime) <= bondCfg.replaceThresh {
state.weak = append(state.weak, bond) // but not yet expired (still live or pending)
c.log.Infof("Soon to expire bond found: %v (%s)",
coinIDString(bond.AssetID, bond.CoinID), unbip(bond.AssetID))
}
liveBonds = append(liveBonds, bond)
}
Expand All @@ -237,6 +240,7 @@ func (c *Core) bondStateOfDEX(dc *dexConnection, bondCfg *dexBondCfg) *dexAcctBo
state.tierChange = dc.acct.tierChange // since last reserves update
state.tier, state.targetTier = dc.acct.tier, dc.acct.targetTier
state.bondAssetID, state.maxBondedAmt = dc.acct.bondAsset, dc.acct.maxBondedAmt
state.inBonds, _ = dc.bondTotalInternal(state.bondAssetID)
// Screen the unexpired bonds slices.
dc.acct.bonds = filterExpiredBonds(dc.acct.bonds)
dc.acct.pendingBonds = filterExpiredBonds(dc.acct.pendingBonds) // possibly expired before confirmed
Expand All @@ -257,8 +261,41 @@ func (c *Core) bondStateOfDEX(dc *dexConnection, bondCfg *dexBondCfg) *dexAcctBo
}
}

tierDeficit := int64(state.targetTier) - state.tier
state.mustPost = tierDeficit + state.weakStrength - state.pendingStrength
// This old way is bugged because we have no way of knowing how many bonus
// tiers the server has assigned us, because it is not directly reported in
// any comms and to calculate it locally, we would need to know the
// server's ban score, which is configurable and not reported. We would also
// need to know our own score, but after the ConnectResult, the server
// doesn't send score updates.
//
// The old way
// -----------
// tierDeficit := int64(state.targetTier) - state.tier
// state.mustPost = tierDeficit + state.weakStrength - state.pendingStrength
// -----------
//
// Instead, if the server says we're short, we assume that we have zero
// bonus tiers and use the old calculation.
serverTierDeficit := int64(state.targetTier) - state.tier + state.weakStrength - state.pendingStrength
if serverTierDeficit > 0 {
state.mustPost = serverTierDeficit
} else {
// But if the server says we're tiered up, we may be so because we have
// bonus tiers, which ultimately causes us to ignore weak bonds. In
// that case, just make sure that our local bond data is sufficient to
// get our target tier. For clients with low targetTier and good trading
// history, this will be the primary route through which weak bonds are
// renewed.
// 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. 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@buck54321 buck54321 Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see! Thank you.

@martonp ok with re-posting all weak bonds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Posting 2 is correct, imo. We should aim to maintain bonded_tier, not effective_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?

Copy link
Member

Choose a reason for hiding this comment

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

Oh. We're basing trading limits on bonded tier anyway, so that's the value we have to maintain.

dcrdex/server/auth/auth.go

Line 815 in 6a7d713

baseScale := client.bondTier()

if bondTierDeficit > 0 {
state.mustPost = bondTierDeficit
}
}

return state
}
Expand All @@ -269,9 +306,9 @@ type bondID struct {
}

type acctUpdate struct {
acct *dexAccount
tierChange int64
reserveDelta int64
acct *dexAccount
tierChange int64
reserve uint64
}

// refundExpiredBonds refunds expired bonds and returns the list of bonds that
Expand Down Expand Up @@ -429,7 +466,16 @@ func (c *Core) repostPendingBonds(dc *dexConnection, cfg *dexBondCfg, state *dex
}

// postRequiredBonds posts any required bond increments for a dexConnection.
func (c *Core) postRequiredBonds(dc *dexConnection, cfg *dexBondCfg, state *dexAcctBondState, bondAsset *msgjson.BondAsset, wallet *xcWallet, expiredStrength int64, unlocked bool) {
func (c *Core) postRequiredBonds(
dc *dexConnection,
cfg *dexBondCfg,
state *dexAcctBondState,
bondAsset *msgjson.BondAsset,
wallet *xcWallet,
expiredStrength int64,
unlocked bool,
) (newlyBonded uint64) {

if state.targetTier == 0 || state.mustPost <= 0 || cfg.bondExpiry <= 0 {
return
}
Expand Down Expand Up @@ -480,8 +526,11 @@ func (c *Core) postRequiredBonds(dc *dexConnection, cfg *dexBondCfg, state *dexA
_, err = c.makeAndPostBond(dc, true, wallet, amt, c.feeSuggestionAny(wallet.AssetID), lockTime, bondAsset)
if err != nil {
c.log.Errorf("Unable to post bond: %v", err)
} // else it's now in pendingBonds
} else { // it's now in pendingBonds
newlyBonded = amt
}
}
return
}

// rotateBonds should only be run sequentially i.e. in the watchBonds loop.
Expand All @@ -498,8 +547,7 @@ func (c *Core) rotateBonds(ctx context.Context) {

now := time.Now().Unix()

netReserveDeltas := make(map[uint32]int64) // for all DEX conns, per asset, compare to lastReserves
actuatedTierChanges := make(map[uint32][]acctUpdate)
acctUpdates := make(map[uint32][]acctUpdate)

for _, dc := range c.dexConnections() {
initialized, unlocked := dc.acct.status()
Expand Down Expand Up @@ -540,20 +588,23 @@ func (c *Core) rotateBonds(ctx context.Context) {
continue
}

if acctBondState.tierChange != 0 {
reserveDelta := bondOverlap * -acctBondState.tierChange * int64(bondAsset.Amt)
netReserveDeltas[acctBondState.bondAssetID] += reserveDelta // negative means free reserves
// After applying the net reserveDelta, we would normally *zero* the
// tierChange field, but it could change, so we remember it.
actuatedTierChanges[acctBondState.bondAssetID] = append(actuatedTierChanges[acctBondState.bondAssetID],
acctUpdate{dc.acct, acctBondState.tierChange, reserveDelta},
)
}

c.postRequiredBonds(dc, bondCfg, acctBondState, bondAsset, wallet, expiredStrength, unlocked)
newlyBonded := c.postRequiredBonds(dc, bondCfg, acctBondState, bondAsset, wallet, expiredStrength, unlocked)
reserve := bondOverlap*acctBondState.targetTier*bondAsset.Amt - acctBondState.inBonds - newlyBonded
acctUpdates[acctBondState.bondAssetID] = append(acctUpdates[acctBondState.bondAssetID],
acctUpdate{dc.acct, acctBondState.tierChange, reserve},
)
}

for assetID, reserveDelta := range netReserveDeltas {
for assetID, update := range acctUpdates {
var reserveDelta int64
// Update the unactuated tier change counter.
for _, s := range update {
s.acct.authMtx.Lock()
reserveDelta += s.acct.totalReserved - int64(s.reserve)
s.acct.totalReserved = int64(s.reserve)
s.acct.tierChange -= s.tierChange
s.acct.authMtx.Unlock()
}
if reserveDelta != 0 { // net adjustment across all dexConnections
wallet, err := c.connectedWallet(assetID)
if err != nil { // we grabbed it above so shouldn't happen
Expand All @@ -564,13 +615,6 @@ func (c *Core) rotateBonds(ctx context.Context) {
wallet.amtStringSigned(reserveDelta))
wallet.ReserveBondFunds(reserveDelta, 0, false) // fee buffer already added when first enabled
}
// Update the unactuated tier change counter.
for _, s := range actuatedTierChanges[assetID] {
s.acct.authMtx.Lock()
s.acct.tierChange -= s.tierChange
s.acct.totalReserved += s.reserveDelta
s.acct.authMtx.Unlock()
}
}
}

Expand Down