client: bond rotation - #2036
Conversation
780c299 to
1ebb919
Compare
|
I have tested at length on testnet, and have ironed out most edges that pop out with the unreliable dextest hidden service. Making ready for review now without the reserves since (a) this is a good deal to review on it's own, (b) we need these API changes for #2025, and (c) I'm wavering on the reserves solution and need more time, see below. We can review reserves in a second PR. I have experimented with two different approaches, each with their own pros/cons:
I really want the virtual reserves to work because the extra transaction creation of UTXO-based reserves has on-chain overhead and requires maintenance as fees eat into this reserve, whereas the virtual reserves is just an amount that is continually respected when funding transactions or orders. Personally I think we want a "softer" kind of reserves like the virtual balance based-approach, although it has more edges. |
| // GapPolicyWrap. The dcrwallet user should set --gaplimit= as needed to prevent | ||
| // address reused depending on their needs. Part of the Wallet interface. | ||
| func (w *rpcWallet) ExternalAddress(ctx context.Context, acctName string) (stdaddr.Address, error) { | ||
| addr, err := w.rpcClient.GetNewAddressGapPolicy(ctx, acctName, dcrwallet.GapPolicyWrap) |
There was a problem hiding this comment.
As the revised comment states, with an external dcrwallet, the user can pick their --gaplimit as desired. Using ignore with the RPC has created difficult recover situations. The InternalAddress method already uses wrap implicitly (getrawchangeaddress does not even expose gap policy).
Separate commit.
There was a problem hiding this comment.
Do you know if addresses that are known to be used are skipped when using GapPolicyWrap?
There was a problem hiding this comment.
When an address is used, the next (unused) address index becomes the address to which wrapping returns. The wrap algo just ensures the gap limit isn't exceeded.
| // InternalAddress returns an internal address using GapPolicyIgnore. | ||
| // Part of the Wallet interface. | ||
| func (w *spvWallet) InternalAddress(ctx context.Context, _ string) (stdaddr.Address, error) { | ||
| return w.NewInternalAddress(ctx, w.acctNum, wallet.WithGapPolicyIgnore()) |
There was a problem hiding this comment.
For the SPV wallet, match the external address method here. Plus I think we have 250 as the gap limit for our native wallet, although I haven't verified that. Native wallet recover with gap limit violations is much harder than external where the user just adds CLI switches, so let's avoid that at all costs.
| // Here we may either refund or renew the bond depending on target | ||
| // tier and timing. Direct renewal (refund and post in one) is only | ||
| // useful if there is insufficient reserves or the client had been | ||
| // stopped for a while. Normally, a bond becoming spendable will not | ||
| // coincide with the need to post bond. | ||
| // | ||
| // TODO: if mustPost > 0 { wallet.RenewBond(...) } |
There was a problem hiding this comment.
This seemed like a logical thing to do in planning, but in practice it is never timed right. I believe it is best to refund the bonds asap into regular wallet addresses rather than allowing to let them sit in a spendable state until a new bond needs to post.
There was a problem hiding this comment.
Is it possible to spend multiple expired bonds in one renewal, rather than renew individually?
There was a problem hiding this comment.
It would be possible yes. Timing would be even less likely in practice though. Putting this PR into work on testnet where the lock times are much longer helps get a feel for more real life cadences where there's a fair amount of bond overlap and very little coincidence of <it's time to post> with <it's time to refund>. The only way to make that happen more readily is to simply not refund until <it's time to post>. I'm reluctant to leave funds locked in bonds longer than needed, but I'm considering it.
Working through some concepts that will help decide about this in a bond-reserves branch for a PR very soon.
| // TODO: subject, detail := c.formatDetails(...) | ||
| details := fmt.Sprintf("Bond %v for %v refunded in %v (%s)", bondIDStr, dc.acct.host, | ||
| coinIDString(assetID, refundCoinID), unbip(bond.AssetID)) | ||
| c.notify(newBondRefundNote(TopicBondRefunded, string(TopicBondRefunded), |
There was a problem hiding this comment.
i18n of the notification texts will happen after we've reviewed them and we are sure of their content and need to exist.
| if match.MetaData.Proof.SelfRevoked { | ||
| return false, false // already self-revoked | ||
| } | ||
| // Here we can check to see if this is a redeem we failed to record... |
There was a problem hiding this comment.
This comment pertains to #2049 (comment)
It's unrelated to bonds, but it snuck in.
martonp
left a comment
There was a problem hiding this comment.
Looks good, haven't tested though.
This will be useful for bot inventory management as well. |
| // pendingBuffer gives the duration in seconds prior to reaching bond expiry | ||
| // (account, not lockTime) after which a new bond should be posted to avoid | ||
| // account tier falling below target while the replacement bond is pending. The | ||
| // network is a parameter because expected block times are network-dependent, | ||
| // and we require short bond lifetimes on simnet. | ||
| func pendingBuffer(net dex.Network) int64 { |
There was a problem hiding this comment.
Is this to say, the estimated duration in seconds from broadcasting a bond to reaching the required confirmations? This would ideally be a function of the number of required confirmations but with the values I see here, we would generally be fine without accounting for the number of req. confs because of the very large values I see here.
If this is the case, I find the following name and description easier to digest:
// estimatedSecsToBondConf is the estimated number of seconds from broadcasting
// a bond till the bond gets the required number of confirmations and is
// accepted by the server. Typically, new bonds will stay in an inactive state
// for this long before becoming active, so new bonds must be broadcast at least
// this amount of time prior to the expiry of an existing bond to avoid account
// tier temporarily falling below target.
There was a problem hiding this comment.
Closely related concepts, but this is a high buffer based on what you've described as estimatedSecsToBondConf, where a very large percentage of real life time-to-confirm would be covered by pendingBuffer. I will allude to that in a revision to the comment.
| // Here we may either refund or renew the bond depending on target | ||
| // tier and timing. Direct renewal (refund and post in one) is only | ||
| // useful if there is insufficient reserves or the client had been | ||
| // stopped for a while. Normally, a bond becoming spendable will not | ||
| // coincide with the need to post bond. | ||
| // | ||
| // TODO: if mustPost > 0 { wallet.RenewBond(...) } |
There was a problem hiding this comment.
Is it possible to spend multiple expired bonds in one renewal, rather than renew individually?
| if int64(bond.LockTime) <= replaceThresh { | ||
| weak = append(weak, bond) // but still live | ||
| } |
There was a problem hiding this comment.
The bond may be pending. Does that require special handling?
There was a problem hiding this comment.
Good question. For purposes of mustPost computation, I believe this comes out correctly:
tierDeficit := int64(targetTier) - tier
mustPost := tierDeficit + weakStrength - pendingStrengthBy putting that pending bond in the weak bucket also (note it falls through this if and stays in pendingBonds too), we negate the pending bond's offset to mustPost, which I believe is correct.
Will consider that more however.
26919ed to
d03b9e0
Compare
| // GapPolicyWrap. The dcrwallet user should set --gaplimit= as needed to prevent | ||
| // address reused depending on their needs. Part of the Wallet interface. | ||
| func (w *rpcWallet) ExternalAddress(ctx context.Context, acctName string) (stdaddr.Address, error) { | ||
| addr, err := w.rpcClient.GetNewAddressGapPolicy(ctx, acctName, dcrwallet.GapPolicyWrap) |
There was a problem hiding this comment.
Do you know if addresses that are known to be used are skipped when using GapPolicyWrap?
| // pendingBuffer gives the duration in seconds prior to reaching bond expiry | ||
| // (account, not lockTime) after which a new bond should be posted to avoid | ||
| // account tier falling below target while the replacement bond is pending. The | ||
| // network is a parameter because expected block times are network-dependent, | ||
| // and we require short bond lifetimes on simnet. |
There was a problem hiding this comment.
Instead of a universal approximation, why not have the Bonder specify generous estimates for the asset-specific variablesp and m and use 2p + m (I think) from above?
To clarify, pendingBuffer is not an approximation for pending (p) = t1 - t0, it's t2 - t2'?
There was a problem hiding this comment.
Instead of a universal approximation, why not have the
Bonderspecify generous estimates for the asset-specific variablespandmand use2p + m(I think) from above?
Right, I've alluded to this idea in the docs for spendableDelay below.
// .... NOTE: to minimize bond overlap, an asset.Bonder method could provide
// this estimate, but it is still very short relative to the entire bond
// lifetime, which is on the order of months.
I'm just convincing myself that it would matter enough to justify the API bloat and additional code. Not yet seeing it, but may yet do this when the reserves is working right.
To clarify, pendingBuffer is not an approximation for pending (p) = t1 - t0, it's t2 - t2'?
That's correct. This pertains to the second "tldr" bullet from the giant block comment above:
// tldr:
// - when creating a bond, set lockTime = 2*(BondExpiry+pendingBuffer)+spendableDelay
// - create a replacement bond at lockTime-BondExpiry-pendingBuffer
| return nil, newError(acctKeyErr, "acct locked %s (login first)", form.Addr) | ||
| } | ||
| if form.MaintainTier != nil || form.MaxBondedAmt != nil { | ||
| return nil, fmt.Errorf("maintain tier and max bonded amount may only be set when registering") |
There was a problem hiding this comment.
Lemme see if I've got this right. Most existing accounts will be maintained by rotateBonds. Legacy accounts will start with target tier and max-bonded amount of 0. So running PostBond with an existing account is a sort of "manual mode" for forcing a bond payment that will not be maintained by rotateBonds, since the targetTier is not updated. For graphical users, none of this matters, since once they have an account we would show options to update maintenance configuration, not to "register" = post first bond. But this does enable RPC users to manually and temporarily increase their tier. Sound right?
There was a problem hiding this comment.
Correct. This path won't apply to UI users, where we'll either give an appropriate registration dialog with options to maintain tier (if a new account), or we'll have the bond options form where they can start/end maintenance to given levels.
There was a problem hiding this comment.
Related comment: https://github.com/decred/dcrdex/pull/2036/files/d03b9e095393c6ae1d8692b0b4f55a072b4808c1#r1071503810
BTW, I'm sorry reserves is taking so long to put up. A lot of things relating to target tier and max bonded amt should become clearer.
| return nil, fmt.Errorf("maintain tier and max bonded amount may only be set when registering") | ||
| } | ||
| } else { | ||
| maxBondedAmt := 4 * form.Bond // default |
There was a problem hiding this comment.
4 could be a const. I'm not quite clear on the purpose of MaxBondedAmt though. I guess so that a user can temporarily increase their tier without maintaining that level, and MaxBondedAmt is a sort of safety feature?
There was a problem hiding this comment.
MaxBondedAmtis a sort of safety feature?
Correct. Client posts bond to hit target tier, but it's server saying what tier you are, so if server keeps reporting low tier (either because penalization or malice), you'll keep posting bonds without limit.
EDIT: BTW I think this really heightens the need for penalty info in the connect/auth response, and possibly other routes to check.
| @@ -170,7 +173,10 @@ type PostBondForm struct { | |||
| Asset *uint32 `json:"assetID,omitempty"` // do not default to 0 | |||
| Bond uint64 `json:"bond"` | |||
There was a problem hiding this comment.
I kinda wish this was Tier uint64 instead of Bond uint64. I suppose using a bond amount allows us to do some validation that might catch user errors, but specifying (tier, asset) seems easier. The UI will use tier as an input (e.g. bondStrengthField in #2025), and it would be better via RPC too, but easier to mess up.
There was a problem hiding this comment.
This is where the bondopts RPC (and UpdateBondOptions method) would apply. As noted in your previous comment, postbond should really just be used to register. So, similar to the legacy reg fee, you should specify an amount to be explicit about just how much you're going to lock up (IMO), otherwise you're kinda crossing your fingers that you have the right understanding of how a tier you specify will translate to the bond you post, and that pertains to server config.
There was a problem hiding this comment.
It looks like the new rotateBonds method will use the server's config value as is. Should it check back with this original value? (Maybe it does and I missed it.)
There was a problem hiding this comment.
It looks like the new
rotateBondsmethod will use the server's config value as is. Should it check back with this original value? (Maybe it does and I missed it.)
Only indirectly via the MaxBondedAmt set, but I think I've broken this just recently.
| MakeBondTx(ver uint16, amt, feeRate uint64, lockTime time.Time, privKey *secp256k1.PrivateKey, acctID []byte) (*Bond, error) | ||
| // RefundBond will refund the bond given the full bond output details and | ||
| // private key to spend it. | ||
| RefundBond(ctx context.Context, ver uint16, coinID, script []byte, amt uint64, privKey *secp256k1.PrivateKey) ([]byte, error) |
There was a problem hiding this comment.
Oh, I should mention that I've made RefundBond actually broadcast it internally in bond-reserves, rather than return the raw tx that gets send by core. I am positive that was suggested in the previous PR and I just neglected to apply the change. That pattern only needs to exist for MakeBondTx
|
|
||
| lockTime := time.Now().Add(2 * bondValidity).Truncate(time.Second) // default lockTime is double | ||
| bondValidity := minBondLifetime(c.net, int64(bondExpiry)) | ||
| lockTime := time.Now().Add(bondValidity).Truncate(time.Second) |
There was a problem hiding this comment.
Few lines below this, expireTime is wrongly set to time.Now().Add(bondValidity). And I think that should be earliestServerExpiry := time.Now().Add(bondExpiry)?
Or... if it's the actual time the server will expire the new bond, then it should be?
expireTime := locktime.Add(-bondExpiry)?
There was a problem hiding this comment.
Good catch. This minBondLifetime refactoring was incomplete here in PostBond.
There was a problem hiding this comment.
JoeGruffins
left a comment
There was a problem hiding this comment.
The answer to this is probably obvious but I'm not sure so, what is the point of the expiry? Why can't the bond be good up until locktime?
Say there's 1 minute until they can reclaim their bond, there's virtually no incentive to play nice (anymore). Now, if our bonds were designed to be confiscated on violations, that would be different, however that's a bit out of the question, so we just invalidate them (indirectly via an account's tier) |
Me too |
Haha. It's working! Just hard to make it pretty and clean. Getting really close. Testing edges and polishing the rough bits. |
buck54321
left a comment
There was a problem hiding this comment.
Looking good.
Feel free to take anything you find useful from buck54321/dcrdex@01a3562...buck54321:dcrdex:rotate-test
| NoteTypeBondPost = "bondpost" | ||
| NoteTypeBondRefund = "bondrefund" |
There was a problem hiding this comment.
Better with 2 note types rather than a single NoteTypeBondUpdate?
| @@ -704,7 +710,10 @@ type dexAccount struct { | |||
| bonds []*db.Bond // confirmed, and not yet expired | |||
| expiredBonds []*db.Bond // expired and needing refund | |||
| tier int64 // check instead of isSuspended | |||
There was a problem hiding this comment.
Shouldn't tier be a function that reads the bonds to determine the value?
There was a problem hiding this comment.
tier is the last known tier that the server/network says we are. This was established previously, but it will become much more clear in reserves work I'm prepping to make public.
There's a concept of bondedTier, which would be what you are describing - a function of known bonds and their statuses. Then there is effective tier, which is what server says we are. Both of these concepts come into play, but this value is the deciding factor as it relates to how we expect server to interact with us.
On a related note, we have future work to do with communicating more penalty info, at least in the form of a single "tier adjustment" integer for this, but ideally with more description of the violations as server sees them.
| dc.acct.expiredBonds = append(dc.acct.expiredBonds, bond) | ||
| c.log.Warnf("Expired bond found: %v", coinIDString(bond.AssetID, bond.CoinID)) | ||
| c.log.Infof("Newly expired bond found: %v (%s)", coinIDString(bond.AssetID, bond.CoinID), unbip(bond.AssetID)) |
There was a problem hiding this comment.
This directly expired bond is not considered in the calculation of tierDeficit below. Partially related to my other comment on acct.tier being an int filed rather than a function.
There was a problem hiding this comment.
Right, I hope this will become much more clear soon, but you get a bondexpired notification shortly after (although possibly before if clocks are set badly), which informs us of the server's understanding of our account tier. Similarly, the tierchange route is used when penalization change the account tier. And for reconnect handling, the auth/connect route also reports tier. Collectively, we never miss a beat in terms of our effective tier (what server thinks our tier is).
There was a problem hiding this comment.
As an example:
2023-01-29 19:10:13.582 [INF] CORE: Newly expired bond found: 90287fa1e27ec54c03a94389c7bf22fe63eb62b0e408ef2cbf8078a0c5cc945e:0 (dcr)
2023-01-29 19:10:13.582 [DBG] CORE: Expired bond 90287fa1e27ec54c03a94389c7bf22fe63eb62b0e408ef2cbf8078a0c5cc945e:0 (dcr) refundable in about 3m55s.
2023-01-29 19:10:15.599 [INF] CORE: Received bondexpired notification from 127.0.0.1:17273 for account ad5afeb6189a56487703568485ea26a8db4e11ebbbe1d32dccddcff9bf8b5812...
2023-01-29 19:10:15.600 [INF] CORE: notify: |SUCCESS| (bondpost) BondExpired - New tier = 2 (target = 2).
2023-01-29 19:10:25.797 [DBG] CORE[dcr]: tip change: 13924 (5c764064241eee42fd95e2f450fe129cccd5d5e69902411b09b0fa1e56e3093b) => 13925 (4fcef9f65c942496e24ca8e3e02d21ea10dd417af31ab5406f30043ef2820935)
2023-01-29 19:10:25.797 [TRC] CORE: Processing tip change for dcr
2023-01-29 19:10:25.797 [TRC] CORE: asset 42: total = 4000000000, active = 2000000000
2023-01-29 19:10:33.582 [DBG] CORE: Expired bond 90287fa1e27ec54c03a94389c7bf22fe63eb62b0e408ef2cbf8078a0c5cc945e:0 (dcr) refundable in about 3m35s.
Sorry for all the typos in my last commment.
There was a problem hiding this comment.
Ah. So since the server wouldn't consider the expired bond as part of its ConnectResult.Tier, but would still consider the weak bond, we're okay to trust acct.tier below in our tierDeficit. Do we ever validate that the server is reporting the tier that we expect?
There was a problem hiding this comment.
Do we ever validate that the server is reporting the tier that we expect?
For this, I think the missing piece is the violation reporting. Client software always just does it's best, and generally it's not tracking it's own known violations (e.g. whoops I was offline for a while and I'm pretty sure I messed up on swap X, therefore I expect a -1 tier adjustment). Not sure we'll ever really do that latter thing, but server's comms should certainly list matches and a specific state at which your account failed.
So to have any hope of functioning, we have to be concerned with how server has classified our account. A related concept is MaxBondedAmt, because we'll start posting bond as needed (when maintenance is enabled) to keep a target tier.
I suppose ideally we'd have something in client that considers what we believe to be our own conduct state. That's not unique to bonds though. With the legacy fee, we just got penalized and client had no judgement on the correctness of that, it was up to the user. I expect we'll evolve this.
There was a problem hiding this comment.
Posting bond over rpc doesn't give me back a coin ID:
$ ./dexcctl -p abc --simnet postbond 127.0.0.1:17273 1000000000 42
{
"bondID": "",
"reqConfirms": 0
}
edit: Oh it's working now. Not sure what was happening actually. Will try to make it happen again though.
On a side note creating an dcr SPV wallet over rpc doesn't seem to be working for some reason. I'm sure unrelated.
| // account tier falling below target while the replacement bond is pending. The | ||
| // network is a parameter because expected block times are network-dependent, | ||
| // and we require short bond lifetimes on simnet. | ||
| func pendingBuffer(net dex.Network) int64 { |
There was a problem hiding this comment.
This should be asset specific?
There was a problem hiding this comment.
See previous convo at #2036 (comment)
I'm still on the fence here, but @chappjc makes a good point. On the timelines we're talking about, we could probably get by without asset-specific values.
There was a problem hiding this comment.
I'm still on the fence here, but @chappjc makes a good point. On the timelines we're talking about, we could probably get by without asset-specific values.
Ok, I'll just do it. Doesn't seem complex, and if we get there with ETH, the spendableDelay is likely to be zero (assuming we have a custom contract just for time-locking funds), so that will be helpful.
There was a problem hiding this comment.
I had started on this Friday but moved on to something else because it was a PITA because I have to get the xcWallets in different spots. Can work, just requires a bunch of annoying refactoring.
| // a bond to become spendable after reaching lockTime. This depends on consensus | ||
| // rules and block times for an asset. For some assets this could be zero, while |
There was a problem hiding this comment.
This should also be asset specific?
| // Retry postbond for pending bonds that may have failed during | ||
| // submission after their block waiters triggered. |
There was a problem hiding this comment.
Are these expected to fail because of network conditions?
There was a problem hiding this comment.
Yes. Been running on the testnet hidden service and it happens a lot
| // For the max bonded limit, we'll normalize all bonds to the | ||
| // currently selected bond asset. |
There was a problem hiding this comment.
I would think it's easier to just have max tier and the max bonded limit be whatever the price of one tier for that asset times the max tier. And I guess user would set max tier with knowledge of what the tier prices are per asset.
I thought maybe the point was to allow for half-bond amounts but the line below amt = bondAsset.Amt * uint64(toPost) suggests that we are only dealing with whole bonds.
Another thought this brings to mind is, if bond amount is changed, ideally, the user is not punished. If a full bond was bought yesterday, and the bond price goes up today, that user should not need to buy more bonds for the same tier, imo, which I believe happens here.
There was a problem hiding this comment.
The idea of max bonded is to allow overcoming penalization, otherwise your account can get suspended (go sub zero tier) and it won't post any additional bond to get into a trading tier.
There was a problem hiding this comment.
Another thought this brings to mind is, if bond amount is changed, ideally, the user is not punished. If a full bond was bought yesterday, and the bond price goes up today, that user should not need to buy more bonds for the same tier, imo, which I believe happens here.
We can't address this easily. Server's gonna apply the new bond amount to existing bonds when user authorizes.
| @@ -170,7 +173,10 @@ type PostBondForm struct { | |||
| Asset *uint32 `json:"assetID,omitempty"` // do not default to 0 | |||
| Bond uint64 `json:"bond"` | |||
There was a problem hiding this comment.
It looks like the new rotateBonds method will use the server's config value as is. Should it check back with this original value? (Maybe it does and I missed it.)
| }`, | ||
| }, | ||
| bondOptionsRoute: { | ||
| argsShort: `"addr" targetTier (maxBondedAmt bondAssetID)`, |
There was a problem hiding this comment.
Not a problem with this pr, but the dex address args are all over the place from "addr" to "dex" to "host"
I think you're hitting this path: if paid {
success = true
// The listen goroutine is already running, now track the conn.
c.connMtx.Lock()
c.conns[dc.acct.host] = dc
c.connMtx.Unlock()
return &PostBondResult{ /* no new bond */ }, nil
}That would be existing account discovery. Perhaps the RPC can just say ok in this case. Not sure how else to do this one. |
7522f43 to
4303193
Compare
first tx we are trying to double spend |
|
OK, so another bond. Will investigate shortly. Not sure how that happened, but it feels like a coin locking issue. |
|
You know, maybe I deleted the db, and this is from a new node created from the same seed that apparently did not find the bond tx from before. May have still been in mempool... |
|
|
Starting back up dexc logs... ... ... So, in this very weird circumstance that would be difficult to accomplish on mainnet, I'm kinda stuck with the nonexistent bond. |
|
Oh, I lied, just needed to progress the chain: |
|
I posted a new bond and am now able to trade, so the problem was resolved eventually. |
|
I guess I should have mentioned, that with simnet and the locktimes and fast rates, you really need |
|
Had it happen again, this time just running normally. May be a result of fast simnet things... dexc: dcrd: |
|
And again, with the same It looks like I'm in a perpetual state of orphaned transactions. It's probably an SPV thing and not a problem with bonds. |
|
I don't know what to make of "orphan transaction" from dcrd. Are the simnet
nodes reporting reorgs?
What kind of dcr wallet is dexc using? Native or RPC? I actually haven't
tried external dcrwallet.
…On Tue, Jan 31, 2023, 12:05 AM JoeGruffins ***@***.***> wrote:
And again, with the same orphan transaction in dcrd.
—
Reply to this email directly, view it on GitHub
<#2036 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACHQOSIXUXHKW3LEYX436ADWVCTSXANCNFSM6AAAAAATW3NCYE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
no
internal wallet I bet it is trying to spend that original |
|
Deleting the db and recreating from seed seems to have stopped the bad transactions. I'm pretty sure it was that initial bad tx, the change being "spent". |
d1aeaff to
f3468d8
Compare
| } | ||
| dc.cfgMtx.RUnlock() | ||
| replaceThresh := lockTimeThresh + pendingBuffer(c.net) // replace before expiry to avoid tier drop |
There was a problem hiding this comment.
Should we really keep going if cfg == nil?
There was a problem hiding this comment.
Nah, we just have to be at this spot to process refunds. I'll double check that all the logic below excludes never connected dex (cfg == nil)
There was a problem hiding this comment.
lockTimeThresh and replaceThresh would be too early, which I guess means bonds wouldn't be expired or replaced when maybe they should be, so limited damage, but I'm not feeling great about running filterExpiredBonds with bad numbers.
There was a problem hiding this comment.
which I guess means bonds wouldn't be expired or replaced when maybe they should be, so limited damage
Right, this is totally intended though. In the absence of expiry info, don't move bonds into the expired bucket until we actually reach locktime. A few lines up this is doc'd:
lockTimeThresh := now // in case dex is down, expire (to refund when lock time is passed)replaceThresh only affects creation of the weak slice, which is purely local to this method, no fields of dc.acct modified in doing that. And if all important guards on the posting branch block any action:
if mustPost > 0 && targetTier > 0 && bondExpiry > 0 {
c.log.Infof("Gotta post %d bond increments now. Target tier %d, current tier %d (%d weak, %d pending)",
mustPost, targetTier, tier, weakStrength, pendingStrength)
if !unlocked || dc.status() != comms.Connected {
c.log.Warnf("Unable to post the required bond while disconnected or logged out.")
continue
}Here the absence of a cfg is implied by both of bondExpiry and unlocked, with dc.status being strongly correlated.
Happy to introduce a new flag to be extra safe though.
| priv := secp256k1.PrivKeyFromBytes(bond.PrivKey) | ||
| // c.bondKeyIdx(bond.AssetID, bond.KeyIndex) // TODO, with KeyIndex in DB instead of PrivKey |
There was a problem hiding this comment.
grr dang I forgot about this KeyIndex
We'll have to toast our DBs unless I do it now.
There was a problem hiding this comment.
I mean we'll have to toast them anyway if we were using this PR, but I'd hate to put a bad DB format on master. Sec...
There was a problem hiding this comment.
Ugh, the db.Bond structure on master has BondPrivKey.
I'll put some minimal handling for that interim struct version so we don't have to nuke our testnet and mainnet DBs that we may have used on master. Shame to have perpetual edge case code for unreleased revision handling... let's see how it looks.
| // account tier falling below target while the replacement bond is pending. The | ||
| // network is a parameter because expected block times are network-dependent, | ||
| // and we require short bond lifetimes on simnet. | ||
| func pendingBuffer(net dex.Network) int64 { |
| if form.BondAsset != nil { | ||
| bondAssetID := *form.BondAsset | ||
| wallet, found := c.wallet(bondAssetID) | ||
| if !found || !wallet.connected() { |
There was a problem hiding this comment.
Perhaps we could require a password for UpdateBondOptions and try to connect.
| _, err = wallet.refreshUnlock() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("bond asset wallet %v is locked", unbip(bondAssetID)) | ||
| } |
There was a problem hiding this comment.
This makes me think we should have a password here too.
With sub-second timing, it was possible for the connect response to indicate a tier that was not reflected by the active bonds list. This resolves the issue by building the bonds slice for the response after double checking bond expiry and computing tier.
8f0e289 to
7ed5b58
Compare
- Bond lifetime docs and diagrams - Core.bondXPriv field, set on login - add dexAccount.targetTier field (and PostBondForm.TargetTier) - rotateBonds posts bond as needed - Add (*Core).UpdateBondOptions
7ed5b58 to
a399d74
Compare
|
Added a wallet synced gate because I believe I was able to (automatically) post a bond tx that double spent coins: ca9914c (now squashed) For roughly 6 days I had been using a different copy of the data dir, including the DCR native wallet, to work with ETH/ERC20 swaps, and when I came back to this old data dir, it almost immediately tried to post new bond to hit target tier before the wallet had synced. The DCR wallet happily funded the bond txns with spent coins because it was still scanning blocks. Also added rebroadcasting on certain unusual paths to |
Putting up this draft PR before it's ready to help inform #2025. This draft PR includes the main bond lifecycle machinery for the client. I've not pushed a commit for the reserves, except for a comment in client/asset/interface.go.
EDIT: We can review reserves in a second PR. I have experimented with two different approaches, each with their own pros/cons:
I really want the virtual reserves to work because the extra transaction creation of UTXO-based reserves has on-chain overhead and requires maintenance as fees eat into this reserve, whereas the virtual reserves is just an amount that is continually respected when funding transactions or orders. Personally I think we want a "softer" kind of reserves like the virtual balance based-approach, although it has more edges.
The
client/coreform types: