client/asset/dcr: mixed account support #1498
Conversation
7df521a to
26a4413
Compare
01bf7ca to
86deec3
Compare
client/asset/dcr/dcr.go
Outdated
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // returnCoins is ReturnCoins but without locking fundingMtx. |
There was a problem hiding this comment.
The exported ReturnCoins is now quite a bit more than this method, so the comment isn't quite right anymore. Is it correct to say that the unexported method is generally for use if the coins were spent, while the exported method is needed if unspent coins are being released?
There was a problem hiding this comment.
Generally. Just giving that a look now and see that there's one scenario where the unexported method is used for unspent coins, in FundOrder, after selecting the coins to fund a split tx but the split tx isn't successfully created or broadcast. I'll update both method comments to be more descriptive of what they do, the key difference being that one simply unlocks while the other also moves funds around where necessary. So callers would know which to call to satisfy their needs.
| // funding future orders. | ||
| coinsToTransfer := make([]asset.Coin, 0) | ||
| for _, coin := range returnedCoins { | ||
| if coin.addr == "" { |
There was a problem hiding this comment.
Will you comment on the meaning of an empty addr, and in what normal circumstances an outpoint would not be found in the dcr.fundingCoins map to lead to this? Match resume on startup?
There was a problem hiding this comment.
ReturnCoins seem to be a generic method for returning coins and I just don't want to assume that all coins being returned must already exist in the fundingCoins map. So ideally, we should never meet a scenario where coins being returned are not found in the funding map. I'll add a warning log if that happens.
client/asset/dcr/dcr.go
Outdated
| { | ||
| Key: "unmixedaccount", | ||
| DisplayName: "Change Account Name", | ||
| Description: "dcrwallet change account name, if mixing is enabled on the wallet", |
There was a problem hiding this comment.
Maybe mention that deposit addresses will be from this account if it is set, and that all deposits will be mixed before being available to trade.
Should also mention that if this is set, the "Dedicated/Temporary Trading Account" (below) is also set, and that the primary Account Name should be a mixed account. Because if the primary account is not a mixed account, their trade change and other funds are going to end up in this "change account" and never come back to the primary account, right? This is part of why I figured we'd want some wallet RPC to check if an account is the mixing account and/or a change account.
There was a problem hiding this comment.
if the primary account is not a mixed account, their trade change and other funds are going to end up in this "change account" and never come back to the primary account, right?
Yes.
we'd want some wallet RPC to check if an account is the mixing account and/or a change account
Tricky, but possible.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| unspents = append(unspents, tradingAcctSpendables...) |
There was a problem hiding this comment.
Should these include only the external branch utxos from the temp trading account?
Or is it assumed that any coins in the internal branch would be locked, and when unlocked they would be immediately sent to the unmixed account?
Is this what the TODO comment is getting at?
There was a problem hiding this comment.
As an aside, I feel like we should explicitly document on the Wallet interface that the Unspents method must not return any locked outputs. We're relying on that right?
There was a problem hiding this comment.
Yes, we are relying on that. Should doc, I suppose. Will also need to confirm that's the current behaviour, although I believe it is.
is it assumed that any coins in the internal branch would be locked, and when unlocked they would be immediately sent to the unmixed account?
Yes, but looking at it again, probably not a good assumption since the transfer to the unmixed account could fail (in theory at least) and if wallet.Unspent is called at some point between unlocking the coins and making the transfer to the unmixed account.
Should these include only the external branch utxos from the temp trading account?
I thought about it, maybe. But is it an unnecessary overhead? Because we'd need a validateaddress rpc for each unspent output. Given the previous point about potentially selecting tainted utxos meant for the unmixed account, should probably be worth it to make this check.
There's also the concern about the user sending funds directly to this temp account and ends up being selected for funding orders because the fund was sent to an external branch address and is not locked. Will think up a potentially better way of uniquely identifying unused split tx outputs.
There was a problem hiding this comment.
There's also the concern about the user sending funds directly to this temp account and ends up being selected for funding orders because the fund was sent to an external branch address and is not locked. Will think up a potentially better way of uniquely identifying unused split tx outputs.
I'm not sure we should jump through too many hoops for this case. It's a similar situation with dcrwallet and the user receiving funds directly into the mixed account instead of the unmixed account. Just user error. But maybe...
There was a problem hiding this comment.
I suppose also if sendAll did have an error, the user would have to manually rescue those funds if we did not permit unlocked internal branch utxos to be selected
client/asset/dcr/dcr.go
Outdated
| feeRate, err := dcr.feeRate(2) | ||
| if err != nil { | ||
| dcr.log.Debugf("sendAll feeRate error: %v, using fallbackFeeRate", err) | ||
| feeRate = dcr.fallbackFeeRate |
There was a problem hiding this comment.
Probably could use targetFeeRateWithFallback(2, 0) in place of these 5 or so lines.
We should probably also consider a latest fee rate cache internal to ExchangeWallet that can be passed in cases where we'd otherwise be using 0 for the fee suggestion and it would be a impractical to have it as an input to the method.
There was a problem hiding this comment.
I honestly wouldn't mind using an external API as a fallback.
https://explorer.dcrdata.org/insight/api/utils/estimatefee?nbBlocks=2
There was a problem hiding this comment.
Funny, I've been having the same thoughts recently, for all assets really.
There was a problem hiding this comment.
Possibly for BTC, fastestFee from: https://mempool.space/docs/api/rest#get-recommended-fees
$ curl -sSL "https://mempool.space/api/v1/fees/recommended"
{"fastestFee":6,"halfHourFee":1,"hourFee":1,"minimumFee":1}
Or that plus a sat or two, maybe 110%, since swaps cannot really wait all day.
86deec3 to
4260051
Compare
4260051 to
b2ffdf8
Compare
There was a problem hiding this comment.
Tested this out on testnet using the mixer. Working well. Here is a contract that traded successfully, and you can see the inputs were mixed: https://testnet.dcrdata.org/tx/1d223ee2919c387bc997d3b2066330d6fc6c60cda0ac462980eea9258b2f497c
At one point I hit this error while trading, although I definitely had funds.:
error code 36: coin a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1 is locked
After waiting a bit I was able to trade again.
Some more logs from dexc if they help:
2022-05-13 06:33:54.290 [INF] CORE[dcr]: Funding 5 DCR order with coins [a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1] worth 42.94967296
...
2022-05-13 06:34:42.164 [TRC] CORE: Revoked match 2f08937de48415ec188512f5f583024edb48051479c8b0a50fdacca6bcb3752a (Taker) in status MakerSwapCast considered inactive.
2022-05-13 06:34:42.164 [TRC] CORE: notify: |DATA| (order) - Order: 3e238970f685e312436c87bef37ff024bf656a73da683ad8aa69ee895ee45121
2022-05-13 06:34:42.164 [INF] CORE: Retiring inactive order 3e238970f685e312436c87bef37ff024bf656a73da683ad8aa69ee895ee45121 in status canceled
2022-05-13 06:34:42.164 [DBG] CORE[dcr]: returning coins [a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1]
...
2022-05-13 06:42:27.514 [INF] CORE[dcr]: Funding 3 DCR order with coins [a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1] worth 42.94967296
2022-05-13 06:42:27.515 [DBG] CORE[dcr]: returning coins [a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1]
2022-05-13 06:42:27.516 [ERR] WEB: error placing order: new order request with DEX server 127.0.0.1:17273 market dcr_btc failed: rpc error: error code 36: coin a4f41f58a05f93388925b948f5604e3d3a4a28946dcb54d07a967783055995fb:1 is locked
...
(failed a few more times with the same reason, then it used a different ouput, but maybe because I changed the value?)
...
2022-05-13 06:51:56.812 [INF] CORE[dcr]: Funding 1 DCR order with coins [b8deae581d3346205b8c4e75fe28e17fa7c2f6ce5573865e712948f7cdc14094:22] worth 2.68435456
2022-05-13 06:51:56.819 [DBG] CORE: notify: |POKE| (order) Order placed - selling 1.00000000 DCR, rate = 0.001 (7cf76044) - Order: 7cf7604468764a461e76abed33d923dfc3b6f9656fa2439164d490c3f90e4109
Yeah, that's the reason. The coin is unlocked on dexc but still locked by the server. We've had this error before but not recently. Not sure why it's back. From those logs, it appears the order was canceled but had a match and the match was later revoked. The error seem to be coming from: dcrdex/server/market/orderrouter.go Lines 471 to 473 in efd7c12 And the doc for CoinLocked says |
|
I think there needs to be wiki or readme to set up dcrwallet. The info here https://docs.decred.org/privacy/cspp/how-to-cspp/ is correct for |
It can be 0 though, it's configurable, but 1 seems to be recommended. Wonder if it really matters too. Is it possible that unmixed funds end up in the other branch of the mixed account?
|
I think the docs suggest I'd like to see how Decrediton creates the mixed and unmixed accounts. Will try this. |
|
Here's what Decrediton configures, on testnet at least: |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| unspents = append(unspents, tradingAcctSpendables...) |
There was a problem hiding this comment.
I suppose also if sendAll did have an error, the user would have to manually rescue those funds if we did not permit unlocked internal branch utxos to be selected
Oh, this was my confusing I tested with |
I haven't digested the circumstances of the server coin unlocking issue reported by @JoeGruffins on this PR, but I wonder if it's related to 79d2b9d |
I don't think so. |
|
Bump on this PR. We'll probably be in a place to get it finalized and merged next week. |
Where mixing is enabled, and the change output from a swap is required to be locked for future spending in another swap, the change output should be sent to the trading account rather than the change/unmixed account.
After unlocking previously reserved utxos, select utxos from the temp trading account that are not unused split tx outputs and transfer to the unmixed account for re-mixing into the primary account. Unused split tx outputs are distinguished from other outputs in the trading account by using addresses generated from the external branch of the trading account when preparing the split tx.
50d4ec0 to
87e5830
Compare
|
I am testing this, but it takes a long time because testnet mixing |
There was a problem hiding this comment.
All working as expected.
Thoughts for improving things:
- need to ensure that dcrwallet ticket buyer not enabled (or at least not using any selected account)
- mixing status RPCs: mixed account is actually mixing, account is unmixed change temp/trading is neither
- on dcr wallet (re)configure, log something indicating account setup with names
- somehow do this with simnet, requires running a cspp server locally, which may not ever work with only one mix participant
Resolves #341.
Changes to spending
It is expected (assumed) that spendable utxos in the trading account are unspent split tx outputs which do not get moved to the unmixed account when they're no longer used to fund an order.
Changes to balance reporting
Availablenow includes trading account spendable amount - trading account locked utxos.Immaturenow includes total amount in unmixed account.ImmaturetoAvailableas mixing occurs (balance doesn’t update until new block or lock/unlock).Lockednow includes trading account locked utxos.