client/asset/btc: SPV Wallet Transaction History #2550

martonp · 2023-09-30T08:55:28Z

This diff implements TransactionHistory for btc SPV wallets. A database is created to store all the transactions related to the wallet. Although the SPV wallet stores some transaction information already, the list transactions output is unreliable and confusing, so it is better to not parse it every time the tx history is requested. Also, it is not possible to differentiate a send and a swap transaction.

Incoming transactions are discovered by querying ListTransactionsSinceBlock every block. There is also special handling for bond creation transactions which are not immediately submitted.

JoeGruffins · 2023-10-03T06:45:52Z

client/asset/btc/btc.go

+		btc.txHistoryDB.Store(txHistoryDB)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create tx history db: %v", err)
+		}


Do you store on err here on purpose?

JoeGruffins · 2023-10-03T07:14:19Z

client/asset/btc/btc.go

+		copyWt := *wt
+		copyWt.Submitted = true
+		btc.pendingTxs[hash] = &copyWt


This should work?

Suggested change

copyWt := *wt

copyWt.Submitted = true

btc.pendingTxs[hash] = &copyWt

wt.Submitted = true

I added a mutex.

JoeGruffins · 2023-10-03T07:34:46Z

client/asset/btc/btc.go

+	{
+		const blockQueryBuffer = 3
+		var blockToQuery uint64


Is the new block of code to scope these variables?

No I just put it there so it could be collapsed in the editor.

JoeGruffins · 2023-10-03T07:50:40Z

client/asset/btc/btc.go

+		}
+		recentTxs, err := btc.node.listTransactionsSinceBlock(int32(blockToQuery))
+		if err != nil {
+			btc.log.Errorf("Error listing transactions since block %d: %v", tip-6, err)


Suggested change

btc.log.Errorf("Error listing transactions since block %d: %v", tip-6, err)

btc.log.Errorf("Error listing transactions since block %d: %v", blockToQuery, err)

JoeGruffins · 2023-10-03T08:03:29Z

client/asset/btc/btc.go

+
+				var fee uint64
+				if tx.Fee != nil {
+					fee = toSatoshi(-*tx.Fee)


I guess fee will always be reported as positive? I think dcrwallet returns a negative for fees. I have not checked yet just thinking out loud.

Fee is also negative in btcwallet. That's why I'm negating it.

JoeGruffins · 2023-10-03T08:11:25Z

client/asset/btc/btc.go

+		if tx.BlockNumber > 0 && tip > tx.BlockNumber {
+			confs = tip - tx.BlockNumber + 1
+		}


Suggested change

if tx.BlockNumber > 0 && tip > tx.BlockNumber {

confs = tip - tx.BlockNumber + 1

}

if tx.BlockNumber > 0 && tip >= tx.BlockNumber {

confs = tip - tx.BlockNumber + 1

}

JoeGruffins · 2023-10-03T08:15:11Z

client/asset/btc/btc.go

+		} else if gtr.BlockHash == "" && tx.BlockNumber != 0 {
+			tx.BlockNumber = 0
+			updated = true
+		}


Does this indicate the tx had confs and now does not? So a reorg? If so the final if updated should not remove this one from pendingTxs?

JoeGruffins · 2023-10-03T08:16:59Z

client/asset/btc/btc.go

+// TxHistory returns all the transactions the wallet has made. This
+// includes the ETH wallet and all token wallets. If refID is nil, then


Suggested change

// TxHistory returns all the transactions the wallet has made. This

// includes the ETH wallet and all token wallets. If refID is nil, then

// TxHistory returns all the transactions the wallet has made. If refID is nil, then

JoeGruffins · 2023-10-03T11:46:07Z

client/asset/btc/txdb.go

+			if !wt.Confirmed {
+				txs = append(txs, &wt)
+			}


Shouldn't they all be unconfirmed if in the pending txn bucket?

This doesn't just look at the pending transactions. The elements with blockPrefix come before the elements with pendingPrefix. This looks at those as well.

JoeGruffins · 2023-10-03T12:02:29Z

client/asset/btc/txdb.go

+		it := txn.NewIterator(opts)
+		defer it.Close()
+
+		for it.Seek(maxPendingKey); it.Valid(); it.Next() {


Suggested change

for it.Seek(maxPendingKey); it.Valid(); it.Next() {

for it.Seek(maxPendingKey); it.ValidForPrefix(pendingPrefix); it.Next() {

This search checks both pendingPrefix and blockPrefix.

This diff implements `TransactionHistory` for btc SPV wallets. A database is created to store all the transactions related to the wallet. Although the SPV wallet stores some transaction information already, the list transactions output is unreliable and confusing, so it is better to not parse it every time the tx history is requested. Also, it is not possible to differentiate a send and a swap transaction. Incoming transactions are discovered by querying `ListTransactionsSinceBlock` every block. There is also special handling for bond creation transactions which are not immediately submitted.

buck54321 · 2023-10-17T13:39:16Z

client/asset/btc/btc.go

@@ -450,7 +450,8 @@ type BTCCloneCFG struct {
 	// OmitRPCOptionsArg is for clones that don't take an options argument.
 	OmitRPCOptionsArg bool
 	// AssetID is the asset ID of the clone.
-	AssetID uint32
+	AssetID          uint32
+	SupportTxHistory bool


This setting is only used internally. It's also curious, since we have a WalletHistorian interface, so we should just implement that on the appropriate wallets.

How about buck54321/dcrdex@2acb714...buck54321:dcrdex:btctxhistorianinterface ?

Thanks. I was just putting off testing with LTC and BCH.. I've tested now though and they are both working as well.

buck54321 · 2023-10-17T13:58:40Z

client/asset/btc/btc.go

+// If past is true, the transactions prior to the refID are returned, otherwise
+// the transactions after the refID are returned. n is the number of
+// transactions to return. If n is <= 0, all the transactions will be returned.
+func (btc *intermediaryWallet) TxHistory(n int, refID *dex.Bytes, past bool) ([]*asset.WalletTransaction, error) {


Just make this func (btc *ExchangeWalletSPV), no?

buck54321 · 2023-10-17T14:37:28Z

client/asset/btc/electrum_client.go

+func (wc *electrumWallet) listTransactionsSinceBlock(blockHeight int32) ([]btcjson.ListTransactionsResult, error) {
+	return nil, fmt.Errorf("not supported")
+}
+
 // part of btc.Wallet interface


Prefer to use a custom interface.

buck54321/dcrdex@5beef90...buck54321:dcrdex:txlister

buck54321 · 2023-10-17T15:33:51Z

client/asset/btc/txdb.go

+			if currBlockKey[0] == pendingPrefix[0] {
+				if tx.BlockNumber > 0 {
+					needNewBlockKey = true
+				}
+			} else if currBlockKey[0] == blockPrefix[0] {


How about a helper function?

func hasPrefix(b, prefix []byte) bool { if len(b) < len(prefix) { return false } return bytes.Equal(b[:len(prefix)], prefix) }

buck54321 · 2023-10-28T23:10:02Z

client/asset/btc/btc.go

+		return
+	}
+
+	var hash chainhash.Hash


buck54321 · 2023-10-28T23:11:51Z

client/asset/btc/btc.go

+	txHistoryDB := btc.txDB()
+	if txHistoryDB == nil {
+		return
+	}
+


Don't we want to add the tx to the pendingTxs map?

No, this isn't like ETH where we're using the pendingTxs to calculate a pending balance. pendingTxs is just now getting added in this PR. It's just used to keep track of which transactions in the db may need to be updated whenever there's a new block.

buck54321 · 2023-10-28T23:14:57Z

client/asset/btc/btc.go

+
+		for _, tx := range recentTxs {
+			if tx.Category == "receive" {
+				hash, err := chainhash.NewHashFromStr(tx.TxID)


buck54321 · 2023-10-28T23:18:56Z

client/asset/btc/txdb.go

+type txDB interface {
+	storeTx(tx *extendedWalletTx) error
+	markTxAsSubmitted(txID dex.Bytes) error
+	getTxs(n int, refID *dex.Bytes, past bool) ([]*asset.WalletTransaction, error)


Please document getTxs and other complex areas of code. I see a line like this in checkPendingTxs

_, err = txHistoryDB.getTxs(1, &txID, false)

and it raises a bunch of questions. Does the past argument matter if n is 1? Is refID just a starting point for listing txs, or does that exact key have to be in the db? If you are just checking for the presence of a tx in the db, wouldn't a getTx method be appropriate?

I have to look at the implementation for review anyway, but we don't need to doom others to the same fate for eternity. Good docs save man hours.

Yeah, it's just getting a single transaction, and you're right, a getTx method would be nicer. I'll add some docs.

buck54321 · 2023-10-28T23:31:55Z

client/asset/btc/txdb.go

+		log: log}, nil
+}
+
+func (db *badgerTxDB) findFreeBlockKey(txn *badger.Txn, blockNumber uint64) ([]byte, error) {


I could have sworn I already commented on this, but apparently not.

Why not just use the tx's block index?

buck54321/dcrdex@0cc91cc...buck54321:dcrdex:indexsequence

I made the change, then I realized that btcwallet is not returning the block index. I guess we could update btcwallet, ltcwallet, and bchwallet, but is it really worth it? We have to do the this free key finding loop for pending transactions anyways.

Oof. No. Just use findFreeBlockKey.

buck54321 · 2023-11-05T21:37:05Z

client/asset/btc/btc.go

@@ -5690,11 +5858,16 @@ func (btc *baseWallet) MakeBondTx(ver uint16, amt, feeRate uint64, lockTime time
 		return nil, nil, fmt.Errorf("failed to fund bond tx: %w", err)
 	}

+	var txIDToRemoveFromHistory *chainhash.Hash // will be non-nil if tx was added to history


Just noting that a tx hash != a tx ID. A tx ID is a byte-reversed string of the tx hash.

buck54321 · 2023-11-05T21:47:43Z

client/asset/btc/btc.go

+
+	pendingTxsCopy := make(map[chainhash.Hash]*extendedWalletTx, len(btc.pendingTxs))
+	btc.pendingTxsMtx.RLock()
+	for hash, tx := range btc.pendingTxs {


I'm not going to hold this up, but please don't use hash as a variable. Use h when it unambiguous what kind of hash it is, as it is here, or use txHash or blockHash anywhere. Not only is hash ambiguous in many contexts, it's also the name of a standard library Go package.

JoeGruffins approved these changes Oct 3, 2023

View reviewed changes

martonp force-pushed the btcTxHistory branch 2 times, most recently from b30df47 to 02ed2dd Compare October 6, 2023 08:31

Joe review updates.

2acb714

martonp force-pushed the btcTxHistory branch from 02ed2dd to 2acb714 Compare October 6, 2023 08:40

buck54321 requested changes Oct 17, 2023

View reviewed changes

buck54321 and others added 3 commits October 22, 2023 21:31

rectify interface and config

135bba0

sepearate txLister interface

b84cb8e

hasPrefix helper function

689b96f

buck54321 requested changes Oct 28, 2023

View reviewed changes

Buck review updates.

c762f05

buck54321 approved these changes Nov 5, 2023

View reviewed changes

buck54321 merged commit c4431ff into decred:master Nov 5, 2023
5 checks passed

martonp deleted the btcTxHistory branch January 20, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/asset/btc: SPV Wallet Transaction History #2550

client/asset/btc: SPV Wallet Transaction History #2550

martonp commented Sep 30, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

JoeGruffins Oct 3, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

JoeGruffins Oct 9, 2023

JoeGruffins Oct 3, 2023

JoeGruffins Oct 3, 2023

JoeGruffins Oct 3, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

JoeGruffins Oct 3, 2023

martonp Oct 6, 2023

buck54321 Oct 17, 2023

martonp Oct 23, 2023

buck54321 Oct 17, 2023

buck54321 Oct 17, 2023

buck54321 Oct 17, 2023

buck54321 Oct 28, 2023

buck54321 Oct 28, 2023

martonp Nov 2, 2023

buck54321 Oct 28, 2023

buck54321 Oct 28, 2023

martonp Nov 2, 2023

buck54321 Oct 28, 2023

martonp Nov 2, 2023

buck54321 Nov 2, 2023

buck54321 Nov 5, 2023

buck54321 Nov 5, 2023

	btc.log.Errorf("Error listing transactions since block %d: %v", tip-6, err)
	btc.log.Errorf("Error listing transactions since block %d: %v", blockToQuery, err)

		// TxHistory returns all the transactions the wallet has made. This
		// includes the ETH wallet and all token wallets. If refID is nil, then

	for it.Seek(maxPendingKey); it.Valid(); it.Next() {
	for it.Seek(maxPendingKey); it.ValidForPrefix(pendingPrefix); it.Next() {

client/asset/btc: SPV Wallet Transaction History #2550

client/asset/btc: SPV Wallet Transaction History #2550

Conversation

martonp commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment