Skip to content
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

{client,server}/btc: BTC Fidelity Bonds #2196

Merged
merged 8 commits into from
Mar 29, 2023
Merged

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Mar 1, 2023

This enables BTC fidelity bonds on both the client and the server. The implementation is almost identical to DCR. Notably, on the client, the sendToAddress wallet API is no longer used because sends need to respect the bond reserves. Instead, the transaction is constructed in the client and passed to the wallet through sendRawTransaction.

An UnsignedCoinID field is added to the asset.Bond struct in order to support BTC wallets that have segwit turned off.

This is still untested in the electrum wallet and BTC clones.

if tplEntry.expectInt {
switch {
case data != nil:
val, err := dcrtxscript.MakeScriptNum(data, tplEntry.maxIntBytes)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These "dcrtxscript" functions are identical in the btcd code, but unexported. I'll do a PR in the btcd repo to export them so we can use the correct ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work getting btcsuite/btcd#1956 merged.

We are on github.com/btcsuite/btcd v0.23.3. Here's the diff from v0.23.3 to v0.23.4:
btcsuite/btcd@v0.23.3...v0.23.4
and to your commit on master: btcsuite/btcd@v0.23.4...master
I think we need to wait for a v0.23.5 tag before updating, but I'm glad this is updated.

client/core/bond.go Outdated Show resolved Hide resolved
Comment on lines -391 to -393
// LegacySendToAddr sents legacy raw tx which does not have positional fee
// rate param.
LegacySendToAddr bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That reminds me, I think there was a comment in the dcr code that says something like "TODO: just use sendtoaddress?". Haha, guess that's out of the question now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah I saw that.

Copy link
Contributor

@ukane-philemon ukane-philemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

client/asset/btc/btc.go Show resolved Hide resolved
client/asset/btc/btc.go Show resolved Hide resolved
client/asset/btc/btc.go Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
server/auth/registrar.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 0.6.1 milestone Mar 16, 2023
This enbales BTC fidelity bonds on both the client and the server.
The implementation is almost identical to DCR. Noteably, on the client,
the `sendToAddress` wallet API is no longer used becuase sends need to
respect the bond reserves. Instead, the transaciton is constructed in
the client and passed to the wallet through `sendRawTransaction`.

An `UnsignedCoinID` field is added to the asset.Bond struct in order to
support BTC wallets that have segwit turned off.
}
auth.Sign(preBondRes)
preBondRes.SetSig(auth.SignMsg(append(preBondRes.Serialize(), preBond.RawTx...)))
Copy link
Member

@chappjc chappjc Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just linking back to our previous discussion of the options a #2196 (comment)
The approach implicitly hashes the RawTx but the signature's message is a little non-intuitive, whereas the suggestion from @martonp in the discussion with to put an explicit hash of this sent tx in the response payload. I preferred to omit it from the payload, but wanted to doc here.
Worth noting that there is precedent for this kind of signature with the connect response.

client/asset/btc/btc.go Outdated Show resolved Hide resolved
@chappjc chappjc modified the milestones: 0.6.1, 0.6 Mar 24, 2023
@chappjc
Copy link
Member

chappjc commented Mar 24, 2023

New plan. rc1 tomorrow, rc2 next week with this

client/asset/interface.go Outdated Show resolved Hide resolved
dex/msgjson/types.go Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testnet working well too

/api/updatebondoptions to switch from DCR with target tier 3 to BTC with target tier 5:

2023-03-29 17:00:05.619 [INF] CORE: Attempting to update bond reserves by 0.08000000 BTC (0.00000000 BTC currently locked in bonds with 0.00000000 BTC in-force) for target tier change from 3 to 5, with bond increment 0.02000000 BTC
2023-03-29 17:00:05.622 [TRC] CORE[btc][SPV]: Bals: spendable = 14.05559527 BTC (14.05559527 BTC trusted, 0 BTC untrusted, 0 BTC assumed locked), immature = 0 BTC
2023-03-29 17:00:05.622 [TRC] CORE[btc]: ReserveBondFunds(0.08, true): enforced = 0.15556 / bonded = 0 / nominal = 0.12  ==>  enforced = 0.23556 / bonded = 0 / nominal = 0.2
2023-03-29 17:00:05.622 [INF] CORE: Total reserved for dextesttaggpdj27s26wklczemovhqdzyvgokyumey33uemlw4imawqd.onion:7232 is now 0.20000000 BTC (0.08000000 BTC more in future bonds)
2023-03-29 17:00:05.622 [DBG] CORE: Bond options for dextesttaggpdj27s26wklczemovhqdzyvgokyumey33uemlw4imawqd.onion:7232: target tier 5, bond asset 0, maxBonded 40000000
2023-03-29 17:00:05.629 [TRC] CORE[btc][SPV]: Bals: spendable = 14.05559527 BTC (14.05559527 BTC trusted, 0 BTC untrusted, 0 BTC assumed locked), immature = 0 BTC
2023/03/29 12:00:05 "POST http://127.0.0.2:5758/api/updatebondoptions HTTP/1.1" from 127.0.0.1:49192 - 200 12B in 21.99913ms

Side note: I think when switching to a new asset (BTC) with live bonds in the old asset (DCR), it's not reserving enough in the new asset for when the old asset's bonds refund, but that's not an issue with this PR. It should re-reserve the right amounts on restart after the DCR bonds are refunded.

Shortly after updating bond options, rotateBonds triggers:

2023-03-29 17:00:22.918 [INF] CORE: Gotta post 1 bond increments now. Target tier 5, current tier 4 (0 weak, 0 pending)
2023-03-29 17:00:22.918 [TRC] CORE: Bond lifetime = 7h40m0s (lockTime = 2023-03-29 19:40:22 -0500 CDT)
2023-03-29 17:00:22.919 [TRC] CORE[btc]: feeRateWithFallback using caller's suggestion for fee rate, 1.
2023-03-29 17:00:22.921 [TRC] CORE[btc]: bondLocked (0.02): enforced 0.23556 ==> 0.21556 (with bonded = 0.02 / nominal = 0.2)
2023-03-29 17:00:22.921 [DBG] CORE[btc]: New bond reserves (new post) = 0.215560 BTC with 0.020000 in unspent bonds
2023-03-29 17:00:22.923 [DBG] CORE[btc]: Change output size = 31, addr = tb1q8c6kkz7234j2w90p0ldtkec9e4w9d8kvyk8rd2
2023-03-29 17:00:22.923 [DBG] CORE[btc]: 2 signature cycles to converge on fees for tx 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6: min rate = 1, actual fee rate = 1 (222 for 222 bytes), change = true

https://mempool.space/testnet/tx/2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6

prevalidate:

2023-03-29 17:00:26.373 [INF] CORE: DEX dextesttaggpdj27s26wklczemovhqdzyvgokyumey33uemlw4imawqd.onion:7232 has validated our bond 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 (btc) with strength 1. 1 confirmations required to trade.
2023-03-29 17:00:26.374 [INF] CORE: Broadcasting bond 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 (btc) with lock time 2023-03-29 19:40:22 -0500 CDT, data = 0476da2464b17576a914e82704b22735f58423bbb881abb1226c36bcc18188ac.

BACKUP refund tx paying to current wallet: 01000000000101d6...
...
2023-03-29 17:00:28.374 [DBG] CORE[btc][SPV]: No error from PublishTransaction after 2s for txn 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6. Assuming wallet accepted it.
2023-03-29 17:00:28.378 [TRC] CORE[btc][SPV]: Bals: spendable = 14.03559305 BTC (14.03559305 BTC trusted, 0 BTC untrusted, 0 BTC assumed locked), immature = 0 BTC
2023-03-29 17:00:28.380 [INF] CORE: notify: |SUCCESS| (bondpost) BondConfirming - Waiting for 1 confirmations to post bond 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 (btc) to dextesttaggpdj27s26wklczemovhqdzyvgokyumey33uemlw4imawqd.onion:7232

confirmed and accepted:

2023-03-29 17:14:30.135 [INF] CORE: DEX dextesttaggpdj27s26wklczemovhqdzyvgokyumey33uemlw4imawqd.onion:7232 bond txn 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 now has 1 confirmations. Submitting postbond request...
2023-03-29 17:14:30.769 [INF] CORE: Bond confirmed 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 (btc) with expire time of 2023-03-29 16:40:22 -0500 CDT
2023-03-29 17:14:30.770 [INF] CORE: Bond 2b1f8005471f6cfefd7b43b1b1cb598095c37b19de411d6e963f3ba1a0d7aed6:0 (btc) confirmed.
2023-03-29 17:14:30.770 [INF] CORE: notify: |SUCCESS| (bondpost) BondConfirmed - New tier = 5 (target = 5).

Refund will happen tonight.

@chappjc chappjc merged commit 4a2e0f0 into decred:master Mar 29, 2023
@chappjc chappjc mentioned this pull request Mar 29, 2023
@chappjc
Copy link
Member

chappjc commented Mar 30, 2023

Refund happened fine, as expected: https://mempool.space/testnet/tx/90e8e134493666073bf348d1c3fea8423c5c21661be063c7e5cd1441de1088a7#vin=0

We just need to confident that all the sends that now no longer use the sendtoaddress RPC are still gonna work in all cases.

@chappjc
Copy link
Member

chappjc commented Mar 30, 2023

...and this will make the cut for 0.6. On that note, I don't see why #1993 shouldn't land on 0.6 either.

@chappjc chappjc mentioned this pull request Mar 30, 2023
@chappjc chappjc added the bonds fidelity bonds label Mar 31, 2023
@martonp martonp deleted the btcBonds branch January 20, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bonds fidelity bonds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants