-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assets: implement segwit btc swap contracts #741
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on first pass. Not tested though.
A basic perfect match (complete fill) worked as expected with the redeem txn being authored with a p2wsh contract. The redeem txn:
Note the empty Here is the swap txn that creates the contract output:
Note that this swap was funded by a p2sh-p2wpkh output (bip16 p2sh wrapping segwit p2wpkh), so the size was larger than Going to try some partial fills next... |
Trivial conflicts with #729 now merged. |
This implements segwit as an option for the wallet backends and server's asset backends. Bitcoin is configured to use segwit by defualt. Litecoin likely will too, and I've tested it, but I'm leaving it in non-segwit just to demonstrate the option. It's important to offer the option, because even though some bitcoin forks won't have segwit, our clone backend might still be able to accommodate.
I did run into some trouble when testing with tx split enabled and fee rate at max fee rate (set max fee rate for btc in markets.json to say 3). First issue was with a 9-lot/9-swap order funded with tx split. It got through the swaps fine up until the final swap and it wasn't able to fund the swap.
The successful swaps that came before that looked like the following:
It's unclear why 459 and 366 don't agree, but either way it only funded for 351 in fees. Second was a single lot order funded with tx split. This time it got to the "impossible place":
The root of these issues with max fee rate + tx split may very well be on master though... |
Is this the simnet harness? Did you use a rate of 1.0? Regardless, 375 sats to fund a swap with a p2wpkh-spending input (152 bytes) is nonsense. How did
|
I'd missed some accounting, but I think I've got it all now. The I've added tests for split tx funding edges. I did a 10-lot, 1-lot per match chain with no issue at the end, so hopefully what you saw earlier was fixed with the accounting fixes. |
@@ -157,7 +168,7 @@ const ( | |||
// - 1 wu compact int encoding value 33 | |||
// - 33 wu serialized compressed pubkey | |||
// NOTE: witness data is not script. | |||
RedeemP2WPKHInputWitnessWeight = 1 + DERSigLength + 1 + 33 // 108 | |||
RedeemP2WPKHInputWitnessWeight = 1 + 1 + DERSigLength + 1 + 33 // 109 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing bug here wasn't helping anything.
// 41 vbytes base tx input | ||
// 109wu witness + 2wu segwit marker and flag = 28 vbytes | ||
// total = 153 vbytes | ||
InitTxSizeSegwit = InitTxSizeBaseSegwit + RedeemP2WPKHInputSize + ((RedeemP2WPKHInputWitnessWeight + 2 + 3) / 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to add the 2 marker and flag weight units and take the final vsize calculation for the transaction at the same time. We can't, say, assign a variable P2WPHKInputVBytes = RedeemP2WPKHInputSize + ((RedeemP2WPKHInputWitnessWeight + 2 + 3) / 4)
, because if there were multiple inputs, it would count the marker and flag for each input. We also can't create P2WPHKInputVBytes
without the marker and flag bytes. because the bytes must be added in before rounding.
return txOut.Value*1000/(3*int64(totalSize)) < int64(minRelayTxFee) | ||
return txOut.Value/(3*int64(totalSize)) < int64(minRelayTxFee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was bug. I know there's some motivation to move to a per kB unit throughout, but that would require quite an effort. For now, I'll just bring this function in-line with our current practice of using sats/vbyte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working great now. Order funding is exact now, and the lack of a change output on the final swap provides a sort of implicit buffer to the fees since order funding still assumes a change output on the final swap. This is good IMO.
Ran multi-lot partial maker fill swaps with max fee rate and tx split, with both cases of DCR and BTC makers. Both succeeded.
Using segwit contracts saves us something like 30% in transaction fees. This will give us a little bit of wiggle room on Bitcoin markets where on-chain fees are outrageous.
Segwit is implemented as an option for the wallet backends and server's asset backends. Bitcoin is configured to use segwit by defualt. Litecoin will too, and I've tested it, but I'm leaving it in non-segwit just to demonstrate the option. It's important to offer the option, because even though some bitcoin forks won't have segwit, our clone backend might still be useful.