From abb0e8ccedd4eb610738c9a07267348a59228401 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Fri, 19 Jun 2015 10:42:39 -0400 Subject: [PATCH 1/6] Testing infrastructure: mocktime fixes New, undocumented-on-purpose -mocktime=timestamp command-line argument to startup with mocktime set. Needed because time-related blockchain sanity checks are done on startup, before a test has a chance to make a setmocktime RPC call. And changed the setmocktime RPC call so calling it will not result in currently connected peers being disconnected due to inactivity timeouts. --- src/init.cpp | 3 +++ src/rpcmisc.cpp | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index cff7bfd90..c6f91260a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -981,6 +981,9 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) fAlerts = GetBoolArg("-alerts", DEFAULT_ALERTS); + // Option to startup with mocktime set (used for regression testing): + SetMockTime(GetArg("-mocktime", 0)); // SetMockTime(0) is a no-op + #ifdef ENABLE_MINING if (mapArgs.count("-mineraddress")) { CBitcoinAddress addr; diff --git a/src/rpcmisc.cpp b/src/rpcmisc.cpp index 38610903b..e7f8d0427 100644 --- a/src/rpcmisc.cpp +++ b/src/rpcmisc.cpp @@ -452,10 +452,19 @@ UniValue setmocktime(const UniValue& params, bool fHelp) if (!Params().MineBlocksOnDemand()) throw runtime_error("setmocktime for regression testing (-regtest mode) only"); - LOCK(cs_main); + // cs_vNodes is locked and node send/receive times are updated + // atomically with the time change to prevent peers from being + // disconnected because we think we haven't communicated with them + // in a long time. + LOCK2(cs_main, cs_vNodes); RPCTypeCheck(params, boost::assign::list_of(UniValue::VNUM)); SetMockTime(params[0].get_int64()); + uint64_t t = GetTime(); + BOOST_FOREACH(CNode* pnode, vNodes) { + pnode->nLastSend = pnode->nLastRecv = t; + } + return NullUniValue; } From 1eb6654314bc25b1e539a0aaaaabea70b5486f9d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 7 Jul 2015 18:18:14 +0200 Subject: [PATCH 2/6] tests: Fix bitcoin-tx signing testcase Fixes wrong scriptPubkey problem, which caused the transaction to not actually be signed. --- src/test/data/bitcoin-util-test.json | 2 +- src/test/data/txcreatesign.hex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/data/bitcoin-util-test.json b/src/test/data/bitcoin-util-test.json index e30c53759..c23befe23 100644 --- a/src/test/data/bitcoin-util-test.json +++ b/src/test/data/bitcoin-util-test.json @@ -52,7 +52,7 @@ ["-create", "in=4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485:0", "set=privatekeys:[\"5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf\"]", - "set=prevtxs:[{\"txid\":\"4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485\",\"vout\":0,\"scriptPubKey\":\"4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485\"}]", + "set=prevtxs:[{\"txid\":\"4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485\",\"vout\":0,\"scriptPubKey\":\"76a91491b24bf9f5288532960ac687abb035127b1d28a588ac\"}]", "sign=ALL", "outaddr=0.001:t1Ruz6gK4QPZoPPGpHaieupnnh62mktjQE7"], "output_cmp": "txcreatesign.hex" diff --git a/src/test/data/txcreatesign.hex b/src/test/data/txcreatesign.hex index 56ce28a86..a46fcc88c 100644 --- a/src/test/data/txcreatesign.hex +++ b/src/test/data/txcreatesign.hex @@ -1 +1 @@ -01000000018594c5bdcaec8f06b78b596f31cd292a294fd031e24eec716f43dac91ea7494d0000000000ffffffff01a0860100000000001976a9145834479edbbe0539b31ffd3a8f8ebadc2165ed0188ac00000000 +01000000018594c5bdcaec8f06b78b596f31cd292a294fd031e24eec716f43dac91ea7494d000000008b48304502210096a75056c9e2cc62b7214777b3d2a592cfda7092520126d4ebfcd6d590c99bd8022051bb746359cf98c0603f3004477eac68701132380db8facba19c89dc5ab5c5e201410479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8ffffffff01a0860100000000001976a9145834479edbbe0539b31ffd3a8f8ebadc2165ed0188ac00000000 From 8e4bc69d1fe94c7fd8fa03d70df19786d6d3fb11 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 20 Dec 2014 05:35:33 -0500 Subject: [PATCH 3/6] Assert on probable deadlocks if the second lock isnt try_lock --- src/sync.cpp | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index a42293996..1837e8d53 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -33,20 +33,22 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine) // struct CLockLocation { - CLockLocation(const char* pszName, const char* pszFile, int nLine) + CLockLocation(const char* pszName, const char* pszFile, int nLine, bool fTryIn) { mutexName = pszName; sourceFile = pszFile; sourceLine = nLine; + fTry = fTryIn; } std::string ToString() const { - return mutexName + " " + sourceFile + ":" + itostr(sourceLine); + return mutexName + " " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : ""); } std::string MutexName() const { return mutexName; } + bool fTry; private: std::string mutexName; std::string sourceFile; @@ -62,23 +64,52 @@ static boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { + // We attempt to not assert on probably-not deadlocks by assuming that + // a try lock will immediately have otherwise bailed if it had + // failed to get the lock + // We do this by, for the locks which triggered the potential deadlock, + // in either lockorder, checking that the second of the two which is locked + // is only a TRY_LOCK, ignoring locks if they are reentrant. + bool firstLocked = false; + bool secondLocked = false; + bool onlyMaybeDeadlock = false; + LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + firstLocked = false; + secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + assert(onlyMaybeDeadlock); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) @@ -101,10 +132,8 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) { + if (lockorders.count(p2)) potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); - break; - } } } dd_mutex.unlock(); @@ -119,7 +148,7 @@ static void pop_lock() void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) { - push_lock(cs, CLockLocation(pszName, pszFile, nLine), fTry); + push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry); } void LeaveCritical() From 5093299af09ac06b1596653871806918ae13f44d Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 16 Jun 2015 03:46:36 -0400 Subject: [PATCH 4/6] locking: teach Clang's -Wthread-safety to cope with our scoped lock macros This allows us to use function/variable/class attributes to specify locking requisites, allowing problems to be detected during static analysis. This works perfectly with newer Clang versions (tested with 3.3-3.7). For older versions (tested 3.2), it compiles fine but spews lots of false-positives. --- src/sync.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sync.h b/src/sync.h index 5c1516534..68a944308 100644 --- a/src/sync.h +++ b/src/sync.h @@ -101,7 +101,7 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine); /** Wrapper around boost::unique_lock */ template -class CMutexLock +class SCOPED_LOCKABLE CMutexLock { private: boost::unique_lock lock; @@ -129,7 +129,7 @@ private: } public: - CMutexLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) : lock(mutexIn, boost::defer_lock) + CMutexLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, boost::defer_lock) { if (fTry) TryEnter(pszName, pszFile, nLine); @@ -137,7 +137,7 @@ public: Enter(pszName, pszFile, nLine); } - CMutexLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) + CMutexLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) { if (!pmutexIn) return; @@ -148,7 +148,7 @@ public: Enter(pszName, pszFile, nLine); } - ~CMutexLock() + ~CMutexLock() UNLOCK_FUNCTION() { if (lock.owns_lock()) LeaveCritical(); From 72b25b0ffd980f984aa3086922190f048e496bb6 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 16 Jun 2015 04:08:26 -0400 Subject: [PATCH 5/6] locking: add a quick example of GUARDED_BY This was chosen not because it's necessarily helpful, but because its locking assumptions were already correct. --- src/main.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 207c0d458..616087b72 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -79,9 +79,9 @@ struct COrphanTx { CTransaction tx; NodeId fromPeer; }; -map mapOrphanTransactions; -map > mapOrphanTransactionsByPrev; -void EraseOrphansFor(NodeId peer); +map mapOrphanTransactions GUARDED_BY(cs_main);; +map > mapOrphanTransactionsByPrev GUARDED_BY(cs_main);; +void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Returns true if there are nRequired or more blocks of minVersion or above @@ -554,7 +554,7 @@ CBlockTreeDB *pblocktree = NULL; // mapOrphanTransactions // -bool AddOrphanTx(const CTransaction& tx, NodeId peer) +bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { uint256 hash = tx.GetHash(); if (mapOrphanTransactions.count(hash)) @@ -584,7 +584,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) return true; } -void static EraseOrphanTx(uint256 hash) +void static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { map::iterator it = mapOrphanTransactions.find(hash); if (it == mapOrphanTransactions.end()) @@ -618,7 +618,7 @@ void EraseOrphansFor(NodeId peer) } -unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) +unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { unsigned int nEvicted = 0; while (mapOrphanTransactions.size() > nMaxOrphans) @@ -4140,7 +4140,7 @@ string GetWarnings(string strFor) // -bool static AlreadyHave(const CInv& inv) +bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { switch (inv.type) { From df8f8095b638e439dd5f33a2dc762475ee930f4d Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Wed, 22 Jul 2015 12:03:16 -0400 Subject: [PATCH 6/6] Don't share objects between TestInstances --- qa/rpc-tests/test_framework/comptool.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/test_framework/comptool.py b/qa/rpc-tests/test_framework/comptool.py index 24ae05807..b945f1bf2 100755 --- a/qa/rpc-tests/test_framework/comptool.py +++ b/qa/rpc-tests/test_framework/comptool.py @@ -140,8 +140,8 @@ class TestNode(NodeConnCB): # or false, then only the last tx is tested against outcome.) class TestInstance(object): - def __init__(self, objects=[], sync_every_block=True, sync_every_tx=False): - self.blocks_and_transactions = objects + def __init__(self, objects=None, sync_every_block=True, sync_every_tx=False): + self.blocks_and_transactions = objects if objects else [] self.sync_every_block = sync_every_block self.sync_every_tx = sync_every_tx