-
Notifications
You must be signed in to change notification settings - Fork 92
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
fidelity bonds foundation: comms, account tiering, server tracking #1819
Conversation
21e15de
to
3398666
Compare
3398666
to
72603a9
Compare
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.
partial review
dex/msgjson/types.go
Outdated
// restarts and finds bonds recorded in their DB that is flagged as neither | ||
// accepted nor expired (also maybe if not listed in the 'connect' response). |
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.
Last two lines are difficult to understand.
dex/msgjson/types.go
Outdated
// PostBond should include the unsigned bond transaction for validation prior to | ||
// broadcasting the signed transaction so the client hasn't needlessly locked | ||
// funds if the transaction is rejected. |
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.
If the transaction is rejected then the funds wouldn't be locked. Server should check so that the bond is not rejected by the server?
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.
If the transaction is rejected then the funds wouldn't be locked
The comment means if there was no unsigned txn being checked first. We don't want a published+signed txn rejected.
Server should check so that the bond is not rejected by the server?
Server first validates the transaction structure, bond script, account ID, etc. with the unsigned bond txns, then client submits it again with the confidence that it's good.
Also as you noted elsewhere, this initial comms round can be used to agree on other bond parameters. But at present, the client is free to skip it.
dex/msgjson/types.go
Outdated
CoinID Bytes `json:"coinid"` | ||
AssetID uint32 `json:"assetid"` |
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.
nit: Later you capitalize the ID part of the json tags.
Offline convo with @JoeGruffins @buck54321 and @martonp about some possible bond output script changes.
Ref for both points above:
|
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.
... relevant data pushes (lock time and pubkey[hash]) ...
And the bond version I suppose.
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.
For discussion: I think we should drop the pre-validation step. I don't think it's necessary or amenable to mesh. The server version should advertise all we need to know about the bond output structure needed/supported, and all members of the network should be expected to adhere to the protocol without help from a central authority.
Is there some instance where the server would accept a config
request, advertise their version, but not accept a new properly constructed bond?
It's true that it's not necessary, and in fact if the client skips it, server doesn't know or care. It's also correct that the config response must and does provide all that is needed to form the proper bond. However, I don't think mesh enters into this at all. The "central authority" you refer to is the same one where we get the config response, so I don't see anything particularly problematic in mesh with a courtesy pre-check. This is just a way of ensuring there's not something plainly incorrect, and it should be accepted. If the real thing is rejected, it would have been rejected on pre-validation, so it's just prudent to do so afaict.
Should not be. But it's not hard to imagine reasons why it could happen in reality. These include stale config responses on the client end, bugs on client and or server with serving or interpreting the config, bugs in bond creation or server-side parsing, etc. And although we're versioning the bond formats, something subtle could change that makes a new bond incompatible. But, we can certainly remove this unsigned bond transaction check and it wouldn't be a problem. I would just be more comfortable with it being done before locking up considerable amounts for a duration on the order of months. Related: #1819 (comment) |
72603a9
to
1b81cbb
Compare
e0e888a
to
7dc0f39
Compare
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.
Finally got all the way through. Sorry for taking so long. First pass with some initial comments. More to come.
// APIVersion is the server's communications API version, but we may | ||
// consider APIVersions []uint16, with versioned routes e.g. "initV2". | ||
// APIVersions []uint16 `json:"apivers"` | ||
APIVersion uint16 `json:"apiver"` |
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.
Can we do APIVersions
now, and just never increment APIVersion
.
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.
I don't have a clear picture of how this would work. Let's address it subsequently, or in parallel if you see how it would play out now.
dex/msgjson/types.go
Outdated
// Client should send bond info when their bond tx is fully-confirmed. Server | ||
// should start waiting for required confs when it receives the 'postbond' | ||
// request if the txn is found. Client is responsible for submitting 'postbond' | ||
// for their bond txns when they reach required confs, or when the client | ||
// restarts and finds bonds recorded in their DB that is flagged as neither | ||
// accepted nor expired (also maybe if not listed in the 'connect' response). |
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.
The last part seems like client implementation details.
The or
also seems to suggest that a client who restarts with bonds pending doesn't need to wait for confirmations to send the postbond.
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.
Revised the comment, but note that this block comment is not part of the exported API docs. It is intended to inform a developer.
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.
Some thoughts on the API.
apiVer := int32(cfg.APIVersion) | ||
if apiVer == 0 && cfg.BondExpiry > 0 { | ||
apiVer = 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.
Maybe we should move to a semver scheme or maybe a bitmask.
type APISupportFlag uint64
const (
APISupportV0 APISupportFlag = 1 << iota
APISupportBondsV0
)
type ConfigResult struct {
APIVersion uint64 `json:"apiver"`
...
}
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.
Still breaks v0 clients, but that's something to consider.
Briefly going to draft until #1818 goes in first. |
7dc0f39
to
69d212c
Compare
Back out of draft with #1818 merged. Not re-tested with the integ branch, and I still want to add some |
go func() { | ||
defer auth.wg.Done() | ||
t := time.NewTicker(20 * time.Second) | ||
defer t.Stop() | ||
|
||
for { | ||
select { | ||
case <-t.C: | ||
auth.checkBonds() | ||
case <-ctx.Done(): | ||
return | ||
} | ||
} | ||
}() |
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.
I wonder if it would be more efficient and accurate to prune the user's bonds at order time?
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.
Will look into pruning at order time, but issues I see there are (1) many orders in a narrow time period like an epoch and not checking too fast, and (2) we can't send tier change notifications closer to the time when their tier actually changes and they find out when trying to place an order, particularly important if a user sits online for long periods without making new orders, which is the typical user.
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.
Also worth noting that bond check/prune involves no nodes requests, just a loop through the bond checking expiry times, so it's lightning fast.
When I connect my dcrd node to the P2P network, I ask every node what version they're running (config). When I later go to send a transaction, I don't ask other nodes if the transaction looks okay first (pre-validation). |
So we can't do better? There's little cost for dex to do this, and it's handling for a very specific case. I don't want to support users locking up their funds for months without this. When something doesn't work as expected and if we didn't have a precheck, and people ask why we didn't just verify the transaction was acceptable to the server that would be accepting it before broadcasting it, I would have no good answer. |
In the Decred node network + tx broadcast analogy, consider the consequences of a node ultimately rejecting a transaction in the |
69d212c
to
4d6519e
Compare
Only rebased, no substantive changes. |
4d6519e
to
d9e2796
Compare
Just rebased and added some fixup commits, but not ready for re-review. Need to split the postbond route as per #1819 (comment) |
d9e2796
to
52807de
Compare
Squashed a number of fixups, including separating the pre-validate route from the postbond route. Fixups viewable separately here: https://github.com/chappjc/dcrdex/commits/bond-comms-auth-fixups |
52807de
to
2f024f8
Compare
Version uint16 `json:"version"` // latest version supported | ||
ID uint32 `json:"id"` | ||
Confs uint32 `json:"confs"` | ||
Amt uint64 `json:"amount"` // to be implied by bond version? |
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.
The comment is food for thought, but given variations in exchange rate, I think this has to be a variable.
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.
Seems like this might get complicated. How do we validate old bonds that had a lower Amt
? How do we handle the transitions from one Amt
to a higher Amt
? Do we need to shutdown bond registration for a while, or do we accept the old value for a while after the transition?
I was thinking about something like
type AssetBondRanges struct {
Overlap uint16 // number of blocks to allow old rate after bond rate change
Ranges [][2]uint64 // order list of [block_height, bond_value]
}
var BondRanges = map[uint64] /* asset ID */ map[uint64] /* version */ *AssetBondRanges{
42: {
0: &AssetBondRanges{
Overlap: 20,
Ranges: [][2]uint64{
{0, 5e8},
},
},
},
}
Changing the bond val would require a server upgrade. Not sure how it would translate to mesh, but at lease every node on the same version would have the same numbers.
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.
If we take mesh out of the picture, it could be addressed like the legacy registration fee. Specifically, the presence of the DB entry itself is enough validation for a particular known bond (there are entries for existing accounts with different amounts, but those accounts are still equally paid). The table would just need a column for the bond strength at the time it was accepted e.g. 1, 2, 3, ... instead of just the amount of the assets that we currently divide repeatedly by this Amt
value when computing total bond strength. Since bond confirmation is a non-zero duration, and there's no DB record until it's confirmed, it sounds like we would need some window where an old amt is accepted, like you said, unless we temporarily record the pre-validated bond coin ids and their amounts.
When the amt changes in the config
response, that's what new bonds require.
For mesh, I think we'd need a protocol for member nodes to change the bond amounts based on consensus. It can be an emergent value, whereby proposed values are communicated and operators naturally update their preferences and eventually (?) enough nodes agree and it becomes the new value. This obviously makes the config response more dynamic, and requires new notifications to users.
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.
Not the entire solution, but in the right direction: e80d4a7 (will test in a couple hours, so expect breakage there)
Still would need some kind of overlap handling like you said to avoid rejecting bonds that were confirming at the time of the Amt change.
// BondExpiry defines the duration of time remaining until lockTime below | ||
// which a bond is considered expired. As such, bonds should be created with | ||
// a considerably longer lockTime. NOTE: BondExpiry in the config response | ||
// is temporary, removed when APIVersion reaches BondAPIVersion and we have | ||
// codified the expiries for each network (main,test,sim). Until then, the | ||
// value will be considered variable, and we will communicate to the clients | ||
// what we expect at any given time. BondAsset.Amt may also become implied | ||
// by bond version. | ||
BondExpiry uint64 `json:"DEV_bondExpiry"` |
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.
Please note this BondExpiry comment. My feeling is we can settle on a value by network consensus, perhaps implied by the API version or something, but that's not practical while we figure this out. Testnet bond specs will very likely change frequently prior to release.
// time-locked fidelity bond transaction given the bond's P2SH redeem script. | ||
func (*Backend) ParseBondTx(ver uint16, rawTx []byte) (bondCoinID []byte, amt int64, bondAddr string, | ||
bondPubKeyHash []byte, lockTime int64, acct account.AccountID, err error) { | ||
return ParseBondTx(ver, rawTx) |
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.
Pure functions are more usable in general, but for server/dex, this is simply a convenience to avoid having to mess with a driver method or something like that.
2f024f8
to
51fa111
Compare
server/auth/auth_test.go
Outdated
// rather than a hard close operation. But the cancel ratio is stupid and | ||
// generally disabled in production anyway so maybe screw it. | ||
|
||
/* cancel ratio stuff we should probably just remove |
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.
CoinID Bytes `json:"coinid"` | ||
// For an account-based asset where there is a central bond contract implied | ||
// by Version, do we use AcctPubKey to lookup bonded amount, and CoinID to | ||
// wait for confs of this latest bond addition? |
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.
For eth, like swaps, we shouldn't put any trust in coin id confirmations. We would need to use an init block number like the swaps, in the bond contract. Separate bond entries may be best? The map keys could even be something simple like pubkey + bond number concatenated.
Re-tested with |
This adds new routes to support bonds: PostBondRoute ('postbond'), PreValidateBondRoute('prevalidatebond'), BondExpiredRoute ('bondexpired'), and TierChangeRoute('tierchange'). This also adds the new Bond type, and updates the ConnectResult type with the ActiveBonds slice. The ConnectResult now also has a Tier (*int64) and LegacyFeePaid (*bool) field. The Suspended field is deprecated. This also adds the new BondAsset type, which specifies an accepted bond in terms of the asset ID, bond version, required confirmations, and increment in units of the asset's atomic amount. The ConfigResult is updated with the BondAssets field, and a temporary BondExpiry field to dictate expected minimum bond lock time until it is established by consensus at BondAPIVersion. The reg fee fields are deprecated. The following payloads are added: - BondExpiredNotification, server-originating, when a bond expires - TierChangedNotification, server-originating, for any tier decrease, including as a result of conduct violation - PreValidateBond, client-originating, optional, with response type - PostBond, client-originating, for adding bond or creating a new account with a bond, with response type
This adds the necessary Archiver methods to create accounts with bonds, add new bonds to existing accounts, and remove expired bonds. This also removes the obsolete CloseAccount and RestoreAccount methods since there is no longer a binary ban/unban concept, instead a tier.
This updates AuthManager with handlers for the 'postbond' and 'prevalidatebond' comms routes. New accounts may be created via postbond, or existing accounts may have new bonds activated. The old registration route handlers are now deprecated. This updates the AuthManager and Market to use a new account tier system that is a balance of active bonds and conduct score ("strikes") instead of a hard account suspension after accumulating too many strikes. Active bonds are monitored, and when they reach their expiry, they are removed and the user's account tier is adjusted. 'tierchanged' notifications are sent on such events. The (*AuthManager).Suspended method is replaced with AcctStatus, which returns if the account is connected and current tier. This also removes the server/account/pki package, moving its few types and constants into the server/account package where they were used. server/market: The methods for processing new orders now use AcctStatus to check if an order may be accepted for a given account (tier>0).
This updates server's DCR backend with new Bond methods to support locating bond by coinID (txid) as well as parsing a raw bond transaction. This is required for the 'postbond' route, which is now distinct from 'prevalidatebond', which operates on the unsigned and unbroadcasted bond transaction.
This updates server/dex.DEX, the main DEX subsystem manager, to recognize bond assets specified in markets.json, and prepare the Backends as a Bonder interface that powers the AuthManager's bond validation needs in the bond comms handlers. This introduces a new server/dex.BondAPIVersion constant (1) that will succeed PreAPIVersion as the first non-zero API version. We will advance APIVersion to BondAPIVersion when the "V0PURGE" is actuated. That will be the first stable API version, beyond which any breaking changes will require a version bump. As suggested by reviewers, we should probably introduce a slice of versions to smooth out future version advances. This creates the dex.BondExpiry(dex.Network) helper for retrieving the bond expiry duration (in seconds) for a given network. This or the constants BondExpiry{Mainnet,Testnet,Simnet} should become the authoritative source for bond expiration durations for both client and server. At this stage however, only server should reference these compiled-in values, while the client should check the server's 'config' response and bondAssets map. This also removes the server's "anarchy" setting, which is a relic from early development.
This adapts Core to handle the updated msgjson types. In particular, recognizing the Tier and other bonds fields. Legacy and bond-enabled servers are both supported. The config response now interprets the presence of bond info to mean the effective API version is BondAPIVersion. Such servers continue to advertise v0 (PreAPIVersion) until we decide to purge the unstable API and force client updgrades.
c07d9d8
to
71411e1
Compare
Squashed and re-wrote all commit messages. Merging after CI completes. |
Further breaking up #1480 for feasible review, this piece deals with:
To make this transition smoother, the client and server retain all the legacy registration fee machinery, and the client's continues to use the legacy registration system.
Background from #1480:
This creates a time-locked fidelity bond, which is redeemable by the user who posted the bond after a certain time. This creates a opportunity cost to use DCRDEX instead of a simple monetary cost. It also is a prerequisite for mesh.
The DEX considers a user's bond as active when the bond script's
lockTime
(when it can be redeemed by the user) is at leastbondExpiry
in the future. That is, the bond remains locked for a period even after it becomes inactive for DEX purposes.This also introduces an account tier system. Previously, paying the registration fee would mark the account paid, and it could be closed permanently if the user's violations exceed a certain score (binary and permanent). Now, posting a bond increases a user's tier, which offsets violations and in future work will also scale user order limits. When a bond expires, their tier is reduced. To trade, a user must have a tier > 0.
Protocol
New
postbond
route replacesregister
, but with no fee address and instead a txn with time-locked output (P2SH) and a DEX account commitment output (OP_RETURN).postbond
need not be used only for new accounts. Existing accounts may top-up / supplement their bond.The
config
response should be consulted for bond requirements, including: expiry time, supported bond assets, amounts, and required confirmations.NOTE: the following payload and the prevalidation scheme have changed. See the code.
The user may submit an initial
postbond
prior to broadcasting the bond transaction request containing the raw unsigned transaction for validation. The bond script to which the P2SH bond output pays must also be provided in thepostbond
request for validation by the DEX (correct script structure and lock time). The payload:Server must use provided account pubkey to verify signed
postbond
request, otherwise the user could be spamming or putting up a bond for a bogus account or someone else's account. EDIT: ThisBondSig
will be removed in from this PR since the second output has the account ID and this verification of bond output ownership doesn't seem to matter.EDIT: May nuke
LegacyFeeRefundAddr
.Further validation is described in subsequent sections of this PR description.
The response payload:
After validation of the provided data, the DEX attempts to locate the bond transaction on the asset network:
PostBondResult
is sent immediately withConfs=-1
set. The bond is NOT stored in the DB. This was a courtesy pre-validation. The user should then broadcast the transaction, wait for the required number of confirmations, and sendpostbond
again. Note that assuming the lowest confs ever accepted will be 1, not unconfirmed, this works because it's virtually certain to have at least propagated to the server before it got mined, but we could also have a flag in the request indicating to just pre-validate and skip trying to locate and activate the bond.postbond
request should not be made until the client has observed the required confirmations, and the waiter is only needed for network latency.- Otherwise, a response is sent with the observed confirmation count, and a coin waiter is started to watch for the txn to reach the required number of confirmations. The bond is stored in DB as pending. When the transaction reaches the required confirmations, it is flagged active in the DB and abondconfirmed
notification is sent.When the client receives the initial successful response prior to broadcast, they should then broadcast the bond txn, wait for the required confirmations, and resubmit
postbond
.Server must record all known bond txns and their amounts and expiry times. There is no explicit invalidation of bonds due to conduct, and instead a user's tier is a balance between bond "strength" (a function of the active bond total amounts) and their conduct score.
TODOs
Refining the bond amount. It's pretty clear the bond amount has to be at least an order of magnitude more than the previous registration fee since you get it back, and it's only an opportunity cost having it locked up. I'm thinking like 5 DCR per tier (the bond increment).
Related to bond amount, the violation weights can use rebalancing. Specifically, increase weight for "no swap as taker" (currently 11, increase to 18 or 20) and "no redeem as maker" (currently 7, increase to 12 to 14), the violations that lock counterparty funds (20hr / 8 hr, respectively). Maybe even higher.
Make the tier system scale up order size limits. This was planned outside of fidelity bond work, in terms of conduct score, but it's more intuitive in terms of tier, which is a function of both account bond level and score.
Scale penalty (score change) with amount locked due to violation (e.g. lots/10 * violation score). This was planned regardless of fidelity bonds, but it dovetails nicely with the tier system.
See the project board at https://github.com/orgs/decred/projects/2.