Skip to content
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 package tracking #6654

Merged
merged 2 commits into from Sep 21, 2015
Merged

Conversation

sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Sep 9, 2015

This is a preparatory pull to try to make reviewing #6557 easier.

In #6557, I added tracking packages of transactions to the mempool (for each tx, we track all the "descendant" transactions that depend on that tx), in order to make the mempool limiting code more effective. This PR is a standalone implementation of mempool descendant tracking, including the policy limits on transaction chains (limiting ancestors and descendants) proposed in #6557.

I'll rebase that pull off these commits assuming we can agree on this approach.

@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 9, 2015

@sipa I did end up reworking the tracking of in-mempool parents/children to use iterators rather than hashes as you had suggested (which I never pushed up to #6557).

@btcdrak
Copy link
Contributor

btcdrak commented Sep 9, 2015

@laanwj looks like travis crapped out on one of the jobs and needs restarting.

@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 9, 2015

@btcdrak Actually I think this is some kind of problem with the unit test code -- not sure why it fails to compile only in that one job but I was just able to reproduce locally. Working on a fix...

@sdaftuar sdaftuar force-pushed the mempool-packages-only branch from 315bc92 to d6401c1 Compare Sep 9, 2015
@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 9, 2015

@btcdrak Fixed and pushed back up, travis is happy now...

@@ -466,5 +474,7 @@ static const unsigned int REJECT_HIGHFEE = 0x100;
static const unsigned int REJECT_ALREADY_KNOWN = 0x101;
/** Transaction conflicts with a transaction already known */
static const unsigned int REJECT_CONFLICT = 0x102;
/** Transaction would result in too long in-mempool chain */
static const unsigned int REJECT_LONGCHAIN = 0x103;
Copy link
Member Author

@sdaftuar sdaftuar Sep 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will move this to not be an internal code (so we send a reject message back).

@petertodd
Copy link
Contributor

petertodd commented Sep 14, 2015

In general, it'd be good to think about separating the reorg case entirely from the main mempool codebase. For instance, keep a simple linear list of reorged transactions, and after a reorg attempt to add them back the mempool one-by-one. This separate code could also handle cases where we might want to remine transactions that we otherwise wouldn't, as a "goodwill" gesture to reduce the impact of large reorgs.

{
// Track the number of entries (outside setExclude) that we'd need to visit
// (will bail out if it exceeds maxDescendantsToVisit)
int nChildrenToVisit = 0;
Copy link
Contributor

@petertodd petertodd Sep 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

@pstratem
Copy link
Contributor

pstratem commented Sep 14, 2015

Needs a comment defining ancestor/descendant.


while (!parentHashes.empty()) {
setAncestors.insert(parentHashes.begin(), parentHashes.end());
setEntries stageParentSet;
Copy link
Contributor

@petertodd petertodd Sep 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

@morcos morcos mentioned this pull request Sep 14, 2015
errString = strprintf("too many unconfirmed ancestors [limit: %u]", limitAncestorCount);
return false;
}
}
Copy link
Contributor

@petertodd petertodd Sep 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

@sipa
Copy link
Member

sipa commented Sep 14, 2015

Concept ACK.

@petertodd
Copy link
Contributor

petertodd commented Sep 14, 2015

utACK, modulo nits.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Sep 14, 2015

Concept ACK / code review ACK.
Passes gitian/osx/debian build/unit-tests/rpc-tests.

@petertodd
Copy link
Contributor

petertodd commented Sep 14, 2015

Tested with my FSS and Full RBF stress tests and -checkmempool, no unexpected failures. (this pull-req of course doesn't enable any RBF behavior, so replacements failed!)

@dcousens
Copy link
Contributor

dcousens commented Sep 14, 2015

concept ACK

@@ -921,6 +921,17 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
REJECT_HIGHFEE, "absurdly-high-fee",
strprintf("%d > %d", nFees, ::minRelayTxFee.GetFee(nSize) * 10000));

// Calculate in-mempool ancestors, up to a limit.
CTxMemPool::setEntries setAncestors;
size_t nLimitAncestors = GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
Copy link
Contributor

@dcousens dcousens Sep 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using GetArg (an application level function) in mempool code feels a bit odd.

Copy link
Member Author

@sdaftuar sdaftuar Sep 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but is there a better way to do this? I think I'm following the common practice in the code base, though I look forward to a future refactoring when we have better encapsulation.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Sep 19, 2015

My computer's battery died, but in txmempool.h, the definition of txiter as "boost::multi_index_container::iterator" is "implementation defined" according to boost's docs. In the latest versions it is defined to an equivalent of "boost::multi_index_container::nth_index<0>::type::iterator", so you should use that.

On September 16, 2015 8:44:26 AM CDT, Suhas Daftuar notifications@github.com wrote:

Thanks everyone for reviewing! I've pushed up a series of commits to
address everyone's feedback; I believe all comments should have been
addressed.

These cleanups need to be squashed, and this pull now needs to be
rebased (a merge conflict crept in now that I'm outputting additional
information in getrawmempool). Reviewers -- please let me know if you
prefer I leave these commits unsquashed/unrebased so you can review the
changes. (In the absence of any expressed preferences, I'll plan to
squash/rebase in a day or two so that this can become mergeable.)

@petertodd I added the extra information to the getrawmempool RPC call
(good idea!). Now that the RPC call is there, I realized that adding
an RPC test that exercises the new limits is a good idea, so I've
started work on that, but it's not yet complete.


Reply to this email directly or view it on GitHub:
#6654 (comment)

@dcousens
Copy link
Contributor

dcousens commented Sep 19, 2015

@sdaftuar needs rebase.

ashleyholman and others added 2 commits Sep 19, 2015
Indexes on:
- Tx Hash
- Fee Rate (fee-per-kb)
Associate with each CTxMemPoolEntry all the size/fees of descendant
mempool transactions.  Sort mempool by max(feerate of entry, feerate
of descendants).  Update statistics on-the-fly as transactions enter
or leave the mempool.

Also add ancestor and descendant limiting, so that transactions can
be rejected if the number or size of unconfirmed ancestors exceeds
a target, or if adding a transaction would cause some other mempool
entry to have too many (or too large) a set of unconfirmed in-
mempool descendants.
@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 19, 2015

I addressed the latest comments from @morcos and @TheBlueMatt, added an rpc test (it only tests the ancestor/descendant length limits, not the size limits, nor the handling of reorgs -- so that still can be improved), and then squashed everything down and rebased on master to get rid of the merge conflict in rpcblockchain.cpp.

@Mirobit
Copy link
Contributor

Mirobit commented Sep 19, 2015

Does anyone else think the ancestor limit is way to generous? In what other case than spamming do txs have 100 ancestors in mempool? Shouldn't 5 be enough?

@petertodd
Copy link
Contributor

petertodd commented Sep 20, 2015

@Mirobit The limits are there to limit computational costs in determining things like dependent fees/size etc. There's no need to set them low unless the algorithms take a long time to compute those sums; the data structures in the mempool are mainly pointer following so it's fairly fast.

@laanwj
Copy link
Member

laanwj commented Sep 21, 2015

utACK

@laanwj laanwj merged commit 5add7a7 into bitcoin:master Sep 21, 2015
1 check passed
laanwj added a commit that referenced this issue Sep 21, 2015
5add7a7 Track transaction packages in CTxMemPoolEntry (Suhas Daftuar)
34628a1 TxMemPool: Change mapTx to a boost::multi_index_container (Ashley Holman)
@sipa
Copy link
Member

sipa commented Sep 22, 2015

My node running 5add7a7 with -checkmempool crashed. Last debug.log message was:

2015-09-22 13:06:19 - Disconnect block: 90.04ms

@morcos
Copy link
Member

morcos commented Sep 22, 2015

@sipa can you give us some more details? were you running will all -debug options? for instance I assume you were not running with estimatefee debugging? it helps to narrow down where it crashed based on what didn't get printed to the debug log.

@sdaftuar sdaftuar mentioned this pull request Sep 23, 2015
zkbot added a commit to zcash/zcash that referenced this issue Feb 20, 2018
Mempool improvements, branch ID awareness

Whenever the local chain tip is updated, transactions in the mempool which commit to an
unmineable branch ID (for example, just before a network upgrade activates, where the
next block will have a different branch ID) will be removed.

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6654
  - Only the mempool index change.
- bitcoin/bitcoin#6776
- bitcoin/bitcoin#7020
- bitcoin/bitcoin#6915

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this issue Feb 20, 2018
Mempool improvements, branch ID awareness

Whenever the local chain tip is updated, transactions in the mempool which commit to an
unmineable branch ID (for example, just before a network upgrade activates, where the
next block will have a different branch ID) will be removed.

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6654
  - Only the mempool index change.
- bitcoin/bitcoin#6776
- bitcoin/bitcoin#7020
- bitcoin/bitcoin#6915

Part of #2074.
furszy added a commit to PIVX-Project/PIVX that referenced this issue Jun 14, 2020
2105947 Implement helper class for CTxMemPoolEntry constructor (Alex Morcos)
1cef905 Make -checkmempool=1 not fail through int32 overflow (Pieter Wuille)
0f72ff2 Support -checkmempool=N, which runs checks on average once every N transactions (Pieter Wuille)
89483d0 [Bug] Make operator() a const function in CompareTxMemPoolEntryByX (random-zebra)
a50ad77 Lower default policy limits (random-zebra)
03f7152 fix locking issue with new mempool limiting (random-zebra)
1598961 Fix stale comment in CTxMemPool::TrimToSize. (random-zebra)
98d0d68 Undo GetMinFee-requires-extra-call-to-hit-0 (random-zebra)
6ad6ee6 Add reasonable test case for mempool trimming (random-zebra)
8dcbb7e Only call TrimToSize once per reorg/blocks disconnect (random-zebra)
c20cd38 Implement on-the-fly mempool size limitation. (random-zebra)
aee2e17 Print mempool size in KB when adding txn (random-zebra)
f7c85fd Add CFeeRate += operator (random-zebra)
5bd2a00 Track (and define) ::minRelayTxFee in CTxMemPool (random-zebra)
0b50f6c Add Mempool Expire function to remove old transactions (random-zebra)
d26f5e0 Fix calling mempool directly, instead of pool, in ATMP (random-zebra)
fc5eddb Reverse the sort on the mempool's feerate index (random-zebra)
0ce1df0 [BUG] Fix CTxMemPool::check excluding zerocoins from children checks (random-zebra)
1f7bd52 Track transaction packages in CTxMemPoolEntry (random-zebra)
1fd406b TxMemPool: Change mapTx to a boost::multi_index_container (random-zebra)

Pull request description:

  built on top of
  - [x] #1645

  This PR pulls some updates from upstream in the mempool area, adding the required adjustments for legacy zerocoin txes and updating the functional test suite.

  Specifically, here we:
  - track mempool descendants (in-mempool transactions that depend on other mempool transactions)
  - turn `mapTx` into a `boost::multi_index_container` that sorts the mempool on 3 criteria:
    - transaction hash
    - fee rate
    - time in the mempool
  - Add a max size for the mempool (throwing away the cheapest txs and bumping the min relay fee, when full)
  - Implement on-the-fly mempool size limit with the flag `-maxmempool`
  - Implement `-checkmempool=N` to customize the frequency of the mempool check
  - Implement helper for `CTxMemPoolEntry` for the unit tests.

  Backports:

  - bitcoin#6654
  - bitcoin#6722 [`*`]
  - bitcoin#6889
  - bitcoin#6771
  - bitcoin#6776
  - bitcoin#6896
  - bitcoin#7020

  [`*`] excluding bitcoin@9e93640 as our default minimum tx fee rate of 10k satoshis is only 0,00003 USD at the time of writing.

ACKs for top commit:
  Fuzzbawls:
    utACK 2105947
  furszy:
    Re utACK 2105947 and merging this nice upgrade :) .

Tree-SHA512: 51a7d75bd52f7646d461252c78f0dd9d7e8b5c1c66c22944120bfe293b28f5d48135de339ebf3d8a5b4c61ca5452383ed1b10c417be06dc4a335ac645842ea14
str4d pushed a commit to str4d/zcash that referenced this issue Aug 10, 2021
The score index is meant to represent the order of priority for being included in a block for miners.  Initially this is set to the transactions modified (by any feeDelta) fee rate.  Index improvements and unit tests by sdaftuar.

(cherry picked from commit f3fe836)

Zcash: Also includes some small refactors from bitcoin/bitcoin#6654 which
we have not backported.
str4d pushed a commit to str4d/zcash that referenced this issue Aug 10, 2021
Previously Bitcoin would send 1/4 of transactions out to all peers
 instantly.  This causes high overhead because it makes >80% of
 INVs size 1.  Doing so harms privacy, because it limits the
 amount of source obscurity a transaction can receive.

These randomized broadcasts also disobeyed transaction dependencies
 and required use of the orphan pool.  Because the orphan pool is
 so small this leads to poor propagation for dependent transactions.

When the bypass wasn't in effect, transactions were sent in the
 order they were received.  This avoided creating orphans but
 undermines privacy fairly significantly.

This commit:
 Eliminates the bypass. The bypass is replaced by halving the
  average delay for outbound peers.

 Sorts candidate transactions for INV by their topological
  depth then by their feerate (then hash); removing the
  information leakage and providing priority service to
  higher fee transactions.

 Limits the amount of transactions sent in a single INV to
  7tx/sec (and twice that for outbound); this limits the
  harm of low fee transaction floods, gives faster relay
  service to higher fee transactions. The 7 sounds lower
  than it really is because received advertisements need
  not be sent, and because the aggregate rate is multipled
  by the number of peers.

(cherry picked from commit f2d3ba7)

Zcash: Candidate transactions for INV are not sorted by their
topological depth because we haven't backported bitcoin/bitcoin#6654.
str4d pushed a commit to str4d/zcash that referenced this issue Aug 11, 2021
Previously Bitcoin would send 1/4 of transactions out to all peers
 instantly.  This causes high overhead because it makes >80% of
 INVs size 1.  Doing so harms privacy, because it limits the
 amount of source obscurity a transaction can receive.

These randomized broadcasts also disobeyed transaction dependencies
 and required use of the orphan pool.  Because the orphan pool is
 so small this leads to poor propagation for dependent transactions.

When the bypass wasn't in effect, transactions were sent in the
 order they were received.  This avoided creating orphans but
 undermines privacy fairly significantly.

This commit:
 Eliminates the bypass. The bypass is replaced by halving the
  average delay for outbound peers.

 Sorts candidate transactions for INV by their topological
  depth then by their feerate (then hash); removing the
  information leakage and providing priority service to
  higher fee transactions.

 Limits the amount of transactions sent in a single INV to
  7tx/sec (and twice that for outbound); this limits the
  harm of low fee transaction floods, gives faster relay
  service to higher fee transactions. The 7 sounds lower
  than it really is because received advertisements need
  not be sent, and because the aggregate rate is multipled
  by the number of peers.

(cherry picked from commit f2d3ba7)

Zcash: Candidate transactions for INV are not sorted by their
topological depth because we haven't backported bitcoin/bitcoin#6654.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet