client/assets: add feeRate sanity check for swaps and minor refactoring

[ukane-philemon]

Resolves #1585.

[buck54321]

I wish I remembered better the details of #1587, but I think it was an issue on the server. In that case, it would be fine to check just once at

dcrdex/client/core/trade.go

Lines 2329 to 2330 in 69ace7b

	}
	// swapMatches is no longer idempotent after this point.

if highestFeeRate == 0 {
    return fmt.Errorf(...)
}

[ukane-philemon]

Yes, we can do that and save a few LOC. I initially assumed the check would be better at the asset level.

[chappjc]

I don't have a problem with it in both places TBH, but core also shouldn't request impossible swaps.

[ukane-philemon]

I don't have a problem with it in both places TBH

I left the check at the asset level as an extra precaution, since it is very crucial to the swap process. Or do we really want to remove the check from the asset level? @chappjc

[ukane-philemon]

It seems the new check is effective. https://github.com/decred/dcrdex/actions/runs/3988636125/jobs/6840053009

2023-01-23 16:54:39.188 [ERR] TCORE: somedex.tld:7232 tick error: somedex.tld:7232 tick: {swapMatches order b2c0e3807fc2b2819a125b672c7bea91c991b2f8d5cf74e338332fd72f6415d9 - {swap cannot proceed with a zero fee rate}} [1547](https://github.com/decred/dcrdex/actions/runs/3988636125/jobs/6840053009#step:5:1548) 2023-01-23 16:54:39.188 [ERR] TCORE: Error fetching fee rate for btc: no handler for route "fee_rate" [1548](https://github.com/decred/dcrdex/actions/runs/3988636125/jobs/6840053009#step:5:1549) panic: test timed out after 10m0s [1549](https://github.com/decred/dcrdex/actions/runs/3988636125/jobs/6840053009#step:5:1550)

1550
goroutine 426 [running]:
1551
testing.(*M).startAlarm.func1()
1552
/opt/hostedtoolcache/go/1.19.5/x64/src/testing/testing.go:2036 +0xbb
1553
created by time.goFunc
1554
/opt/hostedtoolcache/go/1.19.5/x64/src/time/sleep.go:176 +0x48
1555

[chappjc]

It seems the new check is effective

Haha, yes it seems. But let's fix the tests that neglect to set a fee rate to use for these fake swaps. Changing the product code to use MaxFeeRate if the server prescribes zero and the wallet gives zero doesn't feel right.

[ukane-philemon]

Changing the product code to use MaxFeeRate if the server prescribes zero and the wallet gives zero doesn't feel right.

But does it mean we don't need to set the fee rate to MaxFeeRate if the server and wallet returned zero?

[chappjc]

Changing the product code to use MaxFeeRate if the server prescribes zero and the wallet gives zero doesn't feel right.

But does it mean we don't need to set the fee rate to MaxFeeRate if the server and wallet returned zero?

Not sure, but it's rather questionable to fall back to MaxFeeRate. If we're getting no informed fee rate from any source, what does that mean? Is the server working? Is the swap even going to negotiate? Will the BTC swap cost a fortune? Will the problem clear up after retrying after some time?

Backing up further, when we first get the msgjson.Match we could see that the swapRate is zero at that point (in (*dexConnection) parseMatches). Why should we even register a match with invalid data in the first place? We reject a match with a rate above MaxFeeRate, so why not reject one with a zero rate? Are there any conceivable use cases for a zero fee rate being valid?

I'm not positive what's best right now, but the red flag can be seen much earlier.

[ukane-philemon]

Are there any conceivable use cases for a zero fee rate being valid?

Right, this is one thing am not so sure about but It makes sense to reject swaps with a zero fee rate atm if defaulting to MaxFeeRate in this scenario is questionable. Input from others would be valuable @buck54321, @martonp.

set the fee rate to MaxFeeRate if the server and wallet returned zero

Removed in db40c8d

[martonp]

If we are unable to get a fee rate from either of the sources, I say we shouldn't be proceeding. The server is clearly malfunctioning, and we also don't have a good connection to the network.

[JoeGruffins]

This works well. The client will continue to try even if max fee is zero in the servers config file. But this is #1932 and #1900

If the server sets max fee to zero with this pr:

So the orders are purged eventually.

[chappjc]

Ok, but I think in the future we'll just wanna block acceptance of such matches early on in (*dexConnection).parseMatches. With this PR we won't make an impossible swap txn, but we may retry until revoke as @JoeGruffins pointed out, or we'll use our own rates in a situation where we know the server is misconfigured / match data is invalid.

[chappjc]

@ukane-philemon To merge, will you prep this branch by squashing the review fixes into the "sanity check" commit, keeping the broadcastTx refactoring commit separate. We'll rebase merge this with 2 commits.