Merge pull request #3669 from gavinandresen/dead_txns

Handle "conflicted" transactions properly

qa/rpc-tests/txnmall.sh

@@ -0,0 +1,144 @@
+#!/usr/bin/env bash
+
+# Test block generation and basic wallet sending
+
+if [ $# -lt 1 ]; then
+        echo "Usage: $0 path_to_binaries"
+        echo "e.g. $0 ../../src"
+        exit 1
+fi
+
+BITCOIND=${1}/bitcoind
+CLI=${1}/bitcoin-cli
+
+DIR="${BASH_SOURCE%/*}"
+SENDANDWAIT="${DIR}/send.sh"
+if [[ ! -d "$DIR" ]]; then DIR="$PWD"; fi
+. "$DIR/util.sh"
+
+D=$(mktemp -d test.XXXXX)
+
+# Two nodes; one will play the part of merchant, the
+# other an evil transaction-mutating miner.
+
+D1=${D}/node1
+CreateDataDir $D1 port=11000 rpcport=11001
+B1ARGS="-datadir=$D1 -debug"
+$BITCOIND $B1ARGS &
+B1PID=$!
+
+D2=${D}/node2
+CreateDataDir $D2 port=11010 rpcport=11011
+B2ARGS="-datadir=$D2 -debug"
+$BITCOIND $B2ARGS &
+B2PID=$!
+
+trap "kill -9 $B1PID $B2PID; rm -rf $D" EXIT
+
+# Wait until all four nodes are at the same block number
+function WaitBlocks {
+    while :
+    do
+        sleep 1
+        BLOCKS1=$( GetBlocks $B1ARGS )
+        BLOCKS2=$( GetBlocks $B2ARGS )
+        if (( $BLOCKS1 == $BLOCKS2 ))
+        then
+            break
+        fi
+    done
+}
+
+# Wait until node has $N peers
+function WaitPeers {
+    while :
+    do
+        PEERS=$( $CLI $1 getconnectioncount )
+        if (( "$PEERS" == $2 ))
+        then
+            break
+        fi
+        sleep 1
+    done
+}
+
+# Start with B2 connected to B1:
+$CLI $B2ARGS addnode 127.0.0.1:11000 onetry
+WaitPeers "$B1ARGS" 1
+
+# 1 block, 50 XBT each == 50 XBT
+$CLI $B1ARGS setgenerate true 1
+
+WaitBlocks
+# 100 blocks, 0 mature == 0 XBT
+$CLI $B2ARGS setgenerate true 100
+WaitBlocks
+
+CheckBalance $B1ARGS 50
+CheckBalance $B2ARGS 0
+
+# restart B2 with no connection
+$CLI $B2ARGS stop > /dev/null 2>&1
+wait $B2PID
+$BITCOIND $B2ARGS &
+B2PID=$!
+
+B2ADDRESS=$( $CLI $B2ARGS getnewaddress )
+
+# Have B1 create two transactions; second will
+# spend change from first, since B1 starts with only a single
+# 50 bitcoin output:
+TXID1=$( $CLI $B1ARGS sendtoaddress $B2ADDRESS 1.0 )
+TXID2=$( $CLI $B1ARGS sendtoaddress $B2ADDRESS 2.0 )
+
+# Mutate TXID1 and add it to B2's memory pool:
+RAWTX1=$( $CLI $B1ARGS getrawtransaction $TXID1 )
+RAWTX2=$( $CLI $B1ARGS getrawtransaction $TXID2 )
+# ... mutate RAWTX1:
+# RAWTX1 is hex-encoded, serialized transaction. So each
+# byte is two characters; we'll prepend the first
+# "push" in the scriptsig with OP_PUSHDATA1 (0x4c),
+# and add one to the length of the signature.
+# Fields are fixed; from the beginning:
+# 4-byte version
+# 1-byte varint number-of inputs (one in this case)
+# 32-byte previous txid
+# 4-byte previous output
+# 1-byte varint length-of-scriptsig
+# 1-byte PUSH this many bytes onto stack
+#  ... etc
+# So: to mutate, we want to get byte 41 (hex characters 82-83),
+# increment it, and insert 0x4c after it.
+L=${RAWTX1:82:2}
+NEWLEN=$( printf "%x" $(( 16#$L + 1 )) )
+MUTATEDTX1=${RAWTX1:0:82}${NEWLEN}4c${RAWTX1:84}
+# ... give mutated tx1 to B2:
+MUTATEDTXID=$( $CLI $B2ARGS sendrawtransaction $MUTATEDTX1 )
+
+echo "TXID1: " $TXID1
+echo "Mutated: " $MUTATEDTXID
+
+# Re-connect nodes, and have B2 mine a block
+$CLI $B2ARGS addnode 127.0.0.1:11000 onetry
+WaitPeers "$B1ARGS" 1
+
+$CLI $B2ARGS setgenerate true 1
+WaitBlocks
+
+$CLI $B2ARGS stop > /dev/null 2>&1
+wait $B2PID
+$CLI $B1ARGS stop > /dev/null 2>&1
+wait $B1PID
+
+trap "" EXIT
+
+echo "Done, bitcoind's shut down. To rerun/poke around:"
+echo "${1}/bitcoind -datadir=$D1 -daemon"
+echo "${1}/bitcoind -datadir=$D2 -daemon -connect=127.0.0.1:11000"
+echo "To cleanup:"
+echo "killall bitcoind; rm -rf test.*"
+exit 0
+
+echo "Tests successful, cleaning up"
+rm -rf $D
+exit 0

src/main.cpp

@@ -872,7 +872,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
 }
 
 
-int CMerkleTx::GetDepthInMainChain(CBlockIndex* &pindexRet) const
+int CMerkleTx::GetDepthInMainChainINTERNAL(CBlockIndex* &pindexRet) const
 {
     if (hashBlock == 0 || nIndex == -1)
         return 0;
@@ -897,6 +897,14 @@ int CMerkleTx::GetDepthInMainChain(CBlockIndex* &pindexRet) const
     return chainActive.Height() - pindex->nHeight + 1;
 }
 
+int CMerkleTx::GetDepthInMainChain(CBlockIndex* &pindexRet) const
+{
+    int nResult = GetDepthInMainChainINTERNAL(pindexRet);
+    if (nResult == 0 && !mempool.exists(GetHash()))
+        return -1; // Not in chain, not in mempool
+
+    return nResult;
+}
 
 int CMerkleTx::GetBlocksToMaturity() const
 {

src/main.h

@@ -423,6 +423,8 @@ public:
 /** A transaction with a merkle branch linking it to the block chain. */
 class CMerkleTx : public CTransaction
 {
+private:
+    int GetDepthInMainChainINTERNAL(CBlockIndex* &pindexRet) const;
 public:
     uint256 hashBlock;
     std::vector<uint256> vMerkleBranch;
@@ -461,9 +463,14 @@ public:
 
 
     int SetMerkleBranch(const CBlock* pblock=NULL);
+
+    // Return depth of transaction in blockchain:
+    // -1  : not in blockchain, and not in memory pool (conflicted transaction)
+    //  0  : in memory pool, waiting to be included in a block
+    // >=1 : this many blocks deep in the main chain
     int GetDepthInMainChain(CBlockIndex* &pindexRet) const;
     int GetDepthInMainChain() const { CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet); }
-    bool IsInMainChain() const { return GetDepthInMainChain() > 0; }
+    bool IsInMainChain() const { CBlockIndex *pindexRet; return GetDepthInMainChainINTERNAL(pindexRet) > 0; }
     int GetBlocksToMaturity() const;
     bool AcceptToMemoryPool(bool fLimitFree=true);
 };

src/qt/Makefile.am

@@ -243,6 +243,7 @@ RES_ICONS = \
   res/icons/toolbar_testnet.png \
   res/icons/transaction0.png \
   res/icons/transaction2.png \
+  res/icons/transaction_conflicted.png \
   res/icons/tx_inout.png \
   res/icons/tx_input.png \
   res/icons/tx_output.png \

src/qt/bitcoin.qrc

@@ -12,6 +12,7 @@
         <file alias="connect_4">res/icons/connect4_16.png</file>
         <file alias="transaction_0">res/icons/transaction0.png</file>
         <file alias="transaction_confirmed">res/icons/transaction2.png</file>
+        <file alias="transaction_conflicted">res/icons/transaction_conflicted.png</file>
         <file alias="transaction_1">res/icons/clock1.png</file>
         <file alias="transaction_2">res/icons/clock2.png</file>
         <file alias="transaction_3">res/icons/clock3.png</file>

src/qt/overviewpage.cpp

@@ -175,6 +175,7 @@ void OverviewPage::setWalletModel(WalletModel *model)
         filter->setLimit(NUM_ITEMS);
         filter->setDynamicSortFilter(true);
         filter->setSortRole(Qt::EditRole);
+        filter->setShowInactive(false);
         filter->sort(TransactionTableModel::Status, Qt::DescendingOrder);
 
         ui->listTransactions->setModel(filter);

src/qt/transactiondesc.cpp

@@ -30,7 +30,9 @@ QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
     else
     {
         int nDepth = wtx.GetDepthInMainChain();
-        if (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0)
+        if (nDepth < 0)
+            return tr("conflicted");
+        else if (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0)
             return tr("%1/offline").arg(nDepth);
         else if (nDepth < 6)
             return tr("%1/unconfirmed").arg(nDepth);

src/qt/transactionfilterproxy.cpp

@@ -5,6 +5,7 @@
 #include "transactionfilterproxy.h"
 
 #include "transactiontablemodel.h"
+#include "transactionrecord.h"
 
 #include <cstdlib>
 
@@ -22,7 +23,8 @@ TransactionFilterProxy::TransactionFilterProxy(QObject *parent) :
     addrPrefix(),
     typeFilter(ALL_TYPES),
     minAmount(0),
-    limitRows(-1)
+    limitRows(-1),
+    showInactive(true)
 {
 }
 
@@ -35,7 +37,10 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex &
     QString address = index.data(TransactionTableModel::AddressRole).toString();
     QString label = index.data(TransactionTableModel::LabelRole).toString();
     qint64 amount = llabs(index.data(TransactionTableModel::AmountRole).toLongLong());
+    int status = index.data(TransactionTableModel::StatusRole).toInt();
 
+    if(!showInactive && status == TransactionStatus::Conflicted)
+        return false;
     if(!(TYPE(type) & typeFilter))
         return false;
     if(datetime < dateFrom || datetime > dateTo)
@@ -78,6 +83,12 @@ void TransactionFilterProxy::setLimit(int limit)
     this->limitRows = limit;
 }
 
+void TransactionFilterProxy::setShowInactive(bool showInactive)
+{
+    this->showInactive = showInactive;
+    invalidateFilter();
+}
+
 int TransactionFilterProxy::rowCount(const QModelIndex &parent) const
 {
     if(limitRows != -1)

src/qt/transactionfilterproxy.h

@@ -36,6 +36,9 @@ public:
     /** Set maximum number of rows returned, -1 if unlimited. */
     void setLimit(int limit);
 
+    /** Set whether to show conflicted transactions. */
+    void setShowInactive(bool showInactive);
+
     int rowCount(const QModelIndex &parent = QModelIndex()) const;
 
 protected:
@@ -48,6 +51,7 @@ private:
     quint32 typeFilter;
     qint64 minAmount;
     int limitRows;
+    bool showInactive;
 };
 
 #endif // TRANSACTIONFILTERPROXY_H

src/qt/transactionrecord.cpp

@@ -183,7 +183,11 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx)
     }
     else
     {
-        if (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0)
+        if (status.depth < 0)
+        {
+            status.status = TransactionStatus::Conflicted;
+        }
+        else if (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0)
         {
             status.status = TransactionStatus::Offline;
         }

src/qt/transactionrecord.h

@@ -36,7 +36,8 @@ public:
         OpenUntilBlock,
         Offline,
         Unconfirmed,
-        HaveConfirmations
+        HaveConfirmations,
+        Conflicted
     };
 
     bool confirmed;

src/qt/transactiontablemodel.cpp

@@ -312,7 +312,7 @@ QString TransactionTableModel::formatTxStatus(const TransactionRecord *wtx) cons
             status = tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx->status.open_for));
             break;
         case TransactionStatus::Offline:
-            status = tr("Offline (%1 confirmations)").arg(wtx->status.depth);
+            status = tr("Offline");
             break;
         case TransactionStatus::Unconfirmed:
             status = tr("Unconfirmed (%1 of %2 confirmations)").arg(wtx->status.depth).arg(TransactionRecord::NumConfirmations);
@@ -320,6 +320,9 @@ QString TransactionTableModel::formatTxStatus(const TransactionRecord *wtx) cons
         case TransactionStatus::HaveConfirmations:
             status = tr("Confirmed (%1 confirmations)").arg(wtx->status.depth);
             break;
+        case TransactionStatus::Conflicted:
+            status = tr("Conflicted");
+            break;
         }
     }
 
@@ -471,7 +474,6 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
         case TransactionStatus::OpenUntilBlock:
         case TransactionStatus::OpenUntilDate:
             return QColor(64,64,255);
-            break;
         case TransactionStatus::Offline:
             return QColor(192,192,192);
         case TransactionStatus::Unconfirmed:
@@ -486,6 +488,8 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
             };
         case TransactionStatus::HaveConfirmations:
             return QIcon(":/icons/transaction_confirmed");
+        case TransactionStatus::Conflicted:
+            return QIcon(":/icons/transaction_conflicted");
         }
     }
     return QColor(0,0,0);
@@ -587,6 +591,8 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const
                                           rec->status.maturity != TransactionStatus::Mature);
     case FormattedAmountRole:
         return formatTxAmount(rec, false);
+    case StatusRole:
+        return rec->status.status;
     }
     return QVariant();
 }

src/qt/transactiontablemodel.h

@@ -53,7 +53,9 @@ public:
         /** Is transaction confirmed? */
         ConfirmedRole,
         /** Formatted amount, without brackets when unconfirmed */
-        FormattedAmountRole
+        FormattedAmountRole,
+        /** Transaction status (TransactionRecord::Status) */
+        StatusRole
     };
 
     int rowCount(const QModelIndex &parent) const;

src/qt/walletmodel.cpp

@@ -494,7 +494,9 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
     BOOST_FOREACH(const COutPoint& outpoint, vOutpoints)
     {
         if (!wallet->mapWallet.count(outpoint.hash)) continue;
-        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, wallet->mapWallet[outpoint.hash].GetDepthInMainChain());
+        int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
+        if (nDepth < 0) continue;
+        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth);
         vOutputs.push_back(out);
     }
 }
@@ -513,7 +515,9 @@ void WalletModel::listCoins(std::map<QString, std::vector<COutput> >& mapCoins)
     BOOST_FOREACH(const COutPoint& outpoint, vLockedCoins)
     {
         if (!wallet->mapWallet.count(outpoint.hash)) continue;
-        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, wallet->mapWallet[outpoint.hash].GetDepthInMainChain());
+        int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
+        if (nDepth < 0) continue;
+        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth);
         vCoins.push_back(out);
     }
 

src/rpcwallet.cpp

@@ -45,7 +45,7 @@ void WalletTxToJSON(const CWalletTx& wtx, Object& entry)
     entry.push_back(Pair("confirmations", confirms));
     if (wtx.IsCoinBase())
         entry.push_back(Pair("generated", true));
-    if (confirms)
+    if (confirms > 0)
     {
         entry.push_back(Pair("blockhash", wtx.hashBlock.GetHex()));
         entry.push_back(Pair("blockindex", wtx.nIndex));
@@ -1109,7 +1109,10 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
             Object entry;
             entry.push_back(Pair("account", strSentAccount));
             MaybePushAddress(entry, s.first);
-            entry.push_back(Pair("category", "send"));
+            if (wtx.GetDepthInMainChain() < 0)
+                entry.push_back(Pair("category", "conflicted"));
+            else
+                entry.push_back(Pair("category", "send"));
             entry.push_back(Pair("amount", ValueFromAmount(-s.second)));
             entry.push_back(Pair("fee", ValueFromAmount(-nFee)));
             if (fLong)
@@ -1141,7 +1144,12 @@ void ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDe
                         entry.push_back(Pair("category", "generate"));
                 }
                 else
-                    entry.push_back(Pair("category", "receive"));
+                {
+                    if (wtx.GetDepthInMainChain() < 0)
+                        entry.push_back(Pair("category", "conflicted"));
+                    else
+                        entry.push_back(Pair("category", "receive"));
+                }
                 entry.push_back(Pair("amount", ValueFromAmount(r.second)));
                 if (fLong)
                     WalletTxToJSON(wtx, entry);

src/wallet.cpp

@@ -1021,11 +1021,15 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
             if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
                 continue;
 
+            int nDepth = pcoin->GetDepthInMainChain();
+            if (nDepth < 0)
+                continue;
+
             for (unsigned int i = 0; i < pcoin->vout.size(); i++) {
                 if (!(pcoin->IsSpent(i)) && IsMine(pcoin->vout[i]) &&
                     !IsLockedCoin((*it).first, i) && pcoin->vout[i].nValue > 0 &&
                     (!coinControl || !coinControl->HasSelected() || coinControl->IsSelected((*it).first, i)))
-                        vCoins.push_back(COutput(pcoin, i, pcoin->GetDepthInMainChain()));
+                        vCoins.push_back(COutput(pcoin, i, nDepth));
             }
         }
     }

src/wallet.h

@@ -701,8 +701,11 @@ public:
         // Quick answer in most cases
         if (!IsFinalTx(*this))
             return false;
-        if (GetDepthInMainChain() >= 1)
+        int nDepth = GetDepthInMainChain();
+        if (nDepth >= 1)
             return true;
+        if (nDepth < 0)
+            return false;
         if (!bSpendZeroConfChange || !IsFromMe()) // using wtx's cached debit
             return false;
 
@@ -718,8 +721,11 @@ public:
 
             if (!IsFinalTx(*ptx))
                 return false;
-            if (ptx->GetDepthInMainChain() >= 1)
+            int nPDepth = ptx->GetDepthInMainChain();
+            if (nPDepth >= 1)
                 continue;
+            if (nPDepth < 0)
+                return false;
             if (!pwallet->IsFromMe(*ptx))
                 return false;