New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@jgarzik
Contributor

jgarzik commented Feb 26, 2014

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:
     while (mempool byte size > limit)
            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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Feb 26, 2014

Contributor

Self-note: does pwalletMain->IsMine() require a lock? I'm thinking yes.

Contributor

jgarzik commented Feb 26, 2014

Self-note: does pwalletMain->IsMine() require a lock? I'm thinking yes.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Feb 26, 2014

Why not use the term gc (garbage collection)?

Diapolo commented Feb 26, 2014

Why not use the term gc (garbage collection)?

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Feb 26, 2014

Nice, thanks.

ABISprotocol commented Feb 26, 2014

Nice, thanks.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Feb 27, 2014

Contributor

Good idea, ACK on concept.

Contributor

gavinandresen commented Feb 27, 2014

Good idea, ACK on concept.

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Feb 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?

Contributor

rebroad commented Feb 27, 2014

What has changed regarding bitcoin that is making this feature more necessary now where it wasn't so necessary in the past?

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Feb 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).

Contributor

gavinandresen commented Feb 27, 2014

@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).

int64_t janitorExpire; // global; expire TXs n seconds older than this
static unsigned int IgnoreWalletTransactions(vector<CTransaction>& vtx)

This comment has been minimized.

@laanwj

laanwj Feb 28, 2014

Member

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.

@laanwj

laanwj Feb 28, 2014

Member

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.

This comment has been minimized.

@sipa

sipa May 20, 2014

Member

Agree.

@sipa

sipa May 20, 2014

Member

Agree.

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

poolbeing -> wallets

@ABISprotocol

ABISprotocol Jan 9, 2015

poolbeing -> wallets

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 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

Member

laanwj commented Feb 28, 2014

@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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 28, 2014

Member

Pool boy.

Member

gmaxwell commented Feb 28, 2014

Pool boy.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 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.

Member

sipa commented Feb 28, 2014

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 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.

Member

sipa commented Feb 28, 2014

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.

@charlie93

This comment has been minimized.

Show comment
Hide comment
@charlie93

charlie93 Mar 1, 2014

Can't believe I registered for this. I'm with gmaxwell, "pool boy" is more appropriate.

charlie93 commented Mar 1, 2014

Can't believe I registered for this. I'm with gmaxwell, "pool boy" is more appropriate.

@mikehearn

This comment has been minimized.

Show comment
Hide comment
@mikehearn

mikehearn Mar 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 ...

Contributor

mikehearn commented Mar 14, 2014

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 ...

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Mar 14, 2014

Contributor

This operation is too slow and expensive to run after every new TX.

Contributor

jgarzik commented Mar 14, 2014

This operation is too slow and expensive to run after every new TX.

@mikehearn

This comment has been minimized.

Show comment
Hide comment
@mikehearn

mikehearn Mar 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.

Contributor

mikehearn commented Mar 14, 2014

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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Mar 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.

Contributor

jgarzik commented Mar 14, 2014

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

Yes, you could do it that way too.

@laanwj laanwj added this to the 0.10.0 milestone Apr 19, 2014

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik May 20, 2014

Contributor

Rebased

Contributor

jgarzik commented May 20, 2014

Rebased

unsigned int nOld = vtx.size();
// pass 2: ignore wallet transactions (remove from vtx)
unsigned int nMine = IgnoreWalletTransactions(vtx);

This comment has been minimized.

@sipa

sipa May 20, 2014

Member

You also need to remove everything that depends on a wallet transaction.

@sipa

sipa May 20, 2014

Member

You also need to remove everything that depends on a wallet transaction.

This comment has been minimized.

@ABISprotocol
@ABISprotocol
@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Jun 24, 2014

Please explain the merge conflict.

ABISprotocol commented Jun 24, 2014

Please explain the merge conflict.

@Diapolo

View changes

Show outdated Hide outdated src/init.cpp
@@ -232,6 +233,8 @@ std::string HelpMessage(HelpMessageMode hmm)
strUsage += " -seednode=<ip> " + _("Connect to a node to retrieve peer addresses, and disconnect") + "\n";
strUsage += " -socks=<n> " + _("Select SOCKS version for -proxy (4 or 5, default: 5)") + "\n";
strUsage += " -timeout=<n> " + _("Specify connection timeout in milliseconds (default: 5000)") + "\n";
strUsage += " -janitorinterval=<n> " + _("Number of seconds between each mempool janitor run (default: 1 day)") + "\n";

This comment has been minimized.

@Diapolo

Diapolo Jul 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.

@Diapolo

Diapolo Jul 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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 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."

Contributor

jgarzik commented Jul 18, 2014

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."

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Jul 18, 2014

@jgarzik Thanks for your work on this!

ABISprotocol commented Jul 18, 2014

@jgarzik Thanks for your work on this!

@jgarzik jgarzik removed this from the 0.10.0 milestone Jul 31, 2014

Jeff Garzik added some commits Feb 26, 2014

Jeff Garzik
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.
@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Aug 18, 2014

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.

BitcoinPullTester commented Aug 18, 2014

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.

@laanwj laanwj referenced this pull request Aug 30, 2014

Closed

mempool contains stale txs #4791

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Aug 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.

Contributor

dgenr8 commented Aug 30, 2014

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.

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Sep 2, 2014

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

ABISprotocol commented Sep 2, 2014

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 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.

Member

laanwj commented Sep 2, 2014

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.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Sep 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

Contributor

dgenr8 commented Sep 3, 2014

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

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 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.

Contributor

jgarzik commented Sep 3, 2014

@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.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Sep 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.

Contributor

dgenr8 commented Sep 3, 2014

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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 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.

Contributor

jgarzik commented Sep 3, 2014

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.

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Sep 9, 2014

Probably unrelated, but 'poolman' as well understood as it might be, seems so hombre-focused.
Alternatives? poolbeing, poolhelper, poolbot, poolio, poolcleaner

ABISprotocol commented Sep 9, 2014

Probably unrelated, but 'poolman' as well understood as it might be, seems so hombre-focused.
Alternatives? poolbeing, poolhelper, poolbot, poolio, poolcleaner

@laanwj laanwj added this to the 0.11.0 milestone Nov 17, 2014

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 29, 2014

Member

Needs rebase.

Member

jonasschnelli commented Dec 29, 2014

Needs rebase.

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Jan 2, 2015

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.

ABISprotocol commented Jan 2, 2015

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.

@laanwj laanwj added the Mempool label Jan 8, 2015

@@ -253,6 +254,8 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += " -proxy=<ip:port> " + _("Connect through SOCKS5 proxy") + "\n";
strUsage += " -seednode=<ip> " + _("Connect to a node to retrieve peer addresses, and disconnect") + "\n";
strUsage += " -timeout=<n> " + _("Specify connection timeout in milliseconds (default: 5000)") + "\n";
strUsage += " -janitorinterval=<n> " + _("Number of seconds between each mempool janitor run (default: 1 day)") + "\n";
strUsage += " -janitorexpire=<n> " + _("Number of seconds transactions live in memory pool, before removal (default: 3 days)") + "\n";

This comment has been minimized.

@Diapolo

Diapolo Jan 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.

@Diapolo

Diapolo Jan 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.

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

see notes on changes from poolman to poolbeing (languaging basically) above

@ABISprotocol

ABISprotocol Jan 9, 2015

see notes on changes from poolman to poolbeing (languaging basically) above

@@ -724,6 +727,16 @@ bool AppInit2(boost::thread_group& threadGroup)
threadGroup.create_thread(&ThreadScriptCheck);
}
// mempool janitor execution interval. default interval: 1 day
int janitorInterval = GetArg("-janitorinterval", (60 * 60 * 24 * 1));

This comment has been minimized.

@Diapolo

Diapolo Jan 8, 2015

This allows negative numbers, is this intended? Perhaps just use unsigned.

@Diapolo

Diapolo Jan 8, 2015

This allows negative numbers, is this intended? Perhaps just use unsigned.

@@ -0,0 +1,58 @@

This comment has been minimized.

@Diapolo

Diapolo Jan 8, 2015

Missing license :).

@Diapolo

Diapolo Jan 8, 2015

Missing license :).

@@ -0,0 +1,9 @@
#ifndef __BITCOIN_POOLMAN_H__

This comment has been minimized.

@Diapolo

Diapolo Jan 8, 2015

Also misses license and correct header define #ifndef BITCOIN_POOLMAN_H (and at the file end).

@Diapolo

Diapolo Jan 8, 2015

Also misses license and correct header define #ifndef BITCOIN_POOLMAN_H (and at the file end).

"sweepmempool\n"
"\nRemoves old, unconfirmed transactions from mempool\n"
"\nResult:\n"
"n (bool) true\n"

This comment has been minimized.

@Diapolo

Diapolo Jan 8, 2015

Nit: Is this correct help text formating?

@Diapolo

Diapolo Jan 8, 2015

Nit: Is this correct help text formating?

@@ -96,6 +96,7 @@ BITCOIN_CORE_H = \
net.h \
noui.h \
pow.h \
poolman.h \

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

change to poolbeing.h \

@ABISprotocol

ABISprotocol Jan 9, 2015

change to poolbeing.h \

@@ -204,6 +205,7 @@ libbitcoin_common_a_SOURCES = \
key.cpp \
keystore.cpp \
netbase.cpp \
poolman.cpp \

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

change to poolbeing.cpp \

@ABISprotocol

ABISprotocol Jan 9, 2015

change to poolbeing.cpp \

@@ -19,6 +19,7 @@
#include "txdb.h"
#include "ui_interface.h"
#include "util.h"
#include "poolman.h"

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

change to "poolbeing.h"

@ABISprotocol

ABISprotocol Jan 9, 2015

change to "poolbeing.h"

@@ -0,0 +1,58 @@
#include "poolman.h"

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

should be changed from poolman.h to poolbeing.h

@ABISprotocol

ABISprotocol Jan 9, 2015

should be changed from poolman.h to poolbeing.h

@@ -7,6 +7,7 @@
#include "main.h"
#include "rpcserver.h"
#include "sync.h"
#include "poolman.h"

This comment has been minimized.

@ABISprotocol

ABISprotocol Jan 9, 2015

poolman.h to be changed to poolbeing.h

@ABISprotocol

ABISprotocol Jan 9, 2015

poolman.h to be changed to poolbeing.h

@NanoAkron

This comment has been minimized.

Show comment
Hide comment
@NanoAkron

NanoAkron Feb 5, 2015

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.

NanoAkron commented Feb 5, 2015

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.

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Feb 15, 2015

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)

ABISprotocol commented Feb 15, 2015

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)
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 15, 2015

Member
Member

sipa commented Feb 15, 2015

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Apr 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:

Contributor

dexX7 commented Apr 24, 2015

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:

@laanwj laanwj removed this from the 0.11.0 milestone May 18, 2015

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 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

Contributor

pstratem commented Jun 13, 2015

@ABISprotocol poolman is short for pool manager, manager is from the latin manus for hand, and has nothing to do with gender

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2015

Member

How to move forward here? (and I don't mean in regard to silly word games)

Member

laanwj commented Jun 13, 2015

How to move forward here? (and I don't mean in regard to silly word games)

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 13, 2015

Contributor

Is expiration of mempool transactions on a fixed known schedule really the best solution?

Contributor

pstratem commented Jun 13, 2015

Is expiration of mempool transactions on a fixed known schedule really the best solution?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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.

Contributor

jgarzik commented Jun 13, 2015

@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.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 13, 2015

Member

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.

Member

jonasschnelli commented Jun 13, 2015

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.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 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?

Contributor

pstratem commented Jun 13, 2015

@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?

@pstratem pstratem referenced this pull request Jun 14, 2015

Closed

Limit mempool size #6281

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Jul 15, 2015

@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 ).

ABISprotocol commented Jul 15, 2015

@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 ).

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 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.

Contributor

jgarzik commented Jul 23, 2015

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.

@jgarzik jgarzik closed this Jul 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment