client/asset/{btc,dcr}: enable split funding transactions by buck54321 · Pull Request #562 · decred/dcrdex

buck54321 · 2020-07-22T15:16:28Z

Creates a "split transaction" to pre-size inputs, with some minimum reasonable consideration for negative net effects. The split transaction is sent during a call to FundOrder if the user has specified such in their wallet configuration's txsplit value.

Resolves #340

buck54321 · 2020-07-22T15:20:39Z

 	// related variation as well as a potential dust change output with no
 	// subtractee specified, in which case the dust goes to the miner.
-	if checkRate > feeRate*3 {
+	if changeAdded && checkRate > feeRate*3 {


With the changes in TestAvailableFund, I actually ran up against the *3 when the change was a dust ouptut. *3 was 69 atoms/byte and the effective rate ended up being 71. I'm sure we could improve this still.

buck54321 · 2020-07-22T15:43:44Z

+	// Add funding for an extra input to accommodate the later combined tests.
+	lottaFunds := calc.RequiredOrderFunds(lottaOrder, 2*dexdcr.P2PKHInputSize, tDCR)


This was a little counterintuitive, so I figured I would mention it. Combining littleFunds + lottaFunds is not enough to cover extraLottaOrder = littleOrder + lottaOrder, even though littleFunds is enough to cover littleOrder and lottaFunds is enough to cover lottaOrder.

In English: you must prove ownership of fewer funds in order to place two one lot orders than you would to place one two lot order with two inputs.

In Go:

func TestThis(t *testing.T) { one := calc.RequiredOrderFunds(tBTC.LotSize, dexbtc.RedeemP2PKHInputSize, tBTC) two := calc.RequiredOrderFunds(2*tBTC.LotSize, 2*dexbtc.RedeemP2PKHInputSize, tBTC) fmt.Println("one =", one, ", two =", two, ", two <= one*2 ?", two <= one*2) }

will print one = 1007650 , two = 2020366 , two <= one*2 ? false.

chappjc

Since we talked about market and immediate limit orders (orders that don't sit on the book indefinitely) not really needing the split txn, I figure we'll eventually want a "use split tx" button on the order dialog with the default set according to the wallet's config. Alternatively or in addition to the per-order option, I can imagine two wallet settings rather than one: use split tx for (1) standing/bookable orders, and (2) unbookable orders like market and immediate force. I bring it up in this PR because useSplitTx is an ExchangeWallet field set on construction, but if it is made a per-order option it would need to be an input arg to FundOrder, and if there were two options pertaining to bookability then that would preclude an ExchangeWallet config field altogether since order type is not something ExchangeWallet knows about at all.

chappjc · 2020-07-27T21:34:37Z

+	// NOTE: Can't return coins yet, because dcrwallet doesn't recognize them as
+	// spent immediately, so subsequent calls to FundOrder might result in a
+	// `-4: rejected transaction: transaction in the pool already spends the
+	// same coins` error.


That's unfortunate. What does it seem dcrwallet needs to recognize them as spent? Just a little time? ~~I guess for now we just leave them locked forever, or at least until dexc restart?~~ This is what logSplitFunds and the map is for now I see.

chappjc · 2020-07-27T22:15:05Z

+		{
+			Key:         "txsplit",
+			DisplayName: "Pre-split funding inputs",
+			Description: "Pre-split funding inputs to prevent locking funds into an order for which a change output may not be immediately available. Only used for standing-type orders.",


Maybe "to prevent locking excess funds".

I get that "change output may not be immediately available" refers to when the swap contract txn happens shortly after the order placement, thus making the change available, but it's unlikely most users will get that. Plus, even if they did get that, they should understand that a swap could take hours (esp. taker waiting for maker's swap before broadcasting their own). How about something more accessible to average users like When placing an order, create a "split" transaction to fund the order without locking more of the wallet balance than necessary. Otherwise, excess funds may be reserved to fund the order until the swap contract transaction is created during trade settlement or the order is canceled. This an extra transaction for which network mining fees are paid. Most useful for standing-type orders that may remain on the book indefinitely.

Also, it currently seems to be used for all trade orders, not just standing.

Also, it currently seems to be used for all trade orders, not just standing.

Oh yeah. I had a solution working for this, but backed off for some reason. I'll get it in now.

So adding asset-specific order options is probably doable, but a large undertaking. For now, I'm just informing the ExchangeWallet whether the funds are for a standing or immediate order, and the funds aren't being split if it's immediate. See aa5fc57.

Works for me.

chappjc · 2020-08-04T19:22:52Z

An interesting consequence of the split tx is that the init/contract txn is less likely to have change, thus the user ends up paying maxFeeRate or higher much of the time.

e.g. Here's a DCR swap contract txn spending the output of a split tx:

{
  "hex": "0100000001c9b7202d32d8bcd4d74d1fa080db0cc127714ac2640b532e18f9f9a4d2dc35bc0000000000ffffffff0100286bee00000000000017a91458ff54430c2c5ad484319150d2fd3bca06bb08548700000000000000000160476bee0000000046010000010000006b483045022100e70214fd337eac6ad4468bfef69d6d5bc6730bf22502223174ea10aa9c457ad10220753d9b278a80329639d1636f5db18f6c395aad50a3a559857d68aed7533f2ba6012102147f5a1a144ed535ee1d35b92d97e45a69a8919372b01f7640837c139f24667c",
  "txid": "60f6b6a4e50a25e0cf1c7f3211400c326a1a3542c3d39c76bb880e47cea73a68",
  "version": 1,
  "locktime": 0,
  "expiry": 0,
  "vin": [
    {
      "txid": "bc35dcd2a4f9f9182e530b64c24a7127c10cdb80a01f4dd7d4bcd8322d20b7c9",
      "vout": 0,
      "tree": 0,
      "sequence": 4294967295,
      "amountin": 40.00008032,
      "blockheight": 326,
      "blockindex": 1,
      "scriptSig": {
        "asm": "3045022100e70214fd337eac6ad4468bfef69d6d5bc6730bf22502223174ea10aa9c457ad10220753d9b278a80329639d1636f5db18f6c395aad50a3a559857d68aed7533f2ba601 02147f5a1a144ed535ee1d35b92d97e45a69a8919372b01f7640837c139f24667c",
        "hex": "483045022100e70214fd337eac6ad4468bfef69d6d5bc6730bf22502223174ea10aa9c457ad10220753d9b278a80329639d1636f5db18f6c395aad50a3a559857d68aed7533f2ba6012102147f5a1a144ed535ee1d35b92d97e45a69a8919372b01f7640837c139f24667c"
      }
    }
  ],
  "vout": [
    {
      "value": 40,
      "n": 0,
      "version": 0,
      "scriptPubKey": {
        "asm": "OP_HASH160 58ff54430c2c5ad484319150d2fd3bca06bb0854 OP_EQUAL",
        "hex": "a91458ff54430c2c5ad484319150d2fd3bca06bb085487",
        "reqSigs": 1,
        "type": "scripthash",
        "addresses": [
          "ScixXbNEV4jHArPgDAfwvLvaqDQErPf2ENk"
        ]
      }
    }
  ],
  "blockhash": "347bd9f25362f1fcc985d0d4cd11cbbd0aa182a06c6827e44f2480fe92292d0e",
  "blockheight": 326,
  "blockindex": 2,
  "confirmations": 1,
  "time": 1596567128,
  "blocktime": 1596567128
}

It omitted the change output because it would have been dust (presumably). The init txn above is 214 bytes, with a 8032 atom fee, for a fee rate of ~37.5 atoms/B.

[DBG] CORE[dcr]: 1 signature cycles to converge on fees for tx 60f6b6a4e50a25e0cf1c7f3211400c326a1a3542c3d39c76bb880e47cea73a68: min rate = 11, actual fee rate = 37 (8032 for 214 bytes), change = false
[DBG] CORE[dcr]: returning coins [bc35dcd2a4f9f9182e530b64c24a7127c10cdb80a01f4dd7d4bcd8322d20b7c9:0]

calc.RequiredOrderFunds always assumes a change output, but it clearly can be omitted.

I'm gonna test some more splits, but it seems to be working.

chappjc · 2020-08-04T19:34:18Z

BTC on the other hand is massively over allocating. A 2 lot (20 DCR lot size) buy order:

[DBG] CORE[btc]: Change output size = 31, addr = bcrt1qskjprt4tg8a8rj2ulw9643y9w5lagztq2ymmxe
[DBG] CORE[btc]: 2 signature cycles to converge on fees for tx 2cfafdfad6ace1aa9aacf9fbd3a70d1ed5653f208a7fd5bb68fbd72c89815d79: min rate = 100, actual fee rate = 100 (22200 for 222 bytes), change = true
[INF] CORE[btc]: sent split transaction 2cfafdfad6ace1aa9aacf9fbd3a70d1ed5653f208a7fd5bb68fbd72c89815d79 to accommodate swap of size 40000000 + fees = 48989200
[DBG] CORE: notify: |POKE| (order) Order placed - buying 40.00000000 dcr (3a0b7278) - Order: 3a0b7278d0a0184df6dea3d4a27a47ffaa1a58c415d5dd6e8ee6ea6dba30b9d1

40000000 + fees = 48989200 is way too much, but I think it was doing too much before this PR.

Anyway, for this level of fee over-allocation, the swap txn that fills it does have change:

{
  "txid": "2992beb25d940e385f137a9b0b310302e581c6ca72a4781c1469d316f773c66a",
  "hash": "f434f5b83288da919d0f1ea564b9e8b864ca73005569f656145a4a688e38e00d",
  "version": 1,
  "size": 223,
  "vsize": 142,
  "weight": 565,
  "locktime": 0,
  "vin": [
    {
      "txid": "2cfafdfad6ace1aa9aacf9fbd3a70d1ed5653f208a7fd5bb68fbd72c89815d79",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "txinwitness": [
        "3044022014d215173336fcd2ce33d843d422ea41fd66d0b99c99c57cb543ae479a1058100220259167fb06b97cbfd56ac9f14446ced61102e3b2c22449ab6ac69713490732d101",
        "03d5f3076a45b845eda1394ff5ecc24f059708aaa4d23198a9144e4e3c5336575f"
      ],
      "sequence": 4294967295
    }
  ],
  "vout": [
    {
      "value": 0.40000000,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_HASH160 6d4ad1df4ec0b879bf72b832ade2b287fc86d3b4 OP_EQUAL",
        "hex": "a9146d4ad1df4ec0b879bf72b832ade2b287fc86d3b487",
        "reqSigs": 1,
        "type": "scripthash",
        "addresses": [
          "2N3D7GmLaeBj2LhiFfh7FNPQd7RK9DT3JUo"
        ]
      }
    },
    {
      "value": 0.08966900,
      "n": 1,
      "scriptPubKey": {
        "asm": "0 85d4fcc14d23021e63d2d80978324b904b5f90e5",
        "hex": "001485d4fcc14d23021e63d2d80978324b904b5f90e5",
        "reqSigs": 1,
        "type": "witness_v0_keyhash",
        "addresses": [
          "bcrt1qsh20es2dyvppuc7jmqyhsvjtjp94ly89fp24gs"
        ]
      }
    }
  ],
  "hex": "01000000000101795d81892cd7fb68bbd57f8a203f65d51e0da7d3fbf9ac9aaae1acd6fafdfa2c0000000000ffffffff02005a62020000000017a9146d4ad1df4ec0b879bf72b832ade2b287fc86d3b487f4d288000000000016001485d4fcc14d23021e63d2d80978324b904b5f90e502473044022014d215173336fcd2ce33d843d422ea41fd66d0b99c99c57cb543ae479a1058100220259167fb06b97cbfd56ac9f14446ced61102e3b2c22449ab6ac69713490732d1012103d5f3076a45b845eda1394ff5ecc24f059708aaa4d23198a9144e4e3c5336575f00000000"
}

So with BTC, it's over allocating for the order, which has the nice effect of not over paying fees on the swap contract.
With DCR, it's allocating less, but the side effect is that the change becomes dust and the contract txn overpays miner fees.

I'm wondering if we want the split tx to make an output that is buffered extra like we see above with BTC (but not so extreme) so that the contract can have change and you don't have to over pay fees.

chappjc · 2020-08-06T15:43:02Z

Got some conflicts with the wireBytes stuff in now.

Do you think it's worth investigating the over/under allocation issues now (currently over for BTC, correct but possibly better off higher for DCR)? It all seems to be working if not optimally.

buck54321 · 2020-08-10T15:27:39Z

I think I see the over-allocation issue. It's how we're using RequiredOrderFunds and it might get hairy to fix. I'll investigate and follow up ASAP.

chappjc · 2020-08-11T13:46:45Z

Since you're hitting the over-allocation issue for quote asset funding in #594, do you have anything else for this PR? I think we could merge and tweak the issue of high fees from dust outputs with well sized outpoints in a follow up.

buck54321 · 2020-08-12T02:57:58Z

Good to go.

chappjc · 2020-08-13T14:34:12Z

Going in. Note to all: the new feature is selectable in the wallet config form. However, there is a known issue with quote asset order (over)funding getting resolved in #594 (issue existed before this PR).
Also note that even with the base asset, like DCR on dex-test, the order funding is so precise that the difference between maxfeerate and optimal fee rate is effectively dust, causing you to end up paying maxfeerate everytime.

buck54321 commented Jul 22, 2020

View reviewed changes

chappjc requested changes Jul 27, 2020

View reviewed changes

chappjc mentioned this pull request Jul 28, 2020

multi: refactor wallet configuration #533

Merged

buck54321 force-pushed the splittx branch from 6352596 to cc3f9b6 Compare July 31, 2020 15:18

chappjc reviewed Jul 31, 2020

View reviewed changes

Comment thread client/asset/btc/btc.go Outdated

buck54321 force-pushed the splittx branch from cc3f9b6 to bc8174e Compare August 4, 2020 18:15

buck54321 force-pushed the splittx branch from bc8174e to 9cbf535 Compare August 10, 2020 15:23

buck54321 mentioned this pull request Aug 11, 2020

multi: specify lots as a parameter to RequiredOrderFunds #594

Merged

buck54321 force-pushed the splittx branch from 9cbf535 to ca4c553 Compare August 12, 2020 02:57

buck54321 added 5 commits August 13, 2020 09:31

enable split funding transactions

dfaa91e

skip split for immediate orders

418f215

fewer string keys

6698f26

add max-width for tooltip

d55fbc1

pointerless btc outPoint structure

47f7989

chappjc force-pushed the splittx branch from ca4c553 to 47f7989 Compare August 13, 2020 14:32

chappjc merged commit 51af9aa into decred:master Aug 13, 2020

		// Add funding for an extra input to accommodate the later combined tests.
		lottaFunds := calc.RequiredOrderFunds(lottaOrder, 2*dexdcr.P2PKHInputSize, tDCR)

Conversation

buck54321 commented Jul 22, 2020 • edited by chappjc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

buck54321 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chappjc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chappjc Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

buck54321 Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

chappjc Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chappjc commented Aug 4, 2020

Uh oh!

chappjc commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 commented Aug 10, 2020

Uh oh!

chappjc commented Aug 11, 2020

Uh oh!

buck54321 commented Aug 12, 2020

Uh oh!

chappjc commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

buck54321 commented Jul 22, 2020 •

edited by chappjc

Loading

chappjc left a comment •

edited

Loading

chappjc Jul 27, 2020 •

edited

Loading

chappjc commented Aug 4, 2020 •

edited

Loading

chappjc commented Aug 6, 2020 •

edited

Loading

chappjc commented Aug 13, 2020 •

edited

Loading