client/asset/dcr: native Decred SPV wallet by buck54321 · Pull Request #1633 · decred/dcrdex

buck54321 · 2022-05-29T18:07:00Z

Use dcrwallet's *wallet.Wallet in a similar fashion to the spvWallet in client/btc.

This is ready for review, but I don't want to merge until this is rebased on top of #1632 and tests implemented.

chappjc

Awesome. I just did a turbo scan through. Very excited for this. Looks straightforward based on the BTC SPV so I bet this can get in short order.

JoeGruffins

I was able to get the client into a totally unresponsive state at one point, but no idea if it was because of these changes.

Otherwise testing well.

Do you think a version somewhere needs to be upped for these changes? If I were to make an spv wallet and go back a version it would no longer be visible. I don't know what would happen if I then attempted to make another wallet and then went back to this diff.

The simnet tests are not working.

JoeGruffins · 2022-06-06T02:35:15Z

-	RPCPass          string  `ini:"password"`
-	RPCListen        string  `ini:"rpclisten"`
-	RPCCert          string  `ini:"rpccert"`
+type WalletConfig struct {


nit: Comment on exported struct.

JoeGruffins · 2022-06-06T02:35:22Z

-	if err := config.Unmapify(settings, cfg); err != nil {
-		return nil, nil, fmt.Errorf("error parsing config: %w", err)
-	}
+type RPCConfig struct {


nit: Comment on exported struct.

JoeGruffins · 2022-06-06T02:52:08Z

+// Exists checks the existence of the wallet. Part of the Creator interface.
+func (d *Driver) Exists(walletType, dataDir string, _ map[string]string, net dex.Network) (bool, error) {
+	if walletType != walletTypeSPV {
+		return false, fmt.Errorf("no Bitcoin wallet of type %q available", walletType)


Bitcoin -> Decred

JoeGruffins · 2022-06-06T03:09:17Z

+		return false, err
+	}
+	netDir := filepath.Join(dataDir, chainParams.Name)
+	// timeout and recoverWindow arguments borrowed from btcwallet directly.


comment references btcwallet.

And doesn't even apply here.

JoeGruffins · 2022-06-06T03:35:23Z

+			// hdr, err := dcr.wallet.GetBlockHeader(ctx, newTipHash)
+			// if err != nil {
+			// 	go dcr.tipChange(fmt.Errorf("error setting new tip: %w", err))
+			// 	continue
+			// }


Can remove.

JoeGruffins · 2022-06-06T05:02:14Z

+	dcrw, err := wallet.Open(ctx, walletConfig(db, w.chainParams))
+	if err != nil {
+		// If this function does not return to completion the database must be
+		// closed.  Otherwise, because the database is locked on opens, any


opens -> open

JoeGruffins · 2022-06-06T06:00:53Z

+	return w.PublishTransaction(ctx, tx, w.spv)
+}
+
+// GetBlockHeader returns block header info for the specified block hash. The


returns the/a block header

JoeGruffins · 2022-06-06T06:03:01Z

+
+	_, tipHeight := w.MainChainTip(ctx)
+	if tipHeight < int32(hdr.Height) {
+		return nil, fmt.Errorf("sumpin's wrong with our tip")


nit: Nothing to format.

JoeGruffins · 2022-06-06T06:35:25Z

+			outputTotal += dcrutil.Amount(output.Value)
+		}
+		fee = debitTotal - outputTotal
+		negFeeF64 = (-fee).ToCoin()


Can just set ret.Fee here.

JoeGruffins · 2022-06-06T07:26:30Z

+}
+
+func newSpvSyncer(w *wallet.Wallet, netDir string, connectPeers []string) *spv.Syncer {
+	addr := &net.TCPAddr{IP: net.ParseIP("::1"), Port: 0}


I have ipv6 turned off at the kernel level so I would think that I can't use this, but seems fine.

This code mirrors what dcrwallet does internally, but interestingly this address seems to be unused by dcrwallet anyway. If you trace into p2p/peering.go, and look for where extaddr is used, it seems to go nowhere. It gets passed to (*LocalPeer).newMsgVersion, but it is unused there, using net.Conn.LocalAddr() instead.

Almost as if nil would work. Maybe we keep it like this with ipv6 to match dcrwallet, but make a comment because it's clearly confusing multiple people.

chappjc · 2022-06-06T13:46:04Z

Do you think a version somewhere needs to be upped for these changes? If I were to make an spv wallet and go back a version it would no longer be visible.

I think an asset version is a value coordinated between client and server as an indication of more fundamental changes like contract structure or block/tx serializations.

chappjc · 2022-06-13T21:31:28Z

Not sure which way you are going to go for conflict resolution, but clearly the user-set account names should stay RPC-only since the SPV wallet should manage the accounts even when we tackle mixing support for the native DCR wallet.
Reviewing as-is though.

chappjc · 2022-06-13T22:21:36Z

+	ret.Amount = (creditTotal - debitTotal).ToCoin()
+	ret.Fee = negFeeF64


TLDR: I'm generally uneasy about computing anything reliably from the Credits and Debits slices (namely Fee or Amount), but fortunately the full GetTransactionResult is mostly unsued, so can we consider paring down the interface to make it both easier to satisfy and more reliable?

Lately I have concluded we should avoid relying on both the Credits and Debits slices from TxDetails (or the Amount and Fee fields that we compute from them or get from gettransaction on the RPC side) because they are very often incomplete or incorrect. (Both BTC and DCR)

As you noted above, Fee can only be determined if every input is a debit, and this is indeed trouble with mix transactions, just as an example. Inspecting one of those in my wallet, the "fee" field is omitted, and "amount" is something negative like "amount": -0.002399 which is presumably indicative of the net change to my wallet's balance.
The help for gettransaction hints at more general conditions "fee": n.nnn, (numeric) The total input value minus the total output value, or 0 if 'txid' is not a sent transaction. Mixed transactions are just one instance.

I chatted with @martonp about this here: #1555 (comment)

To be clear, I don't think the issue is what we are doing with the output from dcrWallet.TxDetails, but that it has weak assurances about what it will include in Credits and Debits, plus the conditions are different between unconfirmed and confirmed txns. In both unminedTxDetails and minedTxDetails, it just grabs what credits and debits it has, and looking through real wallet transactions they are often missing.

Anyway, for the purpose of tx fee, Marton did it the hard but reliable way by pulling prevout tnxs for the input values, and in the Electrum PR I've pared down the GetTransactionResult type because it's nigh impossible to completely satisfy. I think this is OK since consumers generally just want (1) confs, (2) block info, and (3) raw tx hex. Scanning through this PR, I think that's still the case.

chappjc

Feeling great about this. About 2/3 of it reviewed.

chappjc · 2022-06-21T15:35:04Z


-	// Get network settings. Zero value is mainnet, but unknown non-zero cfg.Net
-	// is an error.
+func loadRPCConfig(settings map[string]string, network dex.Network) (*rpcConfig, *chaincfg.Params, error) {


loadRPCConfig is unused. Hmmm

Doesn't look like newRPCWallet has all the defaults that this function provided.

chappjc · 2022-06-21T16:57:35Z

+// NOTE: Why GapPolicyIgnore instead of GapPolicyWrap? Can we recover from an
+// ignored gap policy without extreme measures?


Maybe we are trying a little too hard to avoid address re-use. In the Electrum PR, I spent some time looking at where we are burning through addresses, and I decided to add a refundAddress method to client/asset/btc.Wallet where the addresses are very rarely used and we can tolerate some reuse.

dcrdex/client/asset/btc/wallet.go

Lines 32 to 34 in c63ef10

// The refund addresses are almost never used, so we might tolerate some

// address reuse if the backend requires.

refundAddress() (btcutil.Address, error)

The other big source of wasted addresses is the address provided in orders (if canceled), when the address really could be provided closer to match time although with more messaging, so that is not as easy.

Regarding recovery, when we implement Rescanner, we can use the DiscoverAddresses method with a provided gap limit. Also, I haven't gotten far enough with the review, but we should ensure the seed can be used in an external dcrwallet for manual recovery.

JoeGruffins · 2022-06-23T05:52:53Z

+	// DRAFT NOTE: Maybe?
+	if walletCfg.PrimaryAccount == "" {
+		walletCfg.PrimaryAccount = defaultAcctName
+	}


Looks correct.

JoeGruffins · 2022-06-23T07:41:31Z

It looks like loadbot needs a go mod tidy

chappjc · 2022-06-23T14:04:50Z

It looks like loadbot needs a go mod tidy

That settles it. We're updating CI to tidy loadbot too.

Use dcrwallet's *wallet.Wallet in a similar fashion to the spvWallet in the btc client package.

chappjc

All good. Manually tested: (1) success, (2) refund maker DCR contract, and (3) find maker redemption with hacked ghosting.

buck54321 force-pushed the dcrwallet branch from 221a512 to afc0604 Compare May 29, 2022 18:14

chappjc reviewed May 29, 2022

View reviewed changes

Comment thread client/asset/dcr/dcr.go Outdated

JoeGruffins reviewed Jun 6, 2022

View reviewed changes

chappjc added this to the 0.5 milestone Jun 9, 2022

chappjc self-requested a review June 9, 2022 03:31

chappjc reviewed Jun 13, 2022

View reviewed changes

buck54321 force-pushed the dcrwallet branch from afc0604 to 313f7b2 Compare June 16, 2022 17:09

chappjc requested changes Jun 21, 2022

View reviewed changes

buck54321 force-pushed the dcrwallet branch from 313f7b2 to 204876d Compare June 22, 2022 19:44

chappjc requested a review from JoeGruffins June 22, 2022 21:21

JoeGruffins approved these changes Jun 23, 2022

View reviewed changes

chappjc reviewed Jun 23, 2022

View reviewed changes

Comment thread client/asset/dcr/spv.go

chappjc reviewed Jun 23, 2022

View reviewed changes

Comment thread client/asset/dcr/spv.go Outdated

chappjc reviewed Jun 23, 2022

View reviewed changes

Comment thread client/asset/dcr/spv.go

chappjc reviewed Jun 23, 2022

View reviewed changes

Comment thread client/asset/dcr/spv.go Outdated

buck54321 added 3 commits June 25, 2022 18:52

implement built-in dcr spv wallet

a98543f

Use dcrwallet's *wallet.Wallet in a similar fashion to the spvWallet in the btc client package.

resolve with privacy. updates from review

72de3ef

chappjc review followup

2fed940

buck54321 force-pushed the dcrwallet branch from 204876d to 709bda3 Compare June 25, 2022 23:57

more review followup

0e28766

buck54321 force-pushed the dcrwallet branch from 709bda3 to 0e28766 Compare June 26, 2022 00:12

chappjc approved these changes Jun 27, 2022

View reviewed changes

chappjc merged commit 71f0af1 into decred:master Jun 27, 2022

		ret.Amount = (creditTotal - debitTotal).ToCoin()
		ret.Fee = negFeeF64

		// NOTE: Why GapPolicyIgnore instead of GapPolicyWrap? Can we recover from an
		// ignored gap policy without extreme measures?

	// The refund addresses are almost never used, so we might tolerate some
	// address reuse if the backend requires.
	refundAddress() (btcutil.Address, error)

Conversation

buck54321 commented May 29, 2022

Uh oh!

chappjc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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!

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!

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 commented Jun 6, 2022

Uh oh!

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

Uh oh!

Uh oh!

chappjc Jun 13, 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!

chappjc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chappjc Jun 21, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc Jun 21, 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!

Uh oh!

Uh oh!

JoeGruffins commented Jun 23, 2022

Uh oh!

chappjc commented Jun 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

JoeGruffins left a comment •

edited

Loading

chappjc commented Jun 13, 2022 •

edited

Loading

chappjc Jun 13, 2022 •

edited

Loading

chappjc Jun 21, 2022 •

edited

Loading

chappjc Jun 21, 2022 •

edited

Loading