fidelity bonds foundation: integration, client db by chappjc · Pull Request #1820 · decred/dcrdex

chappjc · 2022-08-24T20:02:15Z

Further breaking up #1480 for feasible review, this piece builds on both #1818 and #1819, and deals with:

Updating client core with methods to create and post bond using new asset methods from 1818 and comms from 1819, and machinery for monitoring and refunding active bonds (see client/core/bond.go)
Update client DB to manage bonds
Add dexc RPCs and HTTP handlers for posting bond

These are the commits beginning with client/db: storing, confirming, and refunding bonds.

To make this transition smoother, the client and server retain all the legacy registration fee machinery, and the client's continues to use the legacy registration system.

For follow-up work

Auto add bond in client.Core. There are comments in the code regarding an option to automatically postbond when previous bonds are about to expire on DEX. There are questions pertaining to timing, UI, and UX. Presently this PR requires using dexcctl to call the postbond RPC (or a direct http API call to the endpoint of the same name, which is created for said UI plans).

As suggested by @buck54321 on the prerequisite PRs, an asset.RenewBond method could be devised for said bond maintenance code to do teh refund and post of a new bond in a single txn.

See the project board at DCRDEX Fidelity Bonds.

chappjc · 2022-09-16T14:47:00Z

This PR is based on both #1818 and #1819, and it can be used to test those pieces with dexcctl e.g. ./dexcctl --simnet -p a postbond 127.0.0.1:17273 1000000000 42.
As such, taking out of draft.
There are clearly many steps that need to follow this work to have bonds working in product, as described in the PRs, including bond maintenance, a possible RenewBond method, UI components, et.

buck54321

I know this is still draft, but I was browsing and made some notes. Conceptually, this looks good. I didn't dig too deeply in the DB bits, but the interface looks about right, I think.

buck54321 · 2022-11-05T09:47:20Z

-	DEXPubKey        Bytes     `json:"pubkey"`

-	RegFees map[string]*FeeAsset `json:"regFees"`
+	BondExpiry uint64                `json:"bondExpiry"`


Does this need to be specified. We have the dex package consts.

Please see new comment here: #1819 (comment)

EDIT: wow this reply was part of a pending review for several days. GitHub can be so irritating.

buck54321 · 2022-11-05T09:56:08Z

+			newRefundTx, err := wallet.RefundBond(ctx, bond.Version, bond.CoinID, bond.Script, bond.Amount, priv)
+			bondAlreadySpent := errors.Is(err, asset.CoinNotFoundError) // or never mined!
+			if err != nil && !bondAlreadySpent {
+				c.log.Errorf("Failed to generate bond refund tx: %v", err)
+				continue
+			}
+
+			// If the user hasn't already manually refunded the bond, broadcast
+			// the refund txn. Mark it refunded and stop tracking regardless.
+			if bondAlreadySpent {
+				c.log.Warnf("Bond output not found, possibly already spent or never mined! "+
+					"Marking refunded. Backup refund transaction: %x", bond.RefundTx)
+			} else {
+				refundCoinID, err := wallet.SendTransaction(newRefundTx)


It's not clear to me what is the benefit of doing this in two steps instead of one.

RefundBond(...) (coinID string, err error)

It can be, but I was after a method that can prepare (as in advance of lock expiry) the bond transaction. The method is more versatile this way when you consider command line tools, recovery actions, bond export/backups etc. Not a big deal to change this later if needed though; it's just generally more useful this way, plus it is a bit more consistent with the mode of operation of the bond creation methods.

buck54321 · 2022-11-05T10:15:57Z

+	if coinNotFound {
+		// Broadcast the bond and start waiting for confs.
+		c.log.Infof("Broadcasting bond %v (%s), script = %x.\n\n"+
+			"BACKUP refund tx paying to current wallet: %x\n\n",
+			coinIDStr, unbip(bond.AssetID), bond.BondData, bond.RedeemTx)
+		if _, err = wallet.SendTransaction(bond.SignedTx); err != nil {
+			c.log.Warnf("Failed to broadcast bond txn: %v")


Feels funny to use this as the indicator for whether to broadcast the bond. Lemme see if I've got the reasoning. If 1) this is the first time we're broadcasting, this should obviously work. If 2) we are picking up monitoring on a transaction that had already been broadcast, but has not been picked up by the server's node, then a) If we're using a full node, we should get an error, which will be logged as just a warning. If b) we're using an SPV node, there will be no error or log. In the end, the result is the same, worst that can happen is a warning. Sound right?

Yah the reasoning is right, but fair point that this feels funny to start this off with a CoinNotFound -> YOLO broadcast. Since the broadcast should have been done in (c *Core) PostBond first, the actions here shouldn't be necessary except for weird edges like clearing full node mempool, spv wallet recovery (wiping data files), etc.
I suspect it would be alright without this though. Will revisit to see if I had other reasoning that I failed to doc.

chappjc · 2022-11-05T14:24:35Z

I know this is still draft, but I was browsing and made some notes. Conceptually, this looks good. I didn't dig too deeply in the DB bits, but the interface looks about right, I think.

Thanks. It was ready for review previously, I just have it back in draft because I haven't re-tested. Very happy to have reviews, but won't be surprised if something small is broken and it doesn't work like it did before.

The two prior PRs were tested using this PR to actually use bonds to get registered on a server.

chappjc · 2022-11-05T16:43:08Z

Only rebased, no substantive changes.

chappjc · 2022-11-14T15:53:30Z

Just rebased and added some fixup commits, but not ready for re-review. Need to split the postbond route as per #1819 (comment)

chappjc · 2022-11-15T17:23:29Z

+		// Still pending on server. Start waiting for confs.
+		c.log.Debugf("Preparing to post pending bond %v (%s).", bondIDStr, symb)
+		c.monitorBondConfs(dc, assetBond(bond), bondAsset.Confs)


Tested this case by forcing a server side error on initial bond submission, then on reconnect, retry and success:

[INF] CORE: Authenticated connection to 127.0.0.1:17273, acct 9f8d5cbea78335c1c3c789ef1b08c193468adddc6a892e006433bd4ca82e5cb2, 0 active bonds, 0 active orders, 0 active matches, score -1, tier [DBG] CORE: Starting coin waiter for pending bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr), (known to server = false) [DBG] CORE: Preparing to post pending bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr). [DBG] CORE: Subscribing to the dcr_btc order book for 127.0.0.1:17273 [DBG] CORE[dcr_btc][book]: Processing 0 cached order notes [INF] CORE: Bond confirmed ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) with expire time of 2022-11-15 10:54:09 -0600 CST [INF] CORE: Bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) confirmed. [INF] CORE: notify: |SUCCESS| (bondpost) BondConfirmed - New tier = 2.

chappjc · 2022-11-15T17:24:14Z

+
+	reqConfs := bondAsset.Confs
+	bondCoinStr := coinIDString(bond.AssetID, bond.CoinID)
+	c.log.Infof("DEX %v has validated our bond %v (%s) with strength %d. %d confirmations required to trade.",


2022-11-15 10:50:09.795 [INF] CORE: DEX 127.0.0.1:17273 has validated our bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) with strength 1. 1 confirmations required to trade. 2022-11-15 10:50:09.796 [INF] CORE: Broadcasting bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) with lock time 2022-11-15 10:58:09 -0600 CST, data = 0421c57363b17576a914fc39ce555e3be56aca916c7f90d64ca0d2ecc9cd88ac. BACKUP refund tx paying to current wallet: 0100000001e329d2180b19ef8a502392d3be8281b2f99bfc052ab324053a974cdc086c9aca0000000000feffffff0164b69a3b0000000000001976a914ad75136d6b4be5c97a2121028926db02414e3f3988ac21c57363000000000100ca9a3b0000000000000000ffffffff8c483045022100dfbf624943905b66aa86c74a2920ab6e0a66430599e278575798b63e49547d7f02204277efa7ae463af39a541078a4b38981c1e26bdf2b8dc989beddd6bf1fdd8868012102a9dbec0914201023d67974b8b6b225673cd7cb9e186e16e30936692980f56d70200421c57363b17576a914fc39ce555e3be56aca916c7f90d64ca0d2ecc9cd88ac 2022-11-15 10:50:09.800 [TRC] CORE: notify: |DATA| (balance) balance updated 2022-11-15 10:50:09.801 [INF] CORE: notify: |SUCCESS| (bondpost) BondConfirming - Waiting for 1 confirmations to post bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) to 127.0.0.1:17273

chappjc · 2022-11-15T17:25:29Z

+					c.log.Errorf("Failed to broadcast bond refund txn %v: %v", newRefundTx, err)
+					continue
+				}
+				c.log.Infof("Bond %v refunded in %v (%s)", bondIDStr, coinIDString(assetID, refundCoinID), unbip(bond.AssetID))


[INF] CORE: Bond ca9a6c08dc4c973a0524b32a05fc9bf9b28182bed39223508aef190b18d229e3:0 (dcr) refunded in 0c21facb520a6e9ae79e81dbd9860a9c7e958b951cb12947e93a96b4c2ef07a3:0 (dcr)

Note that refundExpiredBond would be adapted to "bond maintenance" by renewing bonds directly instead of refunding.

chappjc · 2022-11-15T17:50:24Z

-	redemptionReservesKey  = []byte("redemptionReservesKey")
-	refundReservesKey      = []byte("refundReservesKey")
-	byteTrue               = encode.ByteTrue
-	backupDir              = "backup"
-	disabledRateSourceKey  = []byte("disabledRateSources")
-	walletDisabledKey      = []byte("walletDisabled")
-	programKey             = []byte("program")


Sorry for bloating the diff here, but we had bucket keys, value keys, and values all mixed up in this var block.

martonp

An issue I found:
If I initialize a new dexc, then post a bond I get no error. If I post a second bond, I will get this error:
failed to store account 6789063d3271c851419b0d7d6ddbd1d6392e5f61732c7bd0125013eea9997852 for dex 127.0.0.1:17273: failed to create account bucket

If I restart dexc, I can keep posting as many bonds as I want.

martonp · 2022-11-24T16:11:15Z

+		return nil, newError(bondTimeErr, "lock time of %d has already passed the server's expiry time of %v (bond expiry %d)",
+			form.LockTime, expireTime, bondExpiry)
+	}
+	if lockTime.Add(-time.Minute).Before(expireTime) {


Could use Sub instead of Add. Also, I think an hour gap would be better.. is a minute even enough to do a trade?

Could use Sub instead of Add. Also, I think an hour gap would be better.. is a minute even enough to do a trade?

An hour sounds good.

(Time).Add(Duration) Time takes a duration and gets a Time so I can use Before to be expressive. But (Time).Sub(Time) Duration compares the two Times and then I'd have to do < or > with the Durations. Docs for Sub say // To compute t-d for a duration d, use t.Add(-d)., so even though adding a negative is odd, I prefer to return a Time so I can use Before.

Oh, I couldn't just switch to an hour because the times are meant to be fairly fast on simnet. I'll make some network dependent limits shortly.

Ah yeah, better to leave it short on simnet.

chappjc · 2022-11-25T17:01:58Z

An issue I found: If I initialize a new dexc, then post a bond I get no error. If I post a second bond, I will get this error: failed to store account 6789063d3271c851419b0d7d6ddbd1d6392e5f61732c7bd0125013eea9997852 for dex 127.0.0.1:17273: failed to create account bucket

If I restart dexc, I can keep posting as many bonds as I want.

Good catch! Will fix and update tests for this. Thanks for reviewing and testing!

martonp

Working well, no issues while testing.

martonp · 2022-11-27T21:59:59Z


 // AccountExport is used to retrieve account by host for export.
-func (c *Core) AccountExport(pw []byte, host string) (*Account, error) {
+func (c *Core) AccountExport(pw []byte, host string) (*Account, []*db.Bond, error) {


Why not stick the bonds into Account?

martonp · 2022-11-27T22:38:51Z

+		// EXPERIMENT: Server knows of a bond we do not! Store what we can for
+		// tier accounting, but we can't redeem it. We'll make en entry in
+		// dc.acct.bonds for tier accounting, but this may not work out. i.e.
+		// What handling in the bond monitor / refund loop do we need to discard
+		// these after expiry?


Shouldn't we make an attempt to find the keys that created this?
If it cannot be found, it could be removed when a tier change note or an expired bond note arrives.

Shouldn't we make an attempt to find the keys that created this?

We can search our DB, but they should have already been loaded into localBondMap, otherwise it would have already been redeemed and we don't need the keys, so this whole case feels like server spamming.

I think I just wanted to have it in our active bonds slice so we can locally match the bond-based tier that the server seems to have.

If it cannot be found, it could be removed when a tier change note or an expired bond note arrives.

That's a good plan. Just have the bond monitor / refund loop ignore anything without a PrivKey or a RefundTx, and let them stay until server says they are not longer active.

I meant to search using RefundBondByCoinID once it exists. But yes I think it's good to have in the active bonds slice.

chappjc · 2022-11-28T15:50:10Z

Fix-ups just pushed are not yet tested but address review items.

martonp

LGTM

chappjc · 2022-12-02T16:35:43Z

Rebased, squashed, and re-tested with both account creation with bond and adding bond to legacy accounts.

Existing legacy account

New account

Re-verified automatic refunds, and tier adjustment.

Next steps will be multifaceted. UI, updated messaging (suspended -> sub-tier or whatever), client bond maintenance, settings, switches, etc.

I will begin client db and core work for maintaining a certain tier with limits on total bonding amount.

chappjc mentioned this pull request Aug 24, 2022

fidelity bonds foundation - DCR only, no frontend, manual bond replacement #1480

Closed

chappjc force-pushed the bond-client-integ branch from 113df67 to 7c53fa0 Compare August 25, 2022 15:41

chappjc force-pushed the bond-client-integ branch from 7c53fa0 to f4c0919 Compare September 2, 2022 01:16

chappjc added this to the 0.6/1.0 milestone Sep 6, 2022

chappjc mentioned this pull request Sep 7, 2022

fidelity bonds foundation: define bond tx structure, create and parse #1818

Merged

chappjc force-pushed the bond-client-integ branch 2 times, most recently from 1c6aaf5 to 7843ab0 Compare September 9, 2022 22:18

chappjc force-pushed the bond-client-integ branch from 7843ab0 to 9610fb5 Compare September 16, 2022 14:44

chappjc marked this pull request as ready for review September 16, 2022 14:47

chappjc marked this pull request as draft October 12, 2022 16:10

chappjc force-pushed the bond-client-integ branch from 9610fb5 to c8e7231 Compare October 16, 2022 21:00

buck54321 reviewed Nov 5, 2022

View reviewed changes

chappjc force-pushed the bond-client-integ branch from c8e7231 to 4b8e7f6 Compare November 5, 2022 16:42

chappjc force-pushed the bond-client-integ branch from 4b8e7f6 to 30db927 Compare November 14, 2022 15:52

chappjc force-pushed the bond-client-integ branch 5 times, most recently from 8fbcd9e to 47d86bc Compare November 15, 2022 16:54

chappjc marked this pull request as ready for review November 15, 2022 17:20

chappjc commented Nov 15, 2022

View reviewed changes

chappjc force-pushed the bond-client-integ branch from 47d86bc to f932748 Compare November 16, 2022 21:00

chappjc mentioned this pull request Nov 16, 2022

fidelity bonds foundation: comms, account tiering, server tracking #1819

Merged

chappjc force-pushed the bond-client-integ branch 4 times, most recently from 5647f97 to 86a584f Compare November 18, 2022 21:04

martonp reviewed Nov 25, 2022

View reviewed changes

chappjc force-pushed the bond-client-integ branch from 86a584f to 075c282 Compare November 25, 2022 20:15

martonp reviewed Nov 27, 2022

View reviewed changes

JoeGruffins reviewed Nov 28, 2022

View reviewed changes

chappjc force-pushed the bond-client-integ branch from 075c282 to 3de34ca Compare November 28, 2022 15:49

martonp approved these changes Nov 30, 2022

View reviewed changes

JoeGruffins approved these changes Dec 1, 2022

View reviewed changes

chappjc added 5 commits December 2, 2022 10:14

client/db: storing, confirming, and refunding bonds

b0f9c11

client/core: postbond to register and add bond

6ab3f0c

client/asset: tweak bond data field name

1baf6f9

client/rpcserver: bondassets and addbond, update register

4a01108

client/webserver: add addbond, update register

fc77e85

chappjc force-pushed the bond-client-integ branch from 3de34ca to fc77e85 Compare December 2, 2022 16:20

chappjc merged commit 39030ca into decred:master Dec 2, 2022

chappjc deleted the bond-client-integ branch December 2, 2022 16:37

chappjc mentioned this pull request Dec 8, 2022

Client-side bonding level/tier maintenance - auto posting bonds to keep specified tier #1990

Closed

chappjc added the bonds fidelity bonds label Feb 20, 2023

Uh oh!

Conversation

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

Uh oh!

For follow-up work

Uh oh!

chappjc commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 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!

Choose a reason for hiding this comment

Uh oh!

chappjc Nov 17, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Nov 5, 2022 • 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!

chappjc commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Nov 5, 2022

Uh oh!

chappjc commented Nov 14, 2022

Uh oh!

chappjc Nov 15, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

chappjc Nov 15, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

martonp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chappjc commented Nov 25, 2022

Uh oh!

martonp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chappjc commented Aug 24, 2022 •

edited

Loading

chappjc commented Sep 16, 2022 •

edited

Loading

buck54321 left a comment •

edited

Loading

chappjc Nov 17, 2022 •

edited

Loading

chappjc Nov 5, 2022 •

edited

Loading

chappjc commented Nov 5, 2022 •

edited

Loading

chappjc Nov 15, 2022 •

edited

Loading

chappjc Nov 15, 2022 •

edited

Loading

chappjc commented Dec 2, 2022 •

edited

Loading