mempool janitor: periodic sweep and clean of not-confirming transactions #3753

pull jgarzik wants to merge 2 commits into bitcoin:master from jgarzik:mempool-janitor changing 9 files +139 −2
  1. jgarzik commented at 10:27 pm on February 26, 2014: contributor

    The mempool janitor (“poolman”) is a thread that runs every -janitorinterval seconds. The janitor scans and removes memory pool transactions older than current time minus -janitorexpire seconds.

    By default, janitor runs every 24 hrs, expiring TXs older than 72 hrs [and have failed to make it into a block in that time].

    IsMine() transactions are not touched.

    This is intentionally crude: easily reviewed, reasoned and tested; fitting easily within the current framework, or being backported to an older bitcoind. A key goal was not rewriting mempool.

    Comments:

    • One alternative implementation was considered:
    0     while (mempool byte size > limit)
    1            expire oldest !ismine
    

    This would work, but require a sort step, as mempool is not time-ordered. There is also the question of how often to run such a while{} loop, likely leading to some sort of high-water/low-water system.

    The sweep implementation presented seemed more straightforward.

  2. jgarzik commented at 11:13 pm on February 26, 2014: contributor
    Self-note: does pwalletMain->IsMine() require a lock? I’m thinking yes.
  3. Diapolo commented at 11:36 pm on February 26, 2014: none
    Why not use the term gc (garbage collection)?
  4. ABISprotocol commented at 11:37 pm on February 26, 2014: none
    Nice, thanks.
  5. gavinandresen commented at 2:59 pm on February 27, 2014: contributor
    Good idea, ACK on concept.
  6. rebroad commented at 6:55 pm on February 27, 2014: contributor
    What has changed regarding bitcoin that is making this feature more necessary now where it wasn’t so necessary in the past?
  7. gavinandresen commented at 7:02 pm on February 27, 2014: contributor

    @rebroad : we want to lower the relay fee, to give miners more opportunity to mine lower-fee transactions. But we don’t want attackers to be able to gum up memory with lots of spammy, will-never-get-mined transactions…

    (and this is where Mike Hearn jumps in and points out, again, that we’re doing it wrong and we should relay all transactions until we hit per-peer memory or bandwidth limits… and he is correct, but that is a lot more work).

  8. in src/poolman.cpp: in 9119221019 outdated
     7+
     8+using namespace std;
     9+
    10+int64_t janitorExpire; // global; expire TXs n seconds older than this
    11+
    12+static unsigned int IgnoreWalletTransactions(vector<CTransaction>& vtx)
    


    laanwj commented at 11:38 am on February 28, 2014:

    I’d rather have that the wallets let the mempool know that a transaction is precious (with a flag), than having the mempool manager poll the main wallet.

    If you want to do it this way (poolman -> wallets) please use the RegisterWallet / UnregisterWallet machinery from main.cpp, and add a CWalletInterface method for this.


    sipa commented at 9:52 pm on May 20, 2014:
    Agree.

    ABISprotocol commented at 11:54 am on January 9, 2015:
    poolbeing -> wallets
  9. laanwj commented at 12:03 pm on February 28, 2014: member
    @diapolo The name garbage collection is way, way too general. This cleans up the transactions in the pool, so pool manager is a good name. ACK on concept
  10. gmaxwell commented at 12:06 pm on February 28, 2014: contributor
    Pool boy.
  11. sipa commented at 1:00 pm on February 28, 2014: member

    I believe the current implementation will remove unconfirmed dependencies of wallet transactions, transitively removing wallet transactions too.

    I’d prefer a flag bit in the mempool to mark transactions as precious, and when applied, it automatically propagates up to its dependencies.

  12. sipa commented at 1:35 pm on February 28, 2014: member
    I also don’t really like mempool (management) code needing to know about all connected wallets, and acquire locks on them. The mempool should function independently.
  13. charlie93 commented at 6:23 pm on March 1, 2014: none
    Can’t believe I registered for this. I’m with gmaxwell, “pool boy” is more appropriate.
  14. mikehearn commented at 1:05 pm on March 14, 2014: contributor
    Why a separate thread? It could just run after receiving a new tx, I think? The more threads there are, the easier it is to slip up and slice your thumb off …
  15. jgarzik commented at 1:10 pm on March 14, 2014: contributor
    This operation is too slow and expensive to run after every new TX.
  16. mikehearn commented at 1:13 pm on March 14, 2014: contributor
    The first step is to check if it’s time to run, i.e. if now - lastTime > 24hrs. So I don’t see what the issue is.
  17. jgarzik commented at 1:16 pm on March 14, 2014: contributor

    @mikehearn Ah, I thought you were suggesting to run the operation after receipt of every TX.

    Yes, you could do it that way too.

  18. laanwj added this to the milestone 0.10.0 on Apr 19, 2014
  19. jgarzik commented at 9:27 pm on May 20, 2014: contributor
    Rebased
  20. in src/poolman.cpp: in 646c1a2812 outdated
    41+    vector<CTransaction> vtx;
    42+    mempool.queryOld(vtx, expireTime);
    43+    unsigned int nOld = vtx.size();
    44+
    45+    // pass 2: ignore wallet transactions (remove from vtx)
    46+    unsigned int nMine = IgnoreWalletTransactions(vtx);
    


    sipa commented at 9:53 pm on May 20, 2014:
    You also need to remove everything that depends on a wallet transaction.

    ABISprotocol commented at 7:37 am on May 22, 2014:
    Thank you
  21. ABISprotocol commented at 0:36 am on June 24, 2014: none
    Please explain the merge conflict.
  22. in src/init.cpp: in 646c1a2812 outdated
    232@@ -232,6 +233,8 @@ std::string HelpMessage(HelpMessageMode hmm)
    233     strUsage += "  -seednode=<ip>         " + _("Connect to a node to retrieve peer addresses, and disconnect") + "\n";
    234     strUsage += "  -socks=<n>             " + _("Select SOCKS version for -proxy (4 or 5, default: 5)") + "\n";
    235     strUsage += "  -timeout=<n>           " + _("Specify connection timeout in milliseconds (default: 5000)") + "\n";
    236+    strUsage += "  -janitorinterval=<n>   " + _("Number of seconds between each mempool janitor run (default: 1 day)") + "\n";
    


    Diapolo commented at 11:32 am on July 2, 2014:

    Can you ensure these new commands are alphabetically ordered in the help message list.

    May I also suggest to add a DEFAULT_JANITORINTERVAL, DEFAULT_JANITOREXPIRE and use that here and in the GetArg(). It IMHO nice to use this in the help strings, because when changing a default we don’t need to change the help string (and don’t need to re-translate).

    See e.g. -blockmaxsize= in init.cpp.

  23. jgarzik commented at 6:34 pm on July 18, 2014: contributor

    Rebased. The following feedback from @laanwj @sipa must be addressed before merge, “I’d rather have that the wallets let the mempool know that a transaction is precious (with a flag), than having the mempool manager poll the main wallet.

    If you want to do it this way (poolman -> wallets) please use the RegisterWallet / UnregisterWallet machinery from main.cpp, and add a CWalletInterface method for this.”

  24. ABISprotocol commented at 8:05 pm on July 18, 2014: none
    @jgarzik Thanks for your work on this!
  25. jgarzik added the label Improvement on Jul 31, 2014
  26. jgarzik added the label Feature on Jul 31, 2014
  27. jgarzik removed this from the milestone 0.10.0 on Jul 31, 2014
  28. mempool janitor: periodic sweep and clean of not-confirming transactions
    The mempool janitor ("poolman") is a thread that runs every -janitorinterval
    seconds.  The janitor scans and removes memory pool transactions
    older than current time minus -janitorexpire seconds.
    
    By default, janitor runs every 24 hrs, expiring TXs older than 72 hrs.
    
    IsMine() transactions are not touched.
    f55882fb41
  29. RPC: Add 'sweepmempool' method, to manually initiate poolman b5d216a7b3
  30. BitcoinPullTester commented at 7:41 pm on August 18, 2014: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3753_b5d216a7b3dd68b784601973ff95e15cdfd13ebf/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
  31. dgenr8 commented at 4:08 pm on August 30, 2014: contributor

    Expiring from mempool based on wall clock age, measured from first receipt, suffers from a reseeding problem: any node with a copy of tx can re-introduce it and start the cycle again, and this will happen easily due to clock differences.

    One way to mitigate this problem would be to stop relaying tx for some time before forgetting completely about it.

    A stronger and complimentary improvement is to expire based on nLockTime whenever it is non-zero. The meaning of nLockTime is “last non-final height (or blocktime)”. Expiring after a certain period of finality solves the reseeding problem by using the appropriate property of the tx itself. As many transactions as possible should carry nLockTime, even if they never actually undergo a period of non-finality; #2340 will ensure that txes created by bitcoin core carry it by default.

  32. ABISprotocol commented at 3:19 am on September 2, 2014: none
    Brief note: certain uses of nLockTime can be used intentionally (or perhaps unintentionally as well) to create problems, for details go dig into bitcointalk or, this stackexchange example… https://bitcoin.stackexchange.com/questions/8408/can-nlocktime-tx-be-used-to-flood-mempools
  33. laanwj commented at 8:25 am on September 2, 2014: member
    The possibility of reseeding is intentional. The reason for this pull is resource management, ie, make sure that transactions that no one cares about don’t linger around forever. A node is free to rebroadcast transactions that it cares about.
  34. dgenr8 commented at 6:14 am on September 3, 2014: contributor
    I could accidentally debilitate mempool janitors across the entire network if I set up two nodes to exchange mempools whenever they reconnected to each other, and restarted each frequently. –Kaz Wesley
  35. jgarzik commented at 12:57 pm on September 3, 2014: contributor
    @dgenr8 IGNORING the fact that nodes don’t do that now, such modified nodes would then offer a bunch of TXs that other nodes would ignore.
  36. dgenr8 commented at 11:36 pm on September 3, 2014: contributor
    Ok, “accidentally” is a stretch. These nodes would also have to “accidentally” persist their mempools across restarts to actually be troublesome. But it suggests how easily a trouble-maker could defeat janitors network-wide that rely only on receipt time.
  37. jgarzik commented at 11:55 pm on September 3, 2014: contributor
    As noted, the troublemaker is not just two nodes restarting or remembering longer. That is just fine. A troublemaker would have to actively connect and offer ancient TXs, and that would be noticed.
  38. ABISprotocol commented at 5:00 am on September 9, 2014: none
    Probably unrelated, but ‘poolman’ as well understood as it might be, seems so hombre-focused. Alternatives? poolbeing, poolhelper, poolbot, poolio, poolcleaner
  39. laanwj added this to the milestone 0.11.0 on Nov 17, 2014
  40. jonasschnelli commented at 9:40 am on December 29, 2014: contributor
    Needs rebase.
  41. ABISprotocol commented at 4:03 am on January 2, 2015: none
    I just want to leave this as a comment rather than forking, pull requesting or any such-ness atm. I wanted to suggest that when this goes through to final at such point when rebase please consider the original remark I had some while back which was, I think, to rename from poolman.h to poolbeing.h, (see line 99 at src/Makefile.am for example). Similarly poolman.cpp would become poolbeing.cpp and so forth. Not clear if there was some other consensus for “manager” as per prior discussions. Tired. Thank you if possible. Please e-mail me with any questions.
  42. laanwj added the label Mempool on Jan 8, 2015
  43. in src/init.cpp: in b5d216a7b3
    253@@ -253,6 +254,8 @@ std::string HelpMessage(HelpMessageMode mode)
    254     strUsage += "  -proxy=<ip:port>       " + _("Connect through SOCKS5 proxy") + "\n";
    255     strUsage += "  -seednode=<ip>         " + _("Connect to a node to retrieve peer addresses, and disconnect") + "\n";
    256     strUsage += "  -timeout=<n>           " + _("Specify connection timeout in milliseconds (default: 5000)") + "\n";
    257+    strUsage += "  -janitorinterval=<n>   " + _("Number of seconds between each mempool janitor run (default: 1 day)") + "\n";
    258+    strUsage += "  -janitorexpire=<n>     " + _("Number of seconds transactions live in memory pool, before removal (default: 3 days)") + "\n";
    


    Diapolo commented at 2:04 pm on January 8, 2015:
    Nit: Could you switch these two and place them above -listen please. Also @luke-jr A few weeks ago changed all strings in help messages to use strprintf(_()) as that makes life for translators easier.

    ABISprotocol commented at 11:53 am on January 9, 2015:
    see notes on changes from poolman to poolbeing (languaging basically) above
  44. in src/init.cpp: in b5d216a7b3
    726@@ -724,6 +727,16 @@ bool AppInit2(boost::thread_group& threadGroup)
    727             threadGroup.create_thread(&ThreadScriptCheck);
    728     }
    729 
    730+    // mempool janitor execution interval.  default interval: 1 day
    731+    int janitorInterval = GetArg("-janitorinterval", (60 * 60 * 24 * 1));
    


    Diapolo commented at 2:05 pm on January 8, 2015:
    This allows negative numbers, is this intended? Perhaps just use unsigned.
  45. in src/poolman.cpp: in b5d216a7b3
    0@@ -0,0 +1,58 @@
    1+
    


    Diapolo commented at 2:06 pm on January 8, 2015:
    Missing license :).
  46. in src/poolman.h: in b5d216a7b3
    0@@ -0,0 +1,9 @@
    1+#ifndef __BITCOIN_POOLMAN_H__
    


    Diapolo commented at 2:07 pm on January 8, 2015:
    Also misses license and correct header define #ifndef BITCOIN_POOLMAN_H (and at the file end).
  47. in src/rpcblockchain.cpp: in b5d216a7b3
    412+    if (fHelp || params.size() != 0)
    413+        throw runtime_error(
    414+            "sweepmempool\n"
    415+            "\nRemoves old, unconfirmed transactions from mempool\n"
    416+            "\nResult:\n"
    417+            "n    (bool) true\n"
    


    Diapolo commented at 2:08 pm on January 8, 2015:
    Nit: Is this correct help text formating?
  48. in src/Makefile.am: in b5d216a7b3
    95@@ -96,6 +96,7 @@ BITCOIN_CORE_H = \
    96   net.h \
    97   noui.h \
    98   pow.h \
    99+  poolman.h \
    


    ABISprotocol commented at 11:50 am on January 9, 2015:
    change to poolbeing.h \
  49. in src/Makefile.am: in b5d216a7b3
    204@@ -204,6 +205,7 @@ libbitcoin_common_a_SOURCES = \
    205   key.cpp \
    206   keystore.cpp \
    207   netbase.cpp \
    208+  poolman.cpp \
    


    ABISprotocol commented at 11:51 am on January 9, 2015:
    change to poolbeing.cpp \
  50. in src/init.cpp: in b5d216a7b3
    18@@ -19,6 +19,7 @@
    19 #include "txdb.h"
    20 #include "ui_interface.h"
    21 #include "util.h"
    22+#include "poolman.h"
    


    ABISprotocol commented at 11:52 am on January 9, 2015:
    change to “poolbeing.h”
  51. in src/poolman.cpp: in b5d216a7b3
    0@@ -0,0 +1,58 @@
    1+
    2+#include "poolman.h"
    


    ABISprotocol commented at 11:53 am on January 9, 2015:
    should be changed from poolman.h to poolbeing.h
  52. in src/rpcblockchain.cpp: in b5d216a7b3
     6@@ -7,6 +7,7 @@
     7 #include "main.h"
     8 #include "rpcserver.h"
     9 #include "sync.h"
    10+#include "poolman.h"
    


    ABISprotocol commented at 11:55 am on January 9, 2015:
    poolman.h to be changed to poolbeing.h
  53. ghost commented at 7:55 pm on February 5, 2015: none

    What is a Poolbeing? Politically-correct nonsense which doesn’t belong in bitcoin core is what.

    I vote Poolbot or Poolcleaner if you’re so upset by the ending ‘-man’ in the English language.

  54. ABISprotocol commented at 9:09 pm on February 15, 2015: none

    Hi, my request for poolbeing was alluding for something beyond the standard anthropomorphic reference, and yes, that which ended in man. Not that I was upset by it, because I’m not, but I think that in light of the progress of technology today (consider for example DAOs) it makes more sense to have poolbeing. In light of your question, “what is a poolbeing,” I would ask to you in return, “What is a poolman?” And there is nothing, because there is no poolman. The poolman is not there. We knock at the door, and he is not home.

    ‘In Search of The Eternal Poolbeing’

    “Because of the one-pointed Time awareness in which the conventional mind remains immersed, humans tend to think of everything in a sequential, word-oriented framework. This mental trap produces very short-term concepts of effectiveness and consequences, a condition of constant, unplanned response to crises.”

    • Liet-Kynes The Arrakis Workbook (as quoted from Children of Dune, by Frank Herbert)
  55. sipa commented at 9:38 pm on February 15, 2015: member
    Not sure about the intention, but I always interpreted ‘man’ here as an abbreviation for ‘mamager’…
  56. dexX7 commented at 12:00 pm on April 24, 2015: contributor

    The command "invalidateblock" is handy for testing with one node, but I was looking for mempool manipulation, and this was the closest thing I’ve found. So for what it’s worth, since it’s slightly related, I added a command to clear the whole mempool via the RPC interface:

  57. laanwj removed this from the milestone 0.11.0 on May 18, 2015
  58. pstratem commented at 2:59 am on June 13, 2015: contributor
    @ABISprotocol poolman is short for pool manager, manager is from the latin manus for hand, and has nothing to do with gender
  59. laanwj commented at 5:04 am on June 13, 2015: member
    How to move forward here? (and I don’t mean in regard to silly word games)
  60. pstratem commented at 6:03 am on June 13, 2015: contributor
    Is expiration of mempool transactions on a fixed known schedule really the best solution?
  61. jgarzik commented at 4:50 pm on June 13, 2015: contributor

    @laanwj The merge blocker with this pull request is a @sipa comment from long ago: The janitor must not sweep transactions which are “precious” to the local node. Generally this means wallet transactions.

    This is not a blocker for disabled-wallet configurations; thus two paths forward are,

    1. Disable poolman for wallet nodes.
    2. Do The Right Thing, and create an interface whereby the wallet module tells mempool (or poolman) which transactions are precious and should not be automatically culled. (cc @jonasschnelli) @pstratem No. It is however a rough approximation of a solution that can be deployed today, modulo comments just made.

    The consensus on solution is a “superblock” type model, where

    • Mempool is modelled after a X megabyte sized block (X = 24 hours worth of data, according to proposal)
    • Transactions entering system are judged based on fee/priority, and insertion-sorted accordingly
    • One or more transactions, perhaps include the latest transaction itself, if total mempool size exceeds X

    This real-time priority based culling is preferable to periodic garbage collection; however the “ideal solution” does not yet have any code or testing under its belt.

    poolman is an imperfect stopgap solution until The Ideal Solution arrives.

  62. jonasschnelli commented at 6:15 pm on June 13, 2015: contributor
    For the wallet enabled mode, i think, if there would be a signal over CValidationInterface (something like CheckJanitor(const CTransaction& tx, bool& clean) the wallet could allow/disallow “precious” transactions to be preserved/not affected by the janitor. I will rebase this PR and add the additional wallet/validation-interface signaling (during the next couple of days). IMO this PR (or lets say something that clean the mempool) is long overdue.
  63. pstratem commented at 9:55 pm on June 13, 2015: contributor

    @jgarzik well clearly this not a huge priority for merging (it’s been sitting around for over a year now)

    modeling the mempool as a superblock seems reasonable, but is maybe too expensive to actually run everytime a new transaction is included.

    what do you think about simply removing the transaction (and it’s dependents) with the lowest priority?

  64. ABISprotocol commented at 9:53 am on July 15, 2015: none
    @pstratem removing transaction(s) and dependents with lowest priority is not a good idea because low-priority dust (and/or transactions which exceed dust threshold but which are of lower priorities) should not be rejected due to the importance of inclusion of types of transactions to support a wide range of participants’ needs, income ranges, and re-envisioning of dust as microgiving capacity or opportunity ( see my comments in #6402 - #1536 - #6201 ).
  65. jgarzik commented at 5:47 pm on July 23, 2015: contributor

    Closing out older pull requests.

    This is not a judgement on this pull request or time-based expiration. Time-based mempool expiration continues to be the best way to sample what miners are not confirming.

    There is potential for resubmitting or reopening this in the future.

  66. jgarzik closed this on Jul 23, 2015

  67. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me