Merge #9369: Factor out CWallet::nTimeSmart computation into a method.

630fc54 Clean up braces in CWallet::ComputeTimeSmart (Russell Yanofsky)
6c996c2 Add documentation describing CWallet::nTimeSmart. (Russell Yanofsky)
1f98abe Factor out CWallet::nTimeSmart computation into a method. (Russell Yanofsky)
c6b82d1 Add tests for CWalletTx::nTimeSmart (Russell Yanofsky)

Tree-SHA512: 457a30251e572cf20dac0198af1a94128d269b1e0ce6605a213d56fc14d85c84a0a494e3dcbb18c201c4f39e6f7b000bd9cb6f283930d8452e4bb93ba406f8d4

src/wallet/test/wallet_tests.cpp

@@ -453,4 +453,57 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
     BOOST_CHECK_EQUAL(wtx.GetImmatureCredit(), 50*COIN);
 }
 
+static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)
+{
+    CMutableTransaction tx;
+    tx.nLockTime = lockTime;
+    SetMockTime(mockTime);
+    CBlockIndex* block = nullptr;
+    if (blockTime > 0) {
+        auto inserted = mapBlockIndex.emplace(GetRandHash(), new CBlockIndex);
+        assert(inserted.second);
+        const uint256& hash = inserted.first->first;
+        block = inserted.first->second;
+        block->nTime = blockTime;
+        block->phashBlock = &hash;
+    }
+
+    CWalletTx wtx(&wallet, MakeTransactionRef(tx));
+    if (block) {
+        wtx.SetMerkleBranch(block, 0);
+    }
+    wallet.AddToWallet(wtx);
+    return wallet.mapWallet.at(wtx.GetHash()).nTimeSmart;
+}
+
+// Simple test to verify assignment of CWalletTx::nSmartTime value. Could be
+// expanded to cover more corner cases of smart time logic.
+BOOST_AUTO_TEST_CASE(ComputeTimeSmart)
+{
+    CWallet wallet;
+
+    // New transaction should use clock time if lower than block time.
+    BOOST_CHECK_EQUAL(AddTx(wallet, 1, 100, 120), 100);
+
+    // Test that updating existing transaction does not change smart time.
+    BOOST_CHECK_EQUAL(AddTx(wallet, 1, 200, 220), 100);
+
+    // New transaction should use clock time if there's no block time.
+    BOOST_CHECK_EQUAL(AddTx(wallet, 2, 300, 0), 300);
+
+    // New transaction should use block time if lower than clock time.
+    BOOST_CHECK_EQUAL(AddTx(wallet, 3, 420, 400), 400);
+
+    // New transaction should use latest entry time if higher than
+    // min(block time, clock time).
+    BOOST_CHECK_EQUAL(AddTx(wallet, 4, 500, 390), 400);
+
+    // If there are future entries, new transaction should use time of the
+    // newest entry that is no more than 300 seconds ahead of the clock time.
+    BOOST_CHECK_EQUAL(AddTx(wallet, 5, 50, 600), 300);
+
+    // Reset mock time for other tests.
+    SetMockTime(0);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/wallet/wallet.cpp

@@ -870,51 +870,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
         wtx.nTimeReceived = GetAdjustedTime();
         wtx.nOrderPos = IncOrderPosNext(&walletdb);
         wtxOrdered.insert(make_pair(wtx.nOrderPos, TxPair(&wtx, (CAccountingEntry*)0)));
-
+        wtx.nTimeSmart = ComputeTimeSmart(wtx);
-        wtx.nTimeSmart = wtx.nTimeReceived;
-        if (!wtxIn.hashUnset())
-        {
-            if (mapBlockIndex.count(wtxIn.hashBlock))
-            {
-                int64_t latestNow = wtx.nTimeReceived;
-                int64_t latestEntry = 0;
-                {
-                    // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future
-                    int64_t latestTolerated = latestNow + 300;
-                    const TxItems & txOrdered = wtxOrdered;
-                    for (TxItems::const_reverse_iterator it = txOrdered.rbegin(); it != txOrdered.rend(); ++it)
-                    {
-                        CWalletTx *const pwtx = (*it).second.first;
-                        if (pwtx == &wtx)
-                            continue;
-                        CAccountingEntry *const pacentry = (*it).second.second;
-                        int64_t nSmartTime;
-                        if (pwtx)
-                        {
-                            nSmartTime = pwtx->nTimeSmart;
-                            if (!nSmartTime)
-                                nSmartTime = pwtx->nTimeReceived;
-                        }
-                        else
-                            nSmartTime = pacentry->nTime;
-                        if (nSmartTime <= latestTolerated)
-                        {
-                            latestEntry = nSmartTime;
-                            if (nSmartTime > latestNow)
-                                latestNow = nSmartTime;
-                            break;
-                        }
-                    }
-                }
-
-                int64_t blocktime = mapBlockIndex[wtxIn.hashBlock]->GetBlockTime();
-                wtx.nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
-            }
-            else
-                LogPrintf("AddToWallet(): found %s in block %s not in index\n",
-                         wtxIn.GetHash().ToString(),
-                         wtxIn.hashBlock.ToString());
-        }
         AddToSpends(hash);
     }
 
@@ -3472,6 +3428,71 @@ void CWallet::GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) c
         mapKeyBirth[it->first] = it->second->GetBlockTime() - TIMESTAMP_WINDOW; // block times can be 2h off
 }
 
+/**
+ * Compute smart timestamp for a transaction being added to the wallet.
+ *
+ * Logic:
+ * - If sending a transaction, assign its timestamp to the current time.
+ * - If receiving a transaction outside a block, assign its timestamp to the
+ *   current time.
+ * - If receiving a block with a future timestamp, assign all its (not already
+ *   known) transactions' timestamps to the current time.
+ * - If receiving a block with a past timestamp, before the most recent known
+ *   transaction (that we care about), assign all its (not already known)
+ *   transactions' timestamps to the same timestamp as that most-recent-known
+ *   transaction.
+ * - If receiving a block with a past timestamp, but after the most recent known
+ *   transaction, assign all its (not already known) transactions' timestamps to
+ *   the block time.
+ *
+ * For more information see CWalletTx::nTimeSmart,
+ * https://bitcointalk.org/?topic=54527, or
+ * https://github.com/bitcoin/bitcoin/pull/1393.
+ */
+unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
+{
+    unsigned int nTimeSmart = wtx.nTimeReceived;
+    if (!wtx.hashUnset()) {
+        if (mapBlockIndex.count(wtx.hashBlock)) {
+            int64_t latestNow = wtx.nTimeReceived;
+            int64_t latestEntry = 0;
+
+            // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future
+            int64_t latestTolerated = latestNow + 300;
+            const TxItems& txOrdered = wtxOrdered;
+            for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) {
+                CWalletTx* const pwtx = it->second.first;
+                if (pwtx == &wtx) {
+                    continue;
+                }
+                CAccountingEntry* const pacentry = it->second.second;
+                int64_t nSmartTime;
+                if (pwtx) {
+                    nSmartTime = pwtx->nTimeSmart;
+                    if (!nSmartTime) {
+                        nSmartTime = pwtx->nTimeReceived;
+                    }
+                } else {
+                    nSmartTime = pacentry->nTime;
+                }
+                if (nSmartTime <= latestTolerated) {
+                    latestEntry = nSmartTime;
+                    if (nSmartTime > latestNow) {
+                        latestNow = nSmartTime;
+                    }
+                    break;
+                }
+            }
+
+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else {
+            LogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.hashBlock.ToString());
+        }
+    }
+    return nTimeSmart;
+}
+
 bool CWallet::AddDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
 {
     if (boost::get<CNoDestination>(&dest))

src/wallet/wallet.h

@@ -285,6 +285,15 @@ public:
     std::vector<std::pair<std::string, std::string> > vOrderForm;
     unsigned int fTimeReceivedIsTxTime;
     unsigned int nTimeReceived; //!< time received by this node
+    /**
+     * Stable timestamp that never changes, and reflects the order a transaction
+     * was added to the wallet. Timestamp is based on the block time for a
+     * transaction added as part of a block, or else the time when the
+     * transaction was received if it wasn't part of a block, with the timestamp
+     * adjusted in both cases so timestamp order matches the order transactions
+     * were added to the wallet. More details can be found in
+     * CWallet::ComputeTimeSmart().
+     */
     unsigned int nTimeSmart;
     /**
      * From me flag is set to 1 for transactions that were created by the wallet
@@ -800,6 +809,7 @@ public:
     bool EncryptWallet(const SecureString& strWalletPassphrase);
 
     void GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) const;
+    unsigned int ComputeTimeSmart(const CWalletTx& wtx) const;
 
     /** 
      * Increment the next transaction order id