Skip to content

Make orphan TX map limiting dependent on total TX size instead of TX count #3121

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.

Sign up for GitHub

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

Merged
codablock merged 1 commit into from
Sep 30, 2019
Merged

Make orphan TX map limiting dependent on total TX size instead of TX count #3121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ std::string HelpMessage(HelpMessageMode mode)
}
strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file on startup"));
strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
strUsage += HelpMessageOpt("-maxorphantxsize=<n>", strprintf(_("Maximum total size of all orphan transactions in megabytes (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE));
strUsage += HelpMessageOpt("-maxmempool=<n>", strprintf(_("Keep the transaction memory pool below <n> megabytes (default: %u)"), DEFAULT_MAX_MEMPOOL_SIZE));
strUsage += HelpMessageOpt("-mempoolexpiry=<n>", strprintf(_("Do not keep transactions in the mempool longer than <n> hours (default: %u)"), DEFAULT_MEMPOOL_EXPIRY));
strUsage += HelpMessageOpt("-persistmempool", strprintf(_("Whether to save the mempool on shutdown and load on restart (default: %u)"), DEFAULT_PERSIST_MEMPOOL));
Expand Down Expand Up @@ -1416,6 +1416,10 @@ bool AppInitParameterInteraction()
return InitError("LLMQ type for ChainLocks can only be overridden on devnet.");
}

if (gArgs.IsArgSet("-maxorphantx")) {
InitWarning("-maxorphantx is not supported anymore. Use -maxorphantxsize instead.");
}

return true;
}

Expand Down
17 changes: 12 additions & 5 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ struct COrphanTx {
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
size_t nTxSize;
};
static CCriticalSection g_cs_orphans;
std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);
std::map<COutPoint, std::set<std::map<uint256, COrphanTx>::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans);
size_t nMapOrphanTransactionsSize = 0;
void EraseOrphansFor(NodeId peer);

static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
Expand Down Expand Up @@ -641,14 +643,16 @@ bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRE
return false;
}

auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME});
auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, sz});
assert(ret.second);
for (const CTxIn& txin : tx->vin) {
mapOrphanTransactionsByPrev[txin.prevout].insert(ret.first);
}

AddToCompactExtraTransactions(tx);

nMapOrphanTransactionsSize += sz;

LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(),
mapOrphanTransactions.size(), mapOrphanTransactionsByPrev.size());
return true;
Expand All @@ -668,6 +672,8 @@ int static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
if (itPrev->second.empty())
mapOrphanTransactionsByPrev.erase(itPrev);
}
assert(nMapOrphanTransactionsSize >= it->second.nTxSize);
nMapOrphanTransactionsSize -= it->second.nTxSize;
mapOrphanTransactions.erase(it);
return 1;
}
Expand All @@ -689,7 +695,7 @@ void EraseOrphansFor(NodeId peer)
}


unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
unsigned int LimitOrphanTxSize(unsigned int nMaxOrphansSize)
{
LOCK(g_cs_orphans);

Expand All @@ -714,7 +720,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL;
if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased);
}
while (mapOrphanTransactions.size() > nMaxOrphans)
while (!mapOrphanTransactions.empty() && nMapOrphanTransactionsSize > nMaxOrphansSize)
{
// Evict a random orphan:
uint256 randomhash = GetRandHash();
Expand Down Expand Up @@ -2221,8 +2227,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
AddOrphanTx(ptx, pfrom->GetId());

// DoS prevention: do not allow mapOrphanTransactions to grow unbounded
unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS));
unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx);
unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000;
unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTxSize);
if (nEvicted > 0) {
LogPrint(BCLog::MEMPOOL, "mapOrphan overflow, removed %u tx\n", nEvicted);
}
Expand Down Expand Up @@ -3658,5 +3664,6 @@ class CNetProcessingCleanup
// orphan transactions
mapOrphanTransactions.clear();
mapOrphanTransactionsByPrev.clear();
nMapOrphanTransactionsSize = 0;
}
} instance_of_cnetprocessingcleanup;
4 changes: 2 additions & 2 deletions src/net_processing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
#include "net.h"
#include "validationinterface.h"

/** Default for -maxorphantx, maximum number of orphan transactions kept in memory */
static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
/** Default for -maxorphantxsize, maximum size in megabytes the orphan map can grow before entries are removed */
static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE = 10; // this allows around 100 TXs of max size (and many more of normal size)
/** Expiration time for orphan transactions in seconds */
static const int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
/** Minimum time between orphan transactions expire time checks in seconds */
Expand Down
2 changes: 1 addition & 1 deletion test/functional/mempool_packages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
class MempoolPackagesTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 2
self.extra_args = [["-maxorphantx=1000"], ["-maxorphantx=1000", "-limitancestorcount=5"]]
self.extra_args = [["-maxorphantxsize=1000"], ["-maxorphantxsize=1000", "-limitancestorcount=5"]]
Copy link
Copy Markdown

@nmarley nmarley Sep 29, 2019

Choose a reason for hiding this comment

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

1000 MB would be 1 GB, right? Does this not seem a little high (even for this test script)?

Copy link
Copy Markdown
Author

@codablock codablock Sep 30, 2019

Choose a reason for hiding this comment

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

Yes it is a "little" high, but 1000 orphan TX count was also high. The intent here was most likely to just disable the limit and keeping the number 1000 makes it easier later when conflicts arise (no need to think about that number)


# Build a transaction that spends parent_txid:vout
# Return amount sent
Expand Down
6 changes: 3 additions & 3 deletions test/functional/smartfees.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ def setup_network(self):
But first we need to use one node to create a lot of outputs
which we will use to generate our transactions.
"""
self.add_nodes(3, extra_args=[["-maxorphantx=1000", "-whitelist=127.0.0.1"],
["-blockmaxsize=17000", "-maxorphantx=1000"],
["-blockmaxsize=8000", "-maxorphantx=1000"]])
self.add_nodes(3, extra_args=[["-maxorphantxsize=1000", "-whitelist=127.0.0.1"],
Copy link
Copy Markdown

@nmarley nmarley Sep 29, 2019

Choose a reason for hiding this comment

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

Same question as above.

["-blockmaxsize=17000", "-maxorphantxsize=1000"],
["-blockmaxsize=8000", "-maxorphantxsize=1000"]])
# Use node0 to mine blocks for input splitting
# Node1 mines small blocks but that are bigger than the expected transaction rate.
# NOTE: the CreateNewBlock code starts counting block size at 1,000 bytes,
Expand Down