Evict orphans which are included or precluded by accepted blocks. #8179

gmaxwell commented at 1:52 AM on June 9, 2016: contributor

This eliminates the primary leak that causes the orphan map to always grow to its maximum size.

This does not go so far as to attempt to connect orphans made connectable by a new block.

Keeping the orphan map less full helps improve the reliability of relaying chains of transactions.

laanwj added the label P2P on Jun 9, 2016

in src/main.cpp:None in 70b5f60c76 outdated

2374 | @@ -2365,6 +2375,14 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2375 |              prevheights.resize(tx.vin.size());
2376 |              for (size_t j = 0; j < tx.vin.size(); j++) {
2377 |                  prevheights[j] = view.AccessCoins(tx.vin[j].prevout.hash)->nHeight;
2378 | +                // Which orphan pool entries must we evict?
2379 | +                map<COutPoint, set<uint256> >::iterator itByOutPoint = mapOrphanTransactionsByOutPoint.find(tx.vin[j].prevout);
2380 | +                if (itByOutPoint != mapOrphanTransactionsByOutPoint.end()) {
2381 | +                    for (set<uint256>::iterator mi = itByOutPoint->second.begin(); mi != itByOutPoint->second.end(); ++mi) {
2382 | +                             const uint256& orphanHash = *mi;

pstratem commented at 5:52 AM on June 10, 2016:

can you fix the whitespace here

pstratem commented at 5:53 AM on June 10, 2016: contributor

ACK 70b5f60c761f56e0e84583e77d20a81db3fe1424

modulo whitespace nit

gmaxwell commented at 6:00 AM on June 10, 2016: contributor

So, I'm not super happy with the behavior-- the issue is that it removes included or conflicted orphans, but if those orphans themselves have orphaned children, those won't get removed. The behavior I'm seeing in testing is that it initially removes many transactions but over time seems to remove fewer or fewer, and I think it's because the orphanmap gets full of double-orphans that were conflicted.

I'm not super keen on the performance implications of recursively eliminating there.

pstratem commented at 6:23 AM on June 10, 2016: contributor

@gmaxwell Agreed this behavior is not ideal, but this is an improvement.

in src/main.cpp:None in 70b5f60c76 outdated

  93 | @@ -94,6 +94,7 @@ struct COrphanTx {
  94 |  };
  95 |  map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(cs_main);
  96 |  map<uint256, set<uint256> > mapOrphanTransactionsByPrev GUARDED_BY(cs_main);

sipa commented at 11:47 AM on June 10, 2016:

It would be more efficient to store map<uint256, COrphanTx>::iterators instead of uint256's.

sipa commented at 2:08 PM on June 10, 2016: member

@gmaxwell Here is a commit that switches the mapOrphansByPrev entirely to be by-COutPoint: https://github.com/sipa/bitcoin/commits/reworkorphans

Track orphan by prev COutPoint rather than prev hash 1b0bcc5f95

This eliminates the primary leak that causes the orphan map to

 always grow to its maximum size.

This does not go so far as to attempt to connect orphans made
 connectable by a new block.

Keeping the orphan map less full helps improve the reliability
 of relaying chains of transactions.

db0ffe80a0

in src/main.cpp:None in 0c5dc4c676 outdated

 689 | @@ -689,6 +690,26 @@ void EraseOrphansFor(NodeId peer)
 690 |  unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 691 |  {
 692 |      unsigned int nEvicted = 0;
 693 | +    static int64_t nNextSweep;
 694 | +    int64_t nNow = GetTime();
 695 | +    if (nNextSweep <= nNow) {
 696 | +        // Sweep out expired orphan pool entries:
 697 | +        int nErased = 0;
 698 | +        int64_t nMinExpTime = nNow + 15 * 60;

sipa commented at 12:57 PM on June 11, 2016:

Can you turn this into a named constant?

sipa commented at 1:05 PM on June 11, 2016: member

utACK with nit

Adds an expiration time for orphan tx.

This prevents higher order orphans and other junk from
 holding positions in the orphan map.  Parents delayed
 twenty minutes are more are unlikely to ever arrive.

The freed space will improve the orphan matching success rate for
 other transactions.

11cc143895

Treat orphans as implicit inv for parents, discard when parents rejected.

An orphan whos parents were rejected is never going to connect, so there
 is little utility in keeping it.

Orphans also helpfully tell us what we're missing, so go ahead and treat
 it as INVed.

8c99d1b525

Increase maximum orphan size to 100,000 bytes.

Although this increases node memory usage in the worst case by perhaps
 30MB, the current behavior causes severe issues with dependent tx relay.

54326a6808

gmaxwell commented at 10:30 AM on June 15, 2016: contributor

@sipa Nit picked.

laanwj merged this on Jun 20, 2016

laanwj closed this on Jun 20, 2016

laanwj referenced this in commit 94ab58b5cc on Jun 20, 2016

laanwj commented at 12:54 PM on June 20, 2016: member

utACK 54326a6

codablock referenced this in commit bafaba9255 on Sep 16, 2017

codablock referenced this in commit e739e78feb on Sep 19, 2017

codablock referenced this in commit 9b05b917cc on Dec 27, 2017

codablock referenced this in commit 808936cc30 on Dec 28, 2017

andvgal referenced this in commit 0f1c3d79e3 on Jan 6, 2019

DrahtBot locked this on Sep 8, 2021