-
Notifications
You must be signed in to change notification settings - Fork 91
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
client: bond rotation #2036
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if addresses that are known to be used are skipped when using GapPolicyWrap
?
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.
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.
@@ -546,7 +546,7 @@ func (w *spvWallet) ExternalAddress(ctx context.Context, _ string) (stdaddr.Addr | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to spend multiple expired bonds in one renewal, rather than renew individually?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18n of the notification texts will happen after we've reviewed them and we are sure of their content and need to exist.
@@ -1562,6 +1562,7 @@ func (t *trackedTrade) isRedeemable(ctx context.Context, match *matchTracker) (r | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment pertains to #2049 (comment)
It's unrelated to bonds, but it snuck in.
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.
Looks good, haven't tested though.
This will be useful for bot inventory management as well. |
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 test next.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to spend multiple expired bonds in one renewal, rather than renew individually?
client/core/bond.go
Outdated
if int64(bond.LockTime) <= replaceThresh { | ||
weak = append(weak, bond) // but still live | ||
} |
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 bond may be pending. Does that require special handling?
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.
Good question. For purposes of mustPost
computation, I believe this comes out correctly:
tierDeficit := int64(targetTier) - tier
mustPost := tierDeficit + weakStrength - pendingStrength
By 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a universal approximation, why not have the
Bonder
specify generous estimates for the asset-specific variablesp
andm
and 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
client/core/bond.go
Outdated
c.connMtx.RLock() | ||
dc, acctExists := c.conns[host] | ||
c.connMtx.RUnlock() | ||
if acctExists { | ||
if dc.acct.locked() { // require authDEX first to reconcile any existing bond statuses | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} else { | ||
maxBondedAmt := 4 * form.Bond // default |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxBondedAmt
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
Only indirectly via the MaxBondedAmt
set, but I think I've broken this just recently.
// MakeBondTx authors a DEX time-locked fidelity bond transaction for the | ||
// provided amount, lock time, and dex account ID. An explicit private key | ||
// type is used to guarantee it's not bytes from something else like a | ||
// public key. | ||
MakeBondTx(ver uint16, amt uint64, lockTime time.Time, privKey *secp256k1.PrivateKey, acctID []byte) (*Bond, error) | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
client/core/bond.go
Outdated
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This minBondLifetime
refactoring was incomplete here in 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.
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 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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't tier
be a function that reads the bonds
to determine the value?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be asset specific?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 follow up too.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these expected to fail because of network conditions?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
returns: `Returns: | ||
{ | ||
"bondID" (string): The bond transactions's txid and output index. | ||
"reqConfirms" (int): The number of confirmations required to start trading. | ||
}`, | ||
}, | ||
bondOptionsRoute: { | ||
argsShort: `"addr" targetTier (maxBondedAmt bondAssetID)`, |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really keep going if cfg == nil
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
client/core/bond.go
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to follow up too.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
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 follow up too.
if form.BondAsset != nil { | ||
bondAssetID := *form.BondAsset | ||
wallet, found := c.wallet(bondAssetID) | ||
if !found || !wallet.connected() { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/core
form types: