Merge pull request #1117 from sipa/deadlockfix
authorPieter Wuille <pieter.wuille@gmail.com>
Tue, 17 Apr 2012 18:05:02 +0000 (11:05 -0700)
committerPieter Wuille <pieter.wuille@gmail.com>
Tue, 17 Apr 2012 18:05:02 +0000 (11:05 -0700)
Fix potential deadlock

1  2 
src/main.cpp

diff --combined src/main.cpp
@@@ -2,11 -2,11 +2,11 @@@
  // Copyright (c) 2009-2012 The Bitcoin developers
  // Distributed under the MIT/X11 software license, see the accompanying
  // file license.txt or http://www.opensource.org/licenses/mit-license.php.
 -#include "headers.h"
  #include "checkpoints.h"
  #include "db.h"
  #include "net.h"
  #include "init.h"
 +#include "ui_interface.h"
  #include <boost/algorithm/string/replace.hpp>
  #include <boost/filesystem.hpp>
  #include <boost/filesystem/fstream.hpp>
@@@ -288,7 -288,7 +288,7 @@@ bool CTransaction::AreInputsStandard(co
      if (IsCoinBase())
          return true; // Coinbases don't use vin normally
  
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          const CTxOut& prev = GetOutputFor(vin[i], mapInputs);
  
@@@ -411,7 -411,7 +411,7 @@@ bool CTransaction::CheckTransaction() c
      if (vout.empty())
          return DoS(10, error("CTransaction::CheckTransaction() : vout empty"));
      // Size limits
 -    if (::GetSerializeSize(*this, SER_NETWORK) > MAX_BLOCK_SIZE)
 +    if (::GetSerializeSize(*this, SER_NETWORK, PROTOCOL_VERSION) > MAX_BLOCK_SIZE)
          return DoS(100, error("CTransaction::CheckTransaction() : size limits failed"));
  
      // Check for negative or overflow output values
@@@ -484,7 -484,7 +484,7 @@@ bool CTransaction::AcceptToMemoryPool(C
  
      // Check for conflicts with in-memory transactions
      CTransaction* ptxOld = NULL;
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          COutPoint outpoint = vin[i].prevout;
          if (mapNextTx.count(outpoint))
                  return false;
              if (!IsNewerThan(*ptxOld))
                  return false;
 -            for (int i = 0; i < vin.size(); i++)
 +            for (unsigned int i = 0; i < vin.size(); i++)
              {
                  COutPoint outpoint = vin[i].prevout;
                  if (!mapNextTx.count(outpoint) || mapNextTx[outpoint].ptx != ptxOld)
          // reasonable number of ECDSA signature verifications.
  
          int64 nFees = GetValueIn(mapInputs)-GetValueOut();
 -        unsigned int nSize = ::GetSerializeSize(*this, SER_NETWORK);
 +        unsigned int nSize = ::GetSerializeSize(*this, SER_NETWORK, PROTOCOL_VERSION);
  
          // Don't accept it if it can't get into a block
          if (nFees < GetMinFee(1000, true, GMF_RELAY))
@@@ -603,7 -603,7 +603,7 @@@ bool CTransaction::AddToMemoryPoolUnche
          LOCK(cs_mapTransactions);
          uint256 hash = GetHash();
          mapTransactions[hash] = *this;
 -        for (int i = 0; i < vin.size(); i++)
 +        for (unsigned int i = 0; i < vin.size(); i++)
              mapNextTx[vin[i].prevout] = CInPoint(&mapTransactions[hash], i);
          nTransactionsUpdated++;
          ++nPooledTx;
@@@ -989,7 -989,7 +989,7 @@@ bool CTransaction::FetchInputs(CTxDB& t
      if (IsCoinBase())
          return true; // Coinbase transactions have no inputs to fetch.
  
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          COutPoint prevout = vin[i].prevout;
          if (inputsRet.count(prevout.hash))
      }
  
      // Make sure all prevout.n's are valid:
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          const COutPoint prevout = vin[i].prevout;
          assert(inputsRet.count(prevout.hash) != 0);
@@@ -1071,7 -1071,7 +1071,7 @@@ int64 CTransaction::GetValueIn(const Ma
          return 0;
  
      int64 nResult = 0;
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          nResult += GetOutputFor(vin[i], inputs).nValue;
      }
@@@ -1085,7 -1085,7 +1085,7 @@@ int CTransaction::GetP2SHSigOpCount(con
          return 0;
  
      int nSigOps = 0;
 -    for (int i = 0; i < vin.size(); i++)
 +    for (unsigned int i = 0; i < vin.size(); i++)
      {
          const CTxOut& prevout = GetOutputFor(vin[i], inputs);
          if (prevout.scriptPubKey.IsPayToScriptHash())
@@@ -1106,7 -1106,7 +1106,7 @@@ bool CTransaction::ConnectInputs(MapPre
      {
          int64 nValueIn = 0;
          int64 nFees = 0;
 -        for (int i = 0; i < vin.size(); i++)
 +        for (unsigned int i = 0; i < vin.size(); i++)
          {
              COutPoint prevout = vin[i].prevout;
              assert(inputs.count(prevout.hash) > 0);
@@@ -1185,7 -1185,7 +1185,7 @@@ bool CTransaction::ClientConnectInputs(
      {
          LOCK(cs_mapTransactions);
          int64 nValueIn = 0;
 -        for (int i = 0; i < vin.size(); i++)
 +        for (unsigned int i = 0; i < vin.size(); i++)
          {
              // Get prev tx from single transactions in memory
              COutPoint prevout = vin[i].prevout;
@@@ -1279,7 -1279,7 +1279,7 @@@ bool CBlock::ConnectBlock(CTxDB& txdb, 
      bool fStrictPayToScriptHash = (pindex->nTime >= nBIP16SwitchTime);
  
      //// issue here: it doesn't know the version
 -    unsigned int nTxPos = pindex->nBlockPos + ::GetSerializeSize(CBlock(), SER_DISK) - 1 + GetSizeOfCompactSize(vtx.size());
 +    unsigned int nTxPos = pindex->nBlockPos + ::GetSerializeSize(CBlock(), SER_DISK, CLIENT_VERSION) - 1 + GetSizeOfCompactSize(vtx.size());
  
      map<uint256, CTxIndex> mapQueuedChanges;
      int64 nFees = 0;
              return DoS(100, error("ConnectBlock() : too many sigops"));
  
          CDiskTxPos posThisTx(pindex->nFile, pindex->nBlockPos, nTxPos);
 -        nTxPos += ::GetSerializeSize(tx, SER_DISK);
 +        nTxPos += ::GetSerializeSize(tx, SER_DISK, CLIENT_VERSION);
  
          MapPrevTx mapInputs;
          if (!tx.IsCoinBase())
@@@ -1396,7 -1396,7 +1396,7 @@@ bool static Reorganize(CTxDB& txdb, CBl
  
      // Connect longer branch
      vector<CTransaction> vDelete;
 -    for (int i = 0; i < vConnect.size(); i++)
 +    for (unsigned int i = 0; i < vConnect.size(); i++)
      {
          CBlockIndex* pindex = vConnect[i];
          CBlock block;
@@@ -1621,7 -1621,7 +1621,7 @@@ bool CBlock::CheckBlock() cons
      // that can be verified before saving an orphan block.
  
      // Size limits
 -    if (vtx.empty() || vtx.size() > MAX_BLOCK_SIZE || ::GetSerializeSize(*this, SER_NETWORK) > MAX_BLOCK_SIZE)
 +    if (vtx.empty() || vtx.size() > MAX_BLOCK_SIZE || ::GetSerializeSize(*this, SER_NETWORK, PROTOCOL_VERSION) > MAX_BLOCK_SIZE)
          return DoS(100, error("CheckBlock() : size limits failed"));
  
      // Check proof of work matches claimed amount
      // First transaction must be coinbase, the rest must not be
      if (vtx.empty() || !vtx[0].IsCoinBase())
          return DoS(100, error("CheckBlock() : first tx is not coinbase"));
 -    for (int i = 1; i < vtx.size(); i++)
 +    for (unsigned int i = 1; i < vtx.size(); i++)
          if (vtx[i].IsCoinBase())
              return DoS(100, error("CheckBlock() : more than one coinbase"));
  
@@@ -1691,7 -1691,7 +1691,7 @@@ bool CBlock::AcceptBlock(
          return DoS(100, error("AcceptBlock() : rejected by checkpoint lockin at %d", nHeight));
  
      // Write block to history file
 -    if (!CheckDiskSpace(::GetSerializeSize(*this, SER_DISK)))
 +    if (!CheckDiskSpace(::GetSerializeSize(*this, SER_DISK, CLIENT_VERSION)))
          return error("AcceptBlock() : out of disk space");
      unsigned int nFile = -1;
      unsigned int nBlockPos = 0;
@@@ -1771,7 -1771,7 +1771,7 @@@ bool ProcessBlock(CNode* pfrom, CBlock
      // Recursively process any orphan blocks that depended on this one
      vector<uint256> vWorkQueue;
      vWorkQueue.push_back(hash);
 -    for (int i = 0; i < vWorkQueue.size(); i++)
 +    for (unsigned int i = 0; i < vWorkQueue.size(); i++)
      {
          uint256 hashPrev = vWorkQueue[i];
          for (multimap<uint256, CBlock*>::iterator mi = mapOrphanBlocksByPrev.lower_bound(hashPrev);
@@@ -1995,7 -1995,7 +1995,7 @@@ void PrintBlockTree(
  
          // put the main timechain first
          vector<CBlockIndex*>& vNext = mapNext[pindex];
 -        for (int i = 0; i < vNext.size(); i++)
 +        for (unsigned int i = 0; i < vNext.size(); i++)
          {
              if (vNext[i]->pnext)
              {
          }
  
          // iterate children
 -        for (int i = 0; i < vNext.size(); i++)
 +        for (unsigned int i = 0; i < vNext.size(); i++)
              vStack.push_back(make_pair(nCol+i, vNext[i]));
      }
  }
@@@ -2135,21 -2135,8 +2135,21 @@@ bool static AlreadyHave(CTxDB& txdb, co
  {
      switch (inv.type)
      {
 -    case MSG_TX:    return mapTransactions.count(inv.hash) || mapOrphanTransactions.count(inv.hash) || txdb.ContainsTx(inv.hash);
 -    case MSG_BLOCK: return mapBlockIndex.count(inv.hash) || mapOrphanBlocks.count(inv.hash);
 +    case MSG_TX:
 +        {
 +        bool txInMap = false;
 +            {
 +            LOCK(cs_mapTransactions);
 +            txInMap = (mapTransactions.count(inv.hash) != 0);
 +            }
 +        return txInMap ||
 +               mapOrphanTransactions.count(inv.hash) ||
 +               txdb.ContainsTx(inv.hash);
 +        }
 +
 +    case MSG_BLOCK:
 +        return mapBlockIndex.count(inv.hash) ||
 +               mapOrphanBlocks.count(inv.hash);
      }
      // Don't know what it is, just say we already got one
      return true;
@@@ -2196,7 -2183,7 +2196,7 @@@ bool static ProcessMessage(CNode* pfrom
          CAddress addrFrom;
          uint64 nNonce = 1;
          vRecv >> pfrom->nVersion >> pfrom->nServices >> nTime >> addrMe;
 -        if (pfrom->nVersion < 209)
 +        if (pfrom->nVersion < MIN_PROTO_VERSION)
          {
              // Since February 20, 2012, the protocol is initiated at version 209,
              // and earlier versions are no longer supported
              }
  
              // Get recent addresses
 -            if (pfrom->nVersion >= 31402 || addrman.size() < 1000)
 +            if (pfrom->nVersion >= CADDR_TIME_VERSION || addrman.size() < 1000)
              {
                  pfrom->PushMessage("getaddr");
                  pfrom->fGetAddr = true;
          // Ask the first connected node for block updates
          static int nAskedForBlocks = 0;
          if (!pfrom->fClient &&
 -            (pfrom->nVersion < 32000 || pfrom->nVersion >= 32400) &&
 +            (pfrom->nVersion < NOBLKS_VERSION_START ||
 +             pfrom->nVersion >= NOBLKS_VERSION_END) &&
               (nAskedForBlocks < 1 || vNodes.size() <= 1))
          {
              nAskedForBlocks++;
          vRecv >> vAddr;
  
          // Don't want addr from older versions unless seeding
 -        if (pfrom->nVersion < 31402 && addrman.size() > 1000)
 +        if (pfrom->nVersion < CADDR_TIME_VERSION && addrman.size() > 1000)
              return true;
          if (vAddr.size() > 1000)
          {
                      multimap<uint256, CNode*> mapMix;
                      BOOST_FOREACH(CNode* pnode, vNodes)
                      {
 -                        if (pnode->nVersion < 31402)
 +                        if (pnode->nVersion < CADDR_TIME_VERSION)
                              continue;
                          unsigned int nPointer;
                          memcpy(&nPointer, &pnode, sizeof(nPointer));
          }
  
          CTxDB txdb("r");
 -        for (int nInv = 0; nInv < vInv.size(); nInv++)
 +        for (unsigned int nInv = 0; nInv < vInv.size(); nInv++)
          {
              const CInv &inv = vInv[nInv];
  
              pfrom->PushInventory(CInv(MSG_BLOCK, pindex->GetBlockHash()));
              CBlock block;
              block.ReadFromDisk(pindex, true);
 -            nBytes += block.GetSerializeSize(SER_NETWORK);
 +            nBytes += block.GetSerializeSize(SER_NETWORK, PROTOCOL_VERSION);
              if (--nLimit <= 0 || nBytes >= SendBufferSize()/2)
              {
                  // When this block is requested, we'll send an inv that'll make them
              vWorkQueue.push_back(inv.hash);
  
              // Recursively process any orphan transactions that depended on this one
 -            for (int i = 0; i < vWorkQueue.size(); i++)
 +            for (unsigned int i = 0; i < vWorkQueue.size(); i++)
              {
                  uint256 hashPrev = vWorkQueue[i];
                  for (multimap<uint256, CDataStream*>::iterator mi = mapOrphanTransactionsByPrev.lower_bound(hashPrev);
  
      else if (strCommand == "ping")
      {
 +        if (pfrom->nVersion > BIP0031_VERSION)
 +        {
 +            uint64 nonce = 0;
 +            vRecv >> nonce;
 +            // Echo the message back with the nonce. This allows for two useful features:
 +            //
 +            // 1) A remote node can quickly check if the connection is operational
 +            // 2) Remote nodes can measure the latency of the network thread. If this node
 +            //    is overloaded it won't respond to pings quickly and the remote node can
 +            //    avoid sending us more work, like chain download requests.
 +            //
 +            // The nonce stops the remote getting confused between different pings: without
 +            // it, if the remote node sends a ping once per second and this node takes 5
 +            // seconds to respond to each, the 5th ping the remote sends would appear to
 +            // return very quickly.
 +            pfrom->PushMessage("pong", nonce);
 +        }
      }
  
  
@@@ -2838,20 -2807,15 +2838,20 @@@ bool ProcessMessages(CNode* pfrom
  
  bool SendMessages(CNode* pto, bool fSendTrickle)
  {
-     {
-         LOCK(cs_main);
+     TRY_LOCK(cs_main, lockMain);
+     if (lockMain) {
          // Don't send anything until we get their version message
          if (pto->nVersion == 0)
              return true;
  
 -        // Keep-alive ping
 -        if (pto->nLastSend && GetTime() - pto->nLastSend > 30 * 60 && pto->vSend.empty())
 -            pto->PushMessage("ping");
 +        // Keep-alive ping. We send a nonce of zero because we don't use it anywhere 
 +        // right now.
 +        if (pto->nLastSend && GetTime() - pto->nLastSend > 30 * 60 && pto->vSend.empty()) {
 +            if (pto->nVersion > BIP0031_VERSION)
 +                pto->PushMessage("ping", 0);
 +            else
 +                pto->PushMessage("ping");
 +        }
  
          // Resend wallet transactions that haven't gotten in a block yet
          ResendWalletTransactions();
@@@ -3174,7 -3138,7 +3174,7 @@@ CBlock* CreateNewBlock(CReserveKey& res
              }
  
              // Priority is sum(valuein * age) / txsize
 -            dPriority /= ::GetSerializeSize(tx, SER_NETWORK);
 +            dPriority /= ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
  
              if (porphan)
                  porphan->dPriority = dPriority;
              mapPriority.erase(mapPriority.begin());
  
              // Size limits
 -            unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK);
 +            unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
              if (nBlockSize + nTxSize >= MAX_BLOCK_SIZE_GEN)
                  continue;
  
@@@ -3330,7 -3294,7 +3330,7 @@@ void FormatHashBuffers(CBlock* pblock, 
      FormatHashBlocks(&tmp.hash1, sizeof(tmp.hash1));
  
      // Byte swap all the input buffer
 -    for (int i = 0; i < sizeof(tmp)/4; i++)
 +    for (unsigned int i = 0; i < sizeof(tmp)/4; i++)
          ((unsigned int*)&tmp)[i] = ByteReverse(((unsigned int*)&tmp)[i]);
  
      // Precalc the first half of the first hash, which stays constant
@@@ -3455,7 -3419,7 +3455,7 @@@ void static BitcoinMiner(CWallet *pwall
              // Check if something found
              if (nNonceFound != -1)
              {
 -                for (int i = 0; i < sizeof(hash)/4; i++)
 +                for (unsigned int i = 0; i < sizeof(hash)/4; i++)
                      ((unsigned int*)&hash)[i] = ByteReverse(((unsigned int*)&hash)[i]);
  
                  if (hash <= hashTarget)