Merge pull request #1114 from mohamedawnallah/fix-gettransaction-confirmations

wallet: resolve root cause of broken zero-conf and inflated confirmations in `GetTransaction` API

Author: yyforyongyu

wallet/createtx.go

@@ -374,12 +374,12 @@ func (w *Wallet) findEligibleOutputs(dbtx walletdb.ReadTx,
 		// Only include this output if it meets the required number of
 		// confirmations. Coinbase transactions must have reached
 		// maturity before their outputs may be spent.
-		if !confirmed(minconf, output.Height, bs.Height) {
+		if !hasMinConfs(minconf, output.Height, bs.Height) {
 			continue
 		}
 		if output.FromCoinBase {
 			target := int32(w.chainParams.CoinbaseMaturity)
-			if !confirmed(target, output.Height, bs.Height) {
+			if !hasMinConfs(target, output.Height, bs.Height) {
 				continue
 			}
 		}

wallet/utxos.go

@@ -32,8 +32,10 @@ type OutputSelectionPolicy struct {
 	RequiredConfirmations int32
 }
 
-func (p *OutputSelectionPolicy) meetsRequiredConfs(txHeight, curHeight int32) bool {
-	return confirmed(p.RequiredConfirmations, txHeight, curHeight)
+func (p *OutputSelectionPolicy) meetsRequiredConfs(txHeight,
+	curHeight int32) bool {
+
+	return hasMinConfs(p.RequiredConfirmations, txHeight, curHeight)
 }
 
 // UnspentOutputs fetches all unspent outputs from the wallet that match rules

wallet/wallet.go

@@ -1716,10 +1716,16 @@ func (w *Wallet) CalculateAccountBalances(account uint32, confirms int32) (Balan
 			}
 
 			bals.Total += output.Amount
-			if output.FromCoinBase && !confirmed(int32(w.chainParams.CoinbaseMaturity),
-				output.Height, syncBlock.Height) {
+			if output.FromCoinBase && !hasMinConfs(
+				int32(w.chainParams.CoinbaseMaturity),
+				output.Height, syncBlock.Height,
+			) {
+
 				bals.ImmatureReward += output.Amount
-			} else if confirmed(confirms, output.Height, syncBlock.Height) {
+			} else if hasMinConfs(
+				confirms, output.Height, syncBlock.Height,
+			) {
+
 				bals.Spendable += output.Amount
 			}
 		}
@@ -2120,8 +2126,11 @@ func (c CreditCategory) String() string {
 // this package at a later time.
 func RecvCategory(details *wtxmgr.TxDetails, syncHeight int32, net *chaincfg.Params) CreditCategory {
 	if blockchain.IsCoinBaseTx(&details.MsgTx) {
-		if confirmed(int32(net.CoinbaseMaturity), details.Block.Height,
-			syncHeight) {
+		if hasMinConfs(
+			int32(net.CoinbaseMaturity), details.Block.Height,
+			syncHeight,
+		) {
+
 			return CreditGenerate
 		}
 		return CreditImmature
@@ -2146,7 +2155,9 @@ func listTransactions(tx walletdb.ReadTx, details *wtxmgr.TxDetails, addrMgr *wa
 	if details.Block.Height != -1 {
 		blockHashStr = details.Block.Hash.String()
 		blockTime = details.Block.Time.Unix()
-		confirmations = int64(confirms(details.Block.Height, syncHeight))
+		confirmations = int64(
+			calcConf(details.Block.Height, syncHeight),
+		)
 	}
 
 	results := []btcjson.ListTransactionsResult{}
@@ -2596,15 +2607,24 @@ func (w *Wallet) GetTransaction(txHash chainhash.Hash) (*GetTransactionResult,
 
 		res = GetTransactionResult{
 			Summary:       makeTxSummary(dbtx, w, txDetail),
-			Timestamp:     txDetail.Block.Time.Unix(),
-			Confirmations: txDetail.Block.Height,
+			BlockHash:     nil,
+			Height:        -1,
+			Confirmations: 0,
+			Timestamp:     0,
 		}
 
 		// If it is a confirmed transaction we set the corresponding
-		// block height and hash.
+		// block height, timestamp, hash, and confirmations.
 		if txDetail.Block.Height != -1 {
 			res.Height = txDetail.Block.Height
+			res.Timestamp = txDetail.Block.Time.Unix()
 			res.BlockHash = &txDetail.Block.Hash
+
+			bestBlock := w.SyncedTo()
+			blockHeight := txDetail.Block.Height
+			res.Confirmations = calcConf(
+				blockHeight, bestBlock.Height,
+			)
 		}
 
 		return nil
@@ -2750,11 +2770,18 @@ func (w *Wallet) AccountBalances(scope waddrmgr.KeyScope,
 		}
 		for i := range unspentOutputs {
 			output := &unspentOutputs[i]
-			if !confirmed(requiredConfs, output.Height, syncBlock.Height) {
+			if !hasMinConfs(
+				requiredConfs, output.Height, syncBlock.Height,
+			) {
+
 				continue
 			}
-			if output.FromCoinBase && !confirmed(int32(w.ChainParams().CoinbaseMaturity),
-				output.Height, syncBlock.Height) {
+
+			if output.FromCoinBase && !hasMinConfs(
+				int32(w.ChainParams().CoinbaseMaturity),
+				output.Height, syncBlock.Height,
+			) {
+
 				continue
 			}
 			_, addrs, _, err := txscript.ExtractPkScriptAddrs(output.PkScript, w.chainParams)
@@ -2845,17 +2872,20 @@ func (w *Wallet) ListUnspent(minconf, maxconf int32,
 		for i := range unspent {
 			output := unspent[i]
 
-			// Outputs with fewer confirmations than the minimum or more
-			// confs than the maximum are excluded.
-			confs := confirms(output.Height, syncBlock.Height)
+			// Outputs with fewer confirmations than the minimum or
+			// more confs than the maximum are excluded.
+			confs := calcConf(output.Height, syncBlock.Height)
 			if confs < minconf || confs > maxconf {
 				continue
 			}
 
 			// Only mature coinbase outputs are included.
 			if output.FromCoinBase {
 				target := int32(w.ChainParams().CoinbaseMaturity)
-				if !confirmed(target, output.Height, syncBlock.Height) {
+				if !hasMinConfs(
+					target, output.Height, syncBlock.Height,
+				) {
+
 					continue
 				}
 			}
@@ -3330,19 +3360,27 @@ func (w *Wallet) newChangeAddress(addrmgrNs walletdb.ReadWriteBucket,
 	return addrs[0].Address(), nil
 }
 
-// confirmed checks whether a transaction at height txHeight has met minconf
-// confirmations for a blockchain at height curHeight.
-func confirmed(minconf, txHeight, curHeight int32) bool {
-	return confirms(txHeight, curHeight) >= minconf
+// hasMinConfs returns whether a transaction has met at least minConf
+// confirmations at the current block height.
+func hasMinConfs(minConf, txHeight, curHeight int32) bool {
+	return calcConf(txHeight, curHeight) >= minConf
 }
 
-// confirms returns the number of confirmations for a transaction in a block at
-// height txHeight (or -1 for an unconfirmed tx) given the chain height
-// curHeight.
-func confirms(txHeight, curHeight int32) int32 {
+// calcConf returns the number of confirmations for a transaction given its
+// containing block height and the current best block height. Unconfirmed
+// transactions have a height of -1 and are considered to have 0 confirmations.
+func calcConf(txHeight, curHeight int32) int32 {
 	switch {
-	case txHeight == -1, txHeight > curHeight:
+	// Unconfirmed transactions have 0 confirmations.
+	case txHeight == -1:
 		return 0
+
+	// A transaction in a block after the current best block is considered
+	// unconfirmed. This can happen during a chain reorg.
+	case txHeight > curHeight:
+		return 0
+
+	// Confirmed transactions have at least one confirmation.
 	default:
 		return curHeight - txHeight + 1
 	}
@@ -3414,8 +3452,12 @@ func (w *Wallet) TotalReceivedForAccounts(scope waddrmgr.KeyScope,
 						}
 						res := &results[acctIndex]
 						res.TotalReceived += cred.Amount
-						res.LastConfirmation = confirms(
-							detail.Block.Height, syncBlock.Height)
+
+						confs := calcConf(
+							detail.Block.Height,
+							syncBlock.Height,
+						)
+						res.LastConfirmation = confs
 					}
 				}
 			}

wallet/wallet_test.go

@@ -247,8 +247,9 @@ func TestGetTransaction(t *testing.T) {
 		expectedErr error
 	}{
 		{
-			name: "existing unmined transaction",
-			txid: *TstTxHash,
+			name:           "existing unmined transaction",
+			txid:           *TstTxHash,
+			expectedHeight: -1,
 			// We write txdetail for the tx to disk.
 			f: func(s *wtxmgr.Store, ns walletdb.ReadWriteBucket) (
 				*wtxmgr.Store, error) {
@@ -313,6 +314,181 @@ func TestGetTransaction(t *testing.T) {
 	}
 }
 
+// TestGetTransactionConfirmations tests that GetTransaction correctly
+// calculates confirmations for both confirmed and unconfirmed transactions.
+// This is a regression test for a bug where confirmations were set to the
+// block height instead of being calculated as currentHeight - blockHeight + 1.
+//
+// The bug had several negative impacts:
+//   - Unconfirmed transactions showed -1 confirmations instead of 0, breaking
+//     zero-conf (accepting transactions before block inclusion)
+//   - Confirmed transactions showed block height instead of actual confirmation
+//     count
+//   - LND and other consumers would make incorrect decisions based on wrong
+//     counts
+func TestGetTransactionConfirmations(t *testing.T) {
+	t.Parallel()
+
+	rec, err := wtxmgr.NewTxRecord(TstSerializedTx, time.Now())
+	require.NoError(t, err)
+
+	tests := []struct {
+		name string
+
+		// Block height where transaction is mined (-1 for unmined).
+		txBlockHeight int32
+
+		// Current wallet sync height.
+		currentHeight int32
+
+		// Expected confirmations.
+		expectedConfirmations int32
+
+		// Expected height in result.
+		expectedHeight int32
+
+		// Whether to check for non-zero timestamp.
+		expectTimestamp bool
+	}{
+		{
+			name:                  "unconfirmed tx",
+			txBlockHeight:         -1,
+			currentHeight:         100,
+			expectedConfirmations: 0,
+			expectedHeight:        -1,
+			expectTimestamp:       false,
+		},
+		{
+			name:                  "tx with 1 confirmation",
+			txBlockHeight:         100,
+			currentHeight:         100,
+			expectedConfirmations: 1,
+			expectedHeight:        100,
+			expectTimestamp:       true,
+		},
+		{
+			name:                  "tx with 3 confirmations",
+			txBlockHeight:         8,
+			currentHeight:         10,
+			expectedConfirmations: 3,
+			expectedHeight:        8,
+			expectTimestamp:       true,
+		},
+		{
+			name:                  "old tx with many confirmations",
+			txBlockHeight:         1,
+			currentHeight:         1000,
+			expectedConfirmations: 1000,
+			expectedHeight:        1,
+			expectTimestamp:       true,
+		},
+		{
+			name:                  "tx in future block",
+			txBlockHeight:         105,
+			currentHeight:         100,
+			expectedConfirmations: 0,
+			expectedHeight:        105,
+			expectTimestamp:       true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			w, cleanup := testWallet(t)
+			t.Cleanup(cleanup)
+
+			// Set the wallet's synced height.
+			err := walletdb.Update(
+				w.db, func(tx walletdb.ReadWriteTx) error {
+					addrmgrNs := tx.ReadWriteBucket(
+						waddrmgrNamespaceKey,
+					)
+					bs := &waddrmgr.BlockStamp{
+						Height: tt.currentHeight,
+						Hash:   chainhash.Hash{},
+					}
+
+					return w.Manager.SetSyncedTo(
+						addrmgrNs, bs,
+					)
+				},
+			)
+			require.NoError(t, err)
+
+			// Insert transaction into wallet.
+			err = walletdb.Update(
+				w.db, func(tx walletdb.ReadWriteTx) error {
+					ns := tx.ReadWriteBucket(
+						wtxmgrNamespaceKey,
+					)
+
+					// Create block metadata if transaction
+					// is mined.
+					var blockMeta *wtxmgr.BlockMeta
+					if tt.txBlockHeight != -1 {
+						hash := chainhash.Hash{}
+						height := tt.txBlockHeight
+						block := wtxmgr.Block{
+							Hash:   hash,
+							Height: height,
+						}
+						blockMeta = &wtxmgr.BlockMeta{
+							Block: block,
+							Time:  time.Now(),
+						}
+					}
+
+					return w.TxStore.InsertTx(
+						ns, rec, blockMeta,
+					)
+				},
+			)
+			require.NoError(t, err)
+
+			result, err := w.GetTransaction(*TstTxHash)
+			require.NoError(t, err)
+
+			require.Equal(
+				t, tt.expectedConfirmations,
+				result.Confirmations,
+			)
+
+			require.Equal(t, tt.expectedHeight, result.Height)
+
+			if tt.expectTimestamp {
+				require.NotZero(t, result.Timestamp)
+			} else {
+				require.Zero(t, result.Timestamp)
+			}
+
+			// Additional checks for unconfirmed transactions.
+			if tt.txBlockHeight == -1 {
+				require.Nil(t, result.BlockHash)
+				require.Equal(t, int32(0), result.Confirmations)
+			} else {
+				require.NotNil(t, result.BlockHash)
+				// Only expect positive confirmations when tx is
+				// not in a future block.
+				if tt.txBlockHeight <= tt.currentHeight {
+					require.Positive(
+						t, result.Confirmations,
+					)
+				} else {
+					// Confirmed txns in future blocks for
+					// example due to reorg should be
+					// treated as unconfirmed and have 0
+					// confirmations.
+					require.Equal(
+						t, int32(0),
+						result.Confirmations,
+					)
+				}
+			}
+		})
+	}
+}
+
 // TestDuplicateAddressDerivation tests that duplicate addresses are not
 // derived when multiple goroutines are concurrently requesting new addresses.
 func TestDuplicateAddressDerivation(t *testing.T) {