client/{core, btc}: MultiTrade #2362

martonp · 2023-05-20T12:50:05Z

This adds a core.MultiTrade function. It allows callers to fund multiple trades in one go. It is only implemented for BTC currently, but will be implemented for other assets later, after this is reviewed. In order to fund multiple orders in one go, the BTC wallet first attempts to fund each of the orders with the existing UTXOs. If this was not possible and splitting is not allowed, the orders that are able to be funded are returned. If splitting is allowed, a split transaction with one output per order will be created.

This also updates FundOrder to return the fees spent on any split transactions, and this is stored in the database.

client/asset/dcr/dcr.go

buck54321 · 2023-05-22T13:37:19Z

client/asset/interface.go

+	// Version is the asset version of the "from" asset with the init
+	// transaction (this wallet). Most backends only support one version.


Wallets don't really have a concept of "from" and "to" assets. Saying (this wallet) is curious too. What other wallet would we be talking about? The docstring for Swaps.Version just says Version is the asset version. Most backends only support one version., which is better, but I'd drop the second sentence.

I agree the comment is a bit strange... I just copied it from the Order type. Should we just change Version to SwapVersion? The Order type also has a RedeemVersion. We have fromAsset and toAsset on assetSet and fromWallet and toWallet on walletSet btw.

As soon as we started including information about the redeem asset, this wording became less redundant. It's unfortunate, but there is ambiguity to resolve with these comments.

client/asset/interface.go

buck54321 · 2023-05-22T14:11:15Z

client/asset/btc/btc.go

@@ -761,6 +761,11 @@ func (s *swapOptions) feeBump() (float64, error) {
 	return bump, nil
 }

+type fundMultiOptions struct {
+	Split       *bool   `ini:"swapsplit"`
+	SplitBuffer *uint64 `ini:"splitbuffer"`


I think SplitBuffer should be an asset config option in the WalletDefinition. It can be here too, if you still think it would be necessary, and it would take precedence here, but if we can set the option for using splits at the asset level, we should expose this buffer too. I would definitely use it.

I don't have this implemented for the regular split transactions, only for these MultiTrade splits. I wasn't planning on exposing the MultiTrade functionality through the UI. Why would you use the buffer with a regular trade?

I wasn't planning on exposing the MultiTrade functionality through the UI.

Where are users going to set these values?

Why would you use the buffer with a regular trade?

What do you mean? We're not dealing with "regular trades" here. Just bot stuff, right? Isn't the point of the buffer to enable utxo reuse without additional splits or excessive overlock.

buck54321 · 2023-05-22T14:15:17Z

client/asset/btc/btc.go

+type fundMultiOptions struct {
+	Split       *bool   `ini:"swapsplit"`
+	SplitBuffer *uint64 `ini:"splitbuffer"`
+}


How are these options being exposed to the user?

I wasn't planning to add this to the UI, this would just be available by programming against Core.

So only people who are writing their own software can benefit from these settings?

Yes, or people running market makers. There will be settings in the BotConfig which tell the market maker whether or not to set these.

buck54321 · 2023-05-22T14:54:02Z

client/asset/btc/btc.go

+
+	outputAddresses := make([]btcutil.Address, len(values))
+	for i, req := range requiredForOrders {
+		changeAddr, err := btc.node.changeAddress()


This isn't a change address, so the variable name is misleading. The changeAddress method should really be named internalAddress.

buck54321 · 2023-05-22T15:33:24Z

client/asset/btc/btc.go

+// fundMultiBestEffors makes a best effort to fund every order. If it is not
+// possible, it returns coins for the orders that could be funded. The coins
+// that fund each order are returned in the same order as the values that were
+// passed in.


Returning partial results feels icky. We usually return an error if the caller requests more than is available. What's different here?

They're not really requesting more than is available, it's just that without a split the wallet would not be able to fund all of the orders without leaving keep as available funds.

There is a check a few lines down:

if bal.Available < totalRequiredForOrders+keep { return nil, nil, 0, fmt.Errorf("insufficient funds. %d < %d + %d", bal.Available, totalRequiredForOrders, keep) }

If the caller asks to fund 5 orders, but we return partial results for 3, is the caller going to place those 3 orders? Do they have any say in which orders get funded?

It's kinda confusing because for normal order funding, if we can't do it without a split, we definitely can't do it with a split. I don't think that's the case with FundMulti, because overlocks stack.

I had roughly the same question: #2362 (comment)
It stops on the first one that fails. I agree that's a difficult method to use effectively. I think it would need a flag indicate if you want to allow that, or return the coins and fail entirely if you can't get them all.

Related, in caller, it returns and error even if some orders were submitted: #2362 (comment)

The idea was that the caller passes the orders in order of priority. I can definitely add a flag. I was considering adding an allOrNothing flag for this, but I didn't because I didn't see any of the bots passing true for this.

I think it would be a nice setting in the bot to only allow splitting once every X amount of time.. and in these cases the partial results at least allow funding of some orders until splitting is once again allowed.

buck54321 · 2023-05-22T15:39:45Z

client/asset/btc/btc.go

+		return coins, redeemScripts, 0, nil
+	}
+
+	return btc.fundMultiWithSplit(keep+reserves, values, splitTxFeeRate, maxFeeRate, splitBuffer)


Something feels backwards. If I want a split, and you don't split, that would be wrong. I guess you're changing the verbiage to "allow" a split, rather than to actually request a split? I would definitely want the option to split by default. Not offering that option would potentially limit my ability to trade on multiple markets simultaneously.

Something feels backwards. If I want a split, and you don't split, that would be wrong.

I believe this has been the behavior since the beginning, at least when it is decided the split tx adds more "baggage" than the split would save by avoiding overlock. Perhaps the context is different here? Is the bool even "more optional" in this case?

The MultiTrade will never lock more the balance.Available - keep funds. The bot will pass keep as the amount that has been reserved by bots trading on other markets. If splitting is not allowed, then MultiTrade will only fund the orders that it is able to without dipping into the other bots' allocation.

Something feels backwards. If I want a split, and you don't split, that would be wrong.

I believe this has been the behavior since the beginning, at least when it is decided the split tx adds more "baggage" than the split would save by avoiding overlock. Perhaps the context is different here? Is the bool even "more optional" in this case?

For normal trades, we check both the split and the non-split and compare them. In MultiTrade, if we can do it without splitting we do, regardless of the amount of overlock.

@martonp please check my understanding. Ignoring fees, if we want to fund 3 1-lot orders with a lot size of 1, and we have three utxos of 1, 1, and 1000, we would end up locking up all of it because fundMultiBestEffort would succeed without error, resulting in an overlock of 999 and precluding any additional orders until the overlocked order matches and a swap is generated. And that is regardless of what the user has set for fundMultiOptions.Split.

@martonp please check my understanding. Ignoring fees, if we want to fund 3 1-lot orders with a lot size of 1, and we have three utxos of 1, 1, and 1000, we would end up locking up all of it because fundMultiBestEffort would succeed without error, resulting in an overlock of 999 and precluding any additional orders until the overlocked order matches and a swap is generated. And that is regardless of what the user has set for fundMultiOptions.Split.

Yes, this is correct, as long as maxLock is greater than or equal to 1002. I'm thinking maybe the confusion is coming from the fact that the goal if splitting in FundOrder is avoiding overlock, but the goal of splitting in FundMultiOrder is to be able to fund multiple orders, and maxLock is what deals with the overlocking. Maybe splitting in FundMultiOrder should be called something else?

Adding to the confusion, the default for allowSplit is btc.useSplitTx() which is set in configuration and has a known definition.

True, I'll remove that default.

How would a caller calculate maxLock? They won't know how to calculate fees.

maxLock is set to the balance that is allocated to the bot that is placing the orders. You can see this logic in #2332

Splitting should really be a default behavior for a bot, we just need to be smarter about how we do it. On what reasoning do you think a bot operator may want to not enable splitting?

I think splitting should be the default, but potentially someone would want to turn on the bot and play around with it without automatically having to spend money.

maxLock is set to the balance that is allocated to the bot that is placing the orders. You can see this logic in #2332

Ah. I see form.MaxLock = c.balanceHandler.botBalance(c.botID, fromAsset) now.

.. maxLock is what deals with the overlocking.

If maxLock is being used to indicate the balance available to the bot, can it simultaneously be used to limit overlock?

The bot code could potentially calculate the amount required for the orders, add a buffer on top, and pass that for max lock, if that's desired. The main thing I was concerned with here is not using the other bots' funds.

Another simple approach would be to set maxLock to the smaller of the bot balance or some multiple of the total-value of the multi-trade. e.g. in the (1,1,1000) UTXO case where the total order value is just 3, if you set maxLock to 9 and avoid insane levels of overlock.

Regarding default to split, always doing a split is not great for several reasons, the top being the fees, but also the greatly slowed order submission because server is then waiting on network tx propagation to verify the fresh 0-conf funding UTXO.

client/core/core.go

buck54321 · 2023-05-22T19:41:13Z

client/asset/btc/btc.go

+		totalOutputRequired += req
+	}


Would it be useful to check existing utxos to see if we already have one that satisfies our needs here? Bots will likely cancel and replace a lot of fully unmatched orders, If these orders change by a rate that keeps the old funding coins withing the split buffer, we can reuse the utxos. If we can maximize our reuse of those outputs and minimize the number of inputs that go into the split tx (by reducing the total funding needed), we could potentially see significant fee savings.

fundMultiBestEffort handles this. It tries to fund all of the orders with existing UTXOs. If it is not able to, only then is the split done.

Example: Ignoring fees, orders of (10, 10, 10), utxos of (10, 10, 15), maxLock = 30, allowSplits = true, fundMultiBestEffort would fail because of maxLock. fundMultiWithSplit would consume all three inputs to create outputs of (10, 10, 10, 5), even though we already had two outputs of 10. The ideal split would just consume the utxo of value 15 and create outputs at (10, 5). Inputs are much more expensive than outputs. So the current algo almost triples the fees associated with the split transaction compared to the ideal.

Ah good point. I'll combine the best effort logic with splitting.

I took a swing at a hybrid approach here, but the diff is kinda messy, so might be easier to look at this first. (Not tested and tests not updated)

The primary differences

Separates available funding (maxLock) from allowable overfunding (overfundBuffer)

Only creates splits when existing outputs don't work

A notable consequence

Orders can be funded out of order in partial results. For instance, if we're funding values (1, 3, 1), and we have utxos (1, 1, 50), low overfund allowance, and allowSplit = false, the first and last orders will be funded, but not the middle, since that would require an high overlock

I've also been working on it, I've pushed the change. I think overfundBuffer is probably a good idea to add.

My implementation will be O(n^2) in the number of orders when a split is needed, and yours will always be O(n) , but I think sometimes an order that requires less funds may take the UTXO allocated for the larger one in your implementation. But both are better than the original in that they will only create the split for the orders that actually need it. I think if the splits are done very rarely, better to be more accurate though.

leastOverFund could also be modified to favor the single UTXO instead of the set.

i think as long as we have a correct implementation, we could go with that for now. Joe is working on a testing framework for the bots, when running those we can see how many splits are being done and how the performance is, and adjust based on the results.

This adds a `core.MultiTrade` function. It allows callers to fund multiple trades in one go. It is only implemented for BTC currently, but will be implemented for other assets later. In order to fund multiple orders in one go, the BTC wallet first attempts to fund each of the orders with the existing UTXOs. If this was not possible and splitting is not allowed, the orders that are able to be funded are returned. If splitting is allowed, a split transaction with one output per order will be created. This also updates `FundOrder` to return the fees spent on any split transactions, and this is stored in the database.

client/asset/btc/btc.go

client/rpcserver/types.go

client/core/core.go

client/asset/btc/coin_selection.go

client/asset/btc/btc.go

client/asset/btc/spv_test.go

martonp · 2023-06-25T12:26:12Z

I've been testing using #2409 and this has been working well.

chappjc · 2023-06-25T20:39:37Z

I've been testing using #2409 and this has been working well.

Great. I'll do a few manual mult-trades with the RPC client, but I have no more comments or requests on the code.

chappjc

All working as expected via the RPC server's multitrade command. I'm excited to see how this comes together in the final bot project.

martonp force-pushed the multiTrade branch from 1e9cc6c to f3d41d0 Compare May 20, 2023 12:52

martonp mentioned this pull request May 22, 2023

client/{mm,core}: Market maker balance segregation #2332

Merged

chappjc reviewed May 22, 2023

View reviewed changes

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved

buck54321 requested changes May 22, 2023

View reviewed changes

buck54321 reviewed May 22, 2023

View reviewed changes

martonp added 2 commits May 24, 2023 05:31

Buck/Chappjc reviews minor changes

4922705

martonp force-pushed the multiTrade branch from f3d41d0 to 4922705 Compare May 24, 2023 09:31

martonp added 2 commits May 24, 2023 06:37

Pass 'keep' to MultiTrade

dfcda48

Change keep to maxLock

b221ba2

chappjc requested changes May 30, 2023

View reviewed changes

chappjc reviewed May 31, 2023

View reviewed changes

client/asset/btc/btc.go Outdated Show resolved Hide resolved

chappjc reviewed May 31, 2023

View reviewed changes

client/asset/btc/btc.go Outdated Show resolved Hide resolved

client/rpcserver/types.go Outdated Show resolved Hide resolved

chappjc reviewed May 31, 2023

View reviewed changes

client/core/core.go Outdated Show resolved Hide resolved

Chappjc review updates

e2d6f5b

martonp force-pushed the multiTrade branch from 4697980 to e2d6f5b Compare June 2, 2023 09:28

buck54321 reviewed Jun 3, 2023

View reviewed changes

client/asset/btc/coin_selection.go Outdated Show resolved Hide resolved

client/asset/btc/btc.go Outdated Show resolved Hide resolved

Include as few orders into split as possible

3c15315

JoeGruffins approved these changes Jun 12, 2023

View reviewed changes

chappjc self-requested a review June 12, 2023 13:18

Joe review

30919f6

martonp force-pushed the multiTrade branch from dd0fd8e to 30919f6 Compare June 25, 2023 09:58

Touch ups

d8e1b14

chappjc approved these changes Jun 26, 2023

View reviewed changes

chappjc merged commit d52205b into decred:master Jun 26, 2023

		// Version is the asset version of the "from" asset with the init
		// transaction (this wallet). Most backends only support one version.

client/{core, btc}: MultiTrade #2362

client/{core, btc}: MultiTrade #2362

Conversation

martonp commented May 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp May 24, 2023 • edited Loading

Choose a reason for hiding this comment

buck54321 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

martonp Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Jun 25, 2023

chappjc commented Jun 25, 2023 • edited Loading

chappjc left a comment

Choose a reason for hiding this comment

martonp May 24, 2023 •

edited

Loading

martonp Jun 2, 2023 •

edited

Loading

martonp May 24, 2023 •

edited

Loading

buck54321 May 31, 2023 •

edited

Loading

martonp Jun 2, 2023 •

edited

Loading

chappjc commented Jun 25, 2023 •

edited

Loading