multi: v2 reputation and client bond refactor #2501

buck54321 · 2023-09-05T13:30:12Z

Server

Eliminate "bonus tiers". They weren't doing anything.
Server sends updates for score changes too.
Reverse scoring signage to match common convention.

Client

Simplify asset.Bonder. There was a lot of tracking going on in our Bonder implementations that was used exclusively for logging. I can see an argument being made that the wallet should know about outstanding bond value, but the wallet didn't even know the bond IDs, so the wallets weren't really managing bonds in any way that added security. There was really nothing happening that couldn't be logged by the caller. Also swaps out the diff-based approach to reserves management, which is tedious and error-prone, to a simpler idempotent setter pattern.
Add Strength field to various Bond structs. That's how the server calculates tier already. Client was using a different calc. Changes to bond asset configuration could have gotten client and server out of sync.
Penalties are not auto-compensated by default. User will need to set a non-zero PenaltyComps value for the account in order for penalties to be compensated. PenaltyComps works in conjunction with MaxBondedAmt, but specifically applies to limiting penalties.
Parallel tracks will now merge. This is accomplished by shortening the lifetime of a new bond if there is an existing live bond with which this bond can be merged upon expiration.
For v1 rep servers, we will periodically re-auth since the ConnectResult is the only place we can get our score.

Replaces #2471

martonp

Really nice improvements.. I'm glad the code in the wallets got simplified.

server/account/account.go

server/auth/auth.go

martonp · 2023-09-10T03:16:26Z

client/core/bond.go

+		inBonds, _ := dc.bondTotalInternal(bondAssetID)
+		totalReserves := bondOverlap * bondAssetAmt * targetTier // this dexConnection
+		if totalReserves > inBonds {
+			nominalReserves += totalReserves - inBonds


If we have 2 bonds active, but the user setting is to maintain 1 bond, wouldn't we have insufficient reserves for the overlap period?

I think the way I'm doing it with minBondReserves in 402ddee resolves this and actually makes reserves much more accurate.

client/core/types.go

martonp · 2023-09-10T04:50:51Z

server/account/account.go

+// Reputation is a part of a number of server-originating messages. It was
+// introduced with the v2 ConnectResult.
+type Reputation struct {
+	BondedTier int64  `json:"bondedTier"`


What if the server not only sent the BondTier, but also the list of bond IDs it knows about. This would allow the client to know about any mismatches between bonds it thinks have been posted vs bonds the server knows about.

martonp · 2023-09-10T04:55:16Z

client/core/bond.go

+		expectedServerTier++
+	}
+	reportedServerTier := state.Rep.EffectiveTier()
+	if reportedServerTier < expectedServerTier {


reportedServerTier < expectedServerTier could be true because the client thinks the server knows about some bonds that it actually does not know about. Not just due to penalties. If the BondIDs are added to the Reputation, those existing bonds could be reposted.

I think what you're getting at is handled on connection in authDEX. By this point, any discrepancy is likely unresolvable, and the user should rely on their configuration (PenaltyComps, MaxBondedAmt) to prevent a malicious server from forcing a bunch of bonds they refuse to recognize.

I see it now.

martonp · 2023-09-13T06:18:30Z

client/core/bond.go

+	if targetTier == 0 {
+		return 0
+	}
+	// Keep a list of tuples of [weakTime, bondStrength]. Later, we'll checks


martonp · 2023-09-13T06:21:05Z

client/core/bond.go

+		weakTime := bond.LockTime - bondExpiry - pBuffer
+		ba := dexCfg.BondAssets[dex.BipIDSymbol(bond.AssetID)]
+		if ba == nil {
+			// Bond asset no longer supported Can't calculate strength. Consdier


supported.
Consider

martonp · 2023-09-13T06:30:00Z

client/core/bond.go

+	// we need to add the missing bond strength.
+	reserveTiers := targetTier - tierSum
+	sort.Slice(activeTiers, func(i, j int) bool {
+		return activeTiers[i][0] < activeTiers[j][1]


This sort is comparing the weak time to the strength.

martonp · 2023-09-13T07:57:40Z

client/core/bond.go

+	// If our active+pending bonds don't cover our target tier for some reason,
+	// we need to add the missing bond strength.
+	reserveTiers := targetTier - tierSum


Shouldn't this be double counted? Since we need this amount of tiers now, and then the same amount when the ones we need to submit now become weak.

martonp · 2023-09-13T08:06:07Z

I didn't mean to approve yet, I'm still planning to test it.

JoeGruffins · 2023-09-18T08:05:29Z

This does not appear to solve #2461 which is fine just mentioning in case it is intended to as #2471 was.

JoeGruffins

That's a lot of changes but looks good to me. Will continue to test.

JoeGruffins · 2023-09-18T08:40:05Z

client/core/core.go

+		// DRAFT NOTE: This was wrong? Isn't account suspended at tier 0?
+		// cannotTrade := dc.acct.effectiveTier < 0 || (dc.acct.effectiveTier == 0 && dc.apiVersion() < serverdex.BondAPIVersion)


"Suspended", but maybe just no longer bonded? effectiveTier of zero may not indicate penalties. So it would not make a new account unless there was surely a penalty (<0 effective tier).

JoeGruffins · 2023-09-18T08:42:38Z

client/core/core.go

+		// Using <= though acknowledging that this ignore the possibility that
+		// an existing revoked bond could be ressurected.


Suggested change

// Using <= though acknowledging that this ignore the possibility that

// an existing revoked bond could be ressurected.

// Using <= though acknowledging that this ignores the possibility that

// an existing revoked bond could be resurrected.

JoeGruffins · 2023-09-18T08:46:04Z

client/core/core.go

-			keyIndex++
+			c.log.Infof("HD account key for %s has tier %d, but %d penalties (not able to trade). Deriving another account key.",
+				dc.acct.host, rep.BondedTier, rep.Penalties)


Still need to increment key index?

Woah. Good catch.

JoeGruffins · 2023-09-18T08:54:45Z

client/core/core.go

+			// On the server, they do
+			//    bondTier += int64(bond.Strength)
+			// where bond.Strength is a value stored in the database during
+			// postbond. This means if ther server doubles the bond size for an


Suggested change

// postbond. This means if ther server doubles the bond size for an

// postbond. This means if the server doubles the bond size for an

JoeGruffins · 2023-09-18T08:59:43Z

client/core/core.go

 	// TODO: notify sub consumers e.g. frontend
+	c.notify(newReputationNote(dc.acct.host, rep))


Can remove TODO?

JoeGruffins · 2023-09-18T09:07:18Z

server/account/account.go

+// Reputation is a part of a number of server-originating messages. It was
+// introduced with the v2 ConnectResult.
+type Reputation struct {
+	// BondedTier is the tier indicated by the users active bonds. BondedTier


Suggested change

// BondedTier is the tier indicated by the users active bonds. BondedTier

// BondedTier is the tier indicated by the user's active bonds. BondedTier

buck54321 mentioned this pull request Sep 6, 2023

auth: v2 ConnectResult #2471

Closed

martonp reviewed Sep 10, 2023

View reviewed changes

buck54321 force-pushed the reputation-v2 branch from 17d9168 to 402ddee Compare September 12, 2023 01:51

martonp approved these changes Sep 13, 2023

View reviewed changes

buck54321 force-pushed the reputation-v2 branch from 402ddee to 323d4c3 Compare September 17, 2023 23:38

martonp approved these changes Sep 18, 2023

View reviewed changes

JoeGruffins approved these changes Sep 18, 2023

View reviewed changes

buck54321 force-pushed the reputation-v2 branch from 323d4c3 to e4860c6 Compare September 25, 2023 23:40

buck54321 added 16 commits October 3, 2023 13:56

connect result v2

cc1f696

send score with tier report

3ffd1b1

refactor client reserves

79c1b24

some basic tests

3d6504a

merging bonds

0927111

monitor v1 reputation

795571e

score change notifications

9ce1068

optional balance checks

034eec0

add bond strength

f566974

update exported core types

f013d4c

fix some stuff

5d5d5db

update rpcserver bondopts and some fixes

4988a7b

better reserves calcs. other review fixes

ab4747a

review followup

dfe6812

fix maxbonded when lowering tier

ac22a22

joegruff review

d77aee6

buck54321 force-pushed the reputation-v2 branch from e4860c6 to d77aee6 Compare October 3, 2023 19:14

buck54321 merged commit a407baa into decred:master Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: v2 reputation and client bond refactor #2501

multi: v2 reputation and client bond refactor #2501

buck54321 commented Sep 5, 2023 •

edited

Loading

martonp left a comment

martonp Sep 10, 2023

buck54321 Sep 12, 2023

martonp Sep 10, 2023

martonp Sep 10, 2023

buck54321 Sep 11, 2023

martonp Sep 13, 2023

martonp Sep 13, 2023

martonp Sep 13, 2023

martonp Sep 13, 2023

martonp Sep 13, 2023

martonp commented Sep 13, 2023 •

edited

Loading

JoeGruffins commented Sep 18, 2023

JoeGruffins left a comment

JoeGruffins Sep 18, 2023

JoeGruffins Sep 18, 2023

JoeGruffins Sep 18, 2023

buck54321 Sep 25, 2023

JoeGruffins Sep 18, 2023

JoeGruffins Sep 18, 2023

JoeGruffins Sep 18, 2023

		// DRAFT NOTE: This was wrong? Isn't account suspended at tier 0?
		// cannotTrade := dc.acct.effectiveTier < 0 \|\| (dc.acct.effectiveTier == 0 && dc.apiVersion() < serverdex.BondAPIVersion)

		// Using <= though acknowledging that this ignore the possibility that
		// an existing revoked bond could be ressurected.

	// postbond. This means if ther server doubles the bond size for an
	// postbond. This means if the server doubles the bond size for an

		// TODO: notify sub consumers e.g. frontend
		c.notify(newReputationNote(dc.acct.host, rep))

	// BondedTier is the tier indicated by the users active bonds. BondedTier
	// BondedTier is the tier indicated by the user's active bonds. BondedTier

multi: v2 reputation and client bond refactor #2501

multi: v2 reputation and client bond refactor #2501

Conversation

buck54321 commented Sep 5, 2023 • edited Loading

Server

Client

martonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Sep 13, 2023 • edited Loading

JoeGruffins commented Sep 18, 2023

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 commented Sep 5, 2023 •

edited

Loading

martonp commented Sep 13, 2023 •

edited

Loading