client/core/btc/ui: Accelerate Orders using CPFP #1555
Conversation
martonp
force-pushed
the
accelerate
branch
3 times, most recently
from
d252d79
to
c74a8a1
Compare
|
Will review, but could you give a bit of an overview in terms of code and the different packages and the notable changes. e.g. btc asset changes include..., core does...., this requires the db to do..., etc. Mainly to help review. With 2.5k loc added, it warrants a good deal more desc. of the code changes. I would generally break it into commits to ease review, even if the commits only work when stacked up. |
client/asset/btc/btc.go
Outdated
| return newOutput(txHash, vout, uint64(value)), nil | ||
| } | ||
|
|
||
| // changeCanBeAccelerated will return an error if the change cannot be accelerated. |
There was a problem hiding this comment.
Return values aren't doc'd quite right.
client/core/core.go
Outdated
| requiredForRemainingSwaps = lotsRemaining * swapSize * tracker.metaData.MaxFeeRate | ||
| } |
client/core/wallet.go
Outdated
| // AccelerateOrder uses the Child-Pays-For-Parent technique to accelerate an | ||
| // order if the wallet is an Accelerator. | ||
| func (w *xcWallet) AccelerateOrder(swapCoins, accelerationCoins []dex.Bytes, changeCoin dex.Bytes, requiredForRemainingSwaps, newFeeRate uint64) (asset.Coin, string, error) { |
There was a problem hiding this comment.
Do the xcWallet methods need to be exported?
There was a problem hiding this comment.
Nope. Neither do some other ones.
client/asset/btc/btc.go
Outdated
| // PreAccelerate returns the current average fee rate of the unmined swap initiation | ||
| // and acceleration transactions, and also returns a suggested range that the | ||
| // fee rate should be increased to in order to expedite mining. |
There was a problem hiding this comment.
Wrap docs at 80 characters please.
There was a problem hiding this comment.
The code looks solid to me! Great work.
I'm syncing my testnet nodes to give this a try and see if it works there.
I wonder if it's almost too much control to give the user? Maybe just an "accelerate swap" button would also work, and we could worry about the details. Just my opinion though.
|
It might be time to switch btc's explorer to mempool.space since it is CPFP aware and shows ancestor transactions and the "effective" average fee rate
Slightly bad example since there's no bumped fee there, but it illustrates how it displays such txns. |
dex/networks/btc/script.go
Outdated
| // 41 vbytes base tx input | ||
| // 109wu witness + 2wu segwit marker and flag = 28 vbytes | ||
| // total = 69 vbytes | ||
| RedeemP2PWKHInputTotalSize = RedeemP2WPKHInputSize + ((RedeemP2WPKHInputWitnessWeight + 2 + 3) / 4) |
There was a problem hiding this comment.
The "W" is misplaced in RedeemP2PWKHInputTotalSize.
Also the comment starts with a different variable name.
There was a problem hiding this comment.
I know this formula was just moved from below, but we should probably utilize the witnessWeight const we have in this file.
Also, the "segwit marker and flag" are part of a segwit txns's header, so these 2 wu are not added for each segwit input. That's why it was correct when this was only being included in the formula for InitTxSizeSegwit before, where the total wu for the entire txn were summed before down-weighting to vB.
// ceil(wu/4)
P2WPKHWitnessVBytes = (RedeemP2WPKHInputWitnessWeight + (witnessWeight - 1)) / witnessWeight
RedeemP2WPKHInputTotalSize = RedeemP2WPKHInputSize + P2WPKHWitnessVBytes
InitTxSizeSegwit = InitTxSizeBaseSegwit + RedeemP2WPKHInputSize +
(2 + RedeemP2WPKHInputWitnessWeight + (witnessWeight - 1)) / witnessWeightThere was a problem hiding this comment.
OK that makes sense. So it it would be correct to have MinimumTxOverhead = 10 and also have MinimumSegwitTxOverhead = 11, right? For example in (w *spvWallet) sendWithSubtract, the unfundedTxSize is slightly underestimated due to this. These are just tiny differences though and don't really effect anything.
There was a problem hiding this comment.
Sorta. I think it's important to sum all the witness data first though, before dividing by 4 and rounding up.
There was a problem hiding this comment.
I mean it's certainly a safe estimate if it's better to err toward high estimates
| const ( | ||
| minTimeBeforeAcceleration uint64 = 3600 // 1 hour | ||
| ) |
There was a problem hiding this comment.
One line. Also, I'm not objecting, but it raises the question to me of why it's a const and not an argument that the caller can decide on?
There was a problem hiding this comment.
The point of this was to limit the user from accelerating too frequently and too early. This is the question that @JoeGruffins and I were discussing: should we give the user full control, or should we include things like this to get them to avoid unnecessarily accelerating? What do you think?
There was a problem hiding this comment.
Ah. I think I'll have a better feeling when testing on testnet or mainnet, but I suspect I'd feel a little frustrated if I saw a large fee spike and my tx getting buried under 5 blocks worth of mempool only to have my software tell me "too soon".
On the other hand, people will click accelerate when there is absolutely no use, e.g. their txns were already gonna be in the next block, but they felt the need to "accelerate" because they didn't check mempool.space or whatever.
There was a problem hiding this comment.
How about we put a warning if they are trying to accelerate, but it's very likely that their transaction will be included in the next block?
There was a problem hiding this comment.
As I pointed out, bitpay makes you wait four hours to accelerate according to their docs. I think an hour forced is not that long. Maybe fee goes down and gets mined anyway. Just my opinion, feel free to ignore.
There was a problem hiding this comment.
very likely that their transaction
This could be tricky to characterize. Certainly the fee rate and fee rate estimates don't tell the whole story because next block could be half full, or the tx could be 5 blocks worth of mempool deep but the "optimal" fee according to an oracle is only a little higher.
I was tempted to suggest in the desc/tooltip that the user should consult a mempool oracle to make an informed decision, because that's personally what I would do.
There was a problem hiding this comment.
I'm thinking to still check if the acceleration is too soon, and instead of completely blocking them, have a second popup prompting them to check the mempool oracle to make sure they want to accelerate.
client/asset/btc/btc.go
Outdated
| // the additional swaps that will be required to complete the order. | ||
| // | ||
| // The returned change coin may be nil, and should be checked before use. | ||
| func (btc *baseWallet) AccelerateOrder(swapCoins, accelerationCoins []dex.Bytes, changeCoin dex.Bytes, requiredForRemainingSwaps, newFeeRate uint64) (asset.Coin, string, error) { |
There was a problem hiding this comment.
I wonder if CPFP is not universally supported among all "clones", e.g. BCH https://github.com/bitcoin-cash-node/bitcoin-cash-node/releases/tag/v23.0.0
If we do need to selectively enable this (not sure), this might not be able to be a method of *baseWallet if we don't want all clones to get the capability via ExchangeWalletFullNode. Then again if a resolution looks tricky or messy, I'd probably be OK with an error return depending on a flag like supportsCPFP. I'm thinking we just leave it like you have it for now but consider the possibility.
I don't quite understand if BCH has a mechanism to support effective fee rates for unconfirmed txn chains or not. They ditched the complex CPFP algorithms in the miner code, and use some "modified_feerate" instead. It's not clear if bumping has any effect or if the algorithm just changed. In either case, the txns would be fine, but the low fee parents might not get the intended "bump".
There was a problem hiding this comment.
I've added a new function to the interface, CanAccelerate which if false will not enable the Accelerator trait. It definitely seems like there's no point in using CPFP in bitcoin cash. It definitely seems like CPFP will no longer work in BCH.
There was a problem hiding this comment.
How about this instead?
https://github.com/martonp/dcrdex/compare/accelerate...buck54321:accelerate-composed?expand=1
There was a problem hiding this comment.
That's way nicer, thanks.
client/core/core.go
Outdated
| err = tracker.db.UpdateOrder(tracker.metaOrder()) | ||
| if err != nil { | ||
| c.log.Errorf("AccelerateOrder: failed to update order in database: %v", err) | ||
| } |
There was a problem hiding this comment.
It's hosed either way. If the new change coin isn't stored but the txn is broadcast, everything's wrecked for this trade.
|
Trivial conflicts with DOGE merged. Nothing that holds up review, but does need to be resolved. |
martonp
force-pushed
the
accelerate
branch
2 times, most recently
from
3ac1547
to
30a3057
Compare
So, I can't seem to make a tx that doesn't get mined on testnet. My tx are getting mined just fine at 1sat/vB, and anything under that seems to not propagate on the network at all. If anyone has any ideas... I guess could try on mainnet and just be out some tx fees... |
|
@JoeGruffins As long as it's creating the child transaction and then you see that the effective rate is what you expect it to be, then I would say it's behaving properly. |
client/asset/btc/btc.go
Outdated
| if tx.Confirmations > 0 { | ||
| continue | ||
| } | ||
| feesAlreadyPaid += toSatoshi(math.Abs(tx.Fee)) |
There was a problem hiding this comment.
Dang, I was planning to remove the Fee field from the GetTransactionResult on account of it being a burden for all btc.Wallet implementations to get reliably. 2e857bd#diff-c81bc3516947f2e02649820742fd5b70b5605438a0de497900940e9fca862dc8L1823
For instance, in (*spvWallet).getTransaction there seem to be no guarantees about the correctness/availability of the Fee (or Amount or Details) fields. The method (*spvWallet).getTransaction really looks "best effort" and there are a lot of conditions where these fields can deviate from truth.
dcrdex/client/asset/btc/spv.go
Lines 1836 to 1837 in 4548c2b
I recognize that the only other way to get a txn's prevout's values it to pull those funding transactions as well, and that's not nice either, but it might be better than trying to make heads or tails of the wtxmgr.TxDetails we get out of btcwallet in spvWallet.
However, if this is the only feasible option for getting a txn fee, and we can ensure that a swap tx we produce will hit all the necessary conditions to give a correct fee in spvWallet, OK I guess we can continue with this field. I don't feel great about it presently, but I'm leaving that commented option in the planned diff I linked to.
Can we be certain that we'll always get a correct Fee field from btcwallet from a string of unconfirmed swap txns?
There was a problem hiding this comment.
(*spvWallet).getTransaction does look a bit sketchy. I'll add a function that looks up the prevouts and calculates fees.
There was a problem hiding this comment.
Cool, hopefully it works out. The getTransaction method is written well itself, but the TxDetails btcwallet method it uses as the source is a bit sketch and getTransaction notes some of the quirks. Referring to https://github.com/btcsuite/btcwallet/blob/af5562928b707e0b56c1e44052ece3205bb66552/wtxmgr/query.go#L212 and the helpers used there where there seem to be many ways to end up with not all the debits or credits listed.
Although now I'm uncertain how to change this without breaking NVM, this bit is fine for spvWalletoutputSpendStatus.
client/asset/btc/btc.go
Outdated
| } | ||
|
|
||
| var earlyAcceleration *asset.EarlyAcceleration | ||
| tooEarly, isAcceleration, timePast, err := tooEarlyToAccelerate(txs, accelerationCoins) |
There was a problem hiding this comment.
If my read is right, isAcceleration == len(accelerationCoins) > 0
There was a problem hiding this comment.
What if any accelerations in the order were already mined, but another swap was initiated after?
client/asset/btc/btc.go
Outdated
| } | ||
| } | ||
|
|
||
| btc.fundingMtx.RLock() |
There was a problem hiding this comment.
Need a comment above this line explaining what the next step in the logic is. We're not planning to spend these utxos right? Just that we might need one or a few extra in addition to the swap change? Looks like these 40 or so lines are to get an estimate of maxRate, which is likely to be enormous and thus unused. Any other way to approach this?
There was a problem hiding this comment.
I've redone this bit, let me know what you think. There's a new function, maxAccelerationRate.
Adds support for Accelerating BTC transactions using the Child-Pays-For-Parent technique. Adds a new `Accelerator` wallet trait which includes three functions: `AccelerateOrder` which creates a new transaction sending money to the wallet's change address with a high fee in order to expedite mining, `PreAccelerate`, which gives guidelines to the user, and `AccelerationEstimate` which returns how much it will cost to accelerate the order to a certain fee rate. On the UI, the orders page is updated with a button which shows up if the order is able to be accelerated. When clicking the button, a popup shows up which allows the user to accelerate an order.
|
|
||
| func accelerateOrder(btc *baseWallet, swapCoins, accelerationCoins []dex.Bytes, changeCoin dex.Bytes, requiredForRemainingSwaps, newFeeRate uint64) (asset.Coin, string, error) { | ||
| btc.fundingMtx.Lock() | ||
| defer btc.fundingMtx.Unlock() |
There was a problem hiding this comment.
This is OK I guess, and it's roughly the same amount of stuff happening under lock in e.g. FundOrder (esp. with split enabled), but in general, we're doing a ton with this mutex locked. The broadcast can take ~seconds, and any RPC is generally "long" in terms of lock contention. And this all blocks swaps on completely unrelated orders.
I have no suggestions how to change this given I think we want both the fundingCoins map and the results of listlockunspent fixed until the acceleration is sent. It's just notable and something to consider reworking in the future.
There was a problem hiding this comment.
This looks to be working extremely well: https://mempool.space/testnet/tx/eb640b2254c571eb984c3c337ba68c526d6ac37d111f78ea0e098bb4bdc699ed
I noticed that clicking on the accelerated coin id's opens the explorer in the same tab, while funding is in a new tab. I think it would be nice if the accelerated coins also opened a new tab.
client/asset/btc/btc.go
Outdated
| @@ -609,9 +613,16 @@ type ExchangeWalletFullNode struct { | |||
| *baseWallet | |||
| } | |||
|
|
|||
| // Check that wallets satisfy their supported interfaces. | |||
| // ExchangeWalletAccelerator implements the Accelerator interface on an | |||
| // ExchangeWAlletFullNode. | |||
There was a problem hiding this comment.
ExchangeWAlletFullNode -> ExchangeWalletFullNode
client/asset/btc/btc.go
Outdated
| // transaction with a fee high enough so that the average fee of all the | ||
| // unconfirmed transactions in the chain and the new transaction will have | ||
| // an average fee rate of newFeeRate. The changeCoin argument is the latest | ||
| // chhange in the order. It must be the input in the acceleration transaction |
| if err != nil { | ||
| return makeError(err) | ||
| } | ||
| // Is it safe to assume that transactions will all have some fee? |
There was a problem hiding this comment.
On testnet at least, any tx with fee under 1 sat/vbyte will not make it to even the memory pool and would not be accelerate able. So yes?
client/asset/interface.go
Outdated
| // transaction with a fee high enough so that the average fee of all the | ||
| // unconfirmed transactions in the chain and the new transaction will have | ||
| // an average fee rate of newFeeRate. The changeCoin argument is the latest | ||
| // chhange in the order. It must be the input in the acceleration transaction |
|
Works flawlessly. |
Adds support for Accelerating BTC transactions using the Child-Pays-For-Parent technique.
client/asset:Acceleratorwallet trait is added which includes three functions:AccelerateOrder: Takes the swap transactions, previous acceleration transactions, and the change coin of an order, and creates a new transaction using the change coin that will raise the average fee rate of this chain of transactions to the rate requested by the userPreAccelerate: Returns the current average rate of unmined swap transactions in the order, the current recommended rate, and a suggested range of rates that the user can accelerate to. This information is used to create the UI.AccelerationEstimate: Returns how much it will cost to accelerate the order to a certain fee rate. This is also displayed on the UI. It is called whenever the user moves the slider.client/core:Acceleratorinterface.Orderstruct is updated with a new field,AccelerationCoins, which keeps track of all the acceleration transactions that were done on an order. This is necessary in order to be able to accelerate an order that has been accelerated previously.client/dbAccelerationCoinsfield that was added to the order struct.client/webserverAcceleratorinterface.client/btc:Acceleratorinterface.app:Limitiations: