app: auto-find redemption when Maker goes dark after Taker swap#513
Conversation
itswisdomagain
force-pushed
the
auto-find-redemption
branch
2 times, most recently
from
f684a91 to
d72864f
Compare
Sample test run for maker ghost after redeem |
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
754fa7d to
a4f992b
Compare
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
5c3d1a0 to
e6fe87f
Compare
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
e6fe87f to
d9fae9d
Compare
chappjc
left a comment
chappjc
left a comment
There was a problem hiding this comment.
You mentioned the complexity added with the new async design, and I do think it is too complex. We should explore other ways to make FindRedemption not blocking, but the always-running goroutine isn't ideal. I've noted that the loop in that goroutine just spins presently.
I'm ok with FindRedemption starting a goroutine, perhaps with a findRedemptionReq slice as an input to the goroutine.
|
I'd like to prioritize this PR over #589 unless that creates a challenge. |
|
Yeah, been meaning to get back on this. Will revisit in < 24 hrs and clean it up. |
itswisdomagain
force-pushed
the
auto-find-redemption
branch
3 times, most recently
from
e2dbfef to
055ec29
Compare
itswisdomagain
force-pushed
the
auto-find-redemption
branch
4 times, most recently
from
d3b8b4b to
c0ef178
Compare
chappjc
left a comment
chappjc
left a comment
There was a problem hiding this comment.
I'm still considering the role of Core vs trackedTrade in the goroutine mgmt, but some suggestions...
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
c0ef178 to
9600900
Compare
|
I rebased over master for go 1.15 checks and refactored the FindRedemption methods to accept single swap coin IDs instead of in batches and return a result chan. Also added code in the asset shutdown methods to close open channels when ctx is canceled. Code isn't ridiculously simple but reads better to me. I might be able to further improve the FindRedemption implementation on dcr by using cfilters to determine the blocks that likely spend a contract and only fetch txs for those blocks. Will wait for review first. Oh, there's one more stuff for me to look into - determining the block to begin scanning from on re-orgs. |
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
This review isn't complete. Will finish asap, just posting what I've got so far.
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
Continued. I skipped over client/asset/dcr as it looked similar to client/asset/btc, and my comments would probably be similar.
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
9600900 to
f37da96
Compare
|
I'll be taking off the last commit in this branch (9d6b684) as the buck of the changes there do not directly relate to this PR. I've moved those bits to a new PR to be reviewed separately. Until that PR is finalized though, and given that auto-redemption should only be performed on revoked matches, there's a chance the auto-redemption and auto-refund will not be performed for some revoked matches, if the revoke_match request is not received. |
|
Are you saying that refund and auto-redeem won't even be possible without a revoke_match? refunds at least were able to happen regardless of setting it as revoked, as long as lock time expired. And as for auto-redeems, we want to get some trades unstuck that are in just this case where no revoke_match messages was received (sent while offline). Can a reasonable delay be implemented simply to get by this until match_status? EDIT: I guess I'm ok with just setting missing matches revoked as in f414d47 for this PR, but I do think that refunds should continue to work as long as lock time is expired regardless of revoked status being known. |
Not sure if there's a critical missing unlock, but one possible issue is letting the dcrdex server harness open the markets on the first full epoch. @JoeGruffins brought this to light and I added a message from MKT like However, it's not working for me either: |
|
Got it to work, but it doesn't seem perfectly robust: |
|
Something seems off with tip change:
|
|
Ahhh, look at the order of the tests in a failing case: vs. passing If they must be run in a certain order, that needs to be addressed. |
Haven't narrowed it down but likely. The primary motivation for forcing coin unlocking for spent outputs in #648 was because of #453 and #471. The issue should be fixed now in dcrwallet anyways. |
Yes. A revoke_match request is currently the only way a client knows to abandon natural match negotiations, unless the client is restarted. I do agree with your earlier thought to not take recovery steps such as refunds and auto-redeem unless the server has revoked the match.
Yes, and while it is unlikely to happen, if it does happen that a match is not revoked after taker's locktime expires, taker's refund will effectively cause the natural settlement process to fail. In general, I'd prefer to not auto-refund until the match is revoked AND locktime expires. This is not implemented yet though, so refunds will be executed once locktime expires.
Something like auto-redeem after waiting for taker's redeem X minutes past the time of taker's swap swapConf'th confirms?
Yeah, setting missed matches as revoked is very relevant regardless of the need (or lack thereof) to auto-redeem. Since it's not directly related to this PR, I want to keep it out and add it to say #648 where there's already some work to definitely identify failed matches and revoked orders for the purpose of coin unlocking. |
|
Chatted this out in matrix:
|
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
9d6b684 to
a6b00b2
Compare
|
If anyone is reviewing right away, one more change left:
|
| dbMatch, metaData, proof, _ := match.parts() | ||
| if match.failErr != nil || len(proof.RefundCoin) != 0 { | ||
| log.Debugf("Match %v not redeemable: failErr = %v, RefundCoin = %v", | ||
| log.Tracef("Match %v not redeemable: failErr = %v, RefundCoin = %v", |
There was a problem hiding this comment.
Let's just be certain that failErr is logged elsewhere when it is set to something non-nil.
There was a problem hiding this comment.
Do you mind if I get to this in a subsequent work? I plan to look into this variable more when working on setting missing matches as revoked, would likely be a good place to do this.
There was a problem hiding this comment.
Sure. It looks like there's a c.notify for any such error anyway.
itswisdomagain
force-pushed
the
auto-find-redemption
branch
from
a6b00b2 to
b11da61
Compare
|
I've narrowed down the cause of tests failing periodically with unmet btc balance expectations in #658. It's not the order of the tests because even with the new ordered pattern, I've had tests fail at the beginning (TestTrading/success). Opened that issue to track the bug further and subsequently get a fix in. |
|
In Lines 999 to 1008 in 38443f3 Don't understand why a cancelOrder is created for a revoked match. Revoked matches don't usually have an accompanying cancel order. Was reading simnet trading test logs when I saw And the middle log message stood out. |
|
Balance check issues should be resolved now. I modified The auto-redeem simnet tests currently cover a scenario where maker redeems, taker does not receive the redemption details, taker attempts refund because locktime expired on his swap (will need to extend the taker swap locktime to prevent this), the refund failed because the contract txout is spent and so find redemption is triggered even though the match has not been revoked. Other scenarios I'll like to test out include:
Not necessary to hold this PR up for those anyways, in case I don't get those additional tests in before next review. Can add them subsequently. |
|
Since the taker's redeem that follows a successful location of the maker/counterparty redeem and secret is done indirectly ( This seems to be because after |
buck54321
left a comment
buck54321
left a comment
There was a problem hiding this comment.
I love the blocking FindRedemption. This is go from me. Tested in the wild by modifying a single line at
Line 1309 in 0abe5f9
to read instead
if !proof.IsRevoked && match.Match.Side == order.Taker {
and then trading with myself. A few minutes after the maker redeemed (a little longer than I expected with broadcast timeout being just 1 minute by default), the server revoked the match, the client marked it as revoked, then at the next tick it successfully redeemed the counter swap.
| // block may get modified concurrently, lock mtx before reading fields. | ||
| c.blockchainMtx.RLock() | ||
| defer c.blockchainMtx.RUnlock() | ||
| return json.Marshal(&verboseBlockTxs{ | ||
| Hash: block.Hash, | ||
| Height: uint64(block.Height), | ||
| NextHash: block.NextHash, | ||
| Tx: block.RawTx, | ||
| }) |
There was a problem hiding this comment.
Are blocks actually being modified, or just replaced? If it's the latter, the locking isn't needed.
| // NOTE: This could potentially be a long and expensive operation if performed | ||
| // long after the swap is broadcast; might be better executed from a goroutine. | ||
| FindRedemption(ctx context.Context, coinID dex.Bytes) (redemptionCoin, secret dex.Bytes, err error) |
There was a problem hiding this comment.
This warning is apt, but really it's just a feature of the method that needs standardizing rather than a NOTE:. The description can say something like "FindRedemption blocks until the redemption is found."
4 participants
Auto-trigger find redemption for Taker swaps that have gotten at least
swapConfconfirmations, has neither been redeemed by Maker or refunded to Taker.The
FindRedemptionmethod is refactored to allow batching multiple contract coin ids in one call.The dcr and btc implementations of the
FindRedemptionmethod are also modified to push find redemption requests to a goroutine dedicated to scanning blocks for potential contract spends.There are 2 motivations for using a separate goroutine for processing find redemption requests:
FindRedemption)