Delete expired txs from the wallet, since they can never be included in a block

src/wallet/wallet.cpp

@@ -1715,9 +1715,9 @@ void CWallet::EraseFromWallet(const uint256 &hash)
         if (mapWallet.erase(hash))
             CWalletDB(strWalletFile).EraseTx(hash);
     }
-    if(fDebug) {
+
-        LogPrintf("%s: erased txid %s\n", __func__, hash.ToString().c_str() );
+    LogPrintf("%s: erased txid %s\n", __func__, hash.ToString().c_str() );
-    }
+
     return;
 }
 
@@ -3111,6 +3111,8 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(int64_t nTime)
     // Sort them in chronological order
     multimap<unsigned int, CWalletTx*> mapSorted;
     uint32_t now = (uint32_t)time(NULL);
+
+    // vector of wallet transactions to delete
     std::vector<uint256> vwtxh;
     uint32_t erased = 0, skipped = 0;
 
@@ -3122,9 +3124,11 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(int64_t nTime)
             continue;
 
         // Do not relay expired transactions, to avoid other nodes banning us
+        // Current code will not ban nodes relaying expired txs but older nodes will
         if (wtx.nExpiryHeight > 0 && wtx.nExpiryHeight < chainActive.LastTip()->GetHeight()) {
             fprintf(stderr,"%s: ignoring expired tx %s\n", __func__, wtx.GetHash().ToString().c_str() );
-            // TODO: should we call EraseFromWallet(wtx) right here?
+            // append to list of txs to delete
+            vwtxh.push_back(wtx.GetHash());
             continue;
         }
 
@@ -3134,6 +3138,7 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(int64_t nTime)
                 LogPrintf("%s: skip Relaying wtx %s nLockTime %u vs now.%u\n", __func__, wtx.GetHash().ToString(),(uint32_t)wtx.nLockTime,now);
             }
             skipped++;
+            // TODO: this does not seem to handle rescanning+finding old coinbase txs correctly
             //vwtxh.push_back(wtx.GetHash());
             continue;
         }
@@ -3150,10 +3155,13 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(int64_t nTime)
         }
     }
 
-    // TODO: this does not seem to handle rescanning+finding old coinbase txs correctly
+    // Unless we remove these unconfirmed and/or expired txs from the wallet, they will
-    // Unless we remove these unconfirmed txs from the wallet, they will
     // persist there forever. They are too old to be accepted by network
     // consensus rules, so we erase them.
+    // Expired txs are always unconfirmed, but unconfirmed tx's could be expired or not,
+    // i.e. expired txs are a subset of unconfirmed tx's. Expired tx's can never be included
+    // in a block because they are against consensus rules. Unconfirmed tx's might still be
+    // included in a future block.
     for (auto hash : vwtxh)
     {
         EraseFromWallet(hash);
@@ -3174,6 +3182,7 @@ void CWallet::ResendWalletTransactions(int64_t nBestBlockTime)
     if (GetTime() < nNextResend || !fBroadcastTransactions)
         return;
     bool fFirst = (nNextResend == 0);
+    // TODO: BTC Core changed this to be every 12 hours instead of every 30 mins
     nNextResend = GetTime() + GetRand(30 * 60);
     if (fFirst)
         return;